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

[Kuniyuki Iwashima <kuniyu@amazon.com>]

From: Liang Jie <buaajxlj@163.com>
Date: Wed,  5 Feb 2025 18:09:04 +0800
> 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'.

Finally new hard-coded values are introduced..

I'm not sure this is worth saving just 10 bytes, which is not excessive,
vs extra 5 bytes memcpy(), so I'd rather not touch here.


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

An elegant option might be add a variant of snprintf without terminating
string by \0 ?