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
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)
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-p... [2] https://groups.google.com/forum/#%21msg/syzkaller-netbsd-bugs/8B4CIKN0Xz8/wR... [3] https://github.com/torvalds/linux/commit/8f884e76cae65af65c6bec759a17cb0527c...
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)
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)
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
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)
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
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)
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
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
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;@@ -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;
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:
- 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
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
linux-stable-mirror@lists.linaro.org
-
Dan Carpenter -
David Laight -
Denis Efremov -
Hans de Goede