netdev - Re: [PATCH net] l2tp: Don't sleep and disable BH under writer-side 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20221122141011.GA3303@pc-4.home>
Date:   Tue, 22 Nov 2022 15:10:11 +0100
From:   Guillaume Nault <gnault@...hat.com>
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc:     Jakub Sitnicki <jakub@...udflare.com>, netdev@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Tom Parkin <tparkin@...alix.com>,
        syzbot+703d9e154b3b58277261@...kaller.appspotmail.com,
        syzbot+50680ced9e98a61f7698@...kaller.appspotmail.com,
        syzbot+de987172bb74a381879b@...kaller.appspotmail.com
Subject: Re: [PATCH net] l2tp: Don't sleep and disable BH under writer-side
 sk_callback_lock

On Tue, Nov 22, 2022 at 08:14:33PM +0900, Tetsuo Handa wrote:
> On 2022/11/22 19:46, Jakub Sitnicki wrote:
> >> https://syzkaller.appspot.com/bug?extid=94cc2a66fc228b23f360 is the one
> >> where changing lockdep class is concurrently done on pre-existing sockets.
> >>
> >> I think we need to always create a new socket inside l2tp_tunnel_register(),
> >> rather than trying to serialize setting of sk_user_data under sk_callback_lock.
> > 
> > While that would be easier to handle, I don't see how it can be done in
> > a backward-compatible way. User-space is allowed to pass a socket to
> > l2tp today [1].
> 
> What is the expected usage of the socket which was passed to l2tp_tunnel_register() ?

It receives L2TP packets. Those can be either control or data ones.
Data packets are processed by the kernel. Control packets are queued to
user space.

> Is the userspace supposed to just close() that socket? Or, is the userspace allowed to
> continue using the socket?

User space uses this socket to send and receive L2TP control packets
(tunnel and session configuration, keep alive and tear down). Therefore
it absolutely needs to continue using this socket after the
registration phase.

> If the userspace might continue using the socket, we would
> 
>   create a new socket, copy required attributes (the source and destination addresses?) from
>   the socket fetched via sockfd_lookup(), and call replace_fd() like e.g. umh_pipe_setup() does
> 
> inside l2tp_tunnel_register(). i-node number of the socket would change, but I assume that
> the process which called l2tp_tunnel_register() is not using that i-node number.
> 
> Since the socket is a datagram socket, I think we can copy required attributes. But since
> I'm not familiar with networking code, I don't know what attributes need to be copied. Thus,
> I leave implementing it to netdev people.

That looks fragile to me. If the problem is that setup_udp_tunnel_sock()
can sleep, we can just drop the udp_tunnel_encap_enable() call from
setup_udp_tunnel_sock(), rename it __udp_tunnel_encap_enable() and make
make udp_tunnel_encap_enable() a wrapper around it that'd also call
udp_tunnel_encap_enable().