* [PATCH v2 net 0/2] net: Fix error/warning by -fstrict-flex-arrays=3
@ 2023-07-20 0:44 Kuniyuki Iwashima
2023-07-20 0:44 ` [PATCH v2 net 1/2] af_unix: Fix fortify_panic() in unix_bind_bsd() Kuniyuki Iwashima
2023-07-20 0:44 ` [PATCH v2 net 2/2] af_packet: Fix warning of fortified memcpy() in packet_getname() Kuniyuki Iwashima
0 siblings, 2 replies; 8+ messages in thread
From: Kuniyuki Iwashima @ 2023-07-20 0:44 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: Willem de Bruijn, Kees Cook, Gustavo A. R. Silva, Breno Leitao,
Kuniyuki Iwashima, Kuniyuki Iwashima, netdev
df8fc4e934c1 ("kbuild: Enable -fstrict-flex-arrays=3") started applying
strict rules for standard string functions (strlen(), memcpy(), etc.) if
CONFIG_FORTIFY_SOURCE=y.
This series fixes two false positives caught by syzkaller.
Changes:
v2:
* Patch 2: Fix offset calc.
v1: https://lore.kernel.org/netdev/20230719185322.44255-1-kuniyu@amazon.com/
Kuniyuki Iwashima (2):
af_unix: Fix fortify_panic() in unix_bind_bsd().
af_packet: Fix warning of fortified memcpy() in packet_getname().
net/packet/af_packet.c | 7 ++++++-
net/unix/af_unix.c | 16 +++++++++++++---
2 files changed, 19 insertions(+), 4 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v2 net 1/2] af_unix: Fix fortify_panic() in unix_bind_bsd(). 2023-07-20 0:44 [PATCH v2 net 0/2] net: Fix error/warning by -fstrict-flex-arrays=3 Kuniyuki Iwashima @ 2023-07-20 0:44 ` Kuniyuki Iwashima 2023-07-20 14:39 ` Kees Cook 2023-07-20 0:44 ` [PATCH v2 net 2/2] af_packet: Fix warning of fortified memcpy() in packet_getname() Kuniyuki Iwashima 1 sibling, 1 reply; 8+ messages in thread From: Kuniyuki Iwashima @ 2023-07-20 0:44 UTC (permalink / raw) To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: Willem de Bruijn, Kees Cook, Gustavo A. R. Silva, Breno Leitao, Kuniyuki Iwashima, Kuniyuki Iwashima, netdev, syzkaller, Willem de Bruijn syzkaller found a bug in unix_bind_bsd() [0]. We can reproduce it by bind()ing a socket on a path with length 108. 108 is the size of sun_addr of struct sockaddr_un and is the maximum valid length for the pathname socket. When calling bind(), we use struct sockaddr_storage as the actual buffer size, so terminating sun_addr[108] with null is legitimate. However, strlen(sunaddr) for such a case causes fortify_panic() if CONFIG_FORTIFY_SOURCE=y. __fortify_strlen() has no idea about the actual buffer size and takes 108 as overflow, although 108 still fits in struct sockaddr_storage. Let's make __fortify_strlen() recognise the actual buffer size. [0]: detected buffer overflow in __fortify_strlen kernel BUG at lib/string_helpers.c:1031! Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP Modules linked in: CPU: 0 PID: 255 Comm: syz-executor296 Not tainted 6.5.0-rc1-00330-g60cc1f7d0605 #4 Hardware name: linux,dummy-virt (DT) pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : fortify_panic+0x1c/0x20 lib/string_helpers.c:1030 lr : fortify_panic+0x1c/0x20 lib/string_helpers.c:1030 sp : ffff800089817af0 x29: ffff800089817af0 x28: ffff800089817b40 x27: 1ffff00011302f68 x26: 000000000000006e x25: 0000000000000012 x24: ffff800087e60140 x23: dfff800000000000 x22: ffff800089817c20 x21: ffff800089817c8e x20: 000000000000006c x19: ffff00000c323900 x18: ffff800086ab1630 x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000001 x14: 1ffff00011302eb8 x13: 0000000000000000 x12: 0000000000000000 x11: 0000000000000000 x10: 0000000000000000 x9 : 64a26b65474d2a00 x8 : 64a26b65474d2a00 x7 : 0000000000000001 x6 : 0000000000000001 x5 : ffff800089817438 x4 : ffff800086ac99e0 x3 : ffff800080f19e8c x2 : 0000000000000001 x1 : 0000000100000000 x0 : 000000000000002c Call trace: fortify_panic+0x1c/0x20 lib/string_helpers.c:1030 _Z16__fortify_strlenPKcU25pass_dynamic_object_size1 include/linux/fortify-string.h:217 [inline] unix_bind_bsd net/unix/af_unix.c:1212 [inline] unix_bind+0xba8/0xc58 net/unix/af_unix.c:1326 __sys_bind+0x1ac/0x248 net/socket.c:1792 __do_sys_bind net/socket.c:1803 [inline] __se_sys_bind net/socket.c:1801 [inline] __arm64_sys_bind+0x7c/0x94 net/socket.c:1801 __invoke_syscall arch/arm64/kernel/syscall.c:38 [inline] invoke_syscall+0x98/0x2c0 arch/arm64/kernel/syscall.c:52 el0_svc_common+0x134/0x240 arch/arm64/kernel/syscall.c:139 do_el0_svc+0x64/0x198 arch/arm64/kernel/syscall.c:188 el0_svc+0x2c/0x7c arch/arm64/kernel/entry-common.c:647 el0t_64_sync_handler+0x84/0xfc arch/arm64/kernel/entry-common.c:665 el0t_64_sync+0x190/0x194 arch/arm64/kernel/entry.S:591 Code: aa0003e1 d0000e80 91030000 97ffc91a (d4210000) Fixes: df8fc4e934c1 ("kbuild: Enable -fstrict-flex-arrays=3") Reported-by: syzkaller <syzkaller@googlegroups•com> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon•com> Reviewed-by: Willem de Bruijn <willemb@google•com> --- net/unix/af_unix.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 123b35ddfd71..e1b1819b96d1 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -302,6 +302,18 @@ static void unix_mkname_bsd(struct sockaddr_un *sunaddr, int addr_len) ((char *)sunaddr)[addr_len] = 0; } +static int unix_strlen_bsd(struct sockaddr_un *sunaddr) +{ + /* Don't pass sunaddr->sun_path to strlen(). Otherwise, the + * max valid length UNIX_PATH_MAX (108) will cause panic if + * CONFIG_FORTIFY_SOURCE=y. Let __fortify_strlen() know that + * the actual buffer is struct sockaddr_storage and that 108 + * is within __data[]. See also: unix_mkname_bsd(). + */ + return strlen(((struct sockaddr_storage *)sunaddr)->__data) + + offsetof(struct sockaddr_un, sun_path) + 1; +} + static void __unix_remove_socket(struct sock *sk) { sk_del_node_init(sk); @@ -1209,9 +1221,7 @@ static int unix_bind_bsd(struct sock *sk, struct sockaddr_un *sunaddr, int err; unix_mkname_bsd(sunaddr, addr_len); - addr_len = strlen(sunaddr->sun_path) + - offsetof(struct sockaddr_un, sun_path) + 1; - + addr_len = unix_strlen_bsd(sunaddr); addr = unix_create_addr(sunaddr, addr_len); if (!addr) return -ENOMEM; -- 2.30.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 net 1/2] af_unix: Fix fortify_panic() in unix_bind_bsd(). 2023-07-20 0:44 ` [PATCH v2 net 1/2] af_unix: Fix fortify_panic() in unix_bind_bsd() Kuniyuki Iwashima @ 2023-07-20 14:39 ` Kees Cook 2023-07-20 17:09 ` Kuniyuki Iwashima 0 siblings, 1 reply; 8+ messages in thread From: Kees Cook @ 2023-07-20 14:39 UTC (permalink / raw) To: Kuniyuki Iwashima Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Willem de Bruijn, Gustavo A. R. Silva, Breno Leitao, Kuniyuki Iwashima, netdev, syzkaller, Willem de Bruijn On Wed, Jul 19, 2023 at 05:44:09PM -0700, Kuniyuki Iwashima wrote: > syzkaller found a bug in unix_bind_bsd() [0]. We can reproduce it > by bind()ing a socket on a path with length 108. > > 108 is the size of sun_addr of struct sockaddr_un and is the maximum > valid length for the pathname socket. When calling bind(), we use > struct sockaddr_storage as the actual buffer size, so terminating > sun_addr[108] with null is legitimate. > > However, strlen(sunaddr) for such a case causes fortify_panic() if > CONFIG_FORTIFY_SOURCE=y. __fortify_strlen() has no idea about the > actual buffer size and takes 108 as overflow, although 108 still > fits in struct sockaddr_storage. Oh, the max size is 108, but it's allowed to be unterminated? This seems to contradict the comment for unix_validate_addr() (which then doesn't validate this ...) Reading "max 7 unix" seems to clear this up and confirm that it doesn't need to be terminated. Bleh. Regardless, see below for a simpler solution, since this doesn't need to be arbitrarily long, just potentially unterminated. > Let's make __fortify_strlen() recognise the actual buffer size. > > [0]: > detected buffer overflow in __fortify_strlen > kernel BUG at lib/string_helpers.c:1031! > Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP > Modules linked in: > CPU: 0 PID: 255 Comm: syz-executor296 Not tainted 6.5.0-rc1-00330-g60cc1f7d0605 #4 > Hardware name: linux,dummy-virt (DT) > pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) > pc : fortify_panic+0x1c/0x20 lib/string_helpers.c:1030 > lr : fortify_panic+0x1c/0x20 lib/string_helpers.c:1030 > sp : ffff800089817af0 > x29: ffff800089817af0 x28: ffff800089817b40 x27: 1ffff00011302f68 > x26: 000000000000006e x25: 0000000000000012 x24: ffff800087e60140 > x23: dfff800000000000 x22: ffff800089817c20 x21: ffff800089817c8e > x20: 000000000000006c x19: ffff00000c323900 x18: ffff800086ab1630 > x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000001 > x14: 1ffff00011302eb8 x13: 0000000000000000 x12: 0000000000000000 > x11: 0000000000000000 x10: 0000000000000000 x9 : 64a26b65474d2a00 > x8 : 64a26b65474d2a00 x7 : 0000000000000001 x6 : 0000000000000001 > x5 : ffff800089817438 x4 : ffff800086ac99e0 x3 : ffff800080f19e8c > x2 : 0000000000000001 x1 : 0000000100000000 x0 : 000000000000002c > Call trace: > fortify_panic+0x1c/0x20 lib/string_helpers.c:1030 > _Z16__fortify_strlenPKcU25pass_dynamic_object_size1 include/linux/fortify-string.h:217 [inline] > unix_bind_bsd net/unix/af_unix.c:1212 [inline] > unix_bind+0xba8/0xc58 net/unix/af_unix.c:1326 > __sys_bind+0x1ac/0x248 net/socket.c:1792 > __do_sys_bind net/socket.c:1803 [inline] > __se_sys_bind net/socket.c:1801 [inline] > __arm64_sys_bind+0x7c/0x94 net/socket.c:1801 > __invoke_syscall arch/arm64/kernel/syscall.c:38 [inline] > invoke_syscall+0x98/0x2c0 arch/arm64/kernel/syscall.c:52 > el0_svc_common+0x134/0x240 arch/arm64/kernel/syscall.c:139 > do_el0_svc+0x64/0x198 arch/arm64/kernel/syscall.c:188 > el0_svc+0x2c/0x7c arch/arm64/kernel/entry-common.c:647 > el0t_64_sync_handler+0x84/0xfc arch/arm64/kernel/entry-common.c:665 > el0t_64_sync+0x190/0x194 arch/arm64/kernel/entry.S:591 > Code: aa0003e1 d0000e80 91030000 97ffc91a (d4210000) > > Fixes: df8fc4e934c1 ("kbuild: Enable -fstrict-flex-arrays=3") > Reported-by: syzkaller <syzkaller@googlegroups•com> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon•com> > Reviewed-by: Willem de Bruijn <willemb@google•com> > --- > net/unix/af_unix.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > index 123b35ddfd71..e1b1819b96d1 100644 > --- a/net/unix/af_unix.c > +++ b/net/unix/af_unix.c > @@ -302,6 +302,18 @@ static void unix_mkname_bsd(struct sockaddr_un *sunaddr, int addr_len) > ((char *)sunaddr)[addr_len] = 0; > } > > +static int unix_strlen_bsd(struct sockaddr_un *sunaddr) > +{ > + /* Don't pass sunaddr->sun_path to strlen(). Otherwise, the > + * max valid length UNIX_PATH_MAX (108) will cause panic if > + * CONFIG_FORTIFY_SOURCE=y. Let __fortify_strlen() know that > + * the actual buffer is struct sockaddr_storage and that 108 > + * is within __data[]. See also: unix_mkname_bsd(). > + */ > + return strlen(((struct sockaddr_storage *)sunaddr)->__data) + > + offsetof(struct sockaddr_un, sun_path) + 1; > +} > + > static void __unix_remove_socket(struct sock *sk) > { > sk_del_node_init(sk); > @@ -1209,9 +1221,7 @@ static int unix_bind_bsd(struct sock *sk, struct sockaddr_un *sunaddr, > int err; > > unix_mkname_bsd(sunaddr, addr_len); > - addr_len = strlen(sunaddr->sun_path) + Instead of a whole new function, I think this can just be: strnlen(sunaddr->sun_path, sizeof(sunaddr->sun_path)) + > - offsetof(struct sockaddr_un, sun_path) + 1; > - > + addr_len = unix_strlen_bsd(sunaddr); > addr = unix_create_addr(sunaddr, addr_len); > if (!addr) > return -ENOMEM; > -- > 2.30.2 > -- Kees Cook ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 net 1/2] af_unix: Fix fortify_panic() in unix_bind_bsd(). 2023-07-20 14:39 ` Kees Cook @ 2023-07-20 17:09 ` Kuniyuki Iwashima 0 siblings, 0 replies; 8+ messages in thread From: Kuniyuki Iwashima @ 2023-07-20 17:09 UTC (permalink / raw) To: keescook Cc: davem, edumazet, gustavoars, kuba, kuni1840, kuniyu, leitao, netdev, pabeni, syzkaller, willemb, willemdebruijn.kernel From: Kees Cook <keescook@chromium•org> Date: Thu, 20 Jul 2023 07:39:48 -0700 > On Wed, Jul 19, 2023 at 05:44:09PM -0700, Kuniyuki Iwashima wrote: > > syzkaller found a bug in unix_bind_bsd() [0]. We can reproduce it > > by bind()ing a socket on a path with length 108. > > > > 108 is the size of sun_addr of struct sockaddr_un and is the maximum > > valid length for the pathname socket. When calling bind(), we use > > struct sockaddr_storage as the actual buffer size, so terminating > > sun_addr[108] with null is legitimate. > > > > However, strlen(sunaddr) for such a case causes fortify_panic() if > > CONFIG_FORTIFY_SOURCE=y. __fortify_strlen() has no idea about the > > actual buffer size and takes 108 as overflow, although 108 still > > fits in struct sockaddr_storage. > > Oh, the max size is 108, but it's allowed to be unterminated? This seems > to contradict the comment for unix_validate_addr() (which then doesn't > validate this ...) Because we call it for the abstract socket too which starts with \0 and does not handle it specially. > Reading "max 7 unix" seems to clear this up and > confirm that it doesn't need to be terminated. Bleh. > > Regardless, see below for a simpler solution, since this doesn't need to > be arbitrarily long, just potentially unterminated. unix_mkname_bsd() terminates it. Technically, we need not call strlen() if we don't optimise the allocated string length. connect() does not need strlen() and just calls unix_mkname_bsd() in unix_find_bsd(). > > > Let's make __fortify_strlen() recognise the actual buffer size. > > > > [0]: > > detected buffer overflow in __fortify_strlen > > kernel BUG at lib/string_helpers.c:1031! > > Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP > > Modules linked in: > > CPU: 0 PID: 255 Comm: syz-executor296 Not tainted 6.5.0-rc1-00330-g60cc1f7d0605 #4 > > Hardware name: linux,dummy-virt (DT) > > pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) > > pc : fortify_panic+0x1c/0x20 lib/string_helpers.c:1030 > > lr : fortify_panic+0x1c/0x20 lib/string_helpers.c:1030 > > sp : ffff800089817af0 > > x29: ffff800089817af0 x28: ffff800089817b40 x27: 1ffff00011302f68 > > x26: 000000000000006e x25: 0000000000000012 x24: ffff800087e60140 > > x23: dfff800000000000 x22: ffff800089817c20 x21: ffff800089817c8e > > x20: 000000000000006c x19: ffff00000c323900 x18: ffff800086ab1630 > > x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000001 > > x14: 1ffff00011302eb8 x13: 0000000000000000 x12: 0000000000000000 > > x11: 0000000000000000 x10: 0000000000000000 x9 : 64a26b65474d2a00 > > x8 : 64a26b65474d2a00 x7 : 0000000000000001 x6 : 0000000000000001 > > x5 : ffff800089817438 x4 : ffff800086ac99e0 x3 : ffff800080f19e8c > > x2 : 0000000000000001 x1 : 0000000100000000 x0 : 000000000000002c > > Call trace: > > fortify_panic+0x1c/0x20 lib/string_helpers.c:1030 > > _Z16__fortify_strlenPKcU25pass_dynamic_object_size1 include/linux/fortify-string.h:217 [inline] > > unix_bind_bsd net/unix/af_unix.c:1212 [inline] > > unix_bind+0xba8/0xc58 net/unix/af_unix.c:1326 > > __sys_bind+0x1ac/0x248 net/socket.c:1792 > > __do_sys_bind net/socket.c:1803 [inline] > > __se_sys_bind net/socket.c:1801 [inline] > > __arm64_sys_bind+0x7c/0x94 net/socket.c:1801 > > __invoke_syscall arch/arm64/kernel/syscall.c:38 [inline] > > invoke_syscall+0x98/0x2c0 arch/arm64/kernel/syscall.c:52 > > el0_svc_common+0x134/0x240 arch/arm64/kernel/syscall.c:139 > > do_el0_svc+0x64/0x198 arch/arm64/kernel/syscall.c:188 > > el0_svc+0x2c/0x7c arch/arm64/kernel/entry-common.c:647 > > el0t_64_sync_handler+0x84/0xfc arch/arm64/kernel/entry-common.c:665 > > el0t_64_sync+0x190/0x194 arch/arm64/kernel/entry.S:591 > > Code: aa0003e1 d0000e80 91030000 97ffc91a (d4210000) > > > > Fixes: df8fc4e934c1 ("kbuild: Enable -fstrict-flex-arrays=3") > > Reported-by: syzkaller <syzkaller@googlegroups•com> > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon•com> > > Reviewed-by: Willem de Bruijn <willemb@google•com> > > --- > > net/unix/af_unix.c | 16 +++++++++++++--- > > 1 file changed, 13 insertions(+), 3 deletions(-) > > > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > > index 123b35ddfd71..e1b1819b96d1 100644 > > --- a/net/unix/af_unix.c > > +++ b/net/unix/af_unix.c > > @@ -302,6 +302,18 @@ static void unix_mkname_bsd(struct sockaddr_un *sunaddr, int addr_len) > > ((char *)sunaddr)[addr_len] = 0; > > } > > > > +static int unix_strlen_bsd(struct sockaddr_un *sunaddr) > > +{ > > + /* Don't pass sunaddr->sun_path to strlen(). Otherwise, the > > + * max valid length UNIX_PATH_MAX (108) will cause panic if > > + * CONFIG_FORTIFY_SOURCE=y. Let __fortify_strlen() know that > > + * the actual buffer is struct sockaddr_storage and that 108 > > + * is within __data[]. See also: unix_mkname_bsd(). > > + */ > > + return strlen(((struct sockaddr_storage *)sunaddr)->__data) + > > + offsetof(struct sockaddr_un, sun_path) + 1; > > +} > > + > > static void __unix_remove_socket(struct sock *sk) > > { > > sk_del_node_init(sk); > > @@ -1209,9 +1221,7 @@ static int unix_bind_bsd(struct sock *sk, struct sockaddr_un *sunaddr, > > int err; > > > > unix_mkname_bsd(sunaddr, addr_len); > > - addr_len = strlen(sunaddr->sun_path) + > > Instead of a whole new function, I think this can just be: > > strnlen(sunaddr->sun_path, sizeof(sunaddr->sun_path)) + > > > - offsetof(struct sockaddr_un, sun_path) + 1; > > - > > + addr_len = unix_strlen_bsd(sunaddr); > > addr = unix_create_addr(sunaddr, addr_len); > > if (!addr) > > return -ENOMEM; > > -- > > 2.30.2 > > > > -- > Kees Cook ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 net 2/2] af_packet: Fix warning of fortified memcpy() in packet_getname(). 2023-07-20 0:44 [PATCH v2 net 0/2] net: Fix error/warning by -fstrict-flex-arrays=3 Kuniyuki Iwashima 2023-07-20 0:44 ` [PATCH v2 net 1/2] af_unix: Fix fortify_panic() in unix_bind_bsd() Kuniyuki Iwashima @ 2023-07-20 0:44 ` Kuniyuki Iwashima 2023-07-20 14:06 ` Willem de Bruijn 2023-07-20 15:04 ` Kees Cook 1 sibling, 2 replies; 8+ messages in thread From: Kuniyuki Iwashima @ 2023-07-20 0:44 UTC (permalink / raw) To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: Willem de Bruijn, Kees Cook, Gustavo A. R. Silva, Breno Leitao, Kuniyuki Iwashima, Kuniyuki Iwashima, netdev, syzkaller syzkaller found a warning in packet_getname() [0], where we try to copy 16 bytes to sockaddr_ll.sll_addr[8]. Some devices (ip6gre, vti6, ip6tnl) have 16 bytes address expressed by struct in6_addr. Also, Infiniband has 32 bytes as MAX_ADDR_LEN. The write seems to overflow, but actually not since we use struct sockaddr_storage defined in __sys_getsockname() and its size is 128 (_K_SS_MAXSIZE) bytes. Thus, we have sufficient room after sll_addr[] as __data[]. To avoid the warning, we need to let __fortify_memcpy_chk() know the actual buffer size. Another option would be to use strncpy() and limit the copied length to sizeof(sll_addr), but it will return the partial address and break an application that passes sockaddr_storage to getsockname(). [0]: memcpy: detected field-spanning write (size 16) of single field "sll->sll_addr" at net/packet/af_packet.c:3604 (size 8) WARNING: CPU: 0 PID: 255 at net/packet/af_packet.c:3604 packet_getname+0x25c/0x3a0 net/packet/af_packet.c:3604 Modules linked in: CPU: 0 PID: 255 Comm: syz-executor750 Not tainted 6.5.0-rc1-00330-g60cc1f7d0605 #4 Hardware name: linux,dummy-virt (DT) pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : packet_getname+0x25c/0x3a0 net/packet/af_packet.c:3604 lr : packet_getname+0x25c/0x3a0 net/packet/af_packet.c:3604 sp : ffff800089887bc0 x29: ffff800089887bc0 x28: ffff000010f80f80 x27: 0000000000000003 x26: dfff800000000000 x25: ffff700011310f80 x24: ffff800087d55000 x23: dfff800000000000 x22: ffff800089887c2c x21: 0000000000000010 x20: ffff00000de08310 x19: ffff800089887c20 x18: ffff800086ab1630 x17: 20646c6569662065 x16: 6c676e697320666f x15: 0000000000000001 x14: 1fffe0000d56d7ca x13: 0000000000000000 x12: 0000000000000000 x11: 0000000000000000 x10: 0000000000000000 x9 : 3e60944c3da92b00 x8 : 3e60944c3da92b00 x7 : 0000000000000001 x6 : 0000000000000001 x5 : ffff8000898874f8 x4 : ffff800086ac99e0 x3 : ffff8000803f8808 x2 : 0000000000000001 x1 : 0000000100000000 x0 : 0000000000000000 Call trace: packet_getname+0x25c/0x3a0 net/packet/af_packet.c:3604 __sys_getsockname+0x168/0x24c net/socket.c:2042 __do_sys_getsockname net/socket.c:2057 [inline] __se_sys_getsockname net/socket.c:2054 [inline] __arm64_sys_getsockname+0x7c/0x94 net/socket.c:2054 __invoke_syscall arch/arm64/kernel/syscall.c:38 [inline] invoke_syscall+0x98/0x2c0 arch/arm64/kernel/syscall.c:52 el0_svc_common+0x134/0x240 arch/arm64/kernel/syscall.c:139 do_el0_svc+0x64/0x198 arch/arm64/kernel/syscall.c:188 el0_svc+0x2c/0x7c arch/arm64/kernel/entry-common.c:647 el0t_64_sync_handler+0x84/0xfc arch/arm64/kernel/entry-common.c:665 el0t_64_sync+0x190/0x194 arch/arm64/kernel/entry.S:591 Fixes: df8fc4e934c1 ("kbuild: Enable -fstrict-flex-arrays=3") Reported-by: syzkaller <syzkaller@googlegroups•com> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon•com> --- net/packet/af_packet.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 85ff90a03b0c..7f60b78f170f 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -3601,7 +3601,12 @@ static int packet_getname(struct socket *sock, struct sockaddr *uaddr, if (dev) { sll->sll_hatype = dev->type; sll->sll_halen = dev->addr_len; - memcpy(sll->sll_addr, dev->dev_addr, dev->addr_len); + + /* Let __fortify_memcpy_chk() know the actual buffer size. */ + memcpy(((struct sockaddr_storage *)sll)->__data + + offsetof(struct sockaddr_ll, sll_addr) - + offsetof(struct sockaddr_ll, sll_protocol), + dev->dev_addr, dev->addr_len); } else { sll->sll_hatype = 0; /* Bad: we have no ARPHRD_UNSPEC */ sll->sll_halen = 0; -- 2.30.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* RE: [PATCH v2 net 2/2] af_packet: Fix warning of fortified memcpy() in packet_getname(). 2023-07-20 0:44 ` [PATCH v2 net 2/2] af_packet: Fix warning of fortified memcpy() in packet_getname() Kuniyuki Iwashima @ 2023-07-20 14:06 ` Willem de Bruijn 2023-07-20 15:04 ` Kees Cook 1 sibling, 0 replies; 8+ messages in thread From: Willem de Bruijn @ 2023-07-20 14:06 UTC (permalink / raw) To: Kuniyuki Iwashima, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: Willem de Bruijn, Kees Cook, Gustavo A. R. Silva, Breno Leitao, Kuniyuki Iwashima, Kuniyuki Iwashima, netdev, syzkaller Kuniyuki Iwashima wrote: > syzkaller found a warning in packet_getname() [0], where we try to > copy 16 bytes to sockaddr_ll.sll_addr[8]. > > Some devices (ip6gre, vti6, ip6tnl) have 16 bytes address expressed > by struct in6_addr. Also, Infiniband has 32 bytes as MAX_ADDR_LEN. > > The write seems to overflow, but actually not since we use struct > sockaddr_storage defined in __sys_getsockname() and its size is 128 > (_K_SS_MAXSIZE) bytes. Thus, we have sufficient room after sll_addr[] > as __data[]. > > To avoid the warning, we need to let __fortify_memcpy_chk() know the > actual buffer size. > > Another option would be to use strncpy() and limit the copied length > to sizeof(sll_addr), but it will return the partial address and break > an application that passes sockaddr_storage to getsockname(). > > [0]: > memcpy: detected field-spanning write (size 16) of single field "sll->sll_addr" at net/packet/af_packet.c:3604 (size 8) > WARNING: CPU: 0 PID: 255 at net/packet/af_packet.c:3604 packet_getname+0x25c/0x3a0 net/packet/af_packet.c:3604 > Modules linked in: > CPU: 0 PID: 255 Comm: syz-executor750 Not tainted 6.5.0-rc1-00330-g60cc1f7d0605 #4 > Hardware name: linux,dummy-virt (DT) > pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) > pc : packet_getname+0x25c/0x3a0 net/packet/af_packet.c:3604 > lr : packet_getname+0x25c/0x3a0 net/packet/af_packet.c:3604 > sp : ffff800089887bc0 > x29: ffff800089887bc0 x28: ffff000010f80f80 x27: 0000000000000003 > x26: dfff800000000000 x25: ffff700011310f80 x24: ffff800087d55000 > x23: dfff800000000000 x22: ffff800089887c2c x21: 0000000000000010 > x20: ffff00000de08310 x19: ffff800089887c20 x18: ffff800086ab1630 > x17: 20646c6569662065 x16: 6c676e697320666f x15: 0000000000000001 > x14: 1fffe0000d56d7ca x13: 0000000000000000 x12: 0000000000000000 > x11: 0000000000000000 x10: 0000000000000000 x9 : 3e60944c3da92b00 > x8 : 3e60944c3da92b00 x7 : 0000000000000001 x6 : 0000000000000001 > x5 : ffff8000898874f8 x4 : ffff800086ac99e0 x3 : ffff8000803f8808 > x2 : 0000000000000001 x1 : 0000000100000000 x0 : 0000000000000000 > Call trace: > packet_getname+0x25c/0x3a0 net/packet/af_packet.c:3604 > __sys_getsockname+0x168/0x24c net/socket.c:2042 > __do_sys_getsockname net/socket.c:2057 [inline] > __se_sys_getsockname net/socket.c:2054 [inline] > __arm64_sys_getsockname+0x7c/0x94 net/socket.c:2054 > __invoke_syscall arch/arm64/kernel/syscall.c:38 [inline] > invoke_syscall+0x98/0x2c0 arch/arm64/kernel/syscall.c:52 > el0_svc_common+0x134/0x240 arch/arm64/kernel/syscall.c:139 > do_el0_svc+0x64/0x198 arch/arm64/kernel/syscall.c:188 > el0_svc+0x2c/0x7c arch/arm64/kernel/entry-common.c:647 > el0t_64_sync_handler+0x84/0xfc arch/arm64/kernel/entry-common.c:665 > el0t_64_sync+0x190/0x194 arch/arm64/kernel/entry.S:591 > > Fixes: df8fc4e934c1 ("kbuild: Enable -fstrict-flex-arrays=3") > Reported-by: syzkaller <syzkaller@googlegroups•com> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon•com> Reviewed-by: Willem de Bruijn <willemb@google•com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 net 2/2] af_packet: Fix warning of fortified memcpy() in packet_getname(). 2023-07-20 0:44 ` [PATCH v2 net 2/2] af_packet: Fix warning of fortified memcpy() in packet_getname() Kuniyuki Iwashima 2023-07-20 14:06 ` Willem de Bruijn @ 2023-07-20 15:04 ` Kees Cook 2023-07-20 17:02 ` Kuniyuki Iwashima 1 sibling, 1 reply; 8+ messages in thread From: Kees Cook @ 2023-07-20 15:04 UTC (permalink / raw) To: Kuniyuki Iwashima Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Willem de Bruijn, Gustavo A. R. Silva, Breno Leitao, Kuniyuki Iwashima, netdev, syzkaller On Wed, Jul 19, 2023 at 05:44:10PM -0700, Kuniyuki Iwashima wrote: > syzkaller found a warning in packet_getname() [0], where we try to > copy 16 bytes to sockaddr_ll.sll_addr[8]. > > Some devices (ip6gre, vti6, ip6tnl) have 16 bytes address expressed > by struct in6_addr. Also, Infiniband has 32 bytes as MAX_ADDR_LEN. > > The write seems to overflow, but actually not since we use struct > sockaddr_storage defined in __sys_getsockname() and its size is 128 > (_K_SS_MAXSIZE) bytes. Thus, we have sufficient room after sll_addr[] > as __data[]. Ah, so the issue here is that the UAPI for sll_addr is lying about its size. I think a better fix here is to fix the structure (without breaking UAPI sizes or names): diff --git a/include/uapi/linux/if_packet.h b/include/uapi/linux/if_packet.h index 9efc42382fdb..4d0ad22f83b5 100644 --- a/include/uapi/linux/if_packet.h +++ b/include/uapi/linux/if_packet.h @@ -18,7 +18,11 @@ struct sockaddr_ll { unsigned short sll_hatype; unsigned char sll_pkttype; unsigned char sll_halen; - unsigned char sll_addr[8]; + union { + unsigned char sll_addr[8]; + /* Actual length is in sll_halen. */ + __DECLARE_FLEX_ARRAY(unsigned char, sll_addr_flex); + }; }; /* Packet types */ diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 85ff90a03b0c..8e3ddec4c3d5 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -3601,7 +3601,7 @@ static int packet_getname(struct socket *sock, struct sockaddr *uaddr, if (dev) { sll->sll_hatype = dev->type; sll->sll_halen = dev->addr_len; - memcpy(sll->sll_addr, dev->dev_addr, dev->addr_len); + memcpy(sll->sll_addr_flex, dev->dev_addr, dev->addr_len); } else { sll->sll_hatype = 0; /* Bad: we have no ARPHRD_UNSPEC */ sll->sll_halen = 0; We can't rename sll_data nor change it's size, as userspace uses it pretty extensively: https://codesearch.debian.net/search?q=sizeof.*sll_addr&literal=0 -- Kees Cook ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 net 2/2] af_packet: Fix warning of fortified memcpy() in packet_getname(). 2023-07-20 15:04 ` Kees Cook @ 2023-07-20 17:02 ` Kuniyuki Iwashima 0 siblings, 0 replies; 8+ messages in thread From: Kuniyuki Iwashima @ 2023-07-20 17:02 UTC (permalink / raw) To: keescook Cc: davem, edumazet, gustavoars, kuba, kuni1840, kuniyu, leitao, netdev, pabeni, syzkaller, willemdebruijn.kernel From: Kees Cook <keescook@chromium•org> Date: Thu, 20 Jul 2023 08:04:53 -0700 > On Wed, Jul 19, 2023 at 05:44:10PM -0700, Kuniyuki Iwashima wrote: > > syzkaller found a warning in packet_getname() [0], where we try to > > copy 16 bytes to sockaddr_ll.sll_addr[8]. > > > > Some devices (ip6gre, vti6, ip6tnl) have 16 bytes address expressed > > by struct in6_addr. Also, Infiniband has 32 bytes as MAX_ADDR_LEN. > > > > The write seems to overflow, but actually not since we use struct > > sockaddr_storage defined in __sys_getsockname() and its size is 128 > > (_K_SS_MAXSIZE) bytes. Thus, we have sufficient room after sll_addr[] > > as __data[]. > > Ah, so the issue here is that the UAPI for sll_addr is lying about its > size. I think a better fix here is to fix the structure (without > breaking UAPI sizes or names): Doesn't it forcify sll_addr here ? > > diff --git a/include/uapi/linux/if_packet.h b/include/uapi/linux/if_packet.h > index 9efc42382fdb..4d0ad22f83b5 100644 > --- a/include/uapi/linux/if_packet.h > +++ b/include/uapi/linux/if_packet.h > @@ -18,7 +18,11 @@ struct sockaddr_ll { > unsigned short sll_hatype; > unsigned char sll_pkttype; > unsigned char sll_halen; > - unsigned char sll_addr[8]; > + union { > + unsigned char sll_addr[8]; > + /* Actual length is in sll_halen. */ > + __DECLARE_FLEX_ARRAY(unsigned char, sll_addr_flex); > + }; > }; > > /* Packet types */ > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index 85ff90a03b0c..8e3ddec4c3d5 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -3601,7 +3601,7 @@ static int packet_getname(struct socket *sock, struct sockaddr *uaddr, > if (dev) { > sll->sll_hatype = dev->type; > sll->sll_halen = dev->addr_len; > - memcpy(sll->sll_addr, dev->dev_addr, dev->addr_len); > + memcpy(sll->sll_addr_flex, dev->dev_addr, dev->addr_len); > } else { > sll->sll_hatype = 0; /* Bad: we have no ARPHRD_UNSPEC */ > sll->sll_halen = 0; > > We can't rename sll_data nor change it's size, as userspace uses it > pretty extensively: > https://codesearch.debian.net/search?q=sizeof.*sll_addr&literal=0 > > -- > Kees Cook ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-07-20 17:09 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-07-20 0:44 [PATCH v2 net 0/2] net: Fix error/warning by -fstrict-flex-arrays=3 Kuniyuki Iwashima 2023-07-20 0:44 ` [PATCH v2 net 1/2] af_unix: Fix fortify_panic() in unix_bind_bsd() Kuniyuki Iwashima 2023-07-20 14:39 ` Kees Cook 2023-07-20 17:09 ` Kuniyuki Iwashima 2023-07-20 0:44 ` [PATCH v2 net 2/2] af_packet: Fix warning of fortified memcpy() in packet_getname() Kuniyuki Iwashima 2023-07-20 14:06 ` Willem de Bruijn 2023-07-20 15:04 ` Kees Cook 2023-07-20 17:02 ` Kuniyuki Iwashima
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox