netdev - Re: [PATCH net] net: fix UaF in netns ops registration error path

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y8p0mW+6ZVucucKh@corigine.com>
Date:   Fri, 20 Jan 2023 12:01:45 +0100
From:   Simon Horman <simon.horman@...igine.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     netdev@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Zhengchao Shao <shaozhengchao@...wei.com>
Subject: Re: [PATCH net] net: fix UaF in netns ops registration error path

On Thu, Jan 19, 2023 at 07:55:45PM +0100, Paolo Abeni wrote:
> If net_assign_generic() fails, the current error path in ops_init() tries
> to clear the gen pointer slot. Anyway, in such error path, the gen pointer
> itself has not been modified yet, and the existing and accessed one is
> smaller than the accessed index, causing an out-of-bounds error:
> 
>  BUG: KASAN: slab-out-of-bounds in ops_init+0x2de/0x320
>  Write of size 8 at addr ffff888109124978 by task modprobe/1018
> 
>  CPU: 2 PID: 1018 Comm: modprobe Not tainted 6.2.0-rc2.mptcp_ae5ac65fbed5+ #1641
>  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.1-2.fc37 04/01/2014
>  Call Trace:
>   <TASK>
>   dump_stack_lvl+0x6a/0x9f
>   print_address_description.constprop.0+0x86/0x2b5
>   print_report+0x11b/0x1fb
>   kasan_report+0x87/0xc0
>   ops_init+0x2de/0x320
>   register_pernet_operations+0x2e4/0x750
>   register_pernet_subsys+0x24/0x40
>   tcf_register_action+0x9f/0x560
>   do_one_initcall+0xf9/0x570
>   do_init_module+0x190/0x650
>   load_module+0x1fa5/0x23c0
>   __do_sys_finit_module+0x10d/0x1b0
>   do_syscall_64+0x58/0x80
>   entry_SYSCALL_64_after_hwframe+0x72/0xdc
>  RIP: 0033:0x7f42518f778d
>  Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48
>        89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
>        ff 73 01 c3 48 8b 0d cb 56 2c 00 f7 d8 64 89 01 48
>  RSP: 002b:00007fff96869688 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
>  RAX: ffffffffffffffda RBX: 00005568ef7f7c90 RCX: 00007f42518f778d
>  RDX: 0000000000000000 RSI: 00005568ef41d796 RDI: 0000000000000003
>  RBP: 00005568ef41d796 R08: 0000000000000000 R09: 0000000000000000
>  R10: 0000000000000003 R11: 0000000000000246 R12: 0000000000000000
>  R13: 00005568ef7f7d30 R14: 0000000000040000 R15: 0000000000000000
>   </TASK>
> 
> This change addresses the issue by skipping the gen pointer
> de-reference in the mentioned error-path.
> 
> Found by code inspection and verified with explicit error injection
> on a kasan-enabled kernel.
> 
> Fixes: d266935ac43d ("net: fix UAF issue in nfqnl_nf_hook_drop() when ops_init() failed")
> Signed-off-by: Paolo Abeni <pabeni@...hat.com>

I don't think the error handling in net_assign_generic or
ops_init are helping us to get this right. But, FWIIW:

Reviewed-by: Simon Horman <simon.horman@...igine.com>

> ---
>  net/core/net_namespace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index 5581d22cc191..078a0a420c8a 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -137,12 +137,12 @@ static int ops_init(const struct pernet_operations *ops, struct net *net)
>  		return 0;
>  
>  	if (ops->id && ops->size) {
> -cleanup:
>  		ng = rcu_dereference_protected(net->gen,
>  					       lockdep_is_held(&pernet_ops_rwsem));
>  		ng->ptr[*ops->id] = NULL;
>  	}
>  
> +cleanup:
>  	kfree(data);
>  
>  out:
> -- 
> 2.39.0
>