On Tue, May 28, 2024 at 3:09 PM Mariusz Tkaczyk
<mariusz.tkaczyk@xxxxxxxxxxxxxxx> wrote:
>
> On Tue, 28 May 2024 10:29:03 +0800
> Xiao Ni <xni@xxxxxxxxxx> 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@xxxxxxxxxx>
> > Signed-off-by: Xiao Ni <xni@xxxxxxxxxx>
> > ---
> > 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;
> > }
>
