Re: [PATCH] bpf: fix null ptr deref in dev_map_enqueue

[Edward Adam Davis]

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