1. Patchew
  2. linux
  3. View series

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

Denis Arefev posted 1 patch 1 year, 1 month ago 
drivers/nvme/target/configfs.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
[PATCH] nvme: Enter string size calculation “subsysnqn”
Posted by Denis Arefev 1 year, 1 month ago 
When memory is allocated, the size of the string 
is calculated nvmet_subsys_alloc(...).
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);
 }
 
 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);
 	up_write(&nvmet_config_sem);
 
 	return len;
-- 
2.43.0
Re: [PATCH] nvme: Enter string size calculation “subsysnqn”
Posted by Christoph Hellwig 1 year, 1 month ago 
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.
Re: [PATCH] nvme: Enter string size calculation “subsysnqn”
Posted by Caleb Sander 1 year, 1 month ago 
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.
>
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.