netdev - Re: [PATCH bpf-next v5 3/6] bpf: Add IPv6 Segment Routing helpers

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: <CAKSCvkS0tJ1jsedBQcu2FnZxQNNODJN-zWdUJHJzVxZjFPDxTg@mail.gmail.com>
Date:   Wed, 16 May 2018 22:59:48 +0100
From:   Mathieu Xhonneux <m.xhonneux@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     netdev@...r.kernel.org, David Lebrun <dlebrun@...gle.com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>
Subject: Re: [PATCH bpf-next v5 3/6] bpf: Add IPv6 Segment Routing helpers

 2018-05-14 23:40 GMT+01:00 Daniel Borkmann <daniel@...earbox.net>:
> On 05/12/2018 07:25 PM, Mathieu Xhonneux wrote:
> [...]
>> +BPF_CALL_4(bpf_lwt_seg6_store_bytes, struct sk_buff *, skb, u32, offset,
>> +        const void *, from, u32, len)
>> +{
>> +#if IS_ENABLED(CONFIG_IPV6_SEG6_BPF)
>> +     struct seg6_bpf_srh_state *srh_state =
>> +             this_cpu_ptr(&seg6_bpf_srh_states);
>> +     void *srh_tlvs, *srh_end, *ptr;
>> +     struct ipv6_sr_hdr *srh;
>> +     int srhoff = 0;
>> +
>> +     if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
>> +             return -EINVAL;
>> +
>> +     srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
>> +     srh_tlvs = (void *)((char *)srh + ((srh->first_segment + 1) << 4));
>> +     srh_end = (void *)((char *)srh + sizeof(*srh) + srh_state->hdrlen);
>
> Do we need to check that this cannot go out of bounds wrt skb data?
input_action_bpf_end (which calls the BPF program) already verifies
using get_srh() that the whole SRH is accessible and is not out of
bounds.
The seg6 helpers (e.g. bpf_lwt_seg6_adjust_srh) then modify
srh_state->hdrlen following the evolution of the SRH in size. I don't
think that a check on srh_end is needed here as the SRH is already
verified once, and srh_state->hdrlen is then updated to keep this
bound correct.

>> +     ptr = skb->data + offset;
>> +     if (ptr >= srh_tlvs && ptr + len <= srh_end)
>> +             srh_state->valid = 0;
>> +     else if (ptr < (void *)&srh->flags ||
>> +              ptr + len > (void *)&srh->segments)
>> +             return -EFAULT;
>> +
>> +     if (unlikely(bpf_try_make_writable(skb, offset + len)))
>> +             return -EFAULT;
>> +
>> +     memcpy(ptr, from, len);
>
> You have a use after free here. bpf_try_make_writable() is potentially changing
> underlying skb->data (e.g. see pskb_expand_head()). Therefore memcpy()'ing into
> cached ptr is invalid.
>
OK.

>> +     if (len > 0) {
>> +             ret = skb_cow_head(skb, len);
>> +             if (unlikely(ret < 0))
>> +                     return ret;
>> +
>> +             ret = bpf_skb_net_hdr_push(skb, offset, len);
>> +     } else {
>> +             ret = bpf_skb_net_hdr_pop(skb, offset, -1 * len);
>> +     }
>> +     if (unlikely(ret < 0))
>> +             return ret;
>
> And here as well. You changed underlying pointers via skb_cow_head(), but in
> the error path you leave the cached pointers that now point to already freed
> buffer. Thus, you'd now be able to access the new skb data out of bounds since
> cb->data_end is still the old one due to missing bpf_compute_data_pointers(skb).
> Please fix and audit your whole series carefully against these types of subtle
> bugs.

Right.
I went through the whole series again. I found a similar mistake in
bpf_push_seg6_encap, and added also a bpf_compute_data_pointers(skb)
there. I didn't find anything else, so I hope that we're covered here
(bpf_lwt_seg6_store_bytes, bpf_lwt_seg6_adjust_srh and
bpf_push_seg6_encap are the only functions modifying the packet in
this series).

Thanks. I'll submit a v6 ASAP.