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