linux-kernel - Re: [PATCH 1/2] staging: rtl8192u: Refactoring setKey function

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: <d3725a08531898adc1bfb1e6d875888c434b42d6.camel@perches.com>
Date:   Tue, 14 Apr 2020 09:01:18 -0700
From:   Joe Perches <joe@...ches.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>,
        Camylla Goncalves Cantanheide <c.cantanheide@...il.com>
Cc:     gregkh@...uxfoundation.org, navid.emamdoost@...il.com,
        sylphrenadin@...il.com, nishkadg.linux@...il.com,
        stephen@...nnan.io, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org, lkcamp@...ts.libreplanetbr.org
Subject: Re: [PATCH 1/2] staging: rtl8192u: Refactoring setKey function

On Tue, 2020-04-14 at 15:33 +0300, Dan Carpenter wrote:
> On Mon, Apr 13, 2020 at 03:01:28AM +0000, Camylla Goncalves Cantanheide wrote:
> > Changes of the local variable value and
> > modification in the seletive repetition structure.
> > 
> 
> This changelog isn't totally clear why you're doing this.  Just say:
> "I am refactorying setKey() to make it more clear.  I have unrolled the
> first two iterations through the loop.  This patch will not change
> runtime."
> 
> So long as it's clear what you're trying to do and why, that's the
> important thing with a commit message.
> 
> > Signed-off-by: Camylla Goncalves Cantanheide <c.cantanheide@...il.com>
> > ---
> >  drivers/staging/rtl8192u/r8192U_core.c | 52 ++++++++++++--------------
> >  1 file changed, 24 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8192u/r8192U_core.c b/drivers/staging/rtl8192u/r8192U_core.c
> > index 9b8d85a4855d..87c02aee3854 100644
> > --- a/drivers/staging/rtl8192u/r8192U_core.c
> > +++ b/drivers/staging/rtl8192u/r8192U_core.c
> > @@ -4880,7 +4880,7 @@ void EnableHWSecurityConfig8192(struct net_device *dev)
> >  void setKey(struct net_device *dev, u8 entryno, u8 keyindex, u16 keytype,
> >  	    u8 *macaddr, u8 defaultkey, u32 *keycontent)
> >  {
> > -	u32 target_command = 0;
> > +	u32 target_command = CAM_CONTENT_COUNT * entryno |  BIT(31) | BIT(16);
> >  	u32 target_content = 0;
> >  	u16 us_config = 0;
> >  	u8 i;
> > @@ -4890,39 +4890,35 @@ void setKey(struct net_device *dev, u8 entryno, u8 keyindex, u16 keytype,
> >  
> >  	RT_TRACE(COMP_SEC,
> >  		 "====>to %s, dev:%p, EntryNo:%d, KeyIndex:%d, KeyType:%d, MacAddr%pM\n",
> > -        	 __func__, dev, entryno, keyindex, keytype, macaddr);
> > +		 __func__, dev, entryno, keyindex, keytype, macaddr);
> 
> Do this white space change in a separate patch.
> 
> >  
> >  	if (defaultkey)
> >  		us_config |= BIT(15) | (keytype << 2);
> >  	else
> >  		us_config |= BIT(15) | (keytype << 2) | keyindex;
> >  
> > -	for (i = 0; i < CAM_CONTENT_COUNT; i++) {
> > -		target_command  = i + CAM_CONTENT_COUNT * entryno;
> > -		target_command |= BIT(31) | BIT(16);
> > -
> > -		if (i == 0) { /* MAC|Config */
> > -			target_content = (u32)(*(macaddr + 0)) << 16 |
> > -					(u32)(*(macaddr + 1)) << 24 |
> > -					(u32)us_config;
> > -
> > -			write_nic_dword(dev, WCAMI, target_content);
> > -			write_nic_dword(dev, RWCAM, target_command);
> > -		} else if (i == 1) { /* MAC */
> > -			target_content = (u32)(*(macaddr + 2))	 |
> > -					(u32)(*(macaddr + 3)) <<  8 |
> > -					(u32)(*(macaddr + 4)) << 16 |
> > -					(u32)(*(macaddr + 5)) << 24;
> > -			write_nic_dword(dev, WCAMI, target_content);
> > -			write_nic_dword(dev, RWCAM, target_command);
> > -		} else {
> > -			/* Key Material */
> > -			if (keycontent) {
> > -				write_nic_dword(dev, WCAMI,
> > -						*(keycontent + i - 2));
> > -				write_nic_dword(dev, RWCAM, target_command);
> > -                	}
> > -		}
> > +	target_content = macaddr[0] << 16 |
> > +			 macaddr[0] << 24 |
> > +			(u32)us_config;
> > +
> > +	write_nic_dword(dev, WCAMI, target_content);
> > +	write_nic_dword(dev, RWCAM, target_command++);
> > +
> > +	/* MAC */
> > +	target_content = macaddr[2]	  |
> > +			 macaddr[3] <<  8 |
> > +			 macaddr[4] << 16 |
> > +			 macaddr[5] << 24;
> > +	write_nic_dword(dev, WCAMI, target_content);
> > +	write_nic_dword(dev, RWCAM, target_command++);
> > +
> > +	/* Key Material */
> > +	if (!keycontent)
> > +		return;
> > +
> > +	for (i = 2; i < CAM_CONTENT_COUNT; i++) {
> > +		write_nic_dword(dev, WCAMI, *keycontent++);
> 
> This code was wrong in the original as well, but now that I see the bug
> let's fix it.  CAM_CONTENT_COUNT is 8.  8 - 2 = 6.  We are writing 6
> u32 variables to write_nic_dword().  But the *keycontent buffer only has
> 4 u32 variables so it is a buffer overflow.

Did you find the overflow with smatch?

Perhaps this but it'll write 0's for the
last couple cam entries instead of random
address values.

Dunno what effect that'd have.  Likely none.

---
 drivers/staging/rtl8192u/r8192U_wx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8192u/r8192U_wx.c b/drivers/staging/rtl8192u/r8192U_wx.c
index 100532..923c0c 100644
--- a/drivers/staging/rtl8192u/r8192U_wx.c
+++ b/drivers/staging/rtl8192u/r8192U_wx.c
@@ -733,7 +733,7 @@ static int r8192_wx_set_enc_ext(struct net_device *dev,
 	{
 		u8 broadcast_addr[6] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
 		u8 zero[6] = {0};
-		u32 key[4] = {0};
+		u32 key[6] = {0};
 		struct iw_encode_ext *ext = (struct iw_encode_ext *)extra;
 		struct iw_point *encoding = &wrqu->encoding;
 		u8 idx = 0, alg = 0, group = 0;