Received-SPF: pass (google.com: domain of linux-kernel+bounces-127329-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <tencent_A4FA0DA89270DDAC5D8519424F9B0DB42507@qq.com>
From: Edward Adam Davis <eadavis@qq.com>
To: hawk@kernel.org
Cc: andrii@kernel.org,
	ast@kernel.org,
	bpf@vger.kernel.org,
	daniel@iogearbox.net,
	davem@davemloft.net,
	eadavis@qq.com,
	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
Subject: Re: [PATCH] bpf: fix null ptr deref in dev_map_enqueue
Date: Tue,  2 Apr 2024 11:03:56 +0800
In-Reply-To: <f45ad3e1-4433-422e-be28-17deaba4ade1@kernel.org>
References: <f45ad3e1-4433-422e-be28-17deaba4ade1@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

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.
> 
> 
> > +		return -EINVAL;
> > +	dev = dst->dev;
> >
> >   	return __xdp_enqueue(dev, xdpf, dev_rx, dst->xdp_prog);
> >   }
> 
> 
> Is this fix pampering over another issue?
> 
> To repeat myself:
> I think something is wrong in xdp_test_run_batch().
> The `ri->tgt_value` is being set in __bpf_xdp_redirect_map(), but I
> cannot see __bpf_xdp_redirect_map() being used in xdp_test_run_batch().
As I mentioned earlier, because the value of "ri->map" is NULL, even if it can 
be executed to __bpf_xdp_redirect_map(), it is meaningless because "ri->map" is
used internally to set "ri->tgt_value".
> 
> Is this a case of XDP program returning XDP_REDIRECT without having
> called the BPF helper for redirect?

Edward