[PATCH] nvmet: nul-terminate the NQNs passed in the connect command

[Alon Zahavi]

On Fri, 10 Nov 2023 at 17:37, Caleb Sander <csander at 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 at 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 at gmail.com>
> > Signed-off-by: Christoph Hellwig <hch at 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
> >
> >