netdev - Re: [bpf-next v1 8/9] bpf: Provide helper to do lookups in kernel FIB table

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: <df3ac0c5-de3c-f47d-2d8b-1b3b42a51187@iogearbox.net>
Date:   Fri, 4 May 2018 02:45:36 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     David Ahern <dsahern@...il.com>, netdev@...r.kernel.org,
        borkmann@...earbox.net, ast@...nel.org
Cc:     davem@...emloft.net, shm@...ulusnetworks.com,
        roopa@...ulusnetworks.com, brouer@...hat.com, toke@...e.dk,
        john.fastabend@...il.com
Subject: Re: [bpf-next v1 8/9] bpf: Provide helper to do lookups in kernel FIB
 table

On 05/03/2018 05:53 AM, David Ahern wrote:
[...]
> +
> +BPF_CALL_4(bpf_xdp_fib_lookup, struct xdp_buff *, ctx,
> +	   struct bpf_fib_lookup *, params, int, plen, u32, flags)
> +{
> +	if (plen < sizeof(*params))
> +		return -EINVAL;
> +
> +	switch (params->family) {
> +#if IS_ENABLED(CONFIG_INET)
> +	case AF_INET:
> +		return bpf_ipv4_fib_lookup(dev_net(ctx->rxq->dev), params,
> +					   flags);
> +#endif
> +#if IS_ENABLED(CONFIG_IPV6)
> +	case AF_INET6:
> +		return bpf_ipv6_fib_lookup(dev_net(ctx->rxq->dev), params,
> +					   flags);
> +#endif
> +	}
> +	return -ENOTSUPP;
> +}
> +
> +static const struct bpf_func_proto bpf_xdp_fib_lookup_proto = {
> +	.func		= bpf_xdp_fib_lookup,
> +	.gpl_only	= true,
> +	.pkt_access	= true,

Hmm, this one and ...

> +	.ret_type	= RET_INTEGER,
> +	.arg1_type      = ARG_PTR_TO_CTX,
> +	.arg2_type      = ARG_PTR_TO_MEM,
> +	.arg3_type      = ARG_CONST_SIZE,
> +	.arg4_type	= ARG_ANYTHING,
> +};
> +
> +BPF_CALL_4(bpf_skb_fib_lookup, struct sk_buff *, skb,
> +	   struct bpf_fib_lookup *, params, int, plen, u32, flags)
> +{
> +	if (plen < sizeof(*params))
> +		return -EINVAL;
> +
> +	switch (params->family) {
> +#if IS_ENABLED(CONFIG_INET)
> +	case AF_INET:
> +		return bpf_ipv4_fib_lookup(dev_net(skb->dev), params, flags);
> +#endif
> +#if IS_ENABLED(CONFIG_IPV6)
> +	case AF_INET6:
> +		return bpf_ipv6_fib_lookup(dev_net(skb->dev), params, flags);
> +#endif
> +	}
> +	return -ENOTSUPP;
> +}
> +
> +static const struct bpf_func_proto bpf_skb_fib_lookup_proto = {
> +	.func		= bpf_skb_fib_lookup,
> +	.gpl_only	= true,
> +	.pkt_access	= true,

... this should both not be marked as pkt_access = true. What this means is that
arg2, which is the struct bpf_fib_lookup, could come from the raw packet buffer.
Meaning instead of the struct sitting on stack it could in that case be passed
out of the packet directly. We have this enabled in helpers like csum_diff() or
map lookup/update where it would make sense to pass raw buffer there, but here
it seems unintentional and should be removed instead. See also description in
last paragraph in 36bbef52c7eb ("bpf: direct packet write and access for helpers
for clsact progs").

> +	.ret_type	= RET_INTEGER,
> +	.arg1_type      = ARG_PTR_TO_CTX,
> +	.arg2_type      = ARG_PTR_TO_MEM,
> +	.arg3_type      = ARG_CONST_SIZE,
> +	.arg4_type	= ARG_ANYTHING,
> +};
> +
>  static const struct bpf_func_proto *
>  bpf_base_func_proto(enum bpf_func_id func_id)
>  {
> @@ -3933,6 +4192,8 @@ tc_cls_act_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>  	case BPF_FUNC_skb_get_xfrm_state:
>  		return &bpf_skb_get_xfrm_state_proto;
>  #endif
> +	case BPF_FUNC_fib_lookup:
> +		return &bpf_skb_fib_lookup_proto;
>  	default:
>  		return bpf_base_func_proto(func_id);
>  	}
> @@ -3958,6 +4219,8 @@ xdp_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>  		return &bpf_xdp_redirect_map_proto;
>  	case BPF_FUNC_xdp_adjust_tail:
>  		return &bpf_xdp_adjust_tail_proto;
> +	case BPF_FUNC_fib_lookup:
> +		return &bpf_xdp_fib_lookup_proto;
>  	default:
>  		return bpf_base_func_proto(func_id);
>  	}
>