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

[Jesper Dangaard Brouer <hawk@xxxxxxxxxx>]

On 02/04/2024 05.03, Edward Adam Davis wrote:
> 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.


No, is_valid_dst() is not better, because it will repeat almost same
checks (as I said) as __xdp_enqueue() already contains these checks.
This is fast-path code, we don't want to repeat checks.

--Jesper
(copy-pasted function below to easier compare)

static inline int __xdp_enqueue(struct net_device *dev, struct xdp_frame 
*xdpf,
				struct net_device *dev_rx,
				struct bpf_prog *xdp_prog)
{
	int err;

	if (!(dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT))
		return -EOPNOTSUPP;

	if (unlikely(!(dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT_SG) &&
		     xdp_frame_has_frags(xdpf)))
		return -EOPNOTSUPP;

	err = xdp_ok_fwd_dev(dev, xdp_get_frame_len(xdpf));
	if (unlikely(err))
		return err;

	bq_enqueue(dev, xdpf, dev_rx, xdp_prog);
	return 0;
}


static bool is_valid_dst(struct bpf_dtab_netdev *obj, struct xdp_frame 
*xdpf)
{
	if (!obj)
		return false;

	if (!(obj->dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT))
		return false;

	if (unlikely(!(obj->dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT_SG) &&
		     xdp_frame_has_frags(xdpf)))
		return false;

	if (xdp_ok_fwd_dev(obj->dev, xdp_get_frame_len(xdpf)))
		return false;

	return true;
}