lkml.org 
[lkml]   [2019]   [Oct]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] staging: rtl8723bs: hal: Fix memcpy calls
Date
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@linuxfoundation.org>
>> Cc: Hans de Goede <hdegoede@redhat.com>
>> Cc: Bastien Nocera <hadess@hadess.net>
>> Cc: Larry Finger <Larry.Finger@lwfinger.net>
>> Cc: Jes Sorensen <jes.sorensen@gmail.com>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Denis Efremov <efremov@linux.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
>>
>

\
 
 \ /
  Last update: 2019-10-09 12:43    [W:0.084 / U:25.556 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site