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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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