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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250205082841.94701-1-kuniyu@amazon.com>
Date: Wed, 5 Feb 2025 17:28:41 +0900
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <buaajxlj@....com>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <horms@...nel.org>,
	<kuba@...nel.org>, <kuniyu@...zon.com>, <liangjie@...iang.com>,
	<linux-kernel@...r.kernel.org>, <mhal@...x.co>, <netdev@...r.kernel.org>,
	<pabeni@...hat.com>
Subject: Re: [PATCH net-next] af_unix: Refine UNIX domain sockets autobind identifier length

From: Liang Jie <buaajxlj@....com>
Date: Wed,  5 Feb 2025 14:06:53 +0800
> 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.

Now it's called pathname socket, but I think we don't need a comment here.
We already have enough comment/doc in other places and the man page.

$ man 7 unix
...
The address consists of a null byte followed by 5 bytes in the character set [0-9a-f].


> + * - The subsequent five bytes store a unique identifier for the autobinding process.
> + */
> +#define AUTOBIND_LEN 6

UNIX_AUTOBIND_LEN


> +
>  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);
>  	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);

I feel %05 is easier to read.  Note that man page mentions 5 bytes.

1 is also hard-coded here, but I don't think we should write

sprintf(addr->name->sun_path + UNIX_ABSTRACT_NAME_OFFSET,
        "%0*x", UNIX_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);
> -- 
> 2.25.1