| Message ID | 20250205060653.2221165-1-buaajxlj@163.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] af_unix: Refine UNIX domain sockets autobind identifier length | expand |
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
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
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);
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 ?
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
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
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 > >
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);