On 09/06/2017 11:18 AM, Jesper Dangaard Brouer wrote: > On Wed, 06 Sep 2017 18:24:07 +0200 > Daniel Borkmann <dan...@iogearbox.net> wrote: > >> On 09/06/2017 05:26 PM, Jesper Dangaard Brouer wrote: >>> Using bpf_redirect_map is allowed for generic XDP programs, but the >>> appropriate map lookup was never performed in xdp_do_generic_redirect(). >>> >>> Instead the map-index is directly used as the ifindex. For the >> >> Good point, but ... >> >> [...] >>> net/core/filter.c | 29 +++++++++++++++++++++++++++++ >>> 1 file changed, 29 insertions(+) >>> >>> diff --git a/net/core/filter.c b/net/core/filter.c >>> index 5912c738a7b2..6a4745bf2c9f 100644 >>> --- a/net/core/filter.c >>> +++ b/net/core/filter.c >>> @@ -2562,6 +2562,32 @@ int xdp_do_redirect(struct net_device *dev, struct >>> xdp_buff *xdp, >>> } >>> EXPORT_SYMBOL_GPL(xdp_do_redirect); >>> >>> +static int xdp_do_generic_redirect_map(struct net_device *dev, >>> + struct sk_buff *skb, >>> + struct bpf_prog *xdp_prog) >>> +{ >>> + struct redirect_info *ri = this_cpu_ptr(&redirect_info); >>> + struct bpf_map *map = ri->map; >>> + u32 index = ri->ifindex; >>> + struct net_device *fwd; >>> + int err; >>> + >>> + ri->ifindex = 0; >>> + ri->map = NULL; >>> + >>> + fwd = __dev_map_lookup_elem(map, index); >>> + if (!fwd) { >>> + err = -EINVAL; >>> + goto err; >>> + } >>> + skb->dev = fwd; >>> + _trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index); >>> + return 0; >>> +err: >>> + _trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map, index, err); >>> + return err; >>> +} >>> + >>> int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb, >>> struct bpf_prog *xdp_prog) >>> { >>> @@ -2571,6 +2597,9 @@ int xdp_do_generic_redirect(struct net_device *dev, >>> struct sk_buff *skb, >>> unsigned int len; >>> int err = 0; >>> >>> + if (ri->map) >>> + return xdp_do_generic_redirect_map(dev, skb, xdp_prog); >> >> This is not quite correct. Really, the only thing you want >> to do here is more or less ... >> >> int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb, >> struct bpf_prog *xdp_prog) >> { >> struct redirect_info *ri = this_cpu_ptr(&redirect_info); >> struct bpf_map *map = ri->map; >> u32 index = ri->ifindex; >> struct net_device *fwd; >> unsigned int len; >> int err = 0; >> >> ri->ifindex = 0; >> ri->map = NULL; >> >> if (map) >> fwd = __dev_map_lookup_elem(map, index); >> else >> fwd = dev_get_by_index_rcu(dev_net(dev), index); >> if (unlikely(!fwd)) { >> err = -EINVAL; >> goto err; >> } >> [...] >> >> ... such that you have a common path to also do the IFF_UP >> and MTU checks that are done here, but otherwise omitted in >> your patch. > > Ah, yes. My patch miss the IFF_UP and MTU check. (I was too inspired by > xdp_do_redirect_map). > > >> Otherwise it looks good, but note that it also doesn't really >> resolve the issue you mention wrt stale map pointers by the >> way. [...] > > I actually discovered more cases where we can crash the kernel :-( > > E.g. driver not supporting XDP_REDIRECT, are still allowed to load an > XDP bpf_prog that call bpf_redirect_map() and set the ->map pointer, > but it will never call xdp_do_redirect() (which is responsible for > clearing/consuming ->map pointer). > > Another case: You can also call bpf_redirect_map() and then NOT return > XDP_REDIRECT (it is obviously strange, but the bpf-helper API allows it). >
I think we can cover both these cases with previous suggestion to check prog pointers. Working up a patch now.