[RFC PATCH kernel] serial_core: Check for port state when tty is in error state

[RFC PATCH kernel] serial_core: Check for port state when tty is in error state

[Alexey Kardashevskiy]

At the moment opening a serial device node (such as /dev/ttyS3)
succeeds even if there is no actual serial device behind it.
Reading/writing/ioctls (most) expectantly fail as the uart port is not
initialized (the type is PORT_UNKNOWN) and the TTY_IO_ERROR error state
bit is set fot the tty.

However syzkaller (a syscall fuzzer) found that setting line discipline
does not have these checks all the way down to io_serial_out() in
8250_port.c (8250 is the default choice made by univ8250_console_init()).
As the result of PORT_UNKNOWN, uart_port::iobase is NULL which
a platform translates onto some address accessing which produces a crash
like below.

This adds tty_io_error() to uart_set_ldisc() to prevent the crash.

The example of crash on PPC64/pseries:

BUG: Unable to handle kernel data access on write at 0xc00a000000000001
Faulting instruction address: 0xc000000000c9c9cc
cpu 0x0: Vector: 300 (Data Access) at [c00000000c6d7800]
    pc: c000000000c9c9cc: io_serial_out+0xcc/0xf0
    lr: c000000000c9c9b4: io_serial_out+0xb4/0xf0
    sp: c00000000c6d7a90
   msr: 8000000000009033
   dar: c00a000000000001
 dsisr: 42000000
  current = 0xc00000000cd22500
  paca    = 0xc0000000035c0000   irqmask: 0x03   irq_happened: 0x01
    pid   = 1371, comm = syz-executor.0
Linux version 5.8.0-rc7-le-guest_syzkaller_a+fstn1 (aik@fstn1-p1) (gcc (Ubunt
untu) 2.30) #660 SMP Tue Jul 28 22:29:22 AEST 2020
enter ? for help
[c00000000c6d7a90] c0000000018a8cc0 _raw_spin_lock_irq+0xb0/0xe0 (unreliable)
[c00000000c6d7ad0] c000000000c9bdc0 serial8250_do_set_ldisc+0x140/0x180
[c00000000c6d7b10] c000000000c9bea4 serial8250_set_ldisc+0xa4/0xb0
[c00000000c6d7b50] c000000000c91138 uart_set_ldisc+0xb8/0x160
[c00000000c6d7b90] c000000000c5a22c tty_set_ldisc+0x23c/0x330
[c00000000c6d7c20] c000000000c4c220 tty_ioctl+0x990/0x12f0
[c00000000c6d7d20] c00000000056357c ksys_ioctl+0x14c/0x180
[c00000000c6d7d70] c0000000005635f0 sys_ioctl+0x40/0x60
[c00000000c6d7db0] c00000000003b814 system_call_exception+0x1a4/0x330
[c00000000c6d7e20] c00000000000d368 system_call_common+0xe8/0x214

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---

While looking at it, I noticed that a bunch of callbacks are prone to
this bug and since I wanted to fix them all with minimum effort,
I tried checking for PORT_UNKNOWN in uart_port_check() but it breaks
device opening. Another approach could be checking for uart_port::iobase
in 8250 (and probably uart_port::membase as well) but this will make
the rest of the code to think the device is ok while there is no device
at all.

What would the correct approach be and what is the expectation?

The fact that /dev/ttyS3 opened in the first place is confusing already.

---
 drivers/tty/serial/serial_core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index c15e208d9bec..cdece1c8e123 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1467,6 +1467,9 @@ static void uart_set_ldisc(struct tty_struct *tty)
 	struct uart_state *state = tty->driver_data;
 	struct uart_port *uport;
 
+	if (tty_io_error(tty))
+		return;
+
 	mutex_lock(&state->port.mutex);
 	uport = uart_port_check(state);
 	if (uport && uport->ops && uport->ops->set_ldisc)
-- 
2.17.1

Re: [RFC PATCH kernel] serial_core: Check for port state when tty is in error state

[Alexey Kardashevskiy]

Ping?

ps. Greg, your name came up in "git format-patch -M --stdout -1 HEAD |
./scripts/get_maintainer.pl --norolestats --l" ;)


On 28/07/2020 22:43, Alexey Kardashevskiy wrote:
> At the moment opening a serial device node (such as /dev/ttyS3)
> succeeds even if there is no actual serial device behind it.
> Reading/writing/ioctls (most) expectantly fail as the uart port is not
> initialized (the type is PORT_UNKNOWN) and the TTY_IO_ERROR error state
> bit is set fot the tty.
> 
> However syzkaller (a syscall fuzzer) found that setting line discipline
> does not have these checks all the way down to io_serial_out() in
> 8250_port.c (8250 is the default choice made by univ8250_console_init()).
> As the result of PORT_UNKNOWN, uart_port::iobase is NULL which
> a platform translates onto some address accessing which produces a crash
> like below.
> 
> This adds tty_io_error() to uart_set_ldisc() to prevent the crash.
> 
> The example of crash on PPC64/pseries:
> 
> BUG: Unable to handle kernel data access on write at 0xc00a000000000001
> Faulting instruction address: 0xc000000000c9c9cc
> cpu 0x0: Vector: 300 (Data Access) at [c00000000c6d7800]
>     pc: c000000000c9c9cc: io_serial_out+0xcc/0xf0
>     lr: c000000000c9c9b4: io_serial_out+0xb4/0xf0
>     sp: c00000000c6d7a90
>    msr: 8000000000009033
>    dar: c00a000000000001
>  dsisr: 42000000
>   current = 0xc00000000cd22500
>   paca    = 0xc0000000035c0000   irqmask: 0x03   irq_happened: 0x01
>     pid   = 1371, comm = syz-executor.0
> Linux version 5.8.0-rc7-le-guest_syzkaller_a+fstn1 (aik@fstn1-p1) (gcc (Ubunt
> untu) 2.30) #660 SMP Tue Jul 28 22:29:22 AEST 2020
> enter ? for help
> [c00000000c6d7a90] c0000000018a8cc0 _raw_spin_lock_irq+0xb0/0xe0 (unreliable)
> [c00000000c6d7ad0] c000000000c9bdc0 serial8250_do_set_ldisc+0x140/0x180
> [c00000000c6d7b10] c000000000c9bea4 serial8250_set_ldisc+0xa4/0xb0
> [c00000000c6d7b50] c000000000c91138 uart_set_ldisc+0xb8/0x160
> [c00000000c6d7b90] c000000000c5a22c tty_set_ldisc+0x23c/0x330
> [c00000000c6d7c20] c000000000c4c220 tty_ioctl+0x990/0x12f0
> [c00000000c6d7d20] c00000000056357c ksys_ioctl+0x14c/0x180
> [c00000000c6d7d70] c0000000005635f0 sys_ioctl+0x40/0x60
> [c00000000c6d7db0] c00000000003b814 system_call_exception+0x1a4/0x330
> [c00000000c6d7e20] c00000000000d368 system_call_common+0xe8/0x214
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> 
> While looking at it, I noticed that a bunch of callbacks are prone to
> this bug and since I wanted to fix them all with minimum effort,
> I tried checking for PORT_UNKNOWN in uart_port_check() but it breaks
> device opening. Another approach could be checking for uart_port::iobase
> in 8250 (and probably uart_port::membase as well) but this will make
> the rest of the code to think the device is ok while there is no device
> at all.
> 
> What would the correct approach be and what is the expectation?
> 
> The fact that /dev/ttyS3 opened in the first place is confusing already.
> 
> ---
>  drivers/tty/serial/serial_core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index c15e208d9bec..cdece1c8e123 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1467,6 +1467,9 @@ static void uart_set_ldisc(struct tty_struct *tty)
>  	struct uart_state *state = tty->driver_data;
>  	struct uart_port *uport;
>  
> +	if (tty_io_error(tty))
> +		return;
> +
>  	mutex_lock(&state->port.mutex);
>  	uport = uart_port_check(state);
>  	if (uport && uport->ops && uport->ops->set_ldisc)
> 

-- 
Alexey

Re: [RFC PATCH kernel] serial_core: Check for port state when tty is in error state

[Greg Kroah-Hartman]

On Tue, Aug 04, 2020 at 08:02:27PM +1000, Alexey Kardashevskiy wrote:
> Ping?
> 
> ps. Greg, your name came up in "git format-patch -M --stdout -1 HEAD |
> ./scripts/get_maintainer.pl --norolestats --l" ;)

It's in my queue to review, but can't do anything now that the merge
window is open.

Also, as it's an RFC, that means you don't think it should be merged, so
perhaps do some work to verify that you think this is correct and resend
it?

thanks,

greg k-h

Re: [RFC PATCH kernel] serial_core: Check for port state when tty is in error state

[Alexey Kardashevskiy]

On 04/08/2020 20:09, Greg Kroah-Hartman wrote:
> On Tue, Aug 04, 2020 at 08:02:27PM +1000, Alexey Kardashevskiy wrote:
>> Ping?
>>
>> ps. Greg, your name came up in "git format-patch -M --stdout -1 HEAD |
>> ./scripts/get_maintainer.pl --norolestats --l" ;)
> 
> It's in my queue to review, but can't do anything now that the merge
> window is open.

Ah ok. No hurry.

> Also, as it's an RFC, that means you don't think it should be merged, so
> perhaps do some work to verify that you think this is correct and resend
> it?

It is correct, I am just not too sure if I should add such checks
everywhere, that's all. Thanks,

> 
> thanks,
> 
> greg k-h
> 

-- 
Alexey

Re: [RFC PATCH kernel] serial_core: Check for port state when tty is in error state

[Greg Kroah-Hartman]

On Tue, Jul 28, 2020 at 10:43:59PM +1000, Alexey Kardashevskiy wrote:
> At the moment opening a serial device node (such as /dev/ttyS3)
> succeeds even if there is no actual serial device behind it.
> Reading/writing/ioctls (most) expectantly fail as the uart port is not
> initialized (the type is PORT_UNKNOWN) and the TTY_IO_ERROR error state
> bit is set fot the tty.

That is only if there is no ldisc set for the port, right?  I don't
think that always will be the case if the port is not initialized.

Yes, we do clear this on port open, but we clear it before the
->activate() callback happens.

Why not check for initialized instead?  That would seem to be what you
want to do here instead of checking for an io error.

> However syzkaller (a syscall fuzzer) found that setting line discipline
> does not have these checks all the way down to io_serial_out() in
> 8250_port.c (8250 is the default choice made by univ8250_console_init()).
> As the result of PORT_UNKNOWN, uart_port::iobase is NULL which
> a platform translates onto some address accessing which produces a crash
> like below.
> 
> This adds tty_io_error() to uart_set_ldisc() to prevent the crash.
> 
> The example of crash on PPC64/pseries:
> 
> BUG: Unable to handle kernel data access on write at 0xc00a000000000001
> Faulting instruction address: 0xc000000000c9c9cc
> cpu 0x0: Vector: 300 (Data Access) at [c00000000c6d7800]
>     pc: c000000000c9c9cc: io_serial_out+0xcc/0xf0
>     lr: c000000000c9c9b4: io_serial_out+0xb4/0xf0
>     sp: c00000000c6d7a90
>    msr: 8000000000009033
>    dar: c00a000000000001
>  dsisr: 42000000
>   current = 0xc00000000cd22500
>   paca    = 0xc0000000035c0000   irqmask: 0x03   irq_happened: 0x01
>     pid   = 1371, comm = syz-executor.0
> Linux version 5.8.0-rc7-le-guest_syzkaller_a+fstn1 (aik@fstn1-p1) (gcc (Ubunt
> untu) 2.30) #660 SMP Tue Jul 28 22:29:22 AEST 2020
> enter ? for help
> [c00000000c6d7a90] c0000000018a8cc0 _raw_spin_lock_irq+0xb0/0xe0 (unreliable)
> [c00000000c6d7ad0] c000000000c9bdc0 serial8250_do_set_ldisc+0x140/0x180
> [c00000000c6d7b10] c000000000c9bea4 serial8250_set_ldisc+0xa4/0xb0
> [c00000000c6d7b50] c000000000c91138 uart_set_ldisc+0xb8/0x160
> [c00000000c6d7b90] c000000000c5a22c tty_set_ldisc+0x23c/0x330
> [c00000000c6d7c20] c000000000c4c220 tty_ioctl+0x990/0x12f0
> [c00000000c6d7d20] c00000000056357c ksys_ioctl+0x14c/0x180
> [c00000000c6d7d70] c0000000005635f0 sys_ioctl+0x40/0x60
> [c00000000c6d7db0] c00000000003b814 system_call_exception+0x1a4/0x330
> [c00000000c6d7e20] c00000000000d368 system_call_common+0xe8/0x214
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> 
> While looking at it, I noticed that a bunch of callbacks are prone to
> this bug and since I wanted to fix them all with minimum effort,
> I tried checking for PORT_UNKNOWN in uart_port_check() but it breaks
> device opening. Another approach could be checking for uart_port::iobase
> in 8250 (and probably uart_port::membase as well) but this will make
> the rest of the code to think the device is ok while there is no device
> at all.
> 
> What would the correct approach be and what is the expectation?

We should probably check tty_port_initialized() on these code paths
better, care to fix that up?

thanks,

greg k-h