| Message ID | 1683278317-11774-1-git-send-email-quic_prashk@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: gadget: u_serial: Add null pointer check in gserial_suspend | expand |
On Fri, May 05, 2023 at 02:48:37PM +0530, Prashanth K wrote: > Consider a case where gserial_disconnect has already cleared > gser->ioport. And if gserial_suspend gets called afterwards, > it will lead to accessing of gser->ioport and thus causing > null pointer dereference. > > Avoid this by adding a null pointer check. Added a static > spinlock to prevent gser->ioport from becoming null after > the newly added null pointer check. > > Fixes: aba3a8d01d62 ("usb: gadget: u_serial: add suspend resume callbacks") > Signed-off-by: Prashanth K <quic_prashk@quicinc.com> > --- > drivers/usb/gadget/function/u_serial.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c > index a0ca47f..e5d522d 100644 > --- a/drivers/usb/gadget/function/u_serial.c > +++ b/drivers/usb/gadget/function/u_serial.c > @@ -1420,10 +1420,19 @@ EXPORT_SYMBOL_GPL(gserial_disconnect); > > void gserial_suspend(struct gserial *gser) > { > - struct gs_port *port = gser->ioport; > + struct gs_port *port; > unsigned long flags; > > - spin_lock_irqsave(&port->port_lock, flags); > + spin_lock_irqsave(&serial_port_lock, flags); > + port = gser->ioport; > + > + if (!port) { > + spin_unlock_irqrestore(&serial_port_lock, flags); > + return; > + } > + > + spin_lock(&port->port_lock); > + spin_unlock(&serial_port_lock); > port->suspended = true; > spin_unlock_irqrestore(&port->port_lock, flags); > } This looks fine to me, but I'm not a serial-gadget maintainer. In fact, it looks like we don't have a serial-gadget maintainer. Alan Stern
diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c index a0ca47f..e5d522d 100644 --- a/drivers/usb/gadget/function/u_serial.c +++ b/drivers/usb/gadget/function/u_serial.c @@ -1420,10 +1420,19 @@ EXPORT_SYMBOL_GPL(gserial_disconnect); void gserial_suspend(struct gserial *gser) { - struct gs_port *port = gser->ioport; + struct gs_port *port; unsigned long flags; - spin_lock_irqsave(&port->port_lock, flags); + spin_lock_irqsave(&serial_port_lock, flags); + port = gser->ioport; + + if (!port) { + spin_unlock_irqrestore(&serial_port_lock, flags); + return; + } + + spin_lock(&port->port_lock); + spin_unlock(&serial_port_lock); port->suspended = true; spin_unlock_irqrestore(&port->port_lock, flags); }
Consider a case where gserial_disconnect has already cleared gser->ioport. And if gserial_suspend gets called afterwards, it will lead to accessing of gser->ioport and thus causing null pointer dereference. Avoid this by adding a null pointer check. Added a static spinlock to prevent gser->ioport from becoming null after the newly added null pointer check. Fixes: aba3a8d01d62 ("usb: gadget: u_serial: add suspend resume callbacks") Signed-off-by: Prashanth K <quic_prashk@quicinc.com> --- drivers/usb/gadget/function/u_serial.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)