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]

Date:   Mon, 24 Sep 2018 11:09:39 -0700
From:   Joe Stringer <joe@...d.net.nz>
To:     daniel@...earbox.net
Cc:     Joe Stringer <joe@...d.net.nz>, ast@...nel.org,
        netdev <netdev@...r.kernel.org>,
        john fastabend <john.fastabend@...il.com>, tgraf@...g.ch,
        Martin KaFai Lau <kafai@...com>,
        Nitin Hande <nitin.hande@...il.com>, mauricio.vasquez@...ito.it
Subject: Re: [PATCHv2 bpf-next 07/11] bpf: Add helper to retrieve socket in BPF

On Mon, 24 Sep 2018 at 05:51, Daniel Borkmann <daniel@...earbox.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.