linux-kernel - Re: [PATCH v2] staging: rtl8723bs: replace ternary comparison with min

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] [day] [month] [year] [list]

Message-ID: <20260131225841.6ec19c9d@pumpkin>
Date: Sat, 31 Jan 2026 22:58:41 +0000
From: David Laight <david.laight.linux@...il.com>
To: William Hansen-Baird <william.hansen.baird@...il.com>
Cc: Dan Carpenter <dan.carpenter@...aro.org>, gregkh@...uxfoundation.org,
 straube.linux@...il.com, linux-staging@...ts.linux.dev,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] staging: rtl8723bs: replace ternary comparison with
 min_t()

On Sat, 31 Jan 2026 16:43:54 -0500
William Hansen-Baird <william.hansen.baird@...il.com> wrote:

> On Sat, Jan 31, 2026 at 08:35:55PM +0300, Dan Carpenter wrote:
> > On Sat, Jan 31, 2026 at 03:55:25PM +0000, David Laight wrote:  
> > > > > @@ -1024,7 +1025,7 @@ static unsigned short rtw_parse_assoc_security_ies(struct adapter *padapter,
> > > > >  			pstat->flags |= WLAN_STA_WPS;
> > > > >  			copy_len = 0;
> > > > >  		} else {
> > > > > -			copy_len = ((wpa_ie_len+2) > sizeof(pstat->wpa_ie)) ? (sizeof(pstat->wpa_ie)):(wpa_ie_len+2);
> > > > > +			copy_len = min_t(int, sizeof(pstat->wpa_ie), wpa_ie_len + 2);    
> > > > 
> > > > Use umin().  "int" is wrong because we don't want negative values
> > > > stored in copy_len.  
> > > 
> > > And the correct way to fix min() bleating is to change the type of the variables.
> > > Using min_t() ought to be frowned up and only used as a last resort.
> > > 
> > > In this case I think all the variables declared with wpa_ie_len can/should be
> > > unsigned int.
> > >   
> > 
> > That also works.
> >   
> Hi Dan and David,
> 
> Thanks for your help. After reviewing the code, I do see a few checks 
> ensuring wpa_ie_len is never negative.
> Therefore umin() is safe in this case, and for v3 I'll update the patch accordingly.

umin() is a bit better than min_t().

A subtle change is doing 'wpa_ie_len + 2u'.

> Would you prefer I leave it at that, or is a broader change to 
> make wpa_ie_len unsigned worth doing in a separate patch?

I scanned the code some of the variables seem to be different signedness 
in different functions.

But a 'false error' from min() is really an indication you should be looking
at changing the types of the variables themselves.

My main thought looking at that code is that functions are far too long
and have far too many local variables.
You also pretty much don't want to use u8/u16 for locals just because the
values are small, it doesn't (really) save space and can increase code size.

	David

> 
> Thanks,
> William