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

From: Jesper Dangaard Brouer
Date: Thu Apr 04 2024 - 07:03:47 EST




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;
}