netdev - Re: [PATCH] wifi: mac80211: sband's null check should precede params

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]

Date: Wed, 29 Nov 2023 09:33:23 +0100
From: Johannes Berg <johannes@...solutions.net>
To: Edward Adam Davis <eadavis@...com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	linux-kernel@...r.kernel.org, linux-wireless@...r.kernel.org, 
	llvm@...ts.linux.dev, nathan@...nel.org, ndesaulniers@...gle.com, 
	netdev@...r.kernel.org, pabeni@...hat.com, 
	syzbot+62d7eef57b09bfebcd84@...kaller.appspotmail.com, 
	syzkaller-bugs@...glegroups.com, trix@...hat.com
Subject: Re: [PATCH] wifi: mac80211: sband's null check should precede params

On Wed, 2023-11-29 at 16:18 +0800, Edward Adam Davis wrote:
> On Wed, 29 Nov 2023 07:57:07 +0100, Johannes Berg wrote:
> > > [Analysis]
> > > When ieee80211_get_link_sband() fails to find a valid sband and first checks
> > > for params in sta_link_apply_parameters(), it will return 0 due to new_link
> > > being 0, which will lead to an incorrect process after sta_apply_parameters().
> > > 
> > > [Fix]
> > > First obtain sband and perform a non null check before checking the params.
> > 
> > Not sure I can even disagree with that analysis, it seems right, but ...
> > 
> > > +	if (!link || !link_sta)
> > > +		return -EINVAL;
> > > +
> > > +	sband = ieee80211_get_link_sband(link);
> > > +	if (!sband)
> > > +		return -EINVAL;
> > > +
> > >  	/*
> > >  	 * If there are no changes, then accept a link that doesn't exist,
> > >  	 * unless it's a new link.
> > 
> > There's a comment here which is clearly not true after this change,
> > since you've already returned for !link_sta?
> No, after applying my patch, it will return due to !sband.
> 

Right, OK, but the way I read the comment (now) is that it wanted to
accept it in that case?

That said, I just threw the patch into our internal testing machinery
quickly (probably has more MLO tests than upstream hostap for now), and
it worked just fine ...

Maybe we should just remove the comment?

johannes