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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <tencent_A4FA0DA89270DDAC5D8519424F9B0DB42507@qq.com>
Date: Tue,  2 Apr 2024 11:03:56 +0800
From: Edward Adam Davis <eadavis@...com>
To: hawk@...nel.org
Cc: andrii@...nel.org,
	ast@...nel.org,
	bpf@...r.kernel.org,
	daniel@...earbox.net,
	davem@...emloft.net,
	eadavis@...com,
	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 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.
> 
> 
> > +		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