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

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

[Liang Jie]

From: Liang Jie <liangjie@lixiang.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@lixiang.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.
+ * - The subsequent five bytes store a unique identifier for the autobinding process.
+ */
+#define AUTOBIND_LEN 6
+
 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);
 
 	new_hash = unix_abstract_hash(addr->name, addr->len, sk->sk_type);
 	unix_table_double_lock(net, old_hash, new_hash);
-- 
2.25.1

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

[Kuniyuki Iwashima]

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

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

[Liang Jie]

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

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

[Kuniyuki Iwashima]

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 ?

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

[Liang Jie]

From: Kuniyuki Iwashima <kuniyu@amazon.com>,
Date: Thu, 6 Feb 2025 13:01:18 +0900
> > 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 ?

Thank you very much for your suggestions. It's an elegant solution that
avoids additional overhead and neatly solves the problem. I appreciate your
insight and will incorporate your idea into the updated patch.

Best regards,
Liang Jie

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@163.com> wrote:

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

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

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

[Simon Horman]

On Wed, Feb 05, 2025 at 02:06:53PM +0800, Liang Jie wrote:
> From: Liang Jie <liangjie@lixiang.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@lixiang.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.
> + * - The subsequent five bytes store a unique identifier for the autobinding process.
> + */
> +#define AUTOBIND_LEN 6
> +
>  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);

Hi Liang Jie,

1. While we are here, can we try to move this code to respect
   the preference for lines 80 columns wide or less in Networking code?

   e.g.

	addr = kzalloc(sizeof(*addr) + offsetof(struct sockaddr_un, sun_path) +
		       AUTOBIND_LEN, GFP_KERNEL);

2. More importantly, this allocates AUTOBIND_LEN bytes for sun_path.

   However, because the sprintf() will append a trailing '\0' it will
   write up to AUTOBIND_LEN (that is, AUTOBIND_LEN - 1 + 1) bytes
   at an offset of 1 to sun_path. IOW, the write may be one byte larger
   than the buffer with the '\0' overflowing.

   Flagged by W=1 build with gcc-14

>  	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);
>  
>  	new_hash = unix_abstract_hash(addr->name, addr->len, sk->sk_type);
>  	unix_table_double_lock(net, old_hash, new_hash);

-- 
pw-bot: changes-requested

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

[kernel test robot]

Hi Liang,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Liang-Jie/af_unix-Refine-UNIX-domain-sockets-autobind-identifier-length/20250205-141123
base:   net-next/main
patch link:    https://lore.kernel.org/r/20250205060653.2221165-1-buaajxlj%40163.com
patch subject: [PATCH net-next] af_unix: Refine UNIX domain sockets autobind identifier length
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20250206/202502061416.GZjhJTOs-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250206/202502061416.GZjhJTOs-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502061416.GZjhJTOs-lkp@intel.com/

All warnings (new ones prefixed by >>):

   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);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/sprintf +1227 net/unix/af_unix.c

  1195	
  1196	static int unix_autobind(struct sock *sk)
  1197	{
  1198		struct unix_sock *u = unix_sk(sk);
  1199		unsigned int new_hash, old_hash;
  1200		struct net *net = sock_net(sk);
  1201		struct unix_address *addr;
  1202		u32 lastnum, ordernum;
  1203		int err;
  1204	
  1205		err = mutex_lock_interruptible(&u->bindlock);
  1206		if (err)
  1207			return err;
  1208	
  1209		if (u->addr)
  1210			goto out;
  1211	
  1212		err = -ENOMEM;
  1213		addr = kzalloc(sizeof(*addr) +
  1214			       offsetof(struct sockaddr_un, sun_path) + AUTOBIND_LEN, GFP_KERNEL);
  1215		if (!addr)
  1216			goto out;
  1217	
  1218		addr->len = offsetof(struct sockaddr_un, sun_path) + AUTOBIND_LEN;
  1219		addr->name->sun_family = AF_UNIX;
  1220		refcount_set(&addr->refcnt, 1);
  1221	
  1222		old_hash = sk->sk_hash;
  1223		ordernum = get_random_u32();
  1224		lastnum = ordernum & 0xFFFFF;
  1225	retry:
  1226		ordernum = (ordernum + 1) & 0xFFFFF;
> 1227		sprintf(addr->name->sun_path + 1, "%0*x", AUTOBIND_LEN - 1, ordernum);
  1228	
  1229		new_hash = unix_abstract_hash(addr->name, addr->len, sk->sk_type);
  1230		unix_table_double_lock(net, old_hash, new_hash);
  1231	
  1232		if (__unix_find_socket_byname(net, addr->name, addr->len, new_hash)) {
  1233			unix_table_double_unlock(net, old_hash, new_hash);
  1234	
  1235			/* __unix_find_socket_byname() may take long time if many names
  1236			 * are already in use.
  1237			 */
  1238			cond_resched();
  1239	
  1240			if (ordernum == lastnum) {
  1241				/* Give up if all names seems to be in use. */
  1242				err = -ENOSPC;
  1243				unix_release_addr(addr);
  1244				goto out;
  1245			}
  1246	
  1247			goto retry;
  1248		}
  1249	
  1250		__unix_set_addr_hash(net, sk, addr, new_hash);
  1251		unix_table_double_unlock(net, old_hash, new_hash);
  1252		err = 0;
  1253	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki