* [PATCH] nvmet: nul-terminate the NQNs passed in the connect command
@ 2023-11-10 6:38 Christoph Hellwig
2023-11-10 15:37 ` Caleb Sander
0 siblings, 1 reply; 3+ messages in thread
From: Christoph Hellwig @ 2023-11-10 6:38 UTC (permalink / raw)
To: sagi; +Cc: linux-nvme, Alon Zahavi
The host and subsystem NQNs are passed in the connect command payload and
interpreted as nul-terminated strings. Ensure they actually are
nul-terminated before using them.
Fixes: a07b4970f464 "nvmet: add a generic NVMe target")
Reported-by: Alon Zahavi <zahavi.alon@gmail.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/nvme/target/fabrics-cmd.c | 4 ++++
1 file changed, 4 insertions(+)
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) {
--
2.39.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] nvmet: nul-terminate the NQNs passed in the connect command
2023-11-10 6:38 [PATCH] nvmet: nul-terminate the NQNs passed in the connect command Christoph Hellwig
@ 2023-11-10 15:37 ` Caleb Sander
2023-11-10 15:48 ` Alon Zahavi
0 siblings, 1 reply; 3+ messages in thread
From: Caleb Sander @ 2023-11-10 15:37 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: sagi, linux-nvme, Alon Zahavi
Won't this overrun the subsysnqn and hostnqn fields? Writing to
subsysnqn[NVMF_NQN_FIELD_LEN] will clobber the first char of hostnqn:
struct nvmf_connect_data {
uuid_t hostid;
__le16 cntlid;
char resv4[238];
char subsysnqn[NVMF_NQN_FIELD_LEN];
char hostnqn[NVMF_NQN_FIELD_LEN];
char resv5[256];
};
I think clearing the previous byte (index NVMF_NQN_FIELD_LEN - 1)
would work. The spec requires NQNs to be under 223 bytes anyways, so
they should never take up the whole field.
On Thu, Nov 9, 2023 at 10:45 PM Christoph Hellwig <hch@lst.de> wrote:
>
> The host and subsystem NQNs are passed in the connect command payload and
> interpreted as nul-terminated strings. Ensure they actually are
> nul-terminated before using them.
>
> Fixes: a07b4970f464 "nvmet: add a generic NVMe target")
> Reported-by: Alon Zahavi <zahavi.alon@gmail.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/nvme/target/fabrics-cmd.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> 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) {
> --
> 2.39.2
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] nvmet: nul-terminate the NQNs passed in the connect command
2023-11-10 15:37 ` Caleb Sander
@ 2023-11-10 15:48 ` Alon Zahavi
0 siblings, 0 replies; 3+ messages in thread
From: Alon Zahavi @ 2023-11-10 15:48 UTC (permalink / raw)
To: Caleb Sander; +Cc: Christoph Hellwig, sagi, linux-nvme
On Fri, 10 Nov 2023 at 17:37, Caleb Sander <csander@purestorage.com> wrote:
>
> Won't this overrun the subsysnqn and hostnqn fields? Writing to
> subsysnqn[NVMF_NQN_FIELD_LEN] will clobber the first char of hostnqn:
> struct nvmf_connect_data {
> uuid_t hostid;
> __le16 cntlid;
> char resv4[238];
> char subsysnqn[NVMF_NQN_FIELD_LEN];
> char hostnqn[NVMF_NQN_FIELD_LEN];
> char resv5[256];
> };
>
> I think clearing the previous byte (index NVMF_NQN_FIELD_LEN - 1)
> would work. The spec requires NQNs to be under 223 bytes anyways, so
> they should never take up the whole field.
>
True.
We should zero the byte in the `NVMF_NQN_SIZE` index, making the
fields their real size.
Changing the patch from `d->subsysnqn[NVMF_NQN_FIELD_LEN] = '\0';` to
`d->subsysnqn[NVMF_NQN_SIZE] = '\0';`
> On Thu, Nov 9, 2023 at 10:45 PM Christoph Hellwig <hch@lst.de> wrote:
> >
> > The host and subsystem NQNs are passed in the connect command payload and
> > interpreted as nul-terminated strings. Ensure they actually are
> > nul-terminated before using them.
> >
> > Fixes: a07b4970f464 "nvmet: add a generic NVMe target")
> > Reported-by: Alon Zahavi <zahavi.alon@gmail.com>
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> > drivers/nvme/target/fabrics-cmd.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > 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) {
> > --
> > 2.39.2
> >
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-11-10 15:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-10 6:38 [PATCH] nvmet: nul-terminate the NQNs passed in the connect command Christoph Hellwig
2023-11-10 15:37 ` Caleb Sander
2023-11-10 15:48 ` Alon Zahavi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).