Re: [PATCH net-next v2] af_unix: Refine UNIX pathname sockets autobind identifier length

[Liang Jie]

Hi Kuniyuki,

The logs from 'netdev/build_allmodconfig_warn' is as follows:
  ../net/unix/af_unix.c: In function ‘unix_autobind’:
  ../net/unix/af_unix.c:1222:52: warning: ‘snprintf’ output truncated before the last format character [-Wformat-truncation=]
   1222 |         snprintf(addr->name->sun_path + 1, 5, "%05x", ordernum);
        |                                                    ^
  ../net/unix/af_unix.c:1222:9: note: ‘snprintf’ output 6 bytes into a destination of size 5
   1222 |         snprintf(addr->name->sun_path + 1, 5, "%05x", ordernum);
        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

snprintf() also append a trailing '\0' at the end of the sun_path.

Now, I think of three options. Which one do you think we should choose?

1. Allocate an additional byte during the kzalloc phase.
	addr = kzalloc(sizeof(*addr) + offsetof(struct sockaddr_un, sun_path) +
		       UNIX_AUTOBIND_LEN + 1, GFP_KERNEL);

2. Use temp buffer and memcpy() for handling.

3. Keep the current code as it is.

Do you have any other suggestions?

Best regards,
Liang

> From: Liang Jie <liangjie@xxxxxxxxxxx>
> 
> Refines autobind identifier length for UNIX pathname sockets, addressing
> issues of memory waste and code readability.
> 
> The previous implementation in the unix_autobind function of UNIX pathname
> sockets used hardcoded values such as 16 and 6 for memory allocation and
> setting the length of the autobind identifier, which was not only
> inflexible but also led to reduced code clarity. Additionally, allocating
> 16 bytes of memory for the autobind path was excessive, given that only 6
> bytes were ultimately used.
> 
> To mitigate these issues, introduces the following changes:
>  - A new macro UNIX_AUTOBIND_LEN is defined to clearly represent the total
>    length of the autobind identifier, which improves code readability and
>    maintainability. It is set to 6 bytes to accommodate the unique autobind
>    process identifier.
>  - Memory allocation for the autobind path is now precisely based on
>    UNIX_AUTOBIND_LEN, thereby preventing memory waste.
>  - To avoid buffer overflow and ensure that only the intended number of
>    bytes are written, sprintf is replaced by snprintf with the proper
>    buffer size set explicitly.
> 
> The modifications result in a leaner memory footprint and elevated code
> quality, ensuring that the functional aspect of autobind behavior in UNIX
> pathname sockets remains intact.
> 
> Signed-off-by: Liang Jie <liangjie@xxxxxxxxxxx>
> Suggested-by: Kuniyuki Iwashima <kuniyu@xxxxxxxxxx>
> ---
> 
> Changes in v2:
>  - Removed the comments describing AUTOBIND_LEN.
>  - Renamed the macro AUTOBIND_LEN to UNIX_AUTOBIND_LEN for clarity and
>    specificity.
>  - Corrected the buffer length in snprintf to prevent potential buffer
>    overflow issues.
>  - Addressed warning from checkpatch.
>  - Link to v1: https://lore.kernel.org/all/20250205060653.2221165-1-buaajxlj@xxxxxxx/
> 
>  net/unix/af_unix.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 34945de1fb1f..6c449f78f0a6 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1186,6 +1186,8 @@ static struct sock *unix_find_other(struct net *net,
>  	return sk;
>  }
>  
> +#define UNIX_AUTOBIND_LEN 6
> +
>  static int unix_autobind(struct sock *sk)
>  {
>  	struct unix_sock *u = unix_sk(sk);
> @@ -1203,12 +1205,12 @@ static int unix_autobind(struct sock *sk)
>  		goto out;
>  
>  	err = -ENOMEM;
> -	addr = kzalloc(sizeof(*addr) +
> -		       offsetof(struct sockaddr_un, sun_path) + 16, GFP_KERNEL);
> +	addr = kzalloc(sizeof(*addr) + offsetof(struct sockaddr_un, sun_path) +
> +			UNIX_AUTOBIND_LEN, GFP_KERNEL);
>  	if (!addr)
>  		goto out;
>  
> -	addr->len = offsetof(struct sockaddr_un, sun_path) + 6;
> +	addr->len = offsetof(struct sockaddr_un, sun_path) + UNIX_AUTOBIND_LEN;
>  	addr->name->sun_family = AF_UNIX;
>  	refcount_set(&addr->refcnt, 1);
>  
> @@ -1217,7 +1219,7 @@ static int unix_autobind(struct sock *sk)
>  	lastnum = ordernum & 0xFFFFF;
>  retry:
>  	ordernum = (ordernum + 1) & 0xFFFFF;
> -	sprintf(addr->name->sun_path + 1, "%05x", ordernum);
> +	snprintf(addr->name->sun_path + 1, 5, "%05x", ordernum);
>  
>  	new_hash = unix_abstract_hash(addr->name, addr->len, sk->sk_type);
>  	unix_table_double_lock(net, old_hash, new_hash);
> -- 
> 2.25.1