netdev - Re: [PATCH v1 net 1/2] raw: Fix NULL deref in raw_get

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: <CANn89i+b5DwKtAVe2PF5Qh+r8mL6bZEL_2bpjAHAMOu_52QmFg@mail.gmail.com>
Date:   Tue, 4 Apr 2023 03:46:25 +0200
From:   Eric Dumazet <edumazet@...gle.com>
To:     Kuniyuki Iwashima <kuniyu@...zon.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Kuniyuki Iwashima <kuni1840@...il.com>, netdev@...r.kernel.org,
        syzbot <syzkaller@...glegroups.com>,
        "Dae R . Jeong" <threeearcat@...il.com>
Subject: Re: [PATCH v1 net 1/2] raw: Fix NULL deref in raw_get_next().

On Mon, Apr 3, 2023 at 9:51 PM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
>
> Dae R. Jeong reported a NULL deref in raw_get_next() [0].
>
> It seems that the repro was running these sequences in parallel so
> that one thread was iterating on a socket that was being freed in
> another netns.
>
>   unshare(0x40060200)
>   r0 = syz_open_procfs(0x0, &(0x7f0000002080)='net/raw\x00')
>   socket$inet_icmp_raw(0x2, 0x3, 0x1)
>   pread64(r0, &(0x7f0000000000)=""/10, 0xa, 0x10000000007f)
>
> After commit 0daf07e52709 ("raw: convert raw sockets to RCU"), we
> use RCU and hlist_nulls_for_each_entry() to iterate over SOCK_RAW
> sockets.  However, we should use spinlock for slow paths to avoid
> the NULL deref.
>
> Also, SOCK_RAW does not use SLAB_TYPESAFE_BY_RCU, and the slab object
> is not reused during iteration in the grace period.  In fact, the
> lockless readers do not check the nulls marker with get_nulls_value().
> So, SOCK_RAW should use hlist instead of hlist_nulls.
>
> Instead of adding an unnecessary barrier by sk_nulls_for_each_rcu(),
> let's convert hlist_nulls to hlist and use sk_for_each_rcu() for
> fast paths and sk_for_each() and spinlock for /proc/net/raw.
>
> Fixes: 0daf07e52709 ("raw: convert raw sockets to RCU")
> Reported-by: syzbot <syzkaller@...glegroups.com>
> Reported-by: Dae R. Jeong <threeearcat@...il.com>
> Link: https://lore.kernel.org/netdev/ZCA2mGV_cmq7lIfV@dragonet/
> Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> ---
>  include/net/raw.h   |  4 ++--
>  net/ipv4/raw.c      | 36 +++++++++++++++++++-----------------
>  net/ipv4/raw_diag.c | 10 ++++------
>  net/ipv6/raw.c      | 10 ++++------
>  4 files changed, 29 insertions(+), 31 deletions(-)
>

SGTM, thanks a lot.
Reviewed-by: Eric Dumazet <edumazet@...gle.com>