On Mon, 24 Sep 2018 at 05:51, Daniel Borkmann <dan...@iogearbox.net> wrote:
>
> 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 ..
Good catch, will fix.
