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);
>> +}
