netdev - Re: [PATCH net-next] af_unix: Refine UNIX domain sockets autobind identifier length

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250205181148.GK554665@kernel.org>
Date: Wed, 5 Feb 2025 18:11:48 +0000
From: Simon Horman <horms@...nel.org>
To: Liang Jie <buaajxlj@....com>
Cc: Kuniyuki Iwashima <kuniyu@...zon.com>, Jakub Kicinski <kuba@...nel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Michal Luczaj <mhal@...x.co>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, Liang Jie <liangjie@...iang.com>
Subject: Re: [PATCH net-next] af_unix: Refine UNIX domain sockets autobind
 identifier length

On Wed, Feb 05, 2025 at 02:06:53PM +0800, Liang Jie wrote:
> From: Liang Jie <liangjie@...iang.com>
> 
> Refines autobind identifier length for UNIX domain sockets, addressing
> issues of memory waste and code readability.
> 
> The previous implementation in the unix_autobind function of UNIX domain
> sockets used hardcoded values such as 16, 6, and 5 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 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
>    AUTOBIND_LEN, thereby preventing memory waste.
>  - The sprintf() function call is updated to dynamically format the
>    autobind identifier according to the defined length, further enhancing
>    code consistency and readability.
> 
> The modifications result in a leaner memory footprint and elevated code
> quality, ensuring that the functional aspect of autobind behavior in UNIX
> domain sockets remains intact.
> 
> Signed-off-by: Liang Jie <liangjie@...iang.com>
> ---
>  net/unix/af_unix.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 34945de1fb1f..5dcc55f2e3a1 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1186,6 +1186,13 @@ static struct sock *unix_find_other(struct net *net,
>  	return sk;
>  }
>  
> +/*
> + * Define the total length of the autobind identifier for UNIX domain sockets.
> + * - The first byte distinguishes abstract sockets from filesystem-based sockets.
> + * - The subsequent five bytes store a unique identifier for the autobinding process.
> + */
> +#define AUTOBIND_LEN 6
> +
>  static int unix_autobind(struct sock *sk)
>  {
>  	struct unix_sock *u = unix_sk(sk);
> @@ -1204,11 +1211,11 @@ static int unix_autobind(struct sock *sk)
>  
>  	err = -ENOMEM;
>  	addr = kzalloc(sizeof(*addr) +
> -		       offsetof(struct sockaddr_un, sun_path) + 16, GFP_KERNEL);
> +		       offsetof(struct sockaddr_un, sun_path) + AUTOBIND_LEN, GFP_KERNEL);

Hi Liang Jie,

1. While we are here, can we try to move this code to respect
   the preference for lines 80 columns wide or less in Networking code?

   e.g.

	addr = kzalloc(sizeof(*addr) + offsetof(struct sockaddr_un, sun_path) +
		       AUTOBIND_LEN, GFP_KERNEL);

2. More importantly, this allocates AUTOBIND_LEN bytes for sun_path.

   However, because the sprintf() will append a trailing '\0' it will
   write up to AUTOBIND_LEN (that is, AUTOBIND_LEN - 1 + 1) bytes
   at an offset of 1 to sun_path. IOW, the write may be one byte larger
   than the buffer with the '\0' overflowing.

   Flagged by W=1 build with gcc-14

>  	if (!addr)
>  		goto out;
>  
> -	addr->len = offsetof(struct sockaddr_un, sun_path) + 6;
> +	addr->len = offsetof(struct sockaddr_un, sun_path) + AUTOBIND_LEN;
>  	addr->name->sun_family = AF_UNIX;
>  	refcount_set(&addr->refcnt, 1);
>  
> @@ -1217,7 +1224,7 @@ static int unix_autobind(struct sock *sk)
>  	lastnum = ordernum & 0xFFFFF;
>  retry:
>  	ordernum = (ordernum + 1) & 0xFFFFF;
> -	sprintf(addr->name->sun_path + 1, "%05x", ordernum);
> +	sprintf(addr->name->sun_path + 1, "%0*x", AUTOBIND_LEN - 1, ordernum);
>  
>  	new_hash = unix_abstract_hash(addr->name, addr->len, sk->sk_type);
>  	unix_table_double_lock(net, old_hash, new_hash);

-- 
pw-bot: changes-requested