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: <996e0e51-019a-5d65-026a-9599f9228299@linux.com>
Date:   Wed, 9 Oct 2019 13:43:09 +0300
From:   Denis Efremov <efremov@...ux.com>
To:     Hans de Goede <hdegoede@...hat.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,

On 09.10.2019 12:35, Hans de Goede wrote:
> 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.
> 

Thank you for perfect investigation! I can only agree with you, because this
code is buggy. It looks like no one faced this bug previously and the code
can be safely removed.

Best Regards,
Denis

> 
>>
>>   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
>>
>