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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CADUfDZo2+_Au=3g6_1OOMGcBcJYKuCccWnJUKENVsq-j_8umFA@mail.gmail.com>
Date: Fri, 3 Jan 2025 09:50:29 -0800
From: Caleb Sander <csander@...estorage.com>
To: Christoph Hellwig <hch@....de>
Cc: Denis Arefev <arefev@...mel.ru>, Sagi Grimberg <sagi@...mberg.me>, 
	Chaitanya Kulkarni <kch@...dia.com>, Hannes Reinecke <hare@...nel.org>, Keith Busch <kbusch@...nel.org>, 
	linux-nvme@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	syzbot+a84181c81389771eb46a@...kaller.appspotmail.com
Subject: Re: [PATCH] nvme: Enter string size calculation “subsysnqn”

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