netdev - Re: [PATCH] net: fix out-of-bounds access in ops

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: <CANn89iJpp7AA=bb_BnYFskWVjf61hd1AgPmU-4ZGOUZQhsYgJA@mail.gmail.com>
Date: Tue, 30 Apr 2024 11:13:51 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Thadeu Lima de Souza Cascardo <cascardo@...lia.com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, kernel-dev@...lia.com
Subject: Re: [PATCH] net: fix out-of-bounds access in ops_init

On Tue, Apr 30, 2024 at 10:43 AM Thadeu Lima de Souza Cascardo
<cascardo@...lia.com> wrote:
>
> net_alloc_generic is called by net_alloc, which is called without any
> locking. It reads max_gen_ptrs, which is changed under pernet_ops_rwsem. It
> is read twice, first to allocate an array, then to set s.len, which is
> later used to limit the bounds of the array access.
>
> It is possible that the array is allocated and another thread is
> registering a new pernet ops, increments max_gen_ptrs, which is then used
> to set s.len with a larger than allocated length for the variable array.
>
> Fix it by delaying the allocation to setup_net, which is always called
> under pernet_ops_rwsem, and is called right after net_alloc.
>
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@...lia.com>

Good catch !

Could you provide a Fixes: tag ?

Have you considered reading max_gen_ptrs once in net_alloc_generic() ?
This would make the patch a little less complicated.

diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 2f5190aa2f15cec2e934ebee9c502fb426cf0d7d..dc198ce7e6aeabd8831be32f0a3b5bd1d0a77315
100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -70,11 +70,12 @@ DEFINE_COOKIE(net_cookie);
 static struct net_generic *net_alloc_generic(void)
 {
        struct net_generic *ng;
-       unsigned int generic_size = offsetof(struct net_generic,
ptr[max_gen_ptrs]);
+       /* Paired with WRITE_ONCE() in register_pernet_operations() */
+       unsigned int max = READ_ONCE(max_gen_ptrs);

-       ng = kzalloc(generic_size, GFP_KERNEL);
+       ng = kzalloc(offsetof(struct net_generic, ptr[max]), GFP_KERNEL);
        if (ng)
-               ng->s.len = max_gen_ptrs;
+               ng->s.len = max;

        return ng;
 }
@@ -1308,7 +1309,9 @@ static int register_pernet_operations(struct
list_head *list,
                if (error < 0)
                        return error;
                *ops->id = error;
-               max_gen_ptrs = max(max_gen_ptrs, *ops->id + 1);
+               /* Paired with READ_ONCE() in net_alloc_generic() */
+               WRITE_ONCE(max_gen_ptrs,
+                          max(max_gen_ptrs, *ops->id + 1));
        }
        error = __register_pernet_operations(list, ops);
        if (error) {