|SUCCESS| pw159222 [PATCH v5 11/17] eal: limit maximum runtime directory and socket paths

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

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

_AI Review Comments Available_

git sha: Not provided in patch
Author: Stephen Hemminger <stephen@networkplumber.org>
Subject: eal: limit maximum runtime directory and socket paths

This commit changes the buffer sizes from PATH_MAX to UNIX_PATH_MAX to
prevent unix domain socket path truncation issues, and adds proper bounds
checking in path construction functions.

> diff --git a/lib/eal/common/eal_common_config.c b/lib/eal/common/eal_common_config.c
> index 7fc7611a07..e2e69a75fb 100644
> --- a/lib/eal/common/eal_common_config.c
> +++ b/lib/eal/common/eal_common_config.c
> @@ -6,6 +6,7 @@
>  
>  #include <eal_export.h>
>  #include "eal_private.h"
> +#include "eal_filesystem.h"
>  #include "eal_memcfg.h"
>  
>  /* early configuration structure, when memory config is not mmapped */
> @@ -24,7 +25,7 @@ static struct rte_config rte_config = {
>  };
>  
>  /* platform-specific runtime dir */
> -static char runtime_dir[PATH_MAX];
> +static char runtime_dir[UNIX_PATH_MAX];
>  
>  /* internal configuration */
>  static struct internal_config internal_config;
> @@ -39,7 +40,8 @@ rte_eal_get_runtime_dir(void)
>  int
>  eal_set_runtime_dir(const char *run_dir)
>  {
> -	if (strlcpy(runtime_dir, run_dir, PATH_MAX) >= PATH_MAX) {
> +	/* runtime directory limited by maximum allowable unix domain socket */
> +	if (strlcpy(runtime_dir, run_dir, UNIX_PATH_MAX) >= UNIX_PATH_MAX) {
>  		EAL_LOG(ERR, "Runtime directory string too long");
>  		return -1;
>  	}

[ ... ]

> diff --git a/lib/eal/common/eal_common_proc.c b/lib/eal/common/eal_common_proc.c
> index 62fd4ba88f..dbf749c5b8 100644
> --- a/lib/eal/common/eal_common_proc.c
> +++ b/lib/eal/common/eal_common_proc.c
> @@ -36,10 +36,10 @@
>  
>  static RTE_ATOMIC(int) mp_fd = -1;
>  static rte_thread_t mp_handle_tid;
> -static char mp_filter[PATH_MAX];   /* Filter for secondary process sockets */
> -static char mp_dir_path[PATH_MAX]; /* The directory path for all mp sockets */
> +static char mp_filter[UNIX_PATH_MAX];   /* Filter for secondary process sockets */
> +static char mp_dir_path[UNIX_PATH_MAX]; /* The directory path for all mp sockets */
>  static pthread_mutex_t mp_mutex_action = PTHREAD_MUTEX_INITIALIZER;
> -static char peer_name[PATH_MAX];
> +static char peer_name[UNIX_PATH_MAX];

[ ... ]

> -static void
> -create_socket_path(const char *name, char *buf, int len)
> +static int
> +create_socket_path(const char *name, char *buf, size_t len)
>  {
>  	const char *prefix = eal_mp_socket_path();
>  
> -	if (strlen(name) > 0)
> -		snprintf(buf, len, "%s_%s", prefix, name);
> -	else
> -		strlcpy(buf, prefix, len);
> +	if (strlen(name) > 0) {
> +		if (snprintf(buf, len, "%s_%s", prefix, name) >= (int)len)
> +			return -1;
> +	} else {
> +		if (strlcpy(buf, prefix, len) >= len)
> +			return -1;
> +	}
> +	return 0;
>  }

Does the create_socket_path function properly validate that prefix is not
NULL before using it? The eal_mp_socket_path function returns a static
buffer, but there's no null check here.

[ ... ]

> @@ -572,6 +576,11 @@ open_socket_fd(void)
>  		snprintf(peer_name, sizeof(peer_name),
>  				"%d_%"PRIx64, getpid(), rte_rdtsc());
>  
> +	if (create_socket_path(peer_name, un.sun_path, sizeof(un.sun_path)) < 0) {
> +		EAL_LOG(ERR, "peer '%s' socket path too long", peer_name);
> +		return -1;
> +	}
> +
>  	mp_fd = socket(AF_UNIX, SOCK_DGRAM, 0);
>  	if (mp_fd < 0) {
>  		EAL_LOG(ERR, "failed to create unix socket");

Could this code leak the mp_fd if create_socket_path fails after the socket
is created? The error path returns -1 without calling close on mp_fd.

> @@ -581,8 +590,6 @@ open_socket_fd(void)
>  	memset(&un, 0, sizeof(un));
>  	un.sun_family = AF_UNIX;
>  
> -	create_socket_path(peer_name, un.sun_path, sizeof(un.sun_path));
> -
>  	unlink(un.sun_path); /* May still exist since last run */

[ ... ]

> @@ -599,17 +606,20 @@ open_socket_fd(void)
>  static void
>  close_socket_fd(int fd)
>  {
> -	char path[PATH_MAX];
> +	char path[UNIX_PATH_MAX];
>  
>  	close(fd);
> -	create_socket_path(peer_name, path, sizeof(path));
> -	unlink(path);
> +
> +	if (create_socket_path(peer_name, path, sizeof(path)) < 0)
> +		EAL_LOG(ERR, "file prefix path for peerr '%s' too long", peer_name);
> +	else
> +		unlink(path);
>  }

There's a typo in the error message: "peerr" should be "peer".

[ ... ]

> diff --git a/lib/eal/common/eal_filesystem.h b/lib/eal/common/eal_filesystem.h
> index 5d21f07c20..2de88d7cc2 100644
> --- a/lib/eal/common/eal_filesystem.h
> +++ b/lib/eal/common/eal_filesystem.h
> @@ -45,10 +45,21 @@ eal_runtime_config_path(void)
>  
>  /** Path of primary/secondary communication unix socket file. */
>  #define MP_SOCKET_FNAME "mp_socket"
> +
> +#ifdef RTE_EXEC_ENV_WINDOWS
> +#include <winsock2.h>
> +#include <afunix.h>
> +#else
> +#include <sys/un.h>
> +
> +/** Maximum length of unix domain socket path. */
> +#define UNIX_PATH_MAX (sizeof(((struct sockaddr_un *)0)->sun_path))
> +#endif
> +
>  static inline const char *
>  eal_mp_socket_path(void)
>  {
> -	static char buffer[PATH_MAX]; /* static so auto-zeroed */
> +	static char buffer[UNIX_PATH_MAX]; /* static so auto-zeroed */
>  
>  	snprintf(buffer, sizeof(buffer), "%s/%s", rte_eal_get_runtime_dir(),
>  			MP_SOCKET_FNAME);

What happens on Windows systems where UNIX_PATH_MAX isn't defined? The
code includes Windows headers but doesn't define UNIX_PATH_MAX for that
platform.