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

[Caleb Sander]

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 at 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 at syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=a84181c81389771eb46a
> > Signed-off-by: Denis Arefev <arefev at 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.
>
>