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

From: Denis Efremov <efremov@linux.com>
To: Hans de Goede <hdegoede@redhat.com>, devel@driverdev.osuosl.org
Cc: Jes Sorensen <jes.sorensen@gmail.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-kernel@vger.kernel.org, stable@vger.kernel.org,
	Bastien Nocera <hadess@hadess.net>,
	Larry Finger <Larry.Finger@lwfinger.net>
Subject: Re: [PATCH] staging: rtl8723bs: hal: Fix memcpy calls
Date: Wed, 9 Oct 2019 13:43:09 +0300	[thread overview]
Message-ID: <996e0e51-019a-5d65-026a-9599f9228299@linux.com> (raw)
In-Reply-To: <94af475e-dd7a-6066-146a-30a9915cd325@redhat.com>

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
>>
> 
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel