diff mbox series

[1/2] serdev: ttyport: fix use-after-free on closed TTY

Message ID 20221221163249.1058459-1-krzysztof.kozlowski@linaro.org
State New
Headers show
Series [1/2] serdev: ttyport: fix use-after-free on closed TTY | expand

Commit Message

Krzysztof Kozlowski Dec. 21, 2022, 4:32 p.m. UTC 
use-after-free is visible in serdev-ttyport, e.g. during system reboot
with Qualcomm Atheros Bluetooth.  The TTY is closed, thus "struct
tty_struct" is being released, but the hci_uart_qca driver performs
writes and flushes during system shutdown in qca_serdev_shutdown().

  Unable to handle kernel paging request at virtual address 0072662f67726fd7
  ...
  CPU: 6 PID: 1 Comm: systemd-shutdow Tainted: G        W          6.1.0-rt5-00325-g8a5f56bcfcca #8
  Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
  Call trace:
   tty_driver_flush_buffer+0x4/0x30
   serdev_device_write_flush+0x24/0x34
   qca_serdev_shutdown+0x80/0x130 [hci_uart]
   device_shutdown+0x15c/0x260
   kernel_restart+0x48/0xac

KASAN report:

  BUG: KASAN: use-after-free in tty_driver_flush_buffer+0x1c/0x50
  Read of size 8 at addr ffff16270c2e0018 by task systemd-shutdow/1

  CPU: 7 PID: 1 Comm: systemd-shutdow Not tainted 6.1.0-next-20221220-00014-gb85aaf97fb01-dirty #28
  Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
  Call trace:
   dump_backtrace.part.0+0xdc/0xf0
   show_stack+0x18/0x30
   dump_stack_lvl+0x68/0x84
   print_report+0x188/0x488
   kasan_report+0xa4/0xf0
   __asan_load8+0x80/0xac
   tty_driver_flush_buffer+0x1c/0x50
   ttyport_write_flush+0x34/0x44
   serdev_device_write_flush+0x48/0x60
   qca_serdev_shutdown+0x124/0x274
   device_shutdown+0x1e8/0x350
   kernel_restart+0x48/0xb0
   __do_sys_reboot+0x244/0x2d0
   __arm64_sys_reboot+0x54/0x70
   invoke_syscall+0x60/0x190
   el0_svc_common.constprop.0+0x7c/0x160
   do_el0_svc+0x44/0xf0
   el0_svc+0x2c/0x6c
   el0t_64_sync_handler+0xbc/0x140
   el0t_64_sync+0x190/0x194

Fixes: bed35c6dfa6a ("serdev: add a tty port controller driver")
Cc: <stable@vger.kernel.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/tty/serdev/serdev-ttyport.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Comments

Greg Kroah-Hartman Dec. 21, 2022, 4:39 p.m. UTC | #1 
On Wed, Dec 21, 2022 at 05:32:48PM +0100, Krzysztof Kozlowski wrote:
> use-after-free is visible in serdev-ttyport, e.g. during system reboot
> with Qualcomm Atheros Bluetooth.  The TTY is closed, thus "struct
> tty_struct" is being released, but the hci_uart_qca driver performs
> writes and flushes during system shutdown in qca_serdev_shutdown().
> 
>   Unable to handle kernel paging request at virtual address 0072662f67726fd7
>   ...
>   CPU: 6 PID: 1 Comm: systemd-shutdow Tainted: G        W          6.1.0-rt5-00325-g8a5f56bcfcca #8
>   Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
>   Call trace:
>    tty_driver_flush_buffer+0x4/0x30
>    serdev_device_write_flush+0x24/0x34
>    qca_serdev_shutdown+0x80/0x130 [hci_uart]
>    device_shutdown+0x15c/0x260
>    kernel_restart+0x48/0xac
> 
> KASAN report:
> 
>   BUG: KASAN: use-after-free in tty_driver_flush_buffer+0x1c/0x50
>   Read of size 8 at addr ffff16270c2e0018 by task systemd-shutdow/1
> 
>   CPU: 7 PID: 1 Comm: systemd-shutdow Not tainted 6.1.0-next-20221220-00014-gb85aaf97fb01-dirty #28
>   Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
>   Call trace:
>    dump_backtrace.part.0+0xdc/0xf0
>    show_stack+0x18/0x30
>    dump_stack_lvl+0x68/0x84
>    print_report+0x188/0x488
>    kasan_report+0xa4/0xf0
>    __asan_load8+0x80/0xac
>    tty_driver_flush_buffer+0x1c/0x50
>    ttyport_write_flush+0x34/0x44
>    serdev_device_write_flush+0x48/0x60
>    qca_serdev_shutdown+0x124/0x274
>    device_shutdown+0x1e8/0x350
>    kernel_restart+0x48/0xb0
>    __do_sys_reboot+0x244/0x2d0
>    __arm64_sys_reboot+0x54/0x70
>    invoke_syscall+0x60/0x190
>    el0_svc_common.constprop.0+0x7c/0x160
>    do_el0_svc+0x44/0xf0
>    el0_svc+0x2c/0x6c
>    el0t_64_sync_handler+0xbc/0x140
>    el0t_64_sync+0x190/0x194
> 
> Fixes: bed35c6dfa6a ("serdev: add a tty port controller driver")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  drivers/tty/serdev/serdev-ttyport.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
> index d367803e2044..3d2bab91a988 100644
> --- a/drivers/tty/serdev/serdev-ttyport.c
> +++ b/drivers/tty/serdev/serdev-ttyport.c
> @@ -91,6 +91,9 @@ static void ttyport_write_flush(struct serdev_controller *ctrl)
>  	struct serport *serport = serdev_controller_get_drvdata(ctrl);
>  	struct tty_struct *tty = serport->tty;
>  
> +	if (!test_bit(SERPORT_ACTIVE, &serport->flags))
> +		return;

Shouldn't that be a more useful macro/function instead?
	serport_is_active(serport)

Anyway, what prevents this from changing _right_ after you test it and
before you call the next line in this function (same for all invocations
here.)

thanks,

greg k-h
Krzysztof Kozlowski Dec. 21, 2022, 5:37 p.m. UTC | #2 
On 21/12/2022 17:39, Greg Kroah-Hartman wrote:
> On Wed, Dec 21, 2022 at 05:32:48PM +0100, Krzysztof Kozlowski wrote:
>> use-after-free is visible in serdev-ttyport, e.g. during system reboot
>> with Qualcomm Atheros Bluetooth.  The TTY is closed, thus "struct
>> tty_struct" is being released, but the hci_uart_qca driver performs
>> writes and flushes during system shutdown in qca_serdev_shutdown().
>>
>>   Unable to handle kernel paging request at virtual address 0072662f67726fd7
>>   ...
>>   CPU: 6 PID: 1 Comm: systemd-shutdow Tainted: G        W          6.1.0-rt5-00325-g8a5f56bcfcca #8
>>   Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
>>   Call trace:
>>    tty_driver_flush_buffer+0x4/0x30
>>    serdev_device_write_flush+0x24/0x34
>>    qca_serdev_shutdown+0x80/0x130 [hci_uart]
>>    device_shutdown+0x15c/0x260
>>    kernel_restart+0x48/0xac
>>
>> KASAN report:
>>
>>   BUG: KASAN: use-after-free in tty_driver_flush_buffer+0x1c/0x50
>>   Read of size 8 at addr ffff16270c2e0018 by task systemd-shutdow/1
>>
>>   CPU: 7 PID: 1 Comm: systemd-shutdow Not tainted 6.1.0-next-20221220-00014-gb85aaf97fb01-dirty #28
>>   Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
>>   Call trace:
>>    dump_backtrace.part.0+0xdc/0xf0
>>    show_stack+0x18/0x30
>>    dump_stack_lvl+0x68/0x84
>>    print_report+0x188/0x488
>>    kasan_report+0xa4/0xf0
>>    __asan_load8+0x80/0xac
>>    tty_driver_flush_buffer+0x1c/0x50
>>    ttyport_write_flush+0x34/0x44
>>    serdev_device_write_flush+0x48/0x60
>>    qca_serdev_shutdown+0x124/0x274
>>    device_shutdown+0x1e8/0x350
>>    kernel_restart+0x48/0xb0
>>    __do_sys_reboot+0x244/0x2d0
>>    __arm64_sys_reboot+0x54/0x70
>>    invoke_syscall+0x60/0x190
>>    el0_svc_common.constprop.0+0x7c/0x160
>>    do_el0_svc+0x44/0xf0
>>    el0_svc+0x2c/0x6c
>>    el0t_64_sync_handler+0xbc/0x140
>>    el0t_64_sync+0x190/0x194
>>
>> Fixes: bed35c6dfa6a ("serdev: add a tty port controller driver")
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> ---
>>  drivers/tty/serdev/serdev-ttyport.c | 24 ++++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
>> index d367803e2044..3d2bab91a988 100644
>> --- a/drivers/tty/serdev/serdev-ttyport.c
>> +++ b/drivers/tty/serdev/serdev-ttyport.c
>> @@ -91,6 +91,9 @@ static void ttyport_write_flush(struct serdev_controller *ctrl)
>>  	struct serport *serport = serdev_controller_get_drvdata(ctrl);
>>  	struct tty_struct *tty = serport->tty;
>>  
>> +	if (!test_bit(SERPORT_ACTIVE, &serport->flags))
>> +		return;
> 
> Shouldn't that be a more useful macro/function instead?
> 	serport_is_active(serport)

Sure, makes sense.

> 
> Anyway, what prevents this from changing _right_ after you test it and
> before you call the next line in this function (same for all invocations
> here.)

Eh, you're right. I got suggested by such solution in
ttyport_write_buf() assuming it was correct in the first place. Is
holding tty_lock for entire function here reasonable?

Anyway the issue also is in the caller, which should not talk over
closed TTY, which should be fixed in patch 2.

Best regards,
Krzysztof
Greg Kroah-Hartman Dec. 21, 2022, 5:42 p.m. UTC | #3 
On Wed, Dec 21, 2022 at 06:37:59PM +0100, Krzysztof Kozlowski wrote:
> On 21/12/2022 17:39, Greg Kroah-Hartman wrote:
> > On Wed, Dec 21, 2022 at 05:32:48PM +0100, Krzysztof Kozlowski wrote:
> >> use-after-free is visible in serdev-ttyport, e.g. during system reboot
> >> with Qualcomm Atheros Bluetooth.  The TTY is closed, thus "struct
> >> tty_struct" is being released, but the hci_uart_qca driver performs
> >> writes and flushes during system shutdown in qca_serdev_shutdown().
> >>
> >>   Unable to handle kernel paging request at virtual address 0072662f67726fd7
> >>   ...
> >>   CPU: 6 PID: 1 Comm: systemd-shutdow Tainted: G        W          6.1.0-rt5-00325-g8a5f56bcfcca #8
> >>   Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
> >>   Call trace:
> >>    tty_driver_flush_buffer+0x4/0x30
> >>    serdev_device_write_flush+0x24/0x34
> >>    qca_serdev_shutdown+0x80/0x130 [hci_uart]
> >>    device_shutdown+0x15c/0x260
> >>    kernel_restart+0x48/0xac
> >>
> >> KASAN report:
> >>
> >>   BUG: KASAN: use-after-free in tty_driver_flush_buffer+0x1c/0x50
> >>   Read of size 8 at addr ffff16270c2e0018 by task systemd-shutdow/1
> >>
> >>   CPU: 7 PID: 1 Comm: systemd-shutdow Not tainted 6.1.0-next-20221220-00014-gb85aaf97fb01-dirty #28
> >>   Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
> >>   Call trace:
> >>    dump_backtrace.part.0+0xdc/0xf0
> >>    show_stack+0x18/0x30
> >>    dump_stack_lvl+0x68/0x84
> >>    print_report+0x188/0x488
> >>    kasan_report+0xa4/0xf0
> >>    __asan_load8+0x80/0xac
> >>    tty_driver_flush_buffer+0x1c/0x50
> >>    ttyport_write_flush+0x34/0x44
> >>    serdev_device_write_flush+0x48/0x60
> >>    qca_serdev_shutdown+0x124/0x274
> >>    device_shutdown+0x1e8/0x350
> >>    kernel_restart+0x48/0xb0
> >>    __do_sys_reboot+0x244/0x2d0
> >>    __arm64_sys_reboot+0x54/0x70
> >>    invoke_syscall+0x60/0x190
> >>    el0_svc_common.constprop.0+0x7c/0x160
> >>    do_el0_svc+0x44/0xf0
> >>    el0_svc+0x2c/0x6c
> >>    el0t_64_sync_handler+0xbc/0x140
> >>    el0t_64_sync+0x190/0x194
> >>
> >> Fixes: bed35c6dfa6a ("serdev: add a tty port controller driver")
> >> Cc: <stable@vger.kernel.org>
> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> ---
> >>  drivers/tty/serdev/serdev-ttyport.c | 24 ++++++++++++++++++++++++
> >>  1 file changed, 24 insertions(+)
> >>
> >> diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
> >> index d367803e2044..3d2bab91a988 100644
> >> --- a/drivers/tty/serdev/serdev-ttyport.c
> >> +++ b/drivers/tty/serdev/serdev-ttyport.c
> >> @@ -91,6 +91,9 @@ static void ttyport_write_flush(struct serdev_controller *ctrl)
> >>  	struct serport *serport = serdev_controller_get_drvdata(ctrl);
> >>  	struct tty_struct *tty = serport->tty;
> >>  
> >> +	if (!test_bit(SERPORT_ACTIVE, &serport->flags))
> >> +		return;
> > 
> > Shouldn't that be a more useful macro/function instead?
> > 	serport_is_active(serport)
> 
> Sure, makes sense.
> 
> > 
> > Anyway, what prevents this from changing _right_ after you test it and
> > before you call the next line in this function (same for all invocations
> > here.)
> 
> Eh, you're right. I got suggested by such solution in
> ttyport_write_buf() assuming it was correct in the first place. Is
> holding tty_lock for entire function here reasonable?

For every function you added this check to?  I don't know, would need to
audit them all before being able to answer that :(

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index d367803e2044..3d2bab91a988 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -91,6 +91,9 @@  static void ttyport_write_flush(struct serdev_controller *ctrl)
 	struct serport *serport = serdev_controller_get_drvdata(ctrl);
 	struct tty_struct *tty = serport->tty;
 
+	if (!test_bit(SERPORT_ACTIVE, &serport->flags))
+		return;
+
 	tty_driver_flush_buffer(tty);
 }
 
@@ -99,6 +102,9 @@  static int ttyport_write_room(struct serdev_controller *ctrl)
 	struct serport *serport = serdev_controller_get_drvdata(ctrl);
 	struct tty_struct *tty = serport->tty;
 
+	if (!test_bit(SERPORT_ACTIVE, &serport->flags))
+		return 0;
+
 	return tty_write_room(tty);
 }
 
@@ -172,6 +178,9 @@  static unsigned int ttyport_set_baudrate(struct serdev_controller *ctrl, unsigne
 	struct tty_struct *tty = serport->tty;
 	struct ktermios ktermios = tty->termios;
 
+	if (!test_bit(SERPORT_ACTIVE, &serport->flags))
+		return -ENXIO;
+
 	ktermios.c_cflag &= ~CBAUD;
 	tty_termios_encode_baud_rate(&ktermios, speed, speed);
 
@@ -186,6 +195,9 @@  static void ttyport_set_flow_control(struct serdev_controller *ctrl, bool enable
 	struct tty_struct *tty = serport->tty;
 	struct ktermios ktermios = tty->termios;
 
+	if (!test_bit(SERPORT_ACTIVE, &serport->flags))
+		return;
+
 	if (enable)
 		ktermios.c_cflag |= CRTSCTS;
 	else
@@ -201,6 +213,9 @@  static int ttyport_set_parity(struct serdev_controller *ctrl,
 	struct tty_struct *tty = serport->tty;
 	struct ktermios ktermios = tty->termios;
 
+	if (!test_bit(SERPORT_ACTIVE, &serport->flags))
+		return -ENXIO;
+
 	ktermios.c_cflag &= ~(PARENB | PARODD | CMSPAR);
 	if (parity != SERDEV_PARITY_NONE) {
 		ktermios.c_cflag |= PARENB;
@@ -222,6 +237,9 @@  static void ttyport_wait_until_sent(struct serdev_controller *ctrl, long timeout
 	struct serport *serport = serdev_controller_get_drvdata(ctrl);
 	struct tty_struct *tty = serport->tty;
 
+	if (!test_bit(SERPORT_ACTIVE, &serport->flags))
+		return;
+
 	tty_wait_until_sent(tty, timeout);
 }
 
@@ -230,6 +248,9 @@  static int ttyport_get_tiocm(struct serdev_controller *ctrl)
 	struct serport *serport = serdev_controller_get_drvdata(ctrl);
 	struct tty_struct *tty = serport->tty;
 
+	if (!test_bit(SERPORT_ACTIVE, &serport->flags))
+		return -ENXIO;
+
 	if (!tty->ops->tiocmget)
 		return -ENOTSUPP;
 
@@ -241,6 +262,9 @@  static int ttyport_set_tiocm(struct serdev_controller *ctrl, unsigned int set, u
 	struct serport *serport = serdev_controller_get_drvdata(ctrl);
 	struct tty_struct *tty = serport->tty;
 
+	if (!test_bit(SERPORT_ACTIVE, &serport->flags))
+		return -ENXIO;
+
 	if (!tty->ops->tiocmset)
 		return -ENOTSUPP;