Patchwork TTY: serial, stop accessing potential NULLs

login
register
mail settings
Submitter Jiri Slaby
Date March 13, 2013, 11:30 p.m.
Message ID <1363217434-29262-1-git-send-email-jslaby@suse.cz>
Download mbox | patch
Permalink /patch/227392/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Jiri Slaby - March 13, 2013, 11:30 p.m.
The following commits:
* 6732c8bb8671acbdac6cdc93dd72ddd581dd5e25 (TTY: switch
  tty_schedule_flip)
* 2e124b4a390ca85325fae75764bef92f0547fa25 (TTY: switch
  tty_flip_buffer_push)
* 05c7cd39907184328f48d3e7899f9cdd653ad336 (TTY: switch
  tty_insert_flip_string)
* 92a19f9cec9a80ad93c06e115822deb729e2c6ad (TTY: switch
  tty_insert_flip_char)
* 227434f8986c3827a1faedd1feb437acd6285315 (TTY: switch
  tty_buffer_request_room to tty_port)

introduced a potential NULL dereference to some drivers. In
particular, when the device is used as a console, incoming bytes can
kill the box. This is caused by removed checks for TTY against NULL.

It happened because it was unclear to me why the checks were there. I
assumed them superfluous because the interrupts were unbound or
otherwise stopped. But this is not the case for consoles for these
drivers, as was pointed out by David Miller.

Now, this patch re-introduces the checks (at this point we check
port->state, not the tty proper, as we do not care about tty pointers
anymore). For both of the drivers, we place the check below the
handling of break signal so that sysrq can actually work. (One needs
to issue a break and then sysrq key within the following 5 seconds.)

We do not change sc26xx, sunhv, and sunsu here because they behave the
same as before.  People having that hardware should fix the driver
eventually, however. They always could unconditionally dereference tty
in receive_chars, port->state in uart_handle_dcd_change, and
up->port.state->port.tty.

There is perhaps more to fix in all those drivers, but they are at
least in a state they were before.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: sparclinux@vger.kernel.org
---
 drivers/tty/serial/sunsab.c   | 2 +-
 drivers/tty/serial/sunzilog.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
David Miller - March 20, 2013, 7:02 p.m.
From: Jiri Slaby <jslaby@suse.cz>
Date: Thu, 14 Mar 2013 00:30:34 +0100

> The following commits:
> * 6732c8bb8671acbdac6cdc93dd72ddd581dd5e25 (TTY: switch
>   tty_schedule_flip)
> * 2e124b4a390ca85325fae75764bef92f0547fa25 (TTY: switch
>   tty_flip_buffer_push)
> * 05c7cd39907184328f48d3e7899f9cdd653ad336 (TTY: switch
>   tty_insert_flip_string)
> * 92a19f9cec9a80ad93c06e115822deb729e2c6ad (TTY: switch
>   tty_insert_flip_char)
> * 227434f8986c3827a1faedd1feb437acd6285315 (TTY: switch
>   tty_buffer_request_room to tty_port)
> 
> introduced a potential NULL dereference to some drivers. In
> particular, when the device is used as a console, incoming bytes can
> kill the box. This is caused by removed checks for TTY against NULL.
> 
> It happened because it was unclear to me why the checks were there. I
> assumed them superfluous because the interrupts were unbound or
> otherwise stopped. But this is not the case for consoles for these
> drivers, as was pointed out by David Miller.
> 
> Now, this patch re-introduces the checks (at this point we check
> port->state, not the tty proper, as we do not care about tty pointers
> anymore). For both of the drivers, we place the check below the
> handling of break signal so that sysrq can actually work. (One needs
> to issue a break and then sysrq key within the following 5 seconds.)
> 
> We do not change sc26xx, sunhv, and sunsu here because they behave the
> same as before.  People having that hardware should fix the driver
> eventually, however. They always could unconditionally dereference tty
> in receive_chars, port->state in uart_handle_dcd_change, and
> up->port.state->port.tty.
> 
> There is perhaps more to fix in all those drivers, but they are at
> least in a state they were before.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

Acked-by: David S. Miller <davem@davemloft.net>
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/tty/serial/sunsab.c b/drivers/tty/serial/sunsab.c
index 8de2213..a422c8b 100644
--- a/drivers/tty/serial/sunsab.c
+++ b/drivers/tty/serial/sunsab.c
@@ -203,7 +203,7 @@  receive_chars(struct uart_sunsab_port *up,
 				flag = TTY_FRAME;
 		}
 
-		if (uart_handle_sysrq_char(&up->port, ch))
+		if (uart_handle_sysrq_char(&up->port, ch) || !port)
 			continue;
 
 		if ((stat->sreg.isr0 & (up->port.ignore_status_mask & 0xff)) == 0 &&
diff --git a/drivers/tty/serial/sunzilog.c b/drivers/tty/serial/sunzilog.c
index 27669ff..813ef8e 100644
--- a/drivers/tty/serial/sunzilog.c
+++ b/drivers/tty/serial/sunzilog.c
@@ -388,7 +388,7 @@  sunzilog_receive_chars(struct uart_sunzilog_port *up,
 			else if (r1 & CRC_ERR)
 				flag = TTY_FRAME;
 		}
-		if (uart_handle_sysrq_char(&up->port, ch))
+		if (uart_handle_sysrq_char(&up->port, ch) || !port)
 			continue;
 
 		if (up->port.ignore_status_mask == 0xff ||