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

[Jesper Dangaard Brouer <hawk@kernel.org>]

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@syzkaller.appspotmail.com
>>> Signed-off-by: Edward Adam Davis<eadavis@qq.com>
>>> ---
>>>    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;
}