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: <20180117.142538.1972806008716856078.davem@davemloft.net>
Date:   Wed, 17 Jan 2018 14:25:38 -0500 (EST)
From:   David Miller <davem@...emloft.net>
To:     jchapman@...alix.com
Cc:     tom@...bertland.com, 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

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...