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

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

[Denis Efremov]

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.

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

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[David Laight]

From: Denis Efremov
> Sent: 30 September 2019 12:02
> memcpy() in phy_ConfigBBWithParaFile() and PHY_ConfigRFWithParaFile() is
> called with "src == NULL && len == 0". This is an undefined behavior.

I'm pretty certain it is well defined (to do nothing).

> 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.
> 
...
> ---
> Not tested. I don't have the hardware. The fix is based on my guess.
> 
>  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);

The existing code is clearly garbage.
It only ever does memcpy(tgt, NULL, 0).

Under the assumption that the code has been tested the copy clearly isn't needed at all
and can be deleted completely!

OTOH if the code hasn't been tested maybe the entire source file should be removed :-)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Denis Efremov]

On 9/30/19 4:18 PM, David Laight wrote:
> From: Denis Efremov
>> Sent: 30 September 2019 12:02
>> memcpy() in phy_ConfigBBWithParaFile() and PHY_ConfigRFWithParaFile() is
>> called with "src == NULL && len == 0". This is an undefined behavior.
> 
> I'm pretty certain it is well defined (to do nothing).

Well, technically you are right. However, UBSAN warns about passing NULL
to memcpy and from the formal point of view this is an undefined behavior [1].
There were a discussion [2] about interesting implication of assuming that
memcpy with 0 size and NULL pointer is fine. This could result in that compiler
assume that pointer is not NULL. However, this is not the case here since
this "if then" branch is a dead code in it's current form. I just find this
piece of code very funny regarding this patch [3].

[1] https://stackoverflow.com/questions/5243012/is-it-guaranteed-to-be-safe-to-perform-memcpy0-0-0
[2] https://groups.google.com/forum/#!msg/syzkaller-netbsd-bugs/8B4CIKN0Xz8/wRvIUWxiAgAJ
[3] https://github.com/torvalds/linux/commit/8f884e76cae65af65c6bec759a17cb0527c54a15#diff-a476c238511f9374c2f1b947fdaffbbcL2339

> 
>> 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.
>>
> ...
>> ---
>> Not tested. I don't have the hardware. The fix is based on my guess.
>>
>>  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);
> 
> The existing code is clearly garbage.
> It only ever does memcpy(tgt, NULL, 0).
> 
> Under the assumption that the code has been tested the copy clearly isn't needed at all
> and can be deleted completely!
> 
> OTOH if the code hasn't been tested maybe the entire source file should be removed :-)
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Dan Carpenter]

On Mon, Sep 30, 2019 at 05:25:43PM +0300, Denis Efremov wrote:
> On 9/30/19 4:18 PM, David Laight wrote:
> > From: Denis Efremov
> >> Sent: 30 September 2019 12:02
> >> memcpy() in phy_ConfigBBWithParaFile() and PHY_ConfigRFWithParaFile() is
> >> called with "src == NULL && len == 0". This is an undefined behavior.
> > 
> > I'm pretty certain it is well defined (to do nothing).
> 
> Well, technically you are right. However, UBSAN warns about passing NULL
> to memcpy and from the formal point of view this is an undefined behavior [1].
> There were a discussion [2] about interesting implication of assuming that
> memcpy with 0 size and NULL pointer is fine. This could result in that compiler
> assume that pointer is not NULL.

That's true for glibc memcpy() but not for the kernel memcpy().  In the
kernel there are lots of places which do a zero size memcpy().

The glibc attitude is "the standard allows us to put knives here" so
let's put knives everywhere in the path.  And the GCC attitude is let's
silently remove NULL checks instead of just printing a warning that the
NULL check isn't required...  It could really make someone despondent.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[David Laight]

> From: Dan Carpenter
> Sent: 01 October 2019 14:57
> Subject: Re: [PATCH] staging: rtl8723bs: hal: Fix memcpy calls
...
> That's true for glibc memcpy() but not for the kernel memcpy().  In the
> kernel there are lots of places which do a zero size memcpy().

And probably from NULL (or even garbage) pointers.

After all a pointer to the end of an array (a + ARRAY_SIZE(a)) is valid
but must not be dereferenced - so memcpy() can't dereference it's
source address when the length is zero.

> The glibc attitude is "the standard allows us to put knives here" so
> let's put knives everywhere in the path.  And the GCC attitude is let's
> silently remove NULL checks instead of just printing a warning that the
> NULL check isn't required...  It could really make someone despondent.

gcc is the one that add knives...

This reminds me of me of a compiler that decided to optimise away
checks for function addresses being NULL.
At almost exactly the same time that ELF allowed for undefined weak symbols.
Checking whether a function was actually present was non-trivial.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Denis Efremov]

On 10/1/19 5:36 PM, David Laight wrote:
>> From: Dan Carpenter
>> Sent: 01 October 2019 14:57
>> Subject: Re: [PATCH] staging: rtl8723bs: hal: Fix memcpy calls
> ...
>> That's true for glibc memcpy() but not for the kernel memcpy().  In the
>> kernel there are lots of places which do a zero size memcpy().
> 
> And probably from NULL (or even garbage) pointers.
> 
> After all a pointer to the end of an array (a + ARRAY_SIZE(a)) is valid
> but must not be dereferenced - so memcpy() can't dereference it's
> source address when the length is zero.
> 
>> The glibc attitude is "the standard allows us to put knives here" so
>> let's put knives everywhere in the path.  And the GCC attitude is let's
>> silently remove NULL checks instead of just printing a warning that the
>> NULL check isn't required...  It could really make someone despondent.
> 
> gcc is the one that add knives...
> 

Just found an official documentation to this issue:
https://gcc.gnu.org/gcc-4.9/porting_to.html
"Null pointer checks may be optimized away more aggressively
...
The pointers passed to memmove (and similar functions in <string.h>) must be non-null
even when nbytes==0, so GCC can use that information to remove the check after the
memmove call. Calling copy(p, NULL, 0) can therefore deference a null pointer and crash."

But again, I would say that the bug in this code is because the if condition was copy-pasted
and it should be inverted.

Thanks,
Denis
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[David Laight]

From: Denis Efremov
> Sent: 01 October 2019 16:13
...
> Just found an official documentation to this issue:
> https://gcc.gnu.org/gcc-4.9/porting_to.html
> "Null pointer checks may be optimized away more aggressively
> ...
> The pointers passed to memmove (and similar functions in <string.h>) must be non-null
> even when nbytes==0, so GCC can use that information to remove the check after the
> memmove call. Calling copy(p, NULL, 0) can therefore deference a null pointer and crash."

Right, so just don't code a NULL pointer test after a memcpy() call.
There is no need to avoid the call itself.

> But again, I would say that the bug in this code is because the if condition was copy-pasted
> and it should be inverted.

Oh, the code is question is just stupidly bad.
It seemed to do:
	if (a)
		x;
	else if (!a)
		y;
	else
		error ("all borked")

If the whole driver is written like that it needs fixing before anyone takes a serious look at it.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Dan Carpenter]

On Tue, Oct 01, 2019 at 06:13:21PM +0300, Denis Efremov wrote:
> Just found an official documentation to this issue:
> https://gcc.gnu.org/gcc-4.9/porting_to.html
> "Null pointer checks may be optimized away more aggressively
> ...
> The pointers passed to memmove (and similar functions in <string.h>) must be non-null
> even when nbytes==0, so GCC can use that information to remove the check after the
> memmove call. Calling copy(p, NULL, 0) can therefore deference a null pointer and crash."
> 

Correct.  In glibc those functions are annotated as non-NULL.

extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
                     size_t __n) __THROW __nonnull ((1, 2));

We aren't going to do that in the kernel.  A second difference is that
in the kernel we use -fno-delete-null-pointer-checks so it doesn't
delete the NULL checks.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Denis Efremov]

On 01.10.2019 21:58, Dan Carpenter wrote:
> On Tue, Oct 01, 2019 at 06:13:21PM +0300, Denis Efremov wrote:
>> Just found an official documentation to this issue:
>> https://gcc.gnu.org/gcc-4.9/porting_to.html
>> "Null pointer checks may be optimized away more aggressively
>> ...
>> The pointers passed to memmove (and similar functions in <string.h>) must be non-null
>> even when nbytes==0, so GCC can use that information to remove the check after the
>> memmove call. Calling copy(p, NULL, 0) can therefore deference a null pointer and crash."
>>
> 
> Correct.  In glibc those functions are annotated as non-NULL.
> 
> extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
>                      size_t __n) __THROW __nonnull ((1, 2));
> 
> We aren't going to do that in the kernel.  A second difference is that
> in the kernel we use -fno-delete-null-pointer-checks so it doesn't
> delete the NULL checks.
> 

Thank you for the clarification. This is really helpful.

Best regards,
Denis
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Dan Carpenter]

On Tue, Oct 01, 2019 at 09:58:55PM +0300, Dan Carpenter wrote:
> On Tue, Oct 01, 2019 at 06:13:21PM +0300, Denis Efremov wrote:
> > Just found an official documentation to this issue:
> > https://gcc.gnu.org/gcc-4.9/porting_to.html
> > "Null pointer checks may be optimized away more aggressively
> > ...
> > The pointers passed to memmove (and similar functions in <string.h>) must be non-null
> > even when nbytes==0, so GCC can use that information to remove the check after the
> > memmove call. Calling copy(p, NULL, 0) can therefore deference a null pointer and crash."
> > 
> 
> Correct.  In glibc those functions are annotated as non-NULL.
> 
> extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
>                      size_t __n) __THROW __nonnull ((1, 2));

I was wrong on this.  It's built into GCC so it doesn't matter how it's
annotated.

> 
> We aren't going to do that in the kernel.  A second difference is that
> in the kernel we use -fno-delete-null-pointer-checks so it doesn't
> delete the NULL checks.

But it's true that the kernel has -fno-delete-null-pointer-checks so I
don't think this is worth patching.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Denis Efremov]

On 9/30/19 4:18 PM, David Laight wrote:
> From: Denis Efremov
>> Sent: 30 September 2019 12:02
>> memcpy() in phy_ConfigBBWithParaFile() and PHY_ConfigRFWithParaFile() is
>> called with "src == NULL && len == 0". This is an undefined behavior.
> 
> I'm pretty certain it is well defined (to do nothing).
> 
>> 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.
>>
> ...
>> ---
>> Not tested. I don't have the hardware. The fix is based on my guess.
>>
>>  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);
> 
> The existing code is clearly garbage.
> It only ever does memcpy(tgt, NULL, 0).
> 
> Under the assumption that the code has been tested the copy clearly isn't needed at all
> and can be deleted completely!

Initially I also thought that this is just a dead code and it could be simply removed. However, if
we look at it more carefully, this if condition looks like a copy-paste error:

if (pBufLen && (*pBufLen == 0) && !pBuf) {
	// get proper len
	// allocate pBuf
	...
	memcpy(pBuf, pHalData->para_file_buf, rlen);
	...
} else {
	if (pBufLen && (*pBufLen == 0) && !pBuf) { // <== condition in patch
		memcpy(pHalData->para_file_buf, pBuf, *pBufLen);
		rtStatus = _SUCCESS;
	} else
		DBG_871X("%s(): Critical Error !!!\n", __func__);
}

Thus, I think it will be incorrect to delete the second memcpy.

> 
> OTOH if the code hasn't been tested maybe the entire source file should be removed :-)
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Sasha Levin]

Hi,

[This is an automated email]

This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all

The bot has tested the following trees: v5.3.1, v5.2.17, v4.19.75, v4.14.146, v4.9.194, v4.4.194.

v5.3.1: Build OK!
v5.2.17: Build OK!
v4.19.75: Failed to apply! Possible dependencies:
    8f884e76cae6 ("staging: rtl8723bs: hal: Remove comparison to NULL in hal_com_phycfg.c")

v4.14.146: Failed to apply! Possible dependencies:
    8f884e76cae6 ("staging: rtl8723bs: hal: Remove comparison to NULL in hal_com_phycfg.c")

v4.9.194: Failed to apply! Possible dependencies:
    554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver")
    8f884e76cae6 ("staging: rtl8723bs: hal: Remove comparison to NULL in hal_com_phycfg.c")

v4.4.194: Failed to apply! Possible dependencies:
    554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver")
    8f884e76cae6 ("staging: rtl8723bs: hal: Remove comparison to NULL in hal_com_phycfg.c")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

--
Thanks,
Sasha
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Hans de Goede]

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.

Regards,

Hans



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

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