netdev - Re: [PATCH net-next] kcm: do not attach sockets if sk_user

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: <CALx6S35k2tMnoe1gr5_gVptc_i4MBi1kvFcJZY07=Q=UXKrerA@mail.gmail.com>
Date:   Thu, 18 Jan 2018 09:40:44 -0800
From:   Tom Herbert <tom@...bertland.com>
To:     David Miller <davem@...emloft.net>
Cc:     James Chapman <jchapman@...alix.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        g.nault@...halink.fr
Subject: Re: [PATCH net-next] kcm: do not attach sockets if sk_user_data is
 already used

On Wed, Jan 17, 2018 at 11:25 AM, David Miller <davem@...emloft.net> wrote:
> From: James Chapman <jchapman@...alix.com>
> Date: Wed, 17 Jan 2018 11:13:33 +0000
>
>> On 16 January 2018 at 19:00, David Miller <davem@...emloft.net> wrote:
>>> From: Tom Herbert <tom@...bertland.com>
>>> Date: Tue, 16 Jan 2018 09:36:41 -0800
>>>
>>>> sk_user_data is set with the sk_callback lock held in code below.
>>>> Should be able to take the lock earlier can do this check under the
>>>> lock.
>>>
>>> csock, and this csk, is obtained from an arbitrary one of the
>>> process's FDs.  It can be any socket type or family, and that socket's
>>> family might set sk_user_data without the callback lock.
>>>
>>> The only socket type check is making sure it is not another PF_KCM
>>> socket.  So that doesn't help with this problem.
>>
>> Is it the intention to update all socket code over time to write
>> sk_user_data within the sk_callback lock? If so, I'm happy to address
>> that in the l2tp code (and update the kcm patch to check sk_user_data
>> within the sk_callback lock). Or is the preferred solution to restrict
>> KCM to specific socket families, as suggested by Guillaume earlier in
>> the thread?
>
> I think we have a more fundamental issue here.
>
> sk->sk_user_data is a place where RPC layer specific data is hung off
> of.  By this definition SunRPC, RXRPC, RDS, TIPC, and KCM are all
> using it correctly.
>
> Phonet has a similar issue to the one seen here, it tests and changes
> sk_user_data under lock_sock().  The only requirement it makes is
> that the socket type is not SOCK_STREAM.  However, this one might be OK
> since only pep_sock sockets can be passed down into gprs_attach().
>
> Most of these cases like SunRPC, RXRPC, etc. are fine because they
> only graft on top of TCP and UDP sockets.
>
> The weird situation here is that L2TP does tunneling and stores it's
> private state in sk->sk_user_data like an RPC layer would.  And KCM
> allows basically any socket type to be attached.
>
> The RPC layers create their sockets internally, so I cannot see a way
> that those can be sent to a KCM attach operations.  And I think that
> is why this RPC invariant is important for sk_user_data usage.
>
> If all else was equal, even though it doesn't make much sense to KCM
> attach L2TP sockets to KCM, I would suggest to change L2TP to store
> it's private stuff elsewhere.
>
> But that is not the case.  Anything using the generic UDP
> encapsulation layer is going to make use of sk->sk_user_data like this
> (see setup_udp_tunnel_sock).
>
> It looks like over time we've accumulated this new class of uses
> of sk->sk_user_data, ho hum...
>
> And it's not like we can add a test to KCM to avoid these socket
> types, because they will look like normal UDP datagram sockets.
>
> What a mess...
>
> Furthermore, even if you add a test to KCM, you will now need to
> add the same test to L2TP and anything else which uses sk_user_data
> for tunneling and for which userspace has access to the socket fd.
>
> And it will be racy, indeed, until all such users align to the same
> precise locking scheme for tests and updates to sk_user_data.
>
> Again, what a mess...
>
It's not so surprising that sk_user_data is being used for so many
purposes, it's quite a powerful and useful notion. So, to a large
extent I think it's a victim of it's own success.

Aligning to one locking scheme is the first task to clean this. The
second would be how to deal with multiple simulataneous use on a
socket (or maybe not allow). I've thought about having a chain of
sk_user_data, but that's only useful is the write/read callback are
also chained. All this starts to look like STREAMS at some point ;-)

Tom