[PATCH v1 net 1/2] af_unix: Fix fortify_panic() in unix_bind_bsd().

[Kuniyuki Iwashima]

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 <syzk...@googlegroups.com>
Signed-off-by: Kuniyuki Iwashima <kun...@amazon.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

[Willem de Bruijn]

Reviewed-by: Willem de Bruijn <wil...@google.com>

The extensive comments are really helpful to understand what's
going on.

An alternative would be to just cast sunaddr to a struct
sockaddr_storage *ss and use that both here and in unix_mkname_bsd?
It's not immediately trivial that the caller has always actually
allocated one of those. But the rest becomes self documenting.

[Kuniyuki Iwashima]

From: Kees Cook <ke...@kernel.org>
Date: Wed, 19 Jul 2023 15:34:33 -0700
> On July 19, 2023 3:26:35 PM PDT, Willem de Bruijn <willemdebr...@gmail.com> wrote:

> >Kuniyuki Iwashima wrote:
> >The extensive comments are really helpful to understand what's
> >going on.
> >
> >An alternative would be to just cast sunaddr to a struct
> >sockaddr_storage *ss and use that both here and in unix_mkname_bsd?
> >It's not immediately trivial that the caller has always actually
> >allocated one of those. But the rest becomes self documenting.

Yeah, this is also my initial attempt, and I separted it because
unix_find_bsd() need not to call it and I tried to separate
unnecessary calls in this series (compilers might drop the unused
strlen() though).

https://lore.kernel.org/netdev/20211124021431...@amazon.co.jp/


>
> I would much prefer the internal APIs actually passed around the true sockaddr_storage pointer. This is what I did recently for NFS, for example:
> https://git.kernel.org/linus/cf0d7e7f4520

We can convert struct proto_ops and struct proto later as such
if needed, but I think it's too invasive as a fix.

[Kuniyuki Iwashima]

[Kees Cook]

On July 19, 2023 3:26:35 PM PDT, Willem de Bruijn <willemdebr...@gmail.com> wrote:
>Kuniyuki Iwashima wrote:

>The extensive comments are really helpful to understand what's
>going on.
>
>An alternative would be to just cast sunaddr to a struct
>sockaddr_storage *ss and use that both here and in unix_mkname_bsd?
>It's not immediately trivial that the caller has always actually
>allocated one of those. But the rest becomes self documenting.

I would much prefer the internal APIs actually passed around the true sockaddr_storage pointer. This is what I did recently for NFS, for example:
https://git.kernel.org/linus/cf0d7e7f4520

-Kees


--
Kees Cook

[Kees Cook]

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.

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

[Kuniyuki Iwashima]

From: Kees Cook <kees...@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().

[Kuniyuki Iwashima]

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 as done in unix_mkname_bsd().

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 see the string as unterminated.

Let's use strnlen() to allow sun_addr to be unterminated at 107.

Suggested-by: Kees Cook <kees...@chromium.org>

Signed-off-by: Kuniyuki Iwashima <kun...@amazon.com>
---

net/unix/af_unix.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 123b35ddfd71..bbacf4c60fe3 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1208,10 +1208,8 @@ static int unix_bind_bsd(struct sock *sk, struct sockaddr_un *sunaddr,
struct path parent;
int err;

- unix_mkname_bsd(sunaddr, addr_len);

- addr_len = strlen(sunaddr->sun_path) +

- offsetof(struct sockaddr_un, sun_path) + 1;
-

+ addr_len = strnlen(sunaddr->sun_path, sizeof(sunaddr->sun_path))

+ + offsetof(struct sockaddr_un, sun_path) + 1;

[kernel test robot]

Hello,

kernel test robot noticed "BUG:KASAN:slab-out-of-bounds_in_strlen" on:

commit: 33652e138afbe3f7c814567c4ffdf57492664220 ("[PATCH v3 net 1/2] af_unix: Fix fortify_panic() in unix_bind_bsd().")
url: https://github.com/intel-lab-lkp/linux/commits/Kuniyuki-Iwashima/af_unix-Fix-fortify_panic-in-unix_bind_bsd/20230725-053836
base: https://git.kernel.org/cgit/linux/kernel/git/davem/net.git 22117b3ae6e37d07225653d9ae5ae86b3a54f99c
patch link: https://lore.kernel.org/all/20230724213425...@amazon.com/
patch subject: [PATCH v3 net 1/2] af_unix: Fix fortify_panic() in unix_bind_bsd().

in testcase: boot

compiler: gcc-12
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G

(please refer to attached dmesg/kmsg for entire log/backtrace)


[ 33.452659][ T68] ==================================================================
[ 33.453726][ T68] BUG: KASAN: slab-out-of-bounds in strlen+0x35/0x4f
[ 33.454515][ T68] Read of size 1 at addr ffff88812ff65577 by task udevd/68
[ 33.455352][ T68]
[ 33.455644][ T68] CPU: 0 PID: 68 Comm: udevd Not tainted 6.5.0-rc2-00197-g33652e138afb #1
[ 33.456627][ T68] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 33.457802][ T68] Call Trace:
[ 33.458184][ T68] <TASK>
[ 33.458521][ T68] print_address_description+0x4d/0x2dd
[ 33.459259][ T68] print_report+0x139/0x241
[ 33.459783][ T68] ? __phys_addr+0x91/0xa3
[ 33.460290][ T68] ? virt_to_folio+0x5/0x27
[ 33.460800][ T68] ? strlen+0x35/0x4f
[ 33.461241][ T68] kasan_report+0xaf/0xda
[ 33.461756][ T68] ? strlen+0x35/0x4f
[ 33.462218][ T68] strlen+0x35/0x4f
[ 33.462657][ T68] getname_kernel+0xe/0x234
[ 33.463190][ T68] kern_path_create+0x18/0x4d
[ 33.463727][ T68] unix_bind_bsd+0x180/0x5a4
[ 33.464367][ T68] ? unix_create_addr+0xdd/0xdd
[ 33.464929][ T68] __sys_bind+0xf2/0x15f
[ 33.465437][ T68] ? __ia32_sys_socketpair+0xb3/0xb3
[ 33.466046][ T68] __x64_sys_bind+0x79/0x87
[ 33.466575][ T68] do_syscall_64+0x6b/0x87
[ 33.467094][ T68] ? lockdep_hardirqs_on_prepare+0x326/0x350
[ 33.467780][ T68] ? do_syscall_64+0x78/0x87
[ 33.468305][ T68] ? lockdep_hardirqs_on_prepare+0x326/0x350
[ 33.468989][ T68] ? do_syscall_64+0x78/0x87
[ 33.469538][ T68] ? do_syscall_64+0x78/0x87
[ 33.470051][ T68] ? do_syscall_64+0x78/0x87
[ 33.470576][ T68] ? lockdep_hardirqs_on_prepare+0x326/0x350
[ 33.471268][ T68] entry_SYSCALL_64_after_hwframe+0x5d/0xc7
[ 33.471917][ T68] RIP: 0033:0x7fd5b1091b77
[ 33.472426][ T68] Code: ff ff ff ff c3 48 8b 15 17 b3 0b 00 f7 d8 64 89 02 b8 ff ff ff ff eb ba 66 2e 0f 1f 84 00 00 00 00 00 90 b8 31 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d e9 b2 0b 00 f7 d8 64 89 01 48
[ 33.474908][ T68] RSP: 002b:00007ffddf8bb5c8 EFLAGS: 00000246 ORIG_RAX: 0000000000000031
[ 33.475878][ T68] RAX: ffffffffffffffda RBX: 000055dcbcd8e8c0 RCX: 00007fd5b1091b77
[ 33.476785][ T68] RDX: 0000000000000013 RSI: 000055dcbcd8e8d8 RDI: 0000000000000003
[ 33.477697][ T68] RBP: 000055dcbcd8e8d8 R08: 0000000000000004 R09: 000055dcbcd8ed50
[ 33.478561][ T68] R10: 00007ffddf8bb5d4 R11: 0000000000000246 R12: 00007ffddf8bbe18
[ 33.479431][ T68] R13: 00007ffddf8bbe10 R14: 0000000000000000 R15: 000055dcbcd8e260
[ 33.480352][ T68] </TASK>
[ 33.480723][ T68]
[ 33.481014][ T68] Allocated by task 68:
[ 33.481537][ T68] stack_trace_save+0x77/0x98
[ 33.482086][ T68] kasan_save_stack+0x2e/0x53
[ 33.482644][ T68] kasan_set_track+0x20/0x2c
[ 33.483188][ T68] ____kasan_kmalloc+0x68/0x7b
[ 33.483761][ T68] __kmalloc+0xac/0xe5
[ 33.484260][ T68] unix_create_addr+0x1f/0xdd
[ 33.484821][ T68] unix_bind_bsd+0x161/0x5a4
[ 33.485512][ T68] __sys_bind+0xf2/0x15f
[ 33.486024][ T68] __x64_sys_bind+0x79/0x87
[ 33.486559][ T68] do_syscall_64+0x6b/0x87
[ 33.487066][ T68] entry_SYSCALL_64_after_hwframe+0x5d/0xc7
[ 33.487761][ T68]
[ 33.488048][ T68] The buggy address belongs to the object at ffff88812ff65500
[ 33.488048][ T68] which belongs to the cache kmalloc-128 of size 128
[ 33.489631][ T68] The buggy address is located 0 bytes to the right of
[ 33.489631][ T68] allocated 119-byte region [ffff88812ff65500, ffff88812ff65577)
[ 33.491272][ T68]
[ 33.491571][ T68] The buggy address belongs to the physical page:
[ 33.492321][ T68] page:(____ptrval____) refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x12ff65
[ 33.493503][ T68] flags: 0x8000000000000200(slab|zone=2)
[ 33.494120][ T68] page_type: 0xffffffff()
[ 33.494610][ T68] raw: 8000000000000200 ffff8881000418c0 dead000000000122 0000000000000000
[ 33.495560][ T68] raw: 0000000000000000 0000000080100010 00000001ffffffff 0000000000000000
[ 33.496528][ T68] page dumped because: kasan: bad access detected
[ 33.497265][ T68] page_owner tracks the page as allocated
[ 33.497959][ T68] page last allocated via order 0, migratetype Unmovable, gfp_mask 0x12820(GFP_ATOMIC|__GFP_NOWARN|__GFP_NORETRY), pid 1, tgid 1 (swapper), ts 31189252946, free_ts 0
[ 33.499819][ T68] __set_page_owner+0x15/0x59
[ 33.500391][ T68] prep_new_page+0x17/0x70
[ 33.500931][ T68] get_page_from_freelist+0x1ca/0x3d2
[ 33.501972][ T68] __alloc_pages+0x159/0x25f
[ 33.502494][ T68] alloc_slab_page+0x1a/0x4c
[ 33.503011][ T68] allocate_slab+0x59/0x1b6
[ 33.503517][ T68] ___slab_alloc+0x348/0x510
[ 33.504034][ T68] __slab_alloc+0x11/0x27
[ 33.504641][ T68] __kmem_cache_alloc_node+0x63/0x10d
[ 33.505246][ T68] __kmalloc+0x9b/0xe5
[ 33.505736][ T68] pci_create_attr+0x33/0x3c4
[ 33.506282][ T68] pci_create_resource_files+0x9c/0x126
[ 33.506907][ T68] pci_sysfs_init+0x66/0xf1
[ 33.507434][ T68] do_one_initcall+0xc3/0x274
[ 33.507955][ T68] do_initcalls+0x308/0x366
[ 33.508479][ T68] kernel_init_freeable+0x2b1/0x315
[ 33.509081][ T68] page_owner free stack trace missing
[ 33.509687][ T68]
[ 33.509974][ T68] Memory state around the buggy address:
[ 33.510640][ T68] ffff88812ff65400: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[ 33.511564][ T68] ffff88812ff65480: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[ 33.512499][ T68] >ffff88812ff65500: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 07 fc
[ 33.513453][ T68] ^
[ 33.514393][ T68] ffff88812ff65580: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[ 33.515328][ T68] ffff88812ff65600: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[ 33.516257][ T68] ==================================================================
[ 33.517245][ T68] Disabling lock debugging due to kernel taint
[ 33.518764][ T68] udevd[68]: starting version 3.2.7



To reproduce:

# build kernel
cd linux
cp config-6.5.0-rc2-00197-g33652e138afb .config
make HOSTCC=gcc-12 CC=gcc-12 ARCH=x86_64 olddefconfig prepare modules_prepare bzImage modules
make HOSTCC=gcc-12 CC=gcc-12 ARCH=x86_64 INSTALL_MOD_PATH=<mod-install-dir> modules_install
cd <mod-install-dir>
find lib/ | cpio -o -H newc --quiet | gzip > modules.cgz


git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k <bzImage> -m modules.cgz job-script # job-script is attached in this email

# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.



--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[Kuniyuki Iwashima]

From: kernel test robot <olive...@intel.com>
Date: Wed, 26 Jul 2023 21:52:45 +0800

Ok, we still need to terminate the string with unix_mkname_bsd().. so
I perfer using strlen() here as well to warn about this situation.

I'll post a patch soon.

---8<---
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index bbacf4c60fe3..6056c3bad54e 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1208,7 +1208,8 @@ static int unix_bind_bsd(struct sock *sk, struct sockaddr_un *sunaddr,

struct path parent;
int err;

- addr_len = strnlen(sunaddr->sun_path, sizeof(sunaddr->sun_path))
+ unix_mkname_bsd(sunaddr->sun_path, addr_len);
+ addr_len = strlen(((struct sockaddr_storage *)sunaddr)->__data)

+ offsetof(struct sockaddr_un, sun_path) + 1;
addr = unix_create_addr(sunaddr, addr_len);
if (!addr)

---8<---

[Kees Cook]

Oh! I missed that you removed the unix_mkname_bsd() in the patch:
https://lore.kernel.org/all/20230724213425...@amazon.com/

If you just add that back in, you should be fine. (There is no need for
the casting here, strnlen() will still do the right thing from what I
can see.)

-Kees

--
Kees Cook