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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e2925111-abcf-26fb-59e0-9bd4fb3f7b8e@linaro.org>
Date:   Wed, 21 Dec 2022 18:37:59 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Marcel Holtmann <marcel@...tmann.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        Luiz Augusto von Dentz <luiz.dentz@...il.com>,
        Rob Herring <robh@...nel.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        Zijun Hu <zijuhu@...eaurora.org>,
        linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-serial@...r.kernel.org,
        Sai Teja Aluvala <quic_saluvala@...cinc.com>,
        Panicker Harish <quic_pharish@...cinc.com>,
        Johan Hovold <johan@...nel.org>, stable@...r.kernel.org
Subject: Re: [PATCH 1/2] serdev: ttyport: fix use-after-free on closed TTY

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@...r.kernel.org>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.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