On Wed, Feb 08, 2023 at 09:15:54PM +0530, Prashanth K wrote: > > > On 08-02-23 08:24 pm, Greg Kroah-Hartman wrote: > > On Wed, Feb 08, 2023 at 07:24:47PM +0530, Prashanth K wrote: > > > Consider a case where gserial_disconnect has already cleared > > > gser->ioport. And if a wakeup interrupt triggers afterwards, > > > gserial_resume gets called, which will lead to accessing of > > > gserial->port and thus causing null pointer dereference.Add > > > a null pointer check to prevent this. > > > > > > Fixes: aba3a8d01d62 (" usb: gadget: u_serial: add suspend resume callbacks") > > > > Nit, and our tools will complain, no " " before the "usb:" string here, > > right? > > > Will fix it in next patch. > > > > > > > Signed-off-by: Prashanth K <quic_prashk@xxxxxxxxxxx> > > > --- > > > drivers/usb/gadget/function/u_serial.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c > > > index 840626e..98be2b8 100644 > > > --- a/drivers/usb/gadget/function/u_serial.c > > > +++ b/drivers/usb/gadget/function/u_serial.c > > > @@ -1428,6 +1428,9 @@ void gserial_resume(struct gserial *gser) > > > struct gs_port *port = gser->ioport; > > > unsigned long flags; > > > + if (!port) > > > + return; > > > + > > > > What prevents port from going to NULL right after this check? > In our case we got a null pointer de-reference while performing USB > compliance tests, as the gser->port was null. Because in gserial_resume, > spinlock_irq_save(&port->port_lock) accesses a null-pointer as port was > already marked null by gserial_disconnect. > > And after gserial_resume acquires the spinlock, gserial_disconnect cant mark > it null until the spinlock is released. We need to check if the port->lock > is valid before accessing it, otherwise it can lead to the above mentioned > scenario What happens if gserial_disconnect sets gser->port to NULL immediately after your new check occurs, but before spinlock_irq_save(&port->port_lock) gets called? You may need to add a static spinlock to prevent this from happening. Alan Stern