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

[David Laight]

On Wed,  5 Feb 2025 18:09:04 +0800
Liang Jie <buaajxlj@xxxxxxx> wrote:

> On Wed, 5 Feb 2025 17:28:41 +0900, Kuniyuki Iwashima <kuniyu@xxxxxxxxxx> wrote:
> > From: Liang Jie <buaajxlj@xxxxxxx>
> > Date: Wed,  5 Feb 2025 14:06:53 +0800  
> > > From: Liang Jie <liangjie@xxxxxxxxxxx>
> > > 
> > > 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@xxxxxxxxxxx>
> > > ---
> > >  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'.

Except that the compiler is very likely to bleat about sprintf() possibly
writing more than 5 hex digits.

By far the best thing to do is just make the kmalloc() 'a bit too long'
so that there is space for snprintf() to write the '\0'.

The kmalloc() size is rounded up anyway.
It is extremely unlikely that changing to 16 to 7 (or 6 as you are doing)
makes any difference to the amount of memory actually alloced.

OTOH the code size changes are real.

	David


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