| Message ID | 20240528022903.20039-1-xni@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] mdadm/platform-intel: Fix buffer overflow | expand |
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
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 >
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; > }
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; > > } >
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; > > > } > >
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; }
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(-)