2018-05-14 23:40 GMT+01:00 Daniel Borkmann <dan...@iogearbox.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.
