| 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: Wed, 13 Jun 2018 19:56:15 -0700
From: Y Song <ys114321@...il.com>
To: Toshiaki Makita <makita.toshiaki@....ntt.co.jp>
Cc: Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
netdev <netdev@...r.kernel.org>,
Jesper Dangaard Brouer <brouer@...hat.com>
Subject: Re: [PATCH bpf v2] xdp: Fix handling of devmap in generic XDP
On Wed, Jun 13, 2018 at 7:07 PM, Toshiaki Makita
<makita.toshiaki@....ntt.co.jp> wrote:
> Commit 67f29e07e131 ("bpf: devmap introduce dev_map_enqueue") changed
> the return value type of __devmap_lookup_elem() from struct net_device *
> to struct bpf_dtab_netdev * but forgot to modify generic XDP code
> accordingly.
> Thus generic XDP incorrectly used struct bpf_dtab_netdev where struct
> net_device is expected, then skb->dev was set to invalid value.
>
> v2:
> - Fix compiler warning without CONFIG_BPF_SYSCALL.
>
> Fixes: 67f29e07e131 ("bpf: devmap introduce dev_map_enqueue")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@....ntt.co.jp>
> ---
> include/linux/bpf.h | 12 ++++++++++++
> include/linux/filter.h | 16 ++++++++++++++++
> kernel/bpf/devmap.c | 14 ++++++++++++++
> net/core/filter.c | 21 ++++-----------------
> 4 files changed, 46 insertions(+), 17 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 995c3b1..7df32a3 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -488,12 +488,15 @@ static inline void bpf_long_memcpy(void *dst, const void *src, u32 size)
>
> /* Map specifics */
> struct xdp_buff;
> +struct sk_buff;
>
> struct bpf_dtab_netdev *__dev_map_lookup_elem(struct bpf_map *map, u32 key);
> void __dev_map_insert_ctx(struct bpf_map *map, u32 index);
> void __dev_map_flush(struct bpf_map *map);
> int dev_map_enqueue(struct bpf_dtab_netdev *dst, struct xdp_buff *xdp,
> struct net_device *dev_rx);
> +int dev_map_generic_redirect(struct bpf_dtab_netdev *dst, struct sk_buff *skb,
> + struct bpf_prog *xdp_prog);
>
> struct bpf_cpu_map_entry *__cpu_map_lookup_elem(struct bpf_map *map, u32 key);
> void __cpu_map_insert_ctx(struct bpf_map *map, u32 index);
> @@ -586,6 +589,15 @@ int dev_map_enqueue(struct bpf_dtab_netdev *dst, struct xdp_buff *xdp,
> return 0;
> }
>
> +struct sk_buff;
> +
> +static inline int dev_map_generic_redirect(struct bpf_dtab_netdev *dst,
> + struct sk_buff *skb,
> + struct bpf_prog *xdp_prog)
> +{
> + return 0;
should you return an error code here?
> +}
> +
> static inline
> struct bpf_cpu_map_entry *__cpu_map_lookup_elem(struct bpf_map *map, u32 key)
> {
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 45fc0f5..8ddff1f 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -19,6 +19,7 @@
> #include <linux/cryptohash.h>
> #include <linux/set_memory.h>
> #include <linux/kallsyms.h>
> +#include <linux/if_vlan.h>
>
> #include <net/sch_generic.h>
>
> @@ -786,6 +787,21 @@ static inline bool bpf_dump_raw_ok(void)
> struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
> const struct bpf_insn *patch, u32 len);
>
> +static inline int __xdp_generic_ok_fwd_dev(struct sk_buff *skb,
> + struct net_device *fwd)
Previously this function is only used in filter.c and now it is only
used in devmap.c. Maybe this function should be in devmap.c
until it will be used cross different files?
> +{
> + unsigned int len;
> +
> + if (unlikely(!(fwd->flags & IFF_UP)))
> + return -ENETDOWN;
> +
> + len = fwd->mtu + fwd->hard_header_len + VLAN_HLEN;
> + if (skb->len > len)
> + return -EMSGSIZE;
> +
> + return 0;
> +}
> +
> /* The pair of xdp_do_redirect and xdp_do_flush_map MUST be called in the
> * same cpu context. Further for best results no more than a single map
> * for the do_redirect/do_flush pair should be used. This limitation is
> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> index a7cc7b3..642c97f 100644
> --- a/kernel/bpf/devmap.c
> +++ b/kernel/bpf/devmap.c
> @@ -345,6 +345,20 @@ int dev_map_enqueue(struct bpf_dtab_netdev *dst, struct xdp_buff *xdp,
> return bq_enqueue(dst, xdpf, dev_rx);
> }
>
> +int dev_map_generic_redirect(struct bpf_dtab_netdev *dst, struct sk_buff *skb,
> + struct bpf_prog *xdp_prog)
> +{
> + int err;
> +
> + err = __xdp_generic_ok_fwd_dev(skb, dst->dev);
> + if (unlikely(err))
> + return err;
> + skb->dev = dst->dev;
> + generic_xdp_tx(skb, xdp_prog);
> +
> + return 0;
> +}
> +
> static void *dev_map_lookup_elem(struct bpf_map *map, void *key)
> {
> struct bpf_dtab_netdev *obj = __dev_map_lookup_elem(map, *(u32 *)key);
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 3d9ba7e..e7f12e9 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3214,20 +3214,6 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
> }
> EXPORT_SYMBOL_GPL(xdp_do_redirect);
>
> -static int __xdp_generic_ok_fwd_dev(struct sk_buff *skb, struct net_device *fwd)
> -{
> - unsigned int len;
> -
> - if (unlikely(!(fwd->flags & IFF_UP)))
> - return -ENETDOWN;
> -
> - len = fwd->mtu + fwd->hard_header_len + VLAN_HLEN;
> - if (skb->len > len)
> - return -EMSGSIZE;
> -
> - return 0;
> -}
> -
> static int xdp_do_generic_redirect_map(struct net_device *dev,
> struct sk_buff *skb,
> struct xdp_buff *xdp,
> @@ -3256,10 +3242,11 @@ static int xdp_do_generic_redirect_map(struct net_device *dev,
> }
>
> if (map->map_type == BPF_MAP_TYPE_DEVMAP) {
> - if (unlikely((err = __xdp_generic_ok_fwd_dev(skb, fwd))))
> + struct bpf_dtab_netdev *dst = fwd;
> +
> + err = dev_map_generic_redirect(dst, skb, xdp_prog);
> + if (unlikely(err))
> goto err;
> - skb->dev = fwd;
> - generic_xdp_tx(skb, xdp_prog);
> } else if (map->map_type == BPF_MAP_TYPE_XSKMAP) {
> struct xdp_sock *xs = fwd;
>
> --
> 1.8.3.1
>
>
Powered by blists - more mailing lists