|SUCCESS| pw159223 [PATCH v5 12/17] eal: check for hugefile path overflow

[0-day Robot <robot@bytheb.org>]

Test-Label: 0day Robot - Claude SONNET
Test-Status: SUCCESS
http://dpdk.org/patch/159223

_AI Review Comments Available_

git sha: Not available
Author: Stephen Hemminger <stephen@networkplumber.org>
eal: check for hugefile path overflow

This commit adds overflow checking when constructing huge file paths to
prevent PATH_MAX buffer overruns.

> diff --git a/lib/eal/common/eal_filesystem.h b/lib/eal/common/eal_filesystem.h
> index 2de88d7cc2..90b1cbb484 100644
> --- a/lib/eal/common/eal_filesystem.h
> +++ b/lib/eal/common/eal_filesystem.h
> @@ -100,12 +100,14 @@ eal_hugepage_data_path(void)
> 
>  /** String format for hugepage map files. */
>  #define HUGEFILE_FMT "%s/%smap_%d"
> -static inline const char *
> +static inline __rte_warn_unused_result const char *
>  eal_get_hugefile_path(char *buffer, size_t buflen, const char *hugedir, int f_id)
>  {
> -	snprintf(buffer, buflen, HUGEFILE_FMT, hugedir,
> -			eal_get_hugefile_prefix(), f_id);
> -	return buffer;
> +	if (snprintf(buffer, buflen, HUGEFILE_FMT, hugedir,
> +		     eal_get_hugefile_prefix(), f_id) >= (int)buflen)
> +		return NULL;
> +	else
> +		return buffer;
>  }

The buffer parameter is always modified by snprintf even on overflow.
Will calling code incorrectly use a truncated path when this function
returns NULL?

[ ... ]

> diff --git a/lib/eal/linux/eal_hugepage_info.c b/lib/eal/linux/eal_hugepage_info.c
> index fe3351259e..cddffb9efb 100644
> --- a/lib/eal/linux/eal_hugepage_info.c
> +++ b/lib/eal/linux/eal_hugepage_info.c
> @@ -68,11 +68,14 @@ create_shared_memory(const char *filename, const size_t mem_size)
> 
>  static int get_hp_sysfs_value(const char *subdir, const char *file, unsigned long *val)
>  {
> -	char path[PATH_MAX];
> +	char *path = NULL;
> +	int ret;
> 
> -	snprintf(path, sizeof(path), "%s/%s/%s",
> -			sys_dir_path, subdir, file);
> -	return eal_parse_sysfs_value(path, val);
> +	if (asprintf(&path, "%s/%s/%s", sys_dir_path, subdir, file) < 0)
> +		return -1;
> +	ret = eal_parse_sysfs_value(path, val);
> +	free(path);
> +	return ret;
>  }

[ ... ]

> @@ -133,13 +136,15 @@ get_num_hugepages(const char *subdir, size_t sz, unsigned int reusable_pages)
>  static uint32_t
>  get_num_hugepages_on_node(const char *subdir, unsigned int socket, size_t sz)
>  {
> -	char path[PATH_MAX], socketpath[PATH_MAX];
> +	char *path = NULL, *socketpath = NULL;
>  	DIR *socketdir;
>  	unsigned long num_pages = 0;
>  	const char *nr_hp_file = "free_hugepages";
> 
> -	snprintf(socketpath, sizeof(socketpath), "%s/node%u/hugepages",
> -		sys_pages_numa_dir_path, socket);
> +	if (asprintf(&socketpath, "%s/node%u/hugepages", sys_pages_numa_dir_path, socket) < 0) {
> +		EAL_LOG(ERR, "Can not format node huge page path");
> +		goto nopages;
> +	}

[ ... ]

> @@ -147,17 +152,16 @@ get_num_hugepages_on_node(const char *subdir, unsigned int socket, size_t sz)
>  		closedir(socketdir);
>  	} else {
>  		/* Can't find socket dir, so ignore it */
> -		return 0;
> +		goto nopages;
>  	}
> 
> -	if (snprintf(path, sizeof(path), "%s/%s/%s", socketpath, subdir, nr_hp_file) >= PATH_MAX) {
> -		EAL_LOG(NOTICE, "Socket path %s/%s/%s is truncated",
> -			socketpath, subdir, nr_hp_file);
> -		return 0;
> +	if (asprintf(&path, "%s/%s/%s", socketpath, subdir, nr_hp_file) < 0) {
> +		EAL_LOG(ERR, "Can not format free hugepages path");
> +		goto nopages;
>  	}
> 
>  	if (eal_parse_sysfs_value(path, &num_pages) < 0)
> -		return 0;
> +		goto nopages;

Does this change affect the original behavior? Previously, the function
would return 0 after logging if the path was valid but file parsing
failed. Now it goes to nopages which also returns 0 but cleans up memory.

[ ... ]

> @@ -204,7 +212,7 @@ get_hugepage_dir(uint64_t hugepage_sz, char *hugedir, int len)
>  	const char proc_mounts[] = "/proc/mounts";
>  	const char pagesize_opt[] = "pagesize=";
>  	const size_t pagesize_opt_len = sizeof(pagesize_opt) - 1;
> -	char found[PATH_MAX] = "";
> +	const char *found = NULL;

[ ... ]

> @@ -247,7 +255,7 @@ get_hugepage_dir(uint64_t hugepage_sz, char *hugedir, int len)
>  		 * If no --huge-dir option has been given, we're done.
>  		 */
>  		if (internal_conf->hugepage_dir == NULL) {
> -			strlcpy(found, entry.mnt_dir, len);
> +			found = entry.mnt_dir;
>  			break;
>  		}

[ ... ]

> @@ -267,13 +275,13 @@ get_hugepage_dir(uint64_t hugepage_sz, char *hugedir, int len)
>  		 * We found a match, but only prefer it if it's a longer match
>  		 * (so /mnt/1 is preferred over /mnt for matching /mnt/1/2)).
>  		 */
> -		if (mountpt_len > strlen(found))
> -			strlcpy(found, entry.mnt_dir, len);
> +		if (found == NULL || mountpt_len > strlen(found))
> +			found = strdupa(entry.mnt_dir);

Does this usage of strdupa create a stack memory management issue? The
strdupa allocation will be freed when get_hugepage_dir returns, but
found is only used within this function scope, so this appears safe.

[ ... ]

> diff --git a/lib/eal/linux/eal_memalloc.c b/lib/eal/linux/eal_memalloc.c
> index 1e60e21620..d9e8ea76b9 100644
> --- a/lib/eal/linux/eal_memalloc.c
> +++ b/lib/eal/linux/eal_memalloc.c
> @@ -276,12 +277,18 @@ get_seg_fd(char *path, int buflen, struct hugepage_info *hi,
> 
>  	if (internal_conf->single_file_segments) {
>  		out_fd = &fd_list[list_idx].memseg_list_fd;
> -		eal_get_hugefile_path(path, buflen, hi->hugedir, list_idx);
> +		huge_path = eal_get_hugefile_path(path, buflen, hi->hugedir, list_idx);
>  	} else {
>  		out_fd = &fd_list[list_idx].fds[seg_idx];
> -		eal_get_hugefile_path(path, buflen, hi->hugedir,
> +		huge_path = eal_get_hugefile_path(path, buflen, hi->hugedir,
>  				list_idx * RTE_MAX_MEMSEG_PER_LIST + seg_idx);
>  	}
> +	if (huge_path == NULL) {
> +		EAL_LOG(DEBUG, "%s(): hugefile path truncated: '%s'",
> +			__func__, path);
> +		return -1;
> +	}

When eal_get_hugefile_path returns NULL due to overflow, is the path
buffer in a defined state for logging purposes?

[ ... ]