Received-SPF: pass (google.com: domain of linux-kernel+bounces-131280-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <15064d90-e89d-4522-90a8-52aad643d7c8@kernel.org>
Date: Thu, 4 Apr 2024 13:03:17 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] bpf: fix null ptr deref in dev_map_enqueue
To: Edward Adam Davis <eadavis@qq.com>
Cc: andrii@kernel.org, ast@kernel.org, bpf@vger.kernel.org,
 daniel@iogearbox.net, davem@davemloft.net, eddyz87@gmail.com,
 haoluo@google.com, john.fastabend@gmail.com, jolsa@kernel.org,
 kpsingh@kernel.org, kuba@kernel.org, linux-kernel@vger.kernel.org,
 martin.lau@linux.dev, netdev@vger.kernel.org, sdf@google.com,
 song@kernel.org, syzbot+af9492708df9797198d6@syzkaller.appspotmail.com,
 syzkaller-bugs@googlegroups.com, yonghong.song@linux.dev
References: <f45ad3e1-4433-422e-be28-17deaba4ade1@kernel.org>
 <tencent_A4FA0DA89270DDAC5D8519424F9B0DB42507@qq.com>
Content-Language: en-US
From: Jesper Dangaard Brouer <hawk@kernel.org>
In-Reply-To: <tencent_A4FA0DA89270DDAC5D8519424F9B0DB42507@qq.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


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