Re: [Bug Report] NVMe-oF/TCP - Slab OOB Read in `nvmet_ctrl_find_get`

[Christoph Hellwig <hch@lst.de>]

On Wed, Nov 08, 2023 at 01:02:38PM +0200, Alon Zahavi wrote:
> I think that although those patches will fix the bug in the two
> functions we talked about, the non-terminated strings are still stored
> in the `struct nvmf_connect_data` object.

Yes, and that's a problem.

> Is it possible to add a NULL terminator after each time we use
> `nvmet_copy_from_sgl()` to copy from an SGL to `struct
> nvmf_connect_data` object?

Yes, and that is the best thing to do in the short term as it's
easily backportable.  I'd much prefer to move to counted strings as
in the seq_buf type, but that is a longer term project.

> Also, I think `nvmet_copy_from_sgl()` should be considered as unsafe
> when using it to copy strings, as it doesn't check for NULL
> termination.

Think of nvmet_copy_from_sgl as a memcpy from a different address
space.  It works on a very different abstraction layer and thus
doesn't even know about strings.  The callers really need to,
and maybe we want a helper for that.  For now that patch below should
fix the issue you reported, although for me KASAN doesn't trip up
on the reproducer with or without the patch, so if you could test it
on your setup that would be great:

diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index 43b5bd8bb6a52d..0920fe7ce4ac99 100644
--- a/drivers/nvme/target/fabrics-cmd.c
+++ b/drivers/nvme/target/fabrics-cmd.c
@@ -244,6 +244,8 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
 		goto out;
 	}
 
+	d->subsysnqn[NVMF_NQN_FIELD_LEN] = '\0';
+	d->hostnqn[NVMF_NQN_FIELD_LEN] = '\0';
 	status = nvmet_alloc_ctrl(d->subsysnqn, d->hostnqn, req,
 				  le32_to_cpu(c->kato), &ctrl);
 	if (status)
@@ -313,6 +315,8 @@ static void nvmet_execute_io_connect(struct nvmet_req *req)
 		goto out;
 	}
 
+	d->subsysnqn[NVMF_NQN_FIELD_LEN] = '\0';
+	d->hostnqn[NVMF_NQN_FIELD_LEN] = '\0';
 	ctrl = nvmet_ctrl_find_get(d->subsysnqn, d->hostnqn,
 				   le16_to_cpu(d->cntlid), req);
 	if (!ctrl) {