* [PATCH] staging: rtl8723bs: hal: Fix memcpy calls @ 2019-09-30 11:01 Denis Efremov 2019-09-30 13:18 ` David Laight ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Denis Efremov @ 2019-09-30 11:01 UTC (permalink / raw) To: devel Cc: Jes Sorensen, Greg Kroah-Hartman, linux-kernel, Denis Efremov, Hans de Goede, stable, Bastien Nocera, Larry Finger 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* RE: [PATCH] staging: rtl8723bs: hal: Fix memcpy calls 2019-09-30 11:01 [PATCH] staging: rtl8723bs: hal: Fix memcpy calls Denis Efremov @ 2019-09-30 13:18 ` David Laight 2019-09-30 14:25 ` Denis Efremov 2019-09-30 15:40 ` Denis Efremov 2019-09-30 13:39 ` Sasha Levin 2019-10-09 9:35 ` Hans de Goede 2 siblings, 2 replies; 14+ messages in thread From: David Laight @ 2019-09-30 13:18 UTC (permalink / raw) To: 'Denis Efremov', devel Cc: Jes Sorensen, Greg Kroah-Hartman, linux-kernel, stable, Hans de Goede, Bastien Nocera, Larry Finger 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] staging: rtl8723bs: hal: Fix memcpy calls 2019-09-30 13:18 ` David Laight @ 2019-09-30 14:25 ` Denis Efremov 2019-10-01 13:56 ` Dan Carpenter 2019-09-30 15:40 ` Denis Efremov 1 sibling, 1 reply; 14+ messages in thread From: Denis Efremov @ 2019-09-30 14:25 UTC (permalink / raw) To: David Laight, devel Cc: Jes Sorensen, Greg Kroah-Hartman, linux-kernel, stable, Hans de Goede, Bastien Nocera, Dmitry Vyukov, Larry Finger 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] staging: rtl8723bs: hal: Fix memcpy calls 2019-09-30 14:25 ` Denis Efremov @ 2019-10-01 13:56 ` Dan Carpenter 2019-10-01 14:36 ` David Laight 0 siblings, 1 reply; 14+ messages in thread From: Dan Carpenter @ 2019-10-01 13:56 UTC (permalink / raw) To: Denis Efremov Cc: devel, Jes Sorensen, Greg Kroah-Hartman, linux-kernel, stable, Hans de Goede, David Laight, Bastien Nocera, Dmitry Vyukov, Larry Finger 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH] staging: rtl8723bs: hal: Fix memcpy calls 2019-10-01 13:56 ` Dan Carpenter @ 2019-10-01 14:36 ` David Laight 2019-10-01 15:13 ` Denis Efremov 0 siblings, 1 reply; 14+ messages in thread From: David Laight @ 2019-10-01 14:36 UTC (permalink / raw) To: 'Dan Carpenter', Denis Efremov Cc: devel, Jes Sorensen, Greg Kroah-Hartman, linux-kernel, stable, Hans de Goede, Bastien Nocera, Dmitry Vyukov, Larry Finger > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] staging: rtl8723bs: hal: Fix memcpy calls 2019-10-01 14:36 ` David Laight @ 2019-10-01 15:13 ` Denis Efremov 2019-10-01 16:00 ` David Laight 2019-10-01 18:58 ` Dan Carpenter 0 siblings, 2 replies; 14+ messages in thread From: Denis Efremov @ 2019-10-01 15:13 UTC (permalink / raw) To: David Laight, 'Dan Carpenter' Cc: devel, Jes Sorensen, Greg Kroah-Hartman, linux-kernel, stable, Hans de Goede, Bastien Nocera, Dmitry Vyukov, Larry Finger 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH] staging: rtl8723bs: hal: Fix memcpy calls 2019-10-01 15:13 ` Denis Efremov @ 2019-10-01 16:00 ` David Laight 2019-10-01 18:58 ` Dan Carpenter 1 sibling, 0 replies; 14+ messages in thread From: David Laight @ 2019-10-01 16:00 UTC (permalink / raw) To: 'efremov@linux.com', 'Dan Carpenter' Cc: devel, Jes Sorensen, Greg Kroah-Hartman, linux-kernel, stable, Hans de Goede, Bastien Nocera, Dmitry Vyukov, Larry Finger 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] staging: rtl8723bs: hal: Fix memcpy calls 2019-10-01 15:13 ` Denis Efremov 2019-10-01 16:00 ` David Laight @ 2019-10-01 18:58 ` Dan Carpenter 2019-10-01 20:15 ` Denis Efremov 2019-10-09 14:38 ` Dan Carpenter 1 sibling, 2 replies; 14+ messages in thread From: Dan Carpenter @ 2019-10-01 18:58 UTC (permalink / raw) To: Denis Efremov Cc: devel, Jes Sorensen, Greg Kroah-Hartman, linux-kernel, stable, Hans de Goede, David Laight, Bastien Nocera, Dmitry Vyukov, Larry Finger 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] staging: rtl8723bs: hal: Fix memcpy calls 2019-10-01 18:58 ` Dan Carpenter @ 2019-10-01 20:15 ` Denis Efremov 2019-10-09 14:38 ` Dan Carpenter 1 sibling, 0 replies; 14+ messages in thread From: Denis Efremov @ 2019-10-01 20:15 UTC (permalink / raw) To: Dan Carpenter Cc: devel, Jes Sorensen, Greg Kroah-Hartman, linux-kernel, stable, Hans de Goede, David Laight, Bastien Nocera, Dmitry Vyukov, Larry Finger 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] staging: rtl8723bs: hal: Fix memcpy calls 2019-10-01 18:58 ` Dan Carpenter 2019-10-01 20:15 ` Denis Efremov @ 2019-10-09 14:38 ` Dan Carpenter 1 sibling, 0 replies; 14+ messages in thread From: Dan Carpenter @ 2019-10-09 14:38 UTC (permalink / raw) To: Denis Efremov Cc: devel, Jes Sorensen, Greg Kroah-Hartman, linux-kernel, stable, Hans de Goede, David Laight, Bastien Nocera, Dmitry Vyukov, Larry Finger 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] staging: rtl8723bs: hal: Fix memcpy calls 2019-09-30 13:18 ` David Laight 2019-09-30 14:25 ` Denis Efremov @ 2019-09-30 15:40 ` Denis Efremov 1 sibling, 0 replies; 14+ messages in thread From: Denis Efremov @ 2019-09-30 15:40 UTC (permalink / raw) To: David Laight, devel Cc: Jes Sorensen, Greg Kroah-Hartman, linux-kernel, stable, Hans de Goede, Bastien Nocera, Dmitry Vyukov, Larry Finger 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] staging: rtl8723bs: hal: Fix memcpy calls 2019-09-30 11:01 [PATCH] staging: rtl8723bs: hal: Fix memcpy calls Denis Efremov 2019-09-30 13:18 ` David Laight @ 2019-09-30 13:39 ` Sasha Levin 2019-10-09 9:35 ` Hans de Goede 2 siblings, 0 replies; 14+ messages in thread From: Sasha Levin @ 2019-09-30 13:39 UTC (permalink / raw) To: Sasha Levin, Denis Efremov, devel Cc: , Jes Sorensen, Greg Kroah-Hartman, linux-kernel, Denis Efremov, Hans de Goede, stable, Bastien Nocera, Larry Finger 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] staging: rtl8723bs: hal: Fix memcpy calls 2019-09-30 11:01 [PATCH] staging: rtl8723bs: hal: Fix memcpy calls Denis Efremov 2019-09-30 13:18 ` David Laight 2019-09-30 13:39 ` Sasha Levin @ 2019-10-09 9:35 ` Hans de Goede 2019-10-09 10:43 ` Denis Efremov 2 siblings, 1 reply; 14+ messages in thread From: Hans de Goede @ 2019-10-09 9:35 UTC (permalink / raw) To: Denis Efremov, devel Cc: Jes Sorensen, Greg Kroah-Hartman, linux-kernel, stable, Bastien Nocera, Larry Finger 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] staging: rtl8723bs: hal: Fix memcpy calls 2019-10-09 9:35 ` Hans de Goede @ 2019-10-09 10:43 ` Denis Efremov 0 siblings, 0 replies; 14+ messages in thread From: Denis Efremov @ 2019-10-09 10:43 UTC (permalink / raw) To: Hans de Goede, devel Cc: Jes Sorensen, Greg Kroah-Hartman, linux-kernel, stable, Bastien Nocera, Larry Finger 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-10-09 14:39 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-30 11:01 [PATCH] staging: rtl8723bs: hal: Fix memcpy calls Denis Efremov 2019-09-30 13:18 ` David Laight 2019-09-30 14:25 ` Denis Efremov 2019-10-01 13:56 ` Dan Carpenter 2019-10-01 14:36 ` David Laight 2019-10-01 15:13 ` Denis Efremov 2019-10-01 16:00 ` David Laight 2019-10-01 18:58 ` Dan Carpenter 2019-10-01 20:15 ` Denis Efremov 2019-10-09 14:38 ` Dan Carpenter 2019-09-30 15:40 ` Denis Efremov 2019-09-30 13:39 ` Sasha Levin 2019-10-09 9:35 ` Hans de Goede 2019-10-09 10:43 ` Denis Efremov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).