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: <20200414123326.GG1163@kadam>
Date:   Tue, 14 Apr 2020 15:33:26 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     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 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.

> +		write_nic_dword(dev, RWCAM, target_command++);
>  	}
>  }

regards,
dan carpenter