Message-Id: <20250205100904.2534565-1-buaajxlj@163.com>
Date: Wed,  5 Feb 2025 18:09:04 +0800
From: Liang Jie <buaajxlj@....com>
To: kuniyu@...zon.com
Cc: buaajxlj@....com,
	davem@...emloft.net,
	edumazet@...gle.com,
	horms@...nel.org,
	kuba@...nel.org,
	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

On Wed, 5 Feb 2025 17:28:41 +0900, Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
> 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)
> 

Hi Kuniyuki,

Thank you very much for your suggestions. I will incorporate them and
submit [PATCH v2] accordingly.

The logs from 'netdev/build_allmodconfig_warn' indicate that the patch has
given rise to the following warning:

 - ../net/unix/af_unix.c: In function ‘unix_autobind’:
 - ../net/unix/af_unix.c:1227:48: warning: ‘sprintf’ writing a terminating nul past the end of the destination [-Wformat-overflow=]
 -  1227 |         sprintf(addr->name->sun_path + 1, "%0*x", AUTOBIND_LEN - 1, ordernum);
 -       |                                                ^
 - ../net/unix/af_unix.c:1227:9: note: ‘sprintf’ output 6 bytes into a destination of size 5
 -  1227 |         sprintf(addr->name->sun_path + 1, "%0*x", AUTOBIND_LEN - 1, ordernum);
 -       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

It appears that the 'sprintf' call attempts to write a terminating null
byte past the end of the 'sun_path' array, potentially causing an overflow.

To address this issue, I am considering the following approach:

	char orderstring[6];

	sprintf(orderstring, "%05x", ordernum);
	memcpy(addr->name->sun_path + 1, orderstring, 5);

This would prevent the buffer overflow by using 'memcpy' to safely copy the
formatted string into 'sun_path'.

Before proceeding with a patch submission, I wanted to consult with you to
see if you have any suggestions for a better or more elegant solution to
this problem.

Thank you for your time and assistance. I look forward to your guidance on
this matter.

Best regards,
Liang Jie

> 
> >  
> >  	new_hash = unix_abstract_hash(addr->name, addr->len, sk->sk_type);
> >  	unix_table_double_lock(net, old_hash, new_hash);
> > -- 
> > 2.25.1

Powered by blists - more mailing lists