* [PATCH] staging: rtl8723bs: Check for NULL header value
@ 2022-01-13 0:20 Kees Cook
2022-01-13 4:41 ` Dan Carpenter
2022-01-13 9:13 ` Greg Kroah-Hartman
0 siblings, 2 replies; 6+ messages in thread
From: Kees Cook @ 2022-01-13 0:20 UTC (permalink / raw)
To: Larry Finger
Cc: Kees Cook, Phillip Potter, Greg Kroah-Hartman, Michael Straube,
Fabio Aiuto, linux-staging, Hans de Goede, Dan Carpenter,
Marco Cesati, linux-kernel, linux-hardening
When building with -Warray-bounds, the following warning is emitted:
In file included from ./include/linux/string.h:253,
from ./arch/x86/include/asm/page_32.h:22,
from ./arch/x86/include/asm/page.h:14,
from ./arch/x86/include/asm/thread_info.h:12,
from ./include/linux/thread_info.h:60,
from ./arch/x86/include/asm/preempt.h:7,
from ./include/linux/preempt.h:78,
from ./include/linux/rcupdate.h:27,
from ./include/linux/rculist.h:11,
from ./include/linux/sched/signal.h:5,
from ./drivers/staging/rtl8723bs/include/drv_types.h:17,
from drivers/staging/rtl8723bs/core/rtw_recv.c:7:
In function 'memcpy',
inlined from 'wlanhdr_to_ethhdr' at drivers/staging/rtl8723bs/core/rtw_recv.c:1554:2:
./include/linux/fortify-string.h:41:33: warning: '__builtin_memcpy' offset [0, 5] is out of the bounds [0, 0] [-Warray-bounds]
41 | #define __underlying_memcpy __builtin_memcpy
| ^
This is because the compiler sees it is possible for "ptr" to be a NULL
value, and concludes that it has zero size and attempts to copy to it
would overflow. Instead, detect the NULL return and error out early.
Cc: Larry Finger <Larry.Finger@lwfinger.net>
Cc: Phillip Potter <phil@philpotter.co.uk>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Michael Straube <straube.linux@gmail.com>
Cc: Fabio Aiuto <fabioaiuto83@gmail.com>
Cc: linux-staging@lists.linux.dev
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/staging/rtl8723bs/core/rtw_recv.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c b/drivers/staging/rtl8723bs/core/rtw_recv.c
index 41bfca549c64..61135c49322b 100644
--- a/drivers/staging/rtl8723bs/core/rtw_recv.c
+++ b/drivers/staging/rtl8723bs/core/rtw_recv.c
@@ -1513,6 +1513,9 @@ static signed int wlanhdr_to_ethhdr(union recv_frame *precvframe)
u8 *ptr = get_recvframe_data(precvframe) ; /* point to frame_ctrl field */
struct rx_pkt_attrib *pattrib = &precvframe->u.hdr.attrib;
+ if (!ptr)
+ return _FAIL;
+
if (pattrib->encrypt)
recvframe_pull_tail(precvframe, pattrib->icv_len);
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: rtl8723bs: Check for NULL header value
2022-01-13 0:20 [PATCH] staging: rtl8723bs: Check for NULL header value Kees Cook
@ 2022-01-13 4:41 ` Dan Carpenter
2022-01-13 9:13 ` Greg Kroah-Hartman
1 sibling, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2022-01-13 4:41 UTC (permalink / raw)
To: Kees Cook
Cc: Larry Finger, Phillip Potter, Greg Kroah-Hartman,
Michael Straube, Fabio Aiuto, linux-staging, Hans de Goede,
Marco Cesati, linux-kernel, linux-hardening
On Wed, Jan 12, 2022 at 04:20:01PM -0800, Kees Cook wrote:
> When building with -Warray-bounds, the following warning is emitted:
>
> In file included from ./include/linux/string.h:253,
> from ./arch/x86/include/asm/page_32.h:22,
> from ./arch/x86/include/asm/page.h:14,
> from ./arch/x86/include/asm/thread_info.h:12,
> from ./include/linux/thread_info.h:60,
> from ./arch/x86/include/asm/preempt.h:7,
> from ./include/linux/preempt.h:78,
> from ./include/linux/rcupdate.h:27,
> from ./include/linux/rculist.h:11,
> from ./include/linux/sched/signal.h:5,
> from ./drivers/staging/rtl8723bs/include/drv_types.h:17,
> from drivers/staging/rtl8723bs/core/rtw_recv.c:7:
> In function 'memcpy',
> inlined from 'wlanhdr_to_ethhdr' at drivers/staging/rtl8723bs/core/rtw_recv.c:1554:2:
> ./include/linux/fortify-string.h:41:33: warning: '__builtin_memcpy' offset [0, 5] is out of the bounds [0, 0] [-Warray-bounds]
> 41 | #define __underlying_memcpy __builtin_memcpy
> | ^
>
> This is because the compiler sees it is possible for "ptr" to be a NULL
> value,
It's not really possible though. I bet it's just saying that because of
the NULL check in get_recvframe_data(). Meanwhile we already have
dereferenced "precvframe" so GCC should know that the NULL check can
never be true.
> and concludes that it has zero size and attempts to copy to it
> would overflow. Instead, detect the NULL return and error out early.
>
> Cc: Larry Finger <Larry.Finger@lwfinger.net>
> Cc: Phillip Potter <phil@philpotter.co.uk>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Michael Straube <straube.linux@gmail.com>
> Cc: Fabio Aiuto <fabioaiuto83@gmail.com>
> Cc: linux-staging@lists.linux.dev
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> drivers/staging/rtl8723bs/core/rtw_recv.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c b/drivers/staging/rtl8723bs/core/rtw_recv.c
> index 41bfca549c64..61135c49322b 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_recv.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_recv.c
> @@ -1513,6 +1513,9 @@ static signed int wlanhdr_to_ethhdr(union recv_frame *precvframe)
> u8 *ptr = get_recvframe_data(precvframe) ; /* point to frame_ctrl field */
> struct rx_pkt_attrib *pattrib = &precvframe->u.hdr.attrib;
>
> + if (!ptr)
> + return _FAIL;
> +
Can we put a comment to say /* Silence GCC false positive with -Warray-bounds */
We should have a standard comment so that we can grep for this ten years
from now an remove any outdated work arounds.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: rtl8723bs: Check for NULL header value
2022-01-13 0:20 [PATCH] staging: rtl8723bs: Check for NULL header value Kees Cook
2022-01-13 4:41 ` Dan Carpenter
@ 2022-01-13 9:13 ` Greg Kroah-Hartman
2022-01-13 18:29 ` Kees Cook
1 sibling, 1 reply; 6+ messages in thread
From: Greg Kroah-Hartman @ 2022-01-13 9:13 UTC (permalink / raw)
To: Kees Cook
Cc: Larry Finger, Phillip Potter, Michael Straube, Fabio Aiuto,
linux-staging, Hans de Goede, Dan Carpenter, Marco Cesati,
linux-kernel, linux-hardening
On Wed, Jan 12, 2022 at 04:20:01PM -0800, Kees Cook wrote:
> When building with -Warray-bounds, the following warning is emitted:
>
> In file included from ./include/linux/string.h:253,
> from ./arch/x86/include/asm/page_32.h:22,
> from ./arch/x86/include/asm/page.h:14,
> from ./arch/x86/include/asm/thread_info.h:12,
> from ./include/linux/thread_info.h:60,
> from ./arch/x86/include/asm/preempt.h:7,
> from ./include/linux/preempt.h:78,
> from ./include/linux/rcupdate.h:27,
> from ./include/linux/rculist.h:11,
> from ./include/linux/sched/signal.h:5,
> from ./drivers/staging/rtl8723bs/include/drv_types.h:17,
> from drivers/staging/rtl8723bs/core/rtw_recv.c:7:
> In function 'memcpy',
> inlined from 'wlanhdr_to_ethhdr' at drivers/staging/rtl8723bs/core/rtw_recv.c:1554:2:
> ./include/linux/fortify-string.h:41:33: warning: '__builtin_memcpy' offset [0, 5] is out of the bounds [0, 0] [-Warray-bounds]
> 41 | #define __underlying_memcpy __builtin_memcpy
> | ^
>
> This is because the compiler sees it is possible for "ptr" to be a NULL
> value, and concludes that it has zero size and attempts to copy to it
> would overflow. Instead, detect the NULL return and error out early.
>
> Cc: Larry Finger <Larry.Finger@lwfinger.net>
> Cc: Phillip Potter <phil@philpotter.co.uk>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Michael Straube <straube.linux@gmail.com>
> Cc: Fabio Aiuto <fabioaiuto83@gmail.com>
> Cc: linux-staging@lists.linux.dev
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> drivers/staging/rtl8723bs/core/rtw_recv.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c b/drivers/staging/rtl8723bs/core/rtw_recv.c
> index 41bfca549c64..61135c49322b 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_recv.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_recv.c
> @@ -1513,6 +1513,9 @@ static signed int wlanhdr_to_ethhdr(union recv_frame *precvframe)
> u8 *ptr = get_recvframe_data(precvframe) ; /* point to frame_ctrl field */
> struct rx_pkt_attrib *pattrib = &precvframe->u.hdr.attrib;
>
> + if (!ptr)
> + return _FAIL;
This will never happen, so let's not paper over compiler issues with
stuff like this please.
As the call to get_recvframe_data() is only done in one place in this
driver (in all drivers that look like this as well), it can just be
replaced with the real code instead of the nonsensical test for NULL and
then the compiler should be happy.
I'll gladly take that fix instead of this one, as that would be the
correct solution here.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: rtl8723bs: Check for NULL header value
2022-01-13 9:13 ` Greg Kroah-Hartman
@ 2022-01-13 18:29 ` Kees Cook
2022-01-13 18:51 ` Greg Kroah-Hartman
0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2022-01-13 18:29 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Larry Finger, Phillip Potter, Michael Straube, Fabio Aiuto,
linux-staging, Hans de Goede, Dan Carpenter, Marco Cesati,
linux-kernel, linux-hardening
On Thu, Jan 13, 2022 at 10:13:46AM +0100, Greg Kroah-Hartman wrote:
> On Wed, Jan 12, 2022 at 04:20:01PM -0800, Kees Cook wrote:
> > When building with -Warray-bounds, the following warning is emitted:
> >
> > In file included from ./include/linux/string.h:253,
> > from ./arch/x86/include/asm/page_32.h:22,
> > from ./arch/x86/include/asm/page.h:14,
> > from ./arch/x86/include/asm/thread_info.h:12,
> > from ./include/linux/thread_info.h:60,
> > from ./arch/x86/include/asm/preempt.h:7,
> > from ./include/linux/preempt.h:78,
> > from ./include/linux/rcupdate.h:27,
> > from ./include/linux/rculist.h:11,
> > from ./include/linux/sched/signal.h:5,
> > from ./drivers/staging/rtl8723bs/include/drv_types.h:17,
> > from drivers/staging/rtl8723bs/core/rtw_recv.c:7:
> > In function 'memcpy',
> > inlined from 'wlanhdr_to_ethhdr' at drivers/staging/rtl8723bs/core/rtw_recv.c:1554:2:
> > ./include/linux/fortify-string.h:41:33: warning: '__builtin_memcpy' offset [0, 5] is out of the bounds [0, 0] [-Warray-bounds]
> > 41 | #define __underlying_memcpy __builtin_memcpy
> > | ^
> >
> > This is because the compiler sees it is possible for "ptr" to be a NULL
> > value, and concludes that it has zero size and attempts to copy to it
> > would overflow. Instead, detect the NULL return and error out early.
> >
> > Cc: Larry Finger <Larry.Finger@lwfinger.net>
> > Cc: Phillip Potter <phil@philpotter.co.uk>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Michael Straube <straube.linux@gmail.com>
> > Cc: Fabio Aiuto <fabioaiuto83@gmail.com>
> > Cc: linux-staging@lists.linux.dev
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > drivers/staging/rtl8723bs/core/rtw_recv.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c b/drivers/staging/rtl8723bs/core/rtw_recv.c
> > index 41bfca549c64..61135c49322b 100644
> > --- a/drivers/staging/rtl8723bs/core/rtw_recv.c
> > +++ b/drivers/staging/rtl8723bs/core/rtw_recv.c
> > @@ -1513,6 +1513,9 @@ static signed int wlanhdr_to_ethhdr(union recv_frame *precvframe)
> > u8 *ptr = get_recvframe_data(precvframe) ; /* point to frame_ctrl field */
> > struct rx_pkt_attrib *pattrib = &precvframe->u.hdr.attrib;
> >
> > + if (!ptr)
> > + return _FAIL;
>
> This will never happen, so let's not paper over compiler issues with
> stuff like this please.
>
> As the call to get_recvframe_data() is only done in one place in this
> driver (in all drivers that look like this as well), it can just be
> replaced with the real code instead of the nonsensical test for NULL and
> then the compiler should be happy.
>
> I'll gladly take that fix instead of this one, as that would be the
> correct solution here.
I changed it around, but it doesn't help. I assume this is because we
build with -fno-delete-null-pointer-checks, so the compiler continues
to assume it's possible for the incoming argument to be NULL.
Should I rearrange this to do a NULL check for precvframe before all the
assignments in addition to removing get_recvframe_data()?
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: rtl8723bs: Check for NULL header value
2022-01-13 18:29 ` Kees Cook
@ 2022-01-13 18:51 ` Greg Kroah-Hartman
2022-01-15 4:14 ` Kees Cook
0 siblings, 1 reply; 6+ messages in thread
From: Greg Kroah-Hartman @ 2022-01-13 18:51 UTC (permalink / raw)
To: Kees Cook
Cc: Larry Finger, Phillip Potter, Michael Straube, Fabio Aiuto,
linux-staging, Hans de Goede, Dan Carpenter, Marco Cesati,
linux-kernel, linux-hardening
On Thu, Jan 13, 2022 at 10:29:04AM -0800, Kees Cook wrote:
> On Thu, Jan 13, 2022 at 10:13:46AM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Jan 12, 2022 at 04:20:01PM -0800, Kees Cook wrote:
> > > When building with -Warray-bounds, the following warning is emitted:
> > >
> > > In file included from ./include/linux/string.h:253,
> > > from ./arch/x86/include/asm/page_32.h:22,
> > > from ./arch/x86/include/asm/page.h:14,
> > > from ./arch/x86/include/asm/thread_info.h:12,
> > > from ./include/linux/thread_info.h:60,
> > > from ./arch/x86/include/asm/preempt.h:7,
> > > from ./include/linux/preempt.h:78,
> > > from ./include/linux/rcupdate.h:27,
> > > from ./include/linux/rculist.h:11,
> > > from ./include/linux/sched/signal.h:5,
> > > from ./drivers/staging/rtl8723bs/include/drv_types.h:17,
> > > from drivers/staging/rtl8723bs/core/rtw_recv.c:7:
> > > In function 'memcpy',
> > > inlined from 'wlanhdr_to_ethhdr' at drivers/staging/rtl8723bs/core/rtw_recv.c:1554:2:
> > > ./include/linux/fortify-string.h:41:33: warning: '__builtin_memcpy' offset [0, 5] is out of the bounds [0, 0] [-Warray-bounds]
> > > 41 | #define __underlying_memcpy __builtin_memcpy
> > > | ^
> > >
> > > This is because the compiler sees it is possible for "ptr" to be a NULL
> > > value, and concludes that it has zero size and attempts to copy to it
> > > would overflow. Instead, detect the NULL return and error out early.
> > >
> > > Cc: Larry Finger <Larry.Finger@lwfinger.net>
> > > Cc: Phillip Potter <phil@philpotter.co.uk>
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Cc: Michael Straube <straube.linux@gmail.com>
> > > Cc: Fabio Aiuto <fabioaiuto83@gmail.com>
> > > Cc: linux-staging@lists.linux.dev
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > ---
> > > drivers/staging/rtl8723bs/core/rtw_recv.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c b/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > index 41bfca549c64..61135c49322b 100644
> > > --- a/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > +++ b/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > @@ -1513,6 +1513,9 @@ static signed int wlanhdr_to_ethhdr(union recv_frame *precvframe)
> > > u8 *ptr = get_recvframe_data(precvframe) ; /* point to frame_ctrl field */
> > > struct rx_pkt_attrib *pattrib = &precvframe->u.hdr.attrib;
> > >
> > > + if (!ptr)
> > > + return _FAIL;
> >
> > This will never happen, so let's not paper over compiler issues with
> > stuff like this please.
> >
> > As the call to get_recvframe_data() is only done in one place in this
> > driver (in all drivers that look like this as well), it can just be
> > replaced with the real code instead of the nonsensical test for NULL and
> > then the compiler should be happy.
> >
> > I'll gladly take that fix instead of this one, as that would be the
> > correct solution here.
>
> I changed it around, but it doesn't help. I assume this is because we
> build with -fno-delete-null-pointer-checks, so the compiler continues
> to assume it's possible for the incoming argument to be NULL.
That's a broken compiler then.
> Should I rearrange this to do a NULL check for precvframe before all the
> assignments in addition to removing get_recvframe_data()?
If you walk the call-chain back, you will see that precvframe can't ever
be NULL here (which is why this code doesn't crash), so don't check for
something that is impossible to ever hit please just because the
compiler is broken.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: rtl8723bs: Check for NULL header value
2022-01-13 18:51 ` Greg Kroah-Hartman
@ 2022-01-15 4:14 ` Kees Cook
0 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2022-01-15 4:14 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Larry Finger, Phillip Potter, Michael Straube, Fabio Aiuto,
linux-staging, Hans de Goede, Dan Carpenter, Marco Cesati,
linux-kernel, linux-hardening
On Thu, Jan 13, 2022 at 07:51:24PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Jan 13, 2022 at 10:29:04AM -0800, Kees Cook wrote:
> > On Thu, Jan 13, 2022 at 10:13:46AM +0100, Greg Kroah-Hartman wrote:
> > > On Wed, Jan 12, 2022 at 04:20:01PM -0800, Kees Cook wrote:
> > > > When building with -Warray-bounds, the following warning is emitted:
> > > >
> > > > In file included from ./include/linux/string.h:253,
> > > > from ./arch/x86/include/asm/page_32.h:22,
> > > > from ./arch/x86/include/asm/page.h:14,
> > > > from ./arch/x86/include/asm/thread_info.h:12,
> > > > from ./include/linux/thread_info.h:60,
> > > > from ./arch/x86/include/asm/preempt.h:7,
> > > > from ./include/linux/preempt.h:78,
> > > > from ./include/linux/rcupdate.h:27,
> > > > from ./include/linux/rculist.h:11,
> > > > from ./include/linux/sched/signal.h:5,
> > > > from ./drivers/staging/rtl8723bs/include/drv_types.h:17,
> > > > from drivers/staging/rtl8723bs/core/rtw_recv.c:7:
> > > > In function 'memcpy',
> > > > inlined from 'wlanhdr_to_ethhdr' at drivers/staging/rtl8723bs/core/rtw_recv.c:1554:2:
> > > > ./include/linux/fortify-string.h:41:33: warning: '__builtin_memcpy' offset [0, 5] is out of the bounds [0, 0] [-Warray-bounds]
> > > > 41 | #define __underlying_memcpy __builtin_memcpy
> > > > | ^
> > > >
> > > > This is because the compiler sees it is possible for "ptr" to be a NULL
> > > > value, and concludes that it has zero size and attempts to copy to it
> > > > would overflow. Instead, detect the NULL return and error out early.
> > > >
> > > > Cc: Larry Finger <Larry.Finger@lwfinger.net>
> > > > Cc: Phillip Potter <phil@philpotter.co.uk>
> > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > Cc: Michael Straube <straube.linux@gmail.com>
> > > > Cc: Fabio Aiuto <fabioaiuto83@gmail.com>
> > > > Cc: linux-staging@lists.linux.dev
> > > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > > ---
> > > > drivers/staging/rtl8723bs/core/rtw_recv.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c b/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > > index 41bfca549c64..61135c49322b 100644
> > > > --- a/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > > +++ b/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > > @@ -1513,6 +1513,9 @@ static signed int wlanhdr_to_ethhdr(union recv_frame *precvframe)
> > > > u8 *ptr = get_recvframe_data(precvframe) ; /* point to frame_ctrl field */
> > > > struct rx_pkt_attrib *pattrib = &precvframe->u.hdr.attrib;
> > > >
> > > > + if (!ptr)
> > > > + return _FAIL;
> > >
> > > This will never happen, so let's not paper over compiler issues with
> > > stuff like this please.
> > >
> > > As the call to get_recvframe_data() is only done in one place in this
> > > driver (in all drivers that look like this as well), it can just be
> > > replaced with the real code instead of the nonsensical test for NULL and
> > > then the compiler should be happy.
> > >
> > > I'll gladly take that fix instead of this one, as that would be the
> > > correct solution here.
> >
> > I changed it around, but it doesn't help. I assume this is because we
> > build with -fno-delete-null-pointer-checks, so the compiler continues
> > to assume it's possible for the incoming argument to be NULL.
>
> That's a broken compiler then.
>
> > Should I rearrange this to do a NULL check for precvframe before all the
> > assignments in addition to removing get_recvframe_data()?
>
> If you walk the call-chain back, you will see that precvframe can't ever
> be NULL here (which is why this code doesn't crash), so don't check for
> something that is impossible to ever hit please just because the
> compiler is broken.
I agree that the _original_ precvframe is always non-NULL, but the pointer
landing at memcpy() gets updated along the way. It seems the new problem
is that recvframe_pull() may return NULL and nothing is checking for that.
Adding this silences the warning:
diff --git a/drivers/staging/r8188eu/core/rtw_recv.c b/drivers/staging/r8188eu/core/rtw_recv.c
index 49a02d6239d6..946e659ae97a 100644
--- a/drivers/staging/r8188eu/core/rtw_recv.c
+++ b/drivers/staging/r8188eu/core/rtw_recv.c
@@ -1223,10 +1223,14 @@ static int wlanhdr_to_ethhdr(struct recv_frame *precvframe)
eth_type = 0x8712;
/* append rx status for mp test packets */
ptr = recvframe_pull(precvframe, (rmv_len - sizeof(struct ethhdr) + 2) - 24);
+ if (!ptr)
+ return _FAIL;
memcpy(ptr, get_rxmem(precvframe), 24);
ptr += 24;
} else {
ptr = recvframe_pull(precvframe, (rmv_len - sizeof(struct ethhdr) + (bsnaphdr ? 2 : 0)));
+ if (!ptr)
+ return _FAIL;
}
memcpy(ptr, pattrib->dst, ETH_ALEN);
>
> thanks,
>
> greg k-h
--
Kees Cook
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-15 4:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-13 0:20 [PATCH] staging: rtl8723bs: Check for NULL header value Kees Cook
2022-01-13 4:41 ` Dan Carpenter
2022-01-13 9:13 ` Greg Kroah-Hartman
2022-01-13 18:29 ` Kees Cook
2022-01-13 18:51 ` Greg Kroah-Hartman
2022-01-15 4:14 ` Kees Cook
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.