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