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 <[email protected]>
---
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
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 <[email protected]>
> ---
>
> 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
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
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
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 <[email protected]>
> ---
>
> 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