Re: [PATCH] staging: rtl8723bs: hal: Fix memcpy calls

[Denis Efremov]

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@xxxxxxxxxxxxxxxxxxx>
>> Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
>> Cc: Bastien Nocera <hadess@xxxxxxxxxx>
>> Cc: Larry Finger <Larry.Finger@xxxxxxxxxxxx>
>> Cc: Jes Sorensen <jes.sorensen@xxxxxxxxx>
>> Cc: stable@xxxxxxxxxxxxxxx
>> Signed-off-by: Denis Efremov <efremov@xxxxxxxxx>
>> ---
>> 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
>>
>