linux-kernel - Re: [PATCH net-next v2] af_unix: Refine UNIX pathname 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: <20250206062217.94843-1-kuniyu@amazon.com>
Date: Thu, 6 Feb 2025 15:22:17 +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 v2] af_unix: Refine UNIX pathname sockets autobind identifier length

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

s/pathname/abstract/

In the v1 thread, I meant "filesystem-based sockets" is now called
pathname sockets.  sockets whose name starts with \0 are abstract
sockets.


From: Liang Jie <buaajxlj@....com>
Date: Thu,  6 Feb 2025 13:44:51 +0800
> Refines autobind identifier length for UNIX pathname sockets, addressing

same here, abstract sockets.


> 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

nit: 6 isn't used for mem alloc.


> setting the length of the autobind identifier, which was not only
> inflexible but also led to reduced code clarity. Additionally, allocating

you need not mention inflexibility as the length are fixed and won't be
changed (it was changed once though)


> 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.

s/pathname/abstract/

Overall, the commit message is a bit wordy.  It can be simplified just like

  unix_autobind() allocates 16 bytes but uses 6 bytes only.

  Let's allocate 6 bytes only and use snprintf() to avoid
  unwanted null-termination.


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

nit: indent is wrong here.

If you are using emacs, add the following to your config:

(setq-default c-default-style "linux")