| 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
| ||
|
Date: Tue, 15 May 2018 00:40:42 +0200
From: Daniel Borkmann <daniel@...earbox.net>
To: Mathieu Xhonneux <m.xhonneux@...il.com>, netdev@...r.kernel.org
Cc: dlebrun@...gle.com, alexei.starovoitov@...il.com
Subject: Re: [PATCH bpf-next v5 3/6] bpf: Add IPv6 Segment Routing helpers
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?
> + 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.
> + return 0;
> +#else /* CONFIG_IPV6_SEG6_BPF */
> + return -EOPNOTSUPP;
> +#endif
> +}
> +
> +static const struct bpf_func_proto bpf_lwt_seg6_store_bytes_proto = {
> + .func = bpf_lwt_seg6_store_bytes,
> + .gpl_only = false,
> + .ret_type = RET_INTEGER,
> + .arg1_type = ARG_PTR_TO_CTX,
> + .arg2_type = ARG_ANYTHING,
> + .arg3_type = ARG_PTR_TO_MEM,
> + .arg4_type = ARG_CONST_SIZE
> +};
> +
> +BPF_CALL_4(bpf_lwt_seg6_action, struct sk_buff *, skb,
> + u32, action, void *, param, u32, param_len)
> +{
> +#if IS_ENABLED(CONFIG_IPV6_SEG6_BPF)
> + struct seg6_bpf_srh_state *srh_state =
> + this_cpu_ptr(&seg6_bpf_srh_states);
> + struct ipv6_sr_hdr *srh;
> + int srhoff = 0;
> + int err;
> +
> + if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
> + return -EINVAL;
> + srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
> +
> + if (!srh_state->valid) {
> + if (unlikely((srh_state->hdrlen & 7) != 0))
> + return -EBADMSG;
> +
> + srh->hdrlen = (u8)(srh_state->hdrlen >> 3);
> + if (unlikely(!seg6_validate_srh(srh, (srh->hdrlen + 1) << 3)))
> + return -EBADMSG;
> +
> + srh_state->valid = 1;
> + }
> +
> + switch (action) {
> + case SEG6_LOCAL_ACTION_END_X:
> + if (param_len != sizeof(struct in6_addr))
> + return -EINVAL;
> + return seg6_lookup_nexthop(skb, (struct in6_addr *)param, 0);
> + case SEG6_LOCAL_ACTION_END_T:
> + if (param_len != sizeof(int))
> + return -EINVAL;
> + return seg6_lookup_nexthop(skb, NULL, *(int *)param);
> + case SEG6_LOCAL_ACTION_END_B6:
> + err = bpf_push_seg6_encap(skb, BPF_LWT_ENCAP_SEG6_INLINE,
> + param, param_len);
> + if (!err)
> + srh_state->hdrlen =
> + ((struct ipv6_sr_hdr *)param)->hdrlen << 3;
> + return err;
> + case SEG6_LOCAL_ACTION_END_B6_ENCAP:
> + err = bpf_push_seg6_encap(skb, BPF_LWT_ENCAP_SEG6,
> + param, param_len);
> + if (!err)
> + srh_state->hdrlen =
> + ((struct ipv6_sr_hdr *)param)->hdrlen << 3;
> + return err;
> + default:
> + return -EINVAL;
> + }
> +#else /* CONFIG_IPV6_SEG6_BPF */
> + return -EOPNOTSUPP;
> +#endif
> +}
> +
> +static const struct bpf_func_proto bpf_lwt_seg6_action_proto = {
> + .func = bpf_lwt_seg6_action,
> + .gpl_only = false,
> + .ret_type = RET_INTEGER,
> + .arg1_type = ARG_PTR_TO_CTX,
> + .arg2_type = ARG_ANYTHING,
> + .arg3_type = ARG_PTR_TO_MEM,
> + .arg4_type = ARG_CONST_SIZE
> +};
> +
> +BPF_CALL_3(bpf_lwt_seg6_adjust_srh, struct sk_buff *, skb, u32, offset,
> + s32, len)
> +{
> +#if IS_ENABLED(CONFIG_IPV6_SEG6_BPF)
> + struct seg6_bpf_srh_state *srh_state =
> + this_cpu_ptr(&seg6_bpf_srh_states);
> + void *srh_end, *srh_tlvs, *ptr;
> + struct ipv6_sr_hdr *srh;
> + struct ipv6hdr *hdr;
> + int srhoff = 0;
> + int ret;
> +
> + if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
> + return -EINVAL;
> + srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
> +
> + srh_tlvs = (void *)((unsigned char *)srh + sizeof(*srh) +
> + ((srh->first_segment + 1) << 4));
> + srh_end = (void *)((unsigned char *)srh + sizeof(*srh) +
> + srh_state->hdrlen);
> + ptr = skb->data + offset;
> +
> + if (unlikely(ptr < srh_tlvs || ptr > srh_end))
> + return -EFAULT;
> + if (unlikely(len < 0 && (void *)((char *)ptr - len) > srh_end))
> + return -EFAULT;
> +
> + 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.
> + hdr = (struct ipv6hdr *)skb->data;
> + hdr->payload_len = htons(skb->len - sizeof(struct ipv6hdr));
> +
> + bpf_compute_data_pointers(skb);
> + srh_state->hdrlen += len;
> + srh_state->valid = 0;
> + return 0;
> +#else /* CONFIG_IPV6_SEG6_BPF */
> + return -EOPNOTSUPP;
> +#endif
> +}
> +
> +static const struct bpf_func_proto bpf_lwt_seg6_adjust_srh_proto = {
> + .func = bpf_lwt_seg6_adjust_srh,
> + .gpl_only = false,
> + .ret_type = RET_INTEGER,
> + .arg1_type = ARG_PTR_TO_CTX,
> + .arg2_type = ARG_ANYTHING,
> + .arg3_type = ARG_ANYTHING,
> +};
> +
> +bool bpf_helper_changes_pkt_data(void *func)
> +{
> + if (func == bpf_skb_vlan_push ||
> + func == bpf_skb_vlan_pop ||
> + func == bpf_skb_store_bytes ||
> + func == bpf_skb_change_proto ||
> + func == bpf_skb_change_head ||
> + func == bpf_skb_change_tail ||
> + func == bpf_skb_adjust_room ||
> + func == bpf_skb_pull_data ||
> + func == bpf_clone_redirect ||
> + func == bpf_l3_csum_replace ||
> + func == bpf_l4_csum_replace ||
> + func == bpf_xdp_adjust_head ||
> + func == bpf_xdp_adjust_meta ||
> + func == bpf_msg_pull_data ||
> + func == bpf_xdp_adjust_tail ||
> + func == bpf_lwt_push_encap ||
> + func == bpf_lwt_seg6_store_bytes ||
> + func == bpf_lwt_seg6_adjust_srh ||
> + func == bpf_lwt_seg6_action
> + )
> + return true;
> +
> + return false;
> +}
> +
> static const struct bpf_func_proto *
> bpf_base_func_proto(enum bpf_func_id func_id)
> {
> @@ -4703,7 +4940,6 @@ static bool lwt_is_valid_access(int off, int size,
> return bpf_skb_is_valid_access(off, size, type, prog, info);
> }
>
> -
> /* Attach type specific accesses */
> static bool __sock_filter_check_attach_type(int off,
> enum bpf_access_type access_type,
> diff --git a/net/ipv6/Kconfig b/net/ipv6/Kconfig
> index 6794ddf0547c..f0e8a762ae0c 100644
> --- a/net/ipv6/Kconfig
> +++ b/net/ipv6/Kconfig
> @@ -330,4 +330,9 @@ config IPV6_SEG6_HMAC
>
> If unsure, say N.
>
> +config IPV6_SEG6_BPF
> + def_bool y
> + depends on IPV6_SEG6_LWTUNNEL
> + depends on IPV6 = y
> +
> endif # IPV6
> diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
> index e9b23fb924ad..ae68c1ef8fb0 100644
> --- a/net/ipv6/seg6_local.c
> +++ b/net/ipv6/seg6_local.c
> @@ -449,6 +449,8 @@ static int input_action_end_b6_encap(struct sk_buff *skb,
> return err;
> }
>
> +DEFINE_PER_CPU(struct seg6_bpf_srh_state, seg6_bpf_srh_states);
> +
> static struct seg6_action_desc seg6_action_table[] = {
> {
> .action = SEG6_LOCAL_ACTION_END,
>
Powered by blists - more mailing lists