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