Re: [PATCH] staging: rtl8723bs: Check for NULL header value

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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