On Mon, 1 Apr 2024 13:00:12 +0200, Jesper Dangaard Brouer wrote:
> > [Fix]
> > On the execution path of bpf_prog_test_run(), due to ri->map being NULL,
> > ri->tgtvalue was not set correctly.
> >
> > Reported-and-tested-by: syzbot+af9492708df9797198d6@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: Edward Adam Davis <eadavis@xxxxxx>
> > ---
> > kernel/bpf/devmap.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> > index 4e2cdbb5629f..ef20de14154a 100644
> > --- a/kernel/bpf/devmap.c
> > +++ b/kernel/bpf/devmap.c
> > @@ -86,6 +86,7 @@ struct bpf_dtab {
> > static DEFINE_PER_CPU(struct list_head, dev_flush_list);
> > static DEFINE_SPINLOCK(dev_map_lock);
> > static LIST_HEAD(dev_map_list);
> > +static bool is_valid_dst(struct bpf_dtab_netdev *obj, struct xdp_frame *xdpf);
> >
> > static struct hlist_head *dev_map_create_hash(unsigned int entries,
> > int numa_node)
> > @@ -536,7 +537,10 @@ int dev_xdp_enqueue(struct net_device *dev, struct xdp_frame *xdpf,
> > int dev_map_enqueue(struct bpf_dtab_netdev *dst, struct xdp_frame *xdpf,
> > struct net_device *dev_rx)
> > {
> > - struct net_device *dev = dst->dev;
> > + struct net_device *dev;
> > + if (!is_valid_dst(dst, xdpf))
>
> This is overkill, because __xdp_enqueue() already contains most of the
> checks in is_valid_dst().
>
> Why not:
>
> if (!dst)
> return -EINVAL;
This can work, but I think is_valid_dst() is better, as its internal inspection
of dst is more thorough.
>
>
> > + return -EINVAL;
> > + dev = dst->dev;
> >
> > return __xdp_enqueue(dev, xdpf, dev_rx, dst->xdp_prog);
> > }
>
>
> Is this fix pampering over another issue?
>
> To repeat myself:
> I think something is wrong in xdp_test_run_batch().
> The `ri->tgt_value` is being set in __bpf_xdp_redirect_map(), but I
> cannot see __bpf_xdp_redirect_map() being used in xdp_test_run_batch().
As I mentioned earlier, because the value of "ri->map" is NULL, even if it can
be executed to __bpf_xdp_redirect_map(), it is meaningless because "ri->map" is
used internally to set "ri->tgt_value".
>
> Is this a case of XDP program returning XDP_REDIRECT without having
> called the BPF helper for redirect?
Edward
