| Message ID | yt9dedl2rgoo.fsf@linux.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | AF_UNIX crash in bind() triggered by strace test | expand |
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 --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)