[PATCH 1/2] USB: serial/keyspan_pda, fix potential tty NULL dereferences

[PATCH 1/2] USB: serial/keyspan_pda, fix potential tty NULL dereferences

[Jiri Slaby]

Make sure that we check the return value of tty_port_tty_get.
Sometimes it may return NULL and we later dereference that.

There are several places to check. For easier handling,
tty_port_tty_get is moved directly to the palce where needed in
keyspan_pda_rx_interrupt.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/usb/serial/keyspan_pda.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/serial/keyspan_pda.c b/drivers/usb/serial/keyspan_pda.c
index 554a869..7b690f3 100644
--- a/drivers/usb/serial/keyspan_pda.c
+++ b/drivers/usb/serial/keyspan_pda.c
@@ -173,7 +173,8 @@ static void keyspan_pda_wakeup_write(struct work_struct *work)
 		container_of(work, struct keyspan_pda_private, wakeup_work);
 	struct usb_serial_port *port = priv->port;
 	struct tty_struct *tty = tty_port_tty_get(&port->port);
-	tty_wakeup(tty);
+	if (tty)
+		tty_wakeup(tty);
 	tty_kref_put(tty);
 }
 
@@ -206,7 +207,7 @@ static void keyspan_pda_request_unthrottle(struct work_struct *work)
 static void keyspan_pda_rx_interrupt(struct urb *urb)
 {
 	struct usb_serial_port *port = urb->context;
-	struct tty_struct *tty = tty_port_tty_get(&port->port);
+	struct tty_struct *tty;
 	unsigned char *data = urb->transfer_buffer;
 	int retval;
 	int status = urb->status;
@@ -223,7 +224,7 @@ static void keyspan_pda_rx_interrupt(struct urb *urb)
 		/* this urb is terminated, clean up */
 		dbg("%s - urb shutting down with status: %d",
 		    __func__, status);
-		goto out;
+		return;
 	default:
 		dbg("%s - nonzero urb status received: %d",
 		    __func__, status);
@@ -233,12 +234,14 @@ static void keyspan_pda_rx_interrupt(struct urb *urb)
 	/* see if the message is data or a status interrupt */
 	switch (data[0]) {
 	case 0:
-		/* rest of message is rx data */
-		if (urb->actual_length) {
+		tty = tty_port_tty_get(&port->port);
+		 /* rest of message is rx data */
+		if (tty && urb->actual_length) {
 			tty_insert_flip_string(tty, data + 1,
 						urb->actual_length - 1);
 			tty_flip_buffer_push(tty);
 		}
+		tty_kref_put(tty);
 		break;
 	case 1:
 		/* status interrupt */
@@ -265,8 +268,6 @@ exit:
 		dev_err(&port->dev,
 			"%s - usb_submit_urb failed with result %d",
 			__func__, retval);
-out:
-	tty_kref_put(tty);		     
 }
 
 
-- 
1.7.4.1

[PATCH 2/2] USB: serial/kobil_sct, fix potential tty NULL dereference

[Jiri Slaby]

Make sure that we check the return value of tty_port_tty_get.
Sometimes it may return NULL and we later dereference that.

The only place here is in kobil_read_int_callback, so fix it.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: stable@kernel.org
---
 drivers/usb/serial/kobil_sct.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/serial/kobil_sct.c b/drivers/usb/serial/kobil_sct.c
index bd5bd85..b382d9a 100644
--- a/drivers/usb/serial/kobil_sct.c
+++ b/drivers/usb/serial/kobil_sct.c
@@ -372,7 +372,7 @@ static void kobil_read_int_callback(struct urb *urb)
 	}
 
 	tty = tty_port_tty_get(&port->port);
-	if (urb->actual_length) {
+	if (tty && urb->actual_length) {
 
 		/* BEGIN DEBUG */
 		/*
-- 
1.7.4.1

Re: [PATCH 1/2] USB: serial/keyspan_pda, fix potential tty NULL dereferences

[Alan Stern]

On Mon, 28 Feb 2011, Jiri Slaby wrote:

> Make sure that we check the return value of tty_port_tty_get.
> Sometimes it may return NULL and we later dereference that.
> 
> There are several places to check. For easier handling,
> tty_port_tty_get is moved directly to the palce where needed in
> keyspan_pda_rx_interrupt.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Alan Cox <alan@linux.intel.com>
> Cc: Greg Kroah-Hartman <gregkh@suse.de>

I wonder about these changes.  Does it makes sense to add this checking
everywhere?  A more reasonable approach might be to make sure that
tty_port_tty_get is never called in a context where it could return
NULL.  Or would that involve just as much effort, making work routines
and so on check to see whether there's an open TTY device before doing
anything else?

Alan Stern

Re: [PATCH 1/2] USB: serial/keyspan_pda, fix potential tty NULL dereferences

[Alan Cox]

> I wonder about these changes.  Does it makes sense to add this
> checking everywhere?  A more reasonable approach might be to make
> sure that tty_port_tty_get is never called in a context where it
> could return NULL.  Or would that involve just as much effort, making
> work routines and so on check to see whether there's an open TTY
> device before doing anything else?

The tty is refcounted so it can go away at any time including on
another processor parallel to an IRQ happening.

The right way to fix it longer term is to get every tty to be using a
tty_port (which is something we are close to) and then stuff all the
physical device related properties into the tty_port so most rx paths
simply don't dereference the tty struct.

At that point the lifetime of the relevant objects is the lifetime of
the physical port which means the driver can do

	ref count = 0
		disable interrupts
		free physical port representation & associated tty_port

the locking and tty struct use on most of the irq paths can go away for
most situations (still needed for some cases like SYSRQ).

Unfortunately there is quite a lot to move including the tty buffers,
various bits of flow control, wait queues and the like. Possibly even
the termios data.

At that point we'd only need a tty reference when we actually did the
ldisc processing or in exceptional situations.

Alan