1. Patchew
  2. linux
  3. View series

[PATCH] staging: rtl8723bs: core: avoid NULL pointer dereference in c2h_wk_callback

Nikoloz Bakuradze posted 1 patch 2 weeks, 2 days ago
There is a newer version of this series
drivers/staging/rtl8723bs/core/rtw_cmd.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
[PATCH] staging: rtl8723bs: core: avoid NULL pointer dereference in c2h_wk_callback
Posted by Nikoloz Bakuradze 2 weeks, 2 days ago 
c2h_wk_callback() allocates a 16-byte buffer with kmalloc(GFP_ATOMIC)
when the c2h event needs to be read by the host. The existing guard
only wraps the read step, so on allocation failure the loop body falls
through with a NULL c2h_evt and dereferences it in rtw_hal_c2h_valid()
(via c2h_evt_valid() which reads buf->id).

Restructure the check into an early continue so the rest of the loop
iteration cannot be reached with a NULL pointer.

Fixes: 554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver")
Cc: stable@vger.kernel.org
Signed-off-by: Nikoloz Bakuradze <nbakuradze28@gmail.com>
---
 drivers/staging/rtl8723bs/core/rtw_cmd.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c b/drivers/staging/rtl8723bs/core/rtw_cmd.c
index c1185c25ed369..874970116f920 100644
--- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
+++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
@@ -1702,12 +1702,12 @@ static void c2h_wk_callback(struct work_struct *work)
 			c2h_evt_clear(adapter);
 		} else {
 			c2h_evt = kmalloc(16, GFP_ATOMIC);
-			if (c2h_evt) {
-				/* This C2H event is not read, read & clear now */
-				if (c2h_evt_read_88xx(adapter, c2h_evt) != _SUCCESS) {
-					kfree(c2h_evt);
-					continue;
-				}
+			if (!c2h_evt)
+				continue;
+			/* This C2H event is not read, read & clear now */
+			if (c2h_evt_read_88xx(adapter, c2h_evt) != _SUCCESS) {
+				kfree(c2h_evt);
+				continue;
 			}
 		}
 
-- 
2.54.0
Re: [PATCH] staging: rtl8723bs: core: avoid NULL pointer dereference in c2h_wk_callback
Posted by Andy Shevchenko 2 weeks, 1 day ago 
On Mon, Jun 08, 2026 at 11:06:58PM +0400, Nikoloz Bakuradze wrote:
> c2h_wk_callback() allocates a 16-byte buffer with kmalloc(GFP_ATOMIC)
> when the c2h event needs to be read by the host. The existing guard
> only wraps the read step, so on allocation failure the loop body falls
> through with a NULL c2h_evt and dereferences it in rtw_hal_c2h_valid()
> (via c2h_evt_valid() which reads buf->id).
> 
> Restructure the check into an early continue so the rest of the loop
> iteration cannot be reached with a NULL pointer.


Not sure if we need any Fixes tag. kmalloc(16) won't ever fail (otherwise
the system is already in the state when nothing can help).

...

>  			c2h_evt = kmalloc(16, GFP_ATOMIC);
> -			if (c2h_evt) {
> -				/* This C2H event is not read, read & clear now */
> -				if (c2h_evt_read_88xx(adapter, c2h_evt) != _SUCCESS) {
> -					kfree(c2h_evt);
> -					continue;
> -				}

> +			if (!c2h_evt)
> +				continue;
> +			/* This C2H event is not read, read & clear now */
> +			if (c2h_evt_read_88xx(adapter, c2h_evt) != _SUCCESS) {
> +				kfree(c2h_evt);
> +				continue;

It's too verbose way of saying

			} else
				continue;

here.

>  			}

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH] staging: rtl8723bs: core: avoid NULL pointer dereference in c2h_wk_callback
Posted by nika bakuradze 2 weeks, 1 day ago 
You're right, kmalloc(16) effectively won't fail. This is my first
kernel patch so I was being overcautious with the framing.

Should I resend v2 with the else continue form you suggested,
or drop the patch entirely?

Regards,
Nikoloz Bakuradze

On Tue, Jun 9, 2026 at 11:15 AM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Mon, Jun 08, 2026 at 11:06:58PM +0400, Nikoloz Bakuradze wrote:
> > c2h_wk_callback() allocates a 16-byte buffer with kmalloc(GFP_ATOMIC)
> > when the c2h event needs to be read by the host. The existing guard
> > only wraps the read step, so on allocation failure the loop body falls
> > through with a NULL c2h_evt and dereferences it in rtw_hal_c2h_valid()
> > (via c2h_evt_valid() which reads buf->id).
> >
> > Restructure the check into an early continue so the rest of the loop
> > iteration cannot be reached with a NULL pointer.
>
>
> Not sure if we need any Fixes tag. kmalloc(16) won't ever fail (otherwise
> the system is already in the state when nothing can help).
>
> ...
>
> >                       c2h_evt = kmalloc(16, GFP_ATOMIC);
> > -                     if (c2h_evt) {
> > -                             /* This C2H event is not read, read & clear now */
> > -                             if (c2h_evt_read_88xx(adapter, c2h_evt) != _SUCCESS) {
> > -                                     kfree(c2h_evt);
> > -                                     continue;
> > -                             }
>
> > +                     if (!c2h_evt)
> > +                             continue;
> > +                     /* This C2H event is not read, read & clear now */
> > +                     if (c2h_evt_read_88xx(adapter, c2h_evt) != _SUCCESS) {
> > +                             kfree(c2h_evt);
> > +                             continue;
>
> It's too verbose way of saying
>
>                         } else
>                                 continue;
>
> here.
>
> >                       }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Re: [PATCH] staging: rtl8723bs: core: avoid NULL pointer dereference in c2h_wk_callback
Posted by Andy Shevchenko 2 weeks, 1 day ago 
On Tue, Jun 09, 2026 at 08:40:39PM +0400, nika bakuradze wrote:

First of all, do not top-post!

> You're right, kmalloc(16) effectively won't fail. This is my first
> kernel patch so I was being overcautious with the framing.
> 
> Should I resend v2 with the else continue form you suggested,
> or drop the patch entirely?

To some extent the patch makes sense (at least for you to train your skills in
Linux kernel processes, et cetera). I would go with the v2 that uses my approach.
Also drop Fixes tag, consider this as an improvement to make code robust.

> On Tue, Jun 9, 2026 at 11:15 AM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Mon, Jun 08, 2026 at 11:06:58PM +0400, Nikoloz Bakuradze wrote:
> > > c2h_wk_callback() allocates a 16-byte buffer with kmalloc(GFP_ATOMIC)
> > > when the c2h event needs to be read by the host. The existing guard
> > > only wraps the read step, so on allocation failure the loop body falls
> > > through with a NULL c2h_evt and dereferences it in rtw_hal_c2h_valid()
> > > (via c2h_evt_valid() which reads buf->id).
> > >
> > > Restructure the check into an early continue so the rest of the loop
> > > iteration cannot be reached with a NULL pointer.
> >
> >
> > Not sure if we need any Fixes tag. kmalloc(16) won't ever fail (otherwise
> > the system is already in the state when nothing can help).

...

> > >                       c2h_evt = kmalloc(16, GFP_ATOMIC);
> > > -                     if (c2h_evt) {
> > > -                             /* This C2H event is not read, read & clear now */
> > > -                             if (c2h_evt_read_88xx(adapter, c2h_evt) != _SUCCESS) {
> > > -                                     kfree(c2h_evt);
> > > -                                     continue;
> > > -                             }
> >
> > > +                     if (!c2h_evt)
> > > +                             continue;
> > > +                     /* This C2H event is not read, read & clear now */
> > > +                     if (c2h_evt_read_88xx(adapter, c2h_evt) != _SUCCESS) {
> > > +                             kfree(c2h_evt);
> > > +                             continue;
> >
> > It's too verbose way of saying
> >
> >                         } else
> >                                 continue;
> >
> > here.
> >
> > >                       }

-- 
With Best Regards,
Andy Shevchenko

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.