Re: [PATCH v3] usb: gadget: u_serial: Add null pointer check in gserial_resume

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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

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



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux