linux-kernel - Re: [PATCH] staging: rtl8723bs: hal: Fix memcpy calls

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: <94af475e-dd7a-6066-146a-30a9915cd325@redhat.com>
Date:   Wed, 9 Oct 2019 11:35:32 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Denis Efremov <efremov@...ux.com>, devel@...verdev.osuosl.org
Cc:     linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Bastien Nocera <hadess@...ess.net>,
        Larry Finger <Larry.Finger@...inger.net>,
        Jes Sorensen <jes.sorensen@...il.com>, stable@...r.kernel.org
Subject: Re: [PATCH] staging: rtl8723bs: hal: Fix memcpy calls

Hi Denis,

On 30-09-2019 13:01, Denis Efremov wrote:
> memcpy() in phy_ConfigBBWithParaFile() and PHY_ConfigRFWithParaFile() is
> called with "src == NULL && len == 0". This is an undefined behavior.
> Moreover this if pre-condition "pBufLen && (*pBufLen == 0) && !pBuf"
> is constantly false because it is a nested if in the else brach, i.e.,
> "if (cond) { ... } else { if (cond) {...} }". This patch alters the
> if condition to check "pBufLen && pBuf" pointers are not NULL.
> 
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: Hans de Goede <hdegoede@...hat.com>
> Cc: Bastien Nocera <hadess@...ess.net>
> Cc: Larry Finger <Larry.Finger@...inger.net>
> Cc: Jes Sorensen <jes.sorensen@...il.com>
> Cc: stable@...r.kernel.org
> Signed-off-by: Denis Efremov <efremov@...ux.com>
> ---
> Not tested. I don't have the hardware. The fix is based on my guess.

Thsnk you for your patch.

So I've been doing some digging and this code normally never executes.

For this to execute the user would need to change the rtw_load_phy_file module
param from its default of 0x44 (LOAD_BB_PG_PARA_FILE | LOAD_RF_TXPWR_LMT_PARA_FILE)
to something which includes 0x02 (LOAD_BB_PARA_FILE) as mask.

And even with that param set for this code to actually do something /
for pBuf to ever not be NULL the following conditions would have to
be true:

1) Set the rtw_load_phy_file module param from its default of
    0x44 (LOAD_BB_PG_PARA_FILE | LOAD_RF_TXPWR_LMT_PARA_FILE) to something
    which includes 0x02 as mask; and
2) Set rtw_phy_file_path module parameter to say "/lib/firmware/"; and
3) Store a /lib/firmware/rtl8723b/PHY_REG.txt file in the expected format.

So I've come to the conclusion that all the phy_Config*WithParaFile functions
(and a bunch of stuff they use) can be removed.

I will prepare and submit a patch for this.

Regards,

Hans



> 
>   drivers/staging/rtl8723bs/hal/hal_com_phycfg.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/hal/hal_com_phycfg.c b/drivers/staging/rtl8723bs/hal/hal_com_phycfg.c
> index 6539bee9b5ba..0902dc3c1825 100644
> --- a/drivers/staging/rtl8723bs/hal/hal_com_phycfg.c
> +++ b/drivers/staging/rtl8723bs/hal/hal_com_phycfg.c
> @@ -2320,7 +2320,7 @@ int phy_ConfigBBWithParaFile(
>   			}
>   		}
>   	} else {
> -		if (pBufLen && (*pBufLen == 0) && !pBuf) {
> +		if (pBufLen && pBuf) {
>   			memcpy(pHalData->para_file_buf, pBuf, *pBufLen);
>   			rtStatus = _SUCCESS;
>   		} else
> @@ -2752,7 +2752,7 @@ int PHY_ConfigRFWithParaFile(
>   			}
>   		}
>   	} else {
> -		if (pBufLen && (*pBufLen == 0) && !pBuf) {
> +		if (pBufLen && pBuf) {
>   			memcpy(pHalData->para_file_buf, pBuf, *pBufLen);
>   			rtStatus = _SUCCESS;
>   		} else
>