netdev - Re: [PATCHv2 bpf-next 07/11] bpf: Add helper to retrieve socket in BPF

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: <691934f3-adbc-187c-afa1-9fa9d43f95aa@iogearbox.net>
Date:   Mon, 24 Sep 2018 14:51:28 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Joe Stringer <joe@...d.net.nz>, ast@...nel.org
Cc:     netdev@...r.kernel.org, john.fastabend@...il.com, tgraf@...g.ch,
        kafai@...com, nitin.hande@...il.com, mauricio.vasquez@...ito.it
Subject: Re: [PATCHv2 bpf-next 07/11] bpf: Add helper to retrieve socket in
 BPF

On 09/24/2018 02:12 PM, Daniel Borkmann wrote:
> On 09/21/2018 07:10 PM, Joe Stringer wrote:
[...]
>> +/* bpf_sk_lookup performs the core lookup for different types of sockets,
>> + * taking a reference on the socket if it doesn't have the flag SOCK_RCU_FREE.
>> + * Returns the socket as an 'unsigned long' to simplify the casting in the
>> + * callers to satisfy BPF_CALL declarations.
>> + */
>> +static unsigned long
>> +bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
>> +	      u8 proto, u64 netns_id, u64 flags)
>> +{
>> +	struct net *caller_net = dev_net(skb->dev);
> 
> For sk_skb programs, are we *always* guaranteed to have a skb->dev assigned?
> 
> This definitely holds true for tc programs, but afaik not for sk_skb ones where
> you enable the helpers below, so this would result in a NULL ptr dereference.
> 
>> +	struct sock *sk = NULL;
>> +	u8 family = AF_UNSPEC;
>> +	struct net *net;
>> +
>> +	family = len == sizeof(tuple->ipv4) ? AF_INET : AF_INET6;
>> +	if (unlikely(family == AF_UNSPEC || netns_id > U32_MAX || flags))
>> +		goto out;
>> +
>> +	if (netns_id)
>> +		net = get_net_ns_by_id(caller_net, netns_id);
>> +	else
>> +		net = caller_net;
>> +	if (unlikely(!net))
>> +		goto out;
>> +	sk = sk_lookup(net, tuple, skb, family, proto);
>> +	put_net(net);

Hmm, isn't this also resulting in a use-after-free on net ?

Presume we use dev_net(skb->dev) as net. It does read_pnet(&dev->nd_net), which
either gets you pnet->net when netns is configured or &init_net otherwise.

I don't see where an additional reference was taken in this path, hence you'll
get an imbalance which triggers freeing the underlying netns ..

>> +	if (sk)
>> +		sk = sk_to_full_sk(sk);
>> +out:
>> +	return (unsigned long) sk;
>> +}
>> +
>> +BPF_CALL_5(bpf_sk_lookup_tcp, struct sk_buff *, skb,
>> +	   struct bpf_sock_tuple *, tuple, u32, len, u64, netns_id, u64, flags)
>> +{
>> +	return bpf_sk_lookup(skb, tuple, len, IPPROTO_TCP, netns_id, flags);
>> +}