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

[Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>]

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;
>  	}