linux-kernel - Re: [PATCH] bpf: fix null ptr deref in dev_map

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <15064d90-e89d-4522-90a8-52aad643d7c8@kernel.org>
Date: Thu, 4 Apr 2024 13:03:17 +0200
From: Jesper Dangaard Brouer <hawk@...nel.org>
To: Edward Adam Davis <eadavis@...com>
Cc: andrii@...nel.org, ast@...nel.org, bpf@...r.kernel.org,
 daniel@...earbox.net, davem@...emloft.net, eddyz87@...il.com,
 haoluo@...gle.com, john.fastabend@...il.com, jolsa@...nel.org,
 kpsingh@...nel.org, kuba@...nel.org, linux-kernel@...r.kernel.org,
 martin.lau@...ux.dev, netdev@...r.kernel.org, sdf@...gle.com,
 song@...nel.org, syzbot+af9492708df9797198d6@...kaller.appspotmail.com,
 syzkaller-bugs@...glegroups.com, yonghong.song@...ux.dev
Subject: Re: [PATCH] bpf: fix null ptr deref in dev_map_enqueue



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