[PATCH 1/1] mdadm/platform-intel: Fix buffer overflow

[PATCH 1/1] mdadm/platform-intel: Fix buffer overflow

[Xiao Ni]

It reports buffer overflow detected when creating raid with big
nvme devices. In my test, the size of the nvme device is 1.5T.
It can't reproduce this with nvme device which size is smaller
than 1T.

In function get_nvme_multipath_dev_hw_path it allocs memory in a for
loop and the size it allocs is big. So if the iteration number is
large, it has a risk that the stack space is larger than the limit.
So move the memory allocation at the biginning of the funtion.

Fixes: d835518b6b53 ('imsm: nvme multipath support')
Reported-by: Guang Wu <guazhang@redhat.com>
Signed-off-by: Xiao Ni <xni@redhat.com>
---
 platform-intel.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/platform-intel.c b/platform-intel.c
index 15a9fa5a..0732af2b 100644
--- a/platform-intel.c
+++ b/platform-intel.c
@@ -898,6 +898,7 @@ char *get_nvme_multipath_dev_hw_path(const char *dev_path)
 	DIR *dir;
 	struct dirent *ent;
 	char *rp = NULL;
+	char buf[PATH_MAX];
 
 	if (strncmp(dev_path, NVME_SUBSYS_PATH, strlen(NVME_SUBSYS_PATH)) != 0)
 		return NULL;
@@ -907,14 +908,13 @@ char *get_nvme_multipath_dev_hw_path(const char *dev_path)
 		return NULL;
 
 	for (ent = readdir(dir); ent; ent = readdir(dir)) {
-		char buf[strlen(dev_path) + strlen(ent->d_name) + 1];
 
 		/* Check if dir is a controller, ignore namespaces*/
 		if (!(strncmp(ent->d_name, "nvme", 4) == 0) ||
 		    (strrchr(ent->d_name, 'n') != &ent->d_name[0]))
 			continue;
 
-		sprintf(buf, "%s/%s", dev_path, ent->d_name);
+		snprintf(buf, PATH_MAX, "%s/%s", dev_path, ent->d_name);
 		rp = realpath(buf, NULL);
 		break;
 	}
-- 
2.41.0

Re: [PATCH 1/1] mdadm/platform-intel: Fix buffer overflow

[Paul Menzel]

Dear Xiao,


Thank you for your patch.

Am 28.05.24 um 04:29 schrieb Xiao Ni:
> It reports buffer overflow detected when creating raid with big
> nvme devices. In my test, the size of the nvme device is 1.5T.

I always like the error message and example command pasted, so chances 
are higher for affected people to find this in search engine.

> It can't reproduce this with nvme device which size is smaller

s/It/I/?

> than 1T.
> 
> In function get_nvme_multipath_dev_hw_path it allocs memory in a for
> loop and the size it allocs is big. So if the iteration number is
> large, it has a risk that the stack space is larger than the limit.
> So move the memory allocation at the biginning of the funtion.

… move … *to* the b*e*ginning of the fun*c*tion.

> Fixes: d835518b6b53 ('imsm: nvme multipath support')
> Reported-by: Guang Wu <guazhang@redhat.com>
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>   platform-intel.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/platform-intel.c b/platform-intel.c
> index 15a9fa5a..0732af2b 100644
> --- a/platform-intel.c
> +++ b/platform-intel.c
> @@ -898,6 +898,7 @@ char *get_nvme_multipath_dev_hw_path(const char *dev_path)
>   	DIR *dir;
>   	struct dirent *ent;
>   	char *rp = NULL;
> +	char buf[PATH_MAX];
>   
>   	if (strncmp(dev_path, NVME_SUBSYS_PATH, strlen(NVME_SUBSYS_PATH)) != 0)
>   		return NULL;
> @@ -907,14 +908,13 @@ char *get_nvme_multipath_dev_hw_path(const char *dev_path)
>   		return NULL;
>   
>   	for (ent = readdir(dir); ent; ent = readdir(dir)) {
> -		char buf[strlen(dev_path) + strlen(ent->d_name) + 1];
>   
>   		/* Check if dir is a controller, ignore namespaces*/
>   		if (!(strncmp(ent->d_name, "nvme", 4) == 0) ||
>   		    (strrchr(ent->d_name, 'n') != &ent->d_name[0]))
>   			continue;
>   
> -		sprintf(buf, "%s/%s", dev_path, ent->d_name);
> +		snprintf(buf, PATH_MAX, "%s/%s", dev_path, ent->d_name);
>   		rp = realpath(buf, NULL);
>   		break;
>   	}


Kind regards,

Paul

Re: [PATCH 1/1] mdadm/platform-intel: Fix buffer overflow

[Xiao Ni]

On Tue, May 28, 2024 at 1:10 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Xiao,
>
>
> Thank you for your patch.
>
> Am 28.05.24 um 04:29 schrieb Xiao Ni:
> > It reports buffer overflow detected when creating raid with big
> > nvme devices. In my test, the size of the nvme device is 1.5T.
>
> I always like the error message and example command pasted, so chances
> are higher for affected people to find this in search engine.

Hi Paul

Thanks for the suggestion.

mdadm -CR /dev/md0 -l1 -n2 /dev/nvme0n1 /dev/nvme2n1
*** buffer overflow detected ***: terminated
Aborted (core dumped)

nvme0n1                        259:3    0   1.5T  0 disk
nvme2n1                        259:5    0   1.5T  0 disk


>
> > It can't reproduce this with nvme device which size is smaller
>
> s/It/I/?

Thanks. I want to type "It can't be reproduced" :)
>
> > than 1T.
> >
> > In function get_nvme_multipath_dev_hw_path it allocs memory in a for
> > loop and the size it allocs is big. So if the iteration number is
> > large, it has a risk that the stack space is larger than the limit.
> > So move the memory allocation at the biginning of the funtion.
>
> … move … *to* the b*e*ginning of the fun*c*tion.

Thanks.

Regards
Xiao
>
> > Fixes: d835518b6b53 ('imsm: nvme multipath support')
> > Reported-by: Guang Wu <guazhang@redhat.com>
> > Signed-off-by: Xiao Ni <xni@redhat.com>
> > ---
> >   platform-intel.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/platform-intel.c b/platform-intel.c
> > index 15a9fa5a..0732af2b 100644
> > --- a/platform-intel.c
> > +++ b/platform-intel.c
> > @@ -898,6 +898,7 @@ char *get_nvme_multipath_dev_hw_path(const char *dev_path)
> >       DIR *dir;
> >       struct dirent *ent;
> >       char *rp = NULL;
> > +     char buf[PATH_MAX];
> >
> >       if (strncmp(dev_path, NVME_SUBSYS_PATH, strlen(NVME_SUBSYS_PATH)) != 0)
> >               return NULL;
> > @@ -907,14 +908,13 @@ char *get_nvme_multipath_dev_hw_path(const char *dev_path)
> >               return NULL;
> >
> >       for (ent = readdir(dir); ent; ent = readdir(dir)) {
> > -             char buf[strlen(dev_path) + strlen(ent->d_name) + 1];
> >
> >               /* Check if dir is a controller, ignore namespaces*/
> >               if (!(strncmp(ent->d_name, "nvme", 4) == 0) ||
> >                   (strrchr(ent->d_name, 'n') != &ent->d_name[0]))
> >                       continue;
> >
> > -             sprintf(buf, "%s/%s", dev_path, ent->d_name);
> > +             snprintf(buf, PATH_MAX, "%s/%s", dev_path, ent->d_name);
> >               rp = realpath(buf, NULL);
> >               break;
> >       }
>
>
> Kind regards,
>
> Paul
>

Re: [PATCH 1/1] mdadm/platform-intel: Fix buffer overflow

[Mariusz Tkaczyk]

On Tue, 28 May 2024 10:29:03 +0800
Xiao Ni <xni@redhat.com> wrote:

> It reports buffer overflow detected when creating raid with big
> nvme devices. In my test, the size of the nvme device is 1.5T.
> It can't reproduce this with nvme device which size is smaller
> than 1T.

Hi Xiao,

Size of disks should have nothing to do with this. We are just parsing sysfs.
Weird..

> 
> In function get_nvme_multipath_dev_hw_path it allocs memory in a for
> loop and the size it allocs is big. So if the iteration number is
> large, it has a risk that the stack space is larger than the limit.
> So move the memory allocation at the biginning of the funtion.

I would expect that memory is deallocated after each loop but the fix
is correct and I'm willing to take this because obviously it is a fix for
something.

I don't understand the problem but I trust you. Maybe varied size stack array is
a problem?

Probably, enough would be to just replace [strlen(dev_path) +
strlen(ent->d_name) + 1] by [PATH_MAX] but I'm quite confused why it is an
issue at all.

LGTM. Please fix typos raised by Paul and I will merge it.

Thanks,
Mariusz

> 
> Fixes: d835518b6b53 ('imsm: nvme multipath support')
> Reported-by: Guang Wu <guazhang@redhat.com>
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  platform-intel.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/platform-intel.c b/platform-intel.c
> index 15a9fa5a..0732af2b 100644
> --- a/platform-intel.c
> +++ b/platform-intel.c
> @@ -898,6 +898,7 @@ char *get_nvme_multipath_dev_hw_path(const char *dev_path)
>  	DIR *dir;
>  	struct dirent *ent;
>  	char *rp = NULL;
> +	char buf[PATH_MAX];
>  
>  	if (strncmp(dev_path, NVME_SUBSYS_PATH, strlen(NVME_SUBSYS_PATH)) !=
> 0) return NULL;
> @@ -907,14 +908,13 @@ char *get_nvme_multipath_dev_hw_path(const char
> *dev_path) return NULL;
>  
>  	for (ent = readdir(dir); ent; ent = readdir(dir)) {
> -		char buf[strlen(dev_path) + strlen(ent->d_name) + 1];
>  
>  		/* Check if dir is a controller, ignore namespaces*/
>  		if (!(strncmp(ent->d_name, "nvme", 4) == 0) ||
>  		    (strrchr(ent->d_name, 'n') != &ent->d_name[0]))
>  			continue;
>  
> -		sprintf(buf, "%s/%s", dev_path, ent->d_name);
> +		snprintf(buf, PATH_MAX, "%s/%s", dev_path, ent->d_name);

>  		rp = realpath(buf, NULL);
>  		break;
>  	}

Re: [PATCH 1/1] mdadm/platform-intel: Fix buffer overflow

[Xiao Ni]

On Tue, May 28, 2024 at 3:09 PM Mariusz Tkaczyk
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> On Tue, 28 May 2024 10:29:03 +0800
> Xiao Ni <xni@redhat.com> wrote:
>
> > It reports buffer overflow detected when creating raid with big
> > nvme devices. In my test, the size of the nvme device is 1.5T.
> > It can't reproduce this with nvme device which size is smaller
> > than 1T.
>
> Hi Xiao,
>
> Size of disks should have nothing to do with this. We are just parsing sysfs.
> Weird..

Maybe you're right. The test shows the size affect the result.
>
> >
> > In function get_nvme_multipath_dev_hw_path it allocs memory in a for
> > loop and the size it allocs is big. So if the iteration number is
> > large, it has a risk that the stack space is larger than the limit.
> > So move the memory allocation at the biginning of the funtion.
>
> I would expect that memory is deallocated after each loop but the fix
> is correct and I'm willing to take this because obviously it is a fix for
> something.

If the memory is deallocated after each loop. The comments in this
patch are not right.

>
> I don't understand the problem but I trust you. Maybe varied size stack array is
> a problem?

Let me explain more in detail. It's a strange problem. It only can
happen with the mdadm rpm package which is built already. If I install
src.rpm and build it locally, the problem can't happen. So I added
logs to narrow the problem. Finally, it prints the log in the loop and
doesn't print log when is before returning from this function. So the
memory allocation is suspicious and I tried with the patch and it can
fix this problem.

>
> Probably, enough would be to just replace [strlen(dev_path) +
> strlen(ent->d_name) + 1] by [PATH_MAX] but I'm quite confused why it is an
> issue at all.
>
> LGTM. Please fix typos raised by Paul and I will merge it.

Ok, I'll wait for a bit to give more time to talk about this problem.

Best Regards
Xiao
>
> Thanks,
> Mariusz
>
> >
> > Fixes: d835518b6b53 ('imsm: nvme multipath support')
> > Reported-by: Guang Wu <guazhang@redhat.com>
> > Signed-off-by: Xiao Ni <xni@redhat.com>
> > ---
> >  platform-intel.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/platform-intel.c b/platform-intel.c
> > index 15a9fa5a..0732af2b 100644
> > --- a/platform-intel.c
> > +++ b/platform-intel.c
> > @@ -898,6 +898,7 @@ char *get_nvme_multipath_dev_hw_path(const char *dev_path)
> >       DIR *dir;
> >       struct dirent *ent;
> >       char *rp = NULL;
> > +     char buf[PATH_MAX];
> >
> >       if (strncmp(dev_path, NVME_SUBSYS_PATH, strlen(NVME_SUBSYS_PATH)) !=
> > 0) return NULL;
> > @@ -907,14 +908,13 @@ char *get_nvme_multipath_dev_hw_path(const char
> > *dev_path) return NULL;
> >
> >       for (ent = readdir(dir); ent; ent = readdir(dir)) {
> > -             char buf[strlen(dev_path) + strlen(ent->d_name) + 1];
> >
> >               /* Check if dir is a controller, ignore namespaces*/
> >               if (!(strncmp(ent->d_name, "nvme", 4) == 0) ||
> >                   (strrchr(ent->d_name, 'n') != &ent->d_name[0]))
> >                       continue;
> >
> > -             sprintf(buf, "%s/%s", dev_path, ent->d_name);
> > +             snprintf(buf, PATH_MAX, "%s/%s", dev_path, ent->d_name);
>
> >               rp = realpath(buf, NULL);
> >               break;
> >       }
>

Re: [PATCH 1/1] mdadm/platform-intel: Fix buffer overflow

[Xiao Ni]

On Tue, May 28, 2024 at 3:41 PM Xiao Ni <xni@redhat.com> wrote:
>
> On Tue, May 28, 2024 at 3:09 PM Mariusz Tkaczyk
> <mariusz.tkaczyk@linux.intel.com> wrote:
> >
> > On Tue, 28 May 2024 10:29:03 +0800
> > Xiao Ni <xni@redhat.com> wrote:
> >
> > > It reports buffer overflow detected when creating raid with big
> > > nvme devices. In my test, the size of the nvme device is 1.5T.
> > > It can't reproduce this with nvme device which size is smaller
> > > than 1T.
> >
> > Hi Xiao,
> >
> > Size of disks should have nothing to do with this. We are just parsing sysfs.
> > Weird..
>
> Maybe you're right. The test shows the size affect the result.
> >
> > >
> > > In function get_nvme_multipath_dev_hw_path it allocs memory in a for
> > > loop and the size it allocs is big. So if the iteration number is
> > > large, it has a risk that the stack space is larger than the limit.
> > > So move the memory allocation at the biginning of the funtion.
> >
> > I would expect that memory is deallocated after each loop but the fix
> > is correct and I'm willing to take this because obviously it is a fix for
> > something.
>
> If the memory is deallocated after each loop. The comments in this
> patch are not right.

Hi all

After talking, I tried this way and it can fix the problem. So it
looks like it's caused by api sprintf. After switching sprintf with
snprintf, the problem can be fixed. It looks like it depends on the
glibc or something else related with building.

diff --git a/platform-intel.c b/platform-intel.c
index 15a9fa5a..0e083812 100644
--- a/platform-intel.c
+++ b/platform-intel.c
@@ -907,14 +907,15 @@ char *get_nvme_multipath_dev_hw_path(const char *dev_path)
                return NULL;

        for (ent = readdir(dir); ent; ent = readdir(dir)) {
-               char buf[strlen(dev_path) + strlen(ent->d_name) + 1];
+               int len = strlen(dev_path) + strlen(ent->d_name) + 1;
+               char buf[len];

                /* Check if dir is a controller, ignore namespaces*/
                if (!(strncmp(ent->d_name, "nvme", 4) == 0) ||
                    (strrchr(ent->d_name, 'n') != &ent->d_name[0]))
                        continue;

-               sprintf(buf, "%s/%s", dev_path, ent->d_name);
+               snprintf(buf, len, "%s/%s", dev_path, ent->d_name);
                rp = realpath(buf, NULL);
                break;
        }

Best Regards
Xiao
>
> >
> > I don't understand the problem but I trust you. Maybe varied size stack array is
> > a problem?
>
> Let me explain more in detail. It's a strange problem. It only can
> happen with the mdadm rpm package which is built already. If I install
> src.rpm and build it locally, the problem can't happen. So I added
> logs to narrow the problem. Finally, it prints the log in the loop and
> doesn't print log when is before returning from this function. So the
> memory allocation is suspicious and I tried with the patch and it can
> fix this problem.
>
> >
> > Probably, enough would be to just replace [strlen(dev_path) +
> > strlen(ent->d_name) + 1] by [PATH_MAX] but I'm quite confused why it is an
> > issue at all.
> >
> > LGTM. Please fix typos raised by Paul and I will merge it.
>
> Ok, I'll wait for a bit to give more time to talk about this problem.
>
> Best Regards
> Xiao
> >
> > Thanks,
> > Mariusz
> >
> > >
> > > Fixes: d835518b6b53 ('imsm: nvme multipath support')
> > > Reported-by: Guang Wu <guazhang@redhat.com>
> > > Signed-off-by: Xiao Ni <xni@redhat.com>
> > > ---
> > >  platform-intel.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/platform-intel.c b/platform-intel.c
> > > index 15a9fa5a..0732af2b 100644
> > > --- a/platform-intel.c
> > > +++ b/platform-intel.c
> > > @@ -898,6 +898,7 @@ char *get_nvme_multipath_dev_hw_path(const char *dev_path)
> > >       DIR *dir;
> > >       struct dirent *ent;
> > >       char *rp = NULL;
> > > +     char buf[PATH_MAX];
> > >
> > >       if (strncmp(dev_path, NVME_SUBSYS_PATH, strlen(NVME_SUBSYS_PATH)) !=
> > > 0) return NULL;
> > > @@ -907,14 +908,13 @@ char *get_nvme_multipath_dev_hw_path(const char
> > > *dev_path) return NULL;
> > >
> > >       for (ent = readdir(dir); ent; ent = readdir(dir)) {
> > > -             char buf[strlen(dev_path) + strlen(ent->d_name) + 1];
> > >
> > >               /* Check if dir is a controller, ignore namespaces*/
> > >               if (!(strncmp(ent->d_name, "nvme", 4) == 0) ||
> > >                   (strrchr(ent->d_name, 'n') != &ent->d_name[0]))
> > >                       continue;
> > >
> > > -             sprintf(buf, "%s/%s", dev_path, ent->d_name);
> > > +             snprintf(buf, PATH_MAX, "%s/%s", dev_path, ent->d_name);
> >
> > >               rp = realpath(buf, NULL);
> > >               break;
> > >       }
> >