netdev - Re: [PATCH 6.1-rc6] l2tp: call udp_tunnel_encap_enable() and sock_release() without sk_callback

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 for Android: free password hash cracker in your pocket

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

Message-ID: <87cz9julph.fsf@cloudflare.com>
Date:   Sat, 19 Nov 2022 14:13:07 +0100
From:   Jakub Sitnicki <jakub@...udflare.com>
To:     Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
Cc:     Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>,
        Tom Parkin <tparkin@...alix.com>,
        syzbot <syzbot+703d9e154b3b58277261@...kaller.appspotmail.com>,
        netdev@...r.kernel.org, syzkaller-bugs@...glegroups.com,
        Haowei Yan <g1042620637@...il.com>
Subject: Re: [PATCH 6.1-rc6] l2tp: call udp_tunnel_encap_enable() and
 sock_release() without sk_callback_lock

On Sat, Nov 19, 2022 at 07:08 PM +09, Tetsuo Handa wrote:
> On 2022/11/19 2:50, Jakub Sitnicki wrote:
>> Thanks for the patch, Tetsuo.
>> 
>> As Eric has pointed out [1], there is another problem - in addition to
>> sleeping in atomic context, I have also failed to use the write_lock
>> variant which disabled BH locally.
>> 
>> The latter bug can lead to dead-locks, as reported by syzcaller [2, 3],
>> because we grab sk_callback_lock in softirq context, which can then
>> block waiting on us if:
>
> Below is another approach I was thinking of, for reusing existing locks is prone
> to locking bugs like [2] and [3].
>
> I couldn't interpret "Write-protected by @sk_callback_lock." part because
> it does not say what lock is needed for protecting sk_user_data for read access.

sk_user_data is RCU-protected on reader-side. But we still need to
synchronize writers.

> Is it possible to use a mutex dedicated for l2tp_tunnel_destruct() (and optionally
> setup_udp_tunnel_sock_no_enable() in order not to create l2tp_tunnel_register_mutex =>
> cpu_hotplug_lock chain) ?

No, we need to a common lock to synchronize with other users in the net
stack (reuseport groups, sockmap/psock to name a couple).

> By the way I haven't heard an response on
>
>   Since userspace-supplied file descriptor has to be a datagram socket,
>   can we somehow copy the source/destination addresses from
>   userspace-supplied socket to kernel-created socket?
>
> at https://lkml.kernel.org/r/c9695548-3f27-dda1-3124-ec21da106741@I-love.SAKURA.ne.jp
> (that is, always create a new socket in order to be able to assign lockdep class
> before that socket is used).

This is a drive by fix for me to l2tp, so I might not be the best person
to ask, but I will take a look at the thread.

[...]