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

[Andy Shevchenko]

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@xxxxxxxxx> 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