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

 2018-05-14 23:40 GMT+01:00 Daniel Borkmann [off-list ref]:

On 05/12/2018 07:25 PM, Mathieu Xhonneux wrote:
[...]

quoted

+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.

quoted

+     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.

quoted

+     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.