| Message ID | 1676146033-3948-1-git-send-email-quic_prashk@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] usb: gadget: u_serial: Add null pointer check in gserial_resume | expand |
On Sun, Feb 12, 2023 at 01:37:13AM +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 > gser->ioport and thus causing null pointer dereference.Add > a null pointer check to prevent this. > > Added a static spinlock to prevent gser->ioport from becoming > null after the newly added check. > > Fixes: aba3a8d01d62 ("usb: gadget: u_serial: add suspend resume callbacks") > Signed-off-by: Prashanth K <quic_prashk@quicinc.com> > --- This looks pretty good, except for a couple of small things... > v3: Fixed the spin_lock_irqsave flags. > > drivers/usb/gadget/function/u_serial.c | 22 +++++++++++++++++----- > 1 file changed, 17 insertions(+), 5 deletions(-) > > diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c > index 840626e..471087f 100644 > --- a/drivers/usb/gadget/function/u_serial.c > +++ b/drivers/usb/gadget/function/u_serial.c > @@ -82,6 +82,8 @@ > #define WRITE_BUF_SIZE 8192 /* TX only */ > #define GS_CONSOLE_BUF_SIZE 8192 > > +static DEFINE_SPINLOCK(serial_port_lock); You might put a short comment before this line, explaining what the purpose of serial_port_lock is. Otherwise people will wonder what it is for. > + > /* console info */ > struct gs_console { > struct console console; > @@ -1370,13 +1372,15 @@ EXPORT_SYMBOL_GPL(gserial_connect); > void gserial_disconnect(struct gserial *gser) > { > struct gs_port *port = gser->ioport; > - unsigned long flags; > + unsigned long flags; Unnecessary whitespace change. Leave the original code as it is. > > if (!port) > return; Is it really possible for port to be NULL here? If it is possible, where would gser->ioport be set to NULL? And if it's not possible, this test should be removed. > > + spin_lock_irqsave(&serial_port_lock, flags); > + > /* tell the TTY glue not to do I/O here any more */ > - spin_lock_irqsave(&port->port_lock, flags); > + spin_lock(&port->port_lock); > > gs_console_disconnect(port); > > @@ -1391,7 +1395,8 @@ void gserial_disconnect(struct gserial *gser) > tty_hangup(port->port.tty); > } > port->suspended = false; > - spin_unlock_irqrestore(&port->port_lock, flags); > + spin_unlock(&port->port_lock); > + spin_unlock_irqrestore(&serial_port_lock, flags); > > /* disable endpoints, aborting down any active I/O */ > usb_ep_disable(gser->out); > @@ -1426,9 +1431,16 @@ EXPORT_SYMBOL_GPL(gserial_suspend); > void gserial_resume(struct gserial *gser) > { > struct gs_port *port = gser->ioport; You shouldn't read gser->ioport here; do it under the protection of the static spinlock. If you do the read here then there will still be a data race, because gserial_disconnect() might change the value just as you are reading it. > - unsigned long flags; > + unsigned long flags; Again, unnecessary whitespace change. > > - spin_lock_irqsave(&port->port_lock, flags); > + spin_lock_irqsave(&serial_port_lock, flags); Here is where you should read gser->ioport. > + if (!port) { > + spin_unlock_irqrestore(&serial_port_lock, flags); > + return; > + } > + > + spin_lock(&port->port_lock); > + spin_unlock(&serial_port_lock); > port->suspended = false; > if (!port->start_delayed) { > spin_unlock_irqrestore(&port->port_lock, flags); Alan Stern
On 12-02-23 02:02 am, Alan Stern wrote: > On Sun, Feb 12, 2023 at 01:37:13AM +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 >> gser->ioport and thus causing null pointer dereference.Add >> a null pointer check to prevent this. >> >> Added a static spinlock to prevent gser->ioport from becoming >> null after the newly added check. >> >> Fixes: aba3a8d01d62 ("usb: gadget: u_serial: add suspend resume callbacks") >> Signed-off-by: Prashanth K <quic_prashk@quicinc.com> >> --- > > This looks pretty good, except for a couple of small things... > >> v3: Fixed the spin_lock_irqsave flags. >> >> drivers/usb/gadget/function/u_serial.c | 22 +++++++++++++++++----- >> 1 file changed, 17 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c >> index 840626e..471087f 100644 >> --- a/drivers/usb/gadget/function/u_serial.c >> +++ b/drivers/usb/gadget/function/u_serial.c >> @@ -82,6 +82,8 @@ >> #define WRITE_BUF_SIZE 8192 /* TX only */ >> #define GS_CONSOLE_BUF_SIZE 8192 >> >> +static DEFINE_SPINLOCK(serial_port_lock); > > You might put a short comment before this line, explaining what the > purpose of serial_port_lock is. Otherwise people will wonder what it is > for. That's right, will do it. > >> + >> /* console info */ >> struct gs_console { >> struct console console; >> @@ -1370,13 +1372,15 @@ EXPORT_SYMBOL_GPL(gserial_connect); >> void gserial_disconnect(struct gserial *gser) >> { >> struct gs_port *port = gser->ioport; >> - unsigned long flags; >> + unsigned long flags; > > Unnecessary whitespace change. Leave the original code as it is. > >> >> if (!port) >> return; > > Is it really possible for port to be NULL here? If it is possible, > where would gser->ioport be set to NULL? > > And if it's not possible, this test should be removed. Seems like this is present since the inception of this file, hence I'm not exactly sure why it was added. In my opinion lets keep it, so as to not cause regressions. > >> >> + spin_lock_irqsave(&serial_port_lock, flags); >> + >> /* tell the TTY glue not to do I/O here any more */ >> - spin_lock_irqsave(&port->port_lock, flags); >> + spin_lock(&port->port_lock); >> >> gs_console_disconnect(port); >> >> @@ -1391,7 +1395,8 @@ void gserial_disconnect(struct gserial *gser) >> tty_hangup(port->port.tty); >> } >> port->suspended = false; >> - spin_unlock_irqrestore(&port->port_lock, flags); >> + spin_unlock(&port->port_lock); >> + spin_unlock_irqrestore(&serial_port_lock, flags); >> >> /* disable endpoints, aborting down any active I/O */ >> usb_ep_disable(gser->out); >> @@ -1426,9 +1431,16 @@ EXPORT_SYMBOL_GPL(gserial_suspend); >> void gserial_resume(struct gserial *gser) >> { >> struct gs_port *port = gser->ioport; > > You shouldn't read gser->ioport here; do it under the protection of the > static spinlock. If you do the read here then there will still be a > data race, because gserial_disconnect() might change the value just as > you are reading it. > >> - unsigned long flags; >> + unsigned long flags; > > Again, unnecessary whitespace change. > >> >> - spin_lock_irqsave(&port->port_lock, flags); >> + spin_lock_irqsave(&serial_port_lock, flags); > > Here is where you should read gser->ioport. Yes, understood > >> + if (!port) { >> + spin_unlock_irqrestore(&serial_port_lock, flags); >> + return; >> + } >> + >> + spin_lock(&port->port_lock); >> + spin_unlock(&serial_port_lock); >> port->suspended = false; >> if (!port->start_delayed) { >> spin_unlock_irqrestore(&port->port_lock, flags); > > Alan Stern
diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c index 840626e..471087f 100644 --- a/drivers/usb/gadget/function/u_serial.c +++ b/drivers/usb/gadget/function/u_serial.c @@ -82,6 +82,8 @@ #define WRITE_BUF_SIZE 8192 /* TX only */ #define GS_CONSOLE_BUF_SIZE 8192 +static DEFINE_SPINLOCK(serial_port_lock); + /* console info */ struct gs_console { struct console console; @@ -1370,13 +1372,15 @@ EXPORT_SYMBOL_GPL(gserial_connect); void gserial_disconnect(struct gserial *gser) { struct gs_port *port = gser->ioport; - unsigned long flags; + unsigned long flags; if (!port) return; + spin_lock_irqsave(&serial_port_lock, flags); + /* tell the TTY glue not to do I/O here any more */ - spin_lock_irqsave(&port->port_lock, flags); + spin_lock(&port->port_lock); gs_console_disconnect(port); @@ -1391,7 +1395,8 @@ void gserial_disconnect(struct gserial *gser) tty_hangup(port->port.tty); } port->suspended = false; - spin_unlock_irqrestore(&port->port_lock, flags); + spin_unlock(&port->port_lock); + spin_unlock_irqrestore(&serial_port_lock, flags); /* disable endpoints, aborting down any active I/O */ usb_ep_disable(gser->out); @@ -1426,9 +1431,16 @@ EXPORT_SYMBOL_GPL(gserial_suspend); void gserial_resume(struct gserial *gser) { struct gs_port *port = gser->ioport; - unsigned long flags; + unsigned long flags; - spin_lock_irqsave(&port->port_lock, flags); + spin_lock_irqsave(&serial_port_lock, flags); + if (!port) { + spin_unlock_irqrestore(&serial_port_lock, flags); + return; + } + + spin_lock(&port->port_lock); + spin_unlock(&serial_port_lock); port->suspended = false; if (!port->start_delayed) { spin_unlock_irqrestore(&port->port_lock, flags);
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 gser->ioport and thus causing null pointer dereference.Add a null pointer check to prevent this. Added a static spinlock to prevent gser->ioport from becoming null after the newly added check. Fixes: aba3a8d01d62 ("usb: gadget: u_serial: add suspend resume callbacks") Signed-off-by: Prashanth K <quic_prashk@quicinc.com> --- v3: Fixed the spin_lock_irqsave flags. drivers/usb/gadget/function/u_serial.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)