diff mbox series

AF_UNIX crash in bind() triggered by strace test

Message ID yt9dedl2rgoo.fsf@linux.ibm.com
State New
Headers
Series AF_UNIX crash in bind() triggered by strace test |

Commit Message

Sven Schnelle July 20, 2023, 11 a.m. UTC 
  Hi,

the following crash was reported in our CI on s390x with
debug_defconfig, which enables FORTIFY_SOURCE:

detected buffer overflow in __fortify_strlen
------------[ cut here ]------------
kernel BUG at lib/string_helpers.c:1031!
monitor event: 0040 ilc:2 [#1] PREEMPT SMP 
Modules linked in: [..]
CPU: 0 PID: 243755 Comm: net-accept-conn Not tainted 6.5.0-20230719.rc2.git1.f1311c9e1695.300.fc38.s390x+debug #1
Call Trace:
 [<000000003465b7a2>] fortify_panic+0x2a/0x30 
([<000000003465b79e>] fortify_panic+0x26/0x30)
 [<0000000034a3a77e>] unix_bind_bsd+0x86/0x390 
 [<00000000348839d0>] __sys_bind+0xe0/0xe8 
 [<0000000034926270>] __do_compat_sys_socketcall+0x260/0x4d0 
 [<0000000034be3b66>] __do_syscall+0x1de/0x208 
 [<0000000034bfaf38>] system_call+0x70/0x98 
INFO: lockdep is turned off.
Last Breaking-Event-Address:
 [<0000000034774880>] __s390_indirect_jump_r14+0x0/0x10
Kernel panic - not syncing: Fatal exception: panic_on_oops

This is caused by a test case who sends an unterminated sun_path
to the kernel in a bind() system call from the strace test suite.
As a test i made the following quick fix, which "fixed" the issue:


However, unix(7) says "The pathname in sun_path *should* be
null-terminated." So this change might break userspace. I'm not sure
whether we should return -EINVAL, or just truncate the name to
UNIX_PATH_MAX. From a quick read, it looks like connect() would trigger
the same problem. Any thoughts?

Comments

Eric Dumazet July 20, 2023, 11:03 a.m. UTC | #1 
On Thu, Jul 20, 2023 at 1:00 PM Sven Schnelle <svens@linux.ibm.com> wrote:
>
> Hi,
>
> the following crash was reported in our CI on s390x with
> debug_defconfig, which enables FORTIFY_SOURCE:
>
> detected buffer overflow in __fortify_strlen
> ------------[ cut here ]------------
> kernel BUG at lib/string_helpers.c:1031!
> monitor event: 0040 ilc:2 [#1] PREEMPT SMP
> Modules linked in: [..]
> CPU: 0 PID: 243755 Comm: net-accept-conn Not tainted 6.5.0-20230719.rc2.git1.f1311c9e1695.300.fc38.s390x+debug #1
> Call Trace:
>  [<000000003465b7a2>] fortify_panic+0x2a/0x30
> ([<000000003465b79e>] fortify_panic+0x26/0x30)
>  [<0000000034a3a77e>] unix_bind_bsd+0x86/0x390
>  [<00000000348839d0>] __sys_bind+0xe0/0xe8
>  [<0000000034926270>] __do_compat_sys_socketcall+0x260/0x4d0
>  [<0000000034be3b66>] __do_syscall+0x1de/0x208
>  [<0000000034bfaf38>] system_call+0x70/0x98
> INFO: lockdep is turned off.
> Last Breaking-Event-Address:
>  [<0000000034774880>] __s390_indirect_jump_r14+0x0/0x10
> Kernel panic - not syncing: Fatal exception: panic_on_oops
>
> This is caused by a test case who sends an unterminated sun_path
> to the kernel in a bind() system call from the strace test suite.
> As a test i made the following quick fix, which "fixed" the issue:
>
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 123b35ddfd71..c505edd74d8c 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1206,11 +1206,13 @@ static int unix_bind_bsd(struct sock *sk, struct sockaddr_un *sunaddr,
>         struct unix_address *addr;
>         struct dentry *dentry;
>         struct path parent;
> -       int err;
> +       int pathlen, err;
>
>         unix_mkname_bsd(sunaddr, addr_len);
> -       addr_len = strlen(sunaddr->sun_path) +
> -               offsetof(struct sockaddr_un, sun_path) + 1;
> +       pathlen = strnlen(sunaddr->sun_path, UNIX_PATH_MAX);
> +       if (pathlen == UNIX_PATH_MAX)
> +               return -EINVAL;
> +       addr_len = pathlen + offsetof(struct sockaddr_un, sun_path) + 1;
>
>         addr = unix_create_addr(sunaddr, addr_len);
>         if (!addr)
>
> However, unix(7) says "The pathname in sun_path *should* be
> null-terminated." So this change might break userspace. I'm not sure
> whether we should return -EINVAL, or just truncate the name to
> UNIX_PATH_MAX. From a quick read, it looks like connect() would trigger
> the same problem. Any thoughts?

https://patchwork.kernel.org/project/netdevbpf/patch/20230720004410.87588-2-kuniyu@amazon.com/
diff mbox series

Patch

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 123b35ddfd71..c505edd74d8c 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1206,11 +1206,13 @@  static int unix_bind_bsd(struct sock *sk, struct sockaddr_un *sunaddr,
 	struct unix_address *addr;
 	struct dentry *dentry;
 	struct path parent;
-	int err;
+	int pathlen, err;
 
 	unix_mkname_bsd(sunaddr, addr_len);
-	addr_len = strlen(sunaddr->sun_path) +
-		offsetof(struct sockaddr_un, sun_path) + 1;
+	pathlen = strnlen(sunaddr->sun_path, UNIX_PATH_MAX);
+	if (pathlen == UNIX_PATH_MAX)
+		return -EINVAL;
+	addr_len = pathlen + offsetof(struct sockaddr_un, sun_path) + 1;
 
 	addr = unix_create_addr(sunaddr, addr_len);
 	if (!addr)