netdev - Re: [PATCH v3 net 1/2] af_unix: Fix fortify_panic() in unix_bind

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <202307261501.C836EED808@keescook>
Date: Wed, 26 Jul 2023 15:02:33 -0700
From: Kees Cook <keescook@...omium.org>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: oliver.sang@...el.com, davem@...emloft.net, edumazet@...gle.com,
	gustavoars@...nel.org, kuba@...nel.org, kuni1840@...il.com,
	leitao@...ian.org, lkp@...el.com, netdev@...r.kernel.org,
	oe-lkp@...ts.linux.dev, pabeni@...hat.com,
	syzkaller@...glegroups.com, willemdebruijn.kernel@...il.com
Subject: Re: [PATCH v3 net 1/2] af_unix: Fix fortify_panic() in
 unix_bind_bsd().

On Wed, Jul 26, 2023 at 09:19:33AM -0700, Kuniyuki Iwashima wrote:
> From: kernel test robot <oliver.sang@...el.com>
> Date: Wed, 26 Jul 2023 21:52:45 +0800
> > 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.22920-2-kuniyu@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
> 
> 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<---

Oh! I missed that you removed the unix_mkname_bsd() in the patch:
https://lore.kernel.org/all/20230724213425.22920-2-kuniyu@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