Re: [PATCH] nvme: Enter string size calculation “subsysnqn”

Caleb Sander
2025-01-03T17:50:50Z

The bug has already been fixed in commit 4db3d750ac7e ("nvmet: Don't
overflow subsysnqn"). It is using the kmemdup() approach (technically,
kstrndup()) suggested by Christoph.

On Thu, Jan 2, 2025 at 11:02 PM Christoph Hellwig <hch@lst.de> wrote:
>
> On Thu, Dec 26, 2024 at 01:45:35PM +0300, Denis Arefev wrote:
> > When memory is allocated, the size of the string
> > is calculated nvmet_subsys_alloc(...).
>
> s/is/is in/ ?
>
> > When memory was accessed, constant size was used.
> >
> > Fixes: 95409e277d83 ("nvmet: implement unique discovery NQN")
> > Reported-by: syzbot+a84181c81389771eb46a@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=a84181c81389771eb46a
> > Signed-off-by: Denis Arefev <arefev@swemel.ru>
> > ---
> > drivers/nvme/target/configfs.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> > index eeee9e9b854c..2f74204d000e 100644
> > --- a/drivers/nvme/target/configfs.c
> > +++ b/drivers/nvme/target/configfs.c
> > @@ -2247,14 +2247,15 @@ static struct config_group nvmet_hosts_group;
> > static ssize_t nvmet_root_discovery_nqn_show(struct config_item *item,
> > char *page)
> > {
> > - return snprintf(page, PAGE_SIZE, "%s\n", nvmet_disc_subsys->subsysnqn);
> > + return snprintf(page, strnlen(nvmet_disc_subsys->subsysnqn, PAGE_SIZE),
> > + "%s\n", nvmet_disc_subsys->subsysnqn);
>
> This doesn't really make sense as %s already calculates the length
> of the string internally.
>
> > }
> >
> > static ssize_t nvmet_root_discovery_nqn_store(struct config_item *item,
> > const char *page, size_t count)
> > {
> > struct list_head *entry;
> > - size_t len;
> > + size_t len, nqn_len;
> >
> > len = strcspn(page, "\n");
> > if (!len || len > NVMF_NQN_FIELD_LEN - 1)
> > @@ -2271,8 +2272,9 @@ static ssize_t nvmet_root_discovery_nqn_store(struct config_item *item,
> > return -EINVAL;
> > }
> > }
> > - memset(nvmet_disc_subsys->subsysnqn, 0, NVMF_NQN_FIELD_LEN);
> > - memcpy(nvmet_disc_subsys->subsysnqn, page, len);
> > + nqn_len = strnlen(nvmet_disc_subsys->subsysnqn, NVMF_NQN_SIZE);
> > + memset(nvmet_disc_subsys->subsysnqn, 0, nqn_len);
> > + memcpy(nvmet_disc_subsys->subsysnqn, page, nqn_len);
>
> The memset should still happen for the entire field, only the copy
> needs to me limited.
>
> But i really wonder if we should do away with this mess and just
> use kmemdup()ed allocations for the nqns.
>
>