linux-kernel - Re: [PATCH v1] tty: serdev: serdev-ttyport: Fix use-after-free in ttyport_close() due to uninitialized serport->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: <8e171057-b3c3-4808-b49e-f04ffd310b31@quicinc.com>
Date: Fri, 23 May 2025 10:52:27 +0800
From: Xin Chen <quic_cxin@...cinc.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC: Rob Herring <robh@...nel.org>, Jiri Slaby <jirislaby@...nel.org>,
        <linux-serial@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <liulzhao@....qualcomm.com>, <quic_chejiang@...cinc.com>,
        <zaiyongc@....qualcomm.com>, <quic_zijuhu@...cinc.com>,
        <quic_mohamull@...cinc.com>,
        Panicker Harish <quic_pharish@...cinc.com>
Subject: Re: [PATCH v1] tty: serdev: serdev-ttyport: Fix use-after-free in
 ttyport_close() due to uninitialized serport->tty



On 5/14/2025 5:14 PM, Xin Chen wrote:
> 
> 
> On 5/8/2025 5:41 PM, Greg Kroah-Hartman wrote:
>> On Thu, May 08, 2025 at 05:29:18PM +0800, Xin Chen wrote:
>>>
>>> On 4/30/2025 7:40 PM, Greg Kroah-Hartman wrote:
>>>> On Wed, Apr 30, 2025 at 07:16:17PM +0800, Xin Chen wrote:
>>>>> When ttyport_open() fails to initialize a tty device, serport->tty is not
>>>>> --- a/drivers/tty/serdev/serdev-ttyport.c
>>>>> +++ b/drivers/tty/serdev/serdev-ttyport.c
>>>>> @@ -88,6 +88,10 @@ static void ttyport_write_flush(struct serdev_controller *ctrl)
>>>>>  {
>>>>>  	struct serport *serport = serdev_controller_get_drvdata(ctrl);
>>>>>  	struct tty_struct *tty = serport->tty;
>>>>> +	if (!tty) {
>>>>> +		dev_err(&ctrl->dev, "tty is null\n");
>>>>> +		return;
>>>>> +	}
>>>>
>>>> What prevents tty from going NULL right after you just checked this?
>>>
>>> First sorry for reply so late for I have a long statutory holidays.
>>> Maybe I don't get your point. From my side, there is nothing to prevent it.
>>> Check here is to avoid code go on if tty is NULL.
>>
>> Yes, but the problem is, serport->tty could change to be NULL right
>> after you check it, so you have not removed the real race that can
>> happen here.  There is no lock, so by adding this check you are only
>> reducing the risk of the problem happening, not actually fixing the
>> issue so that it will never happen.
>>
>> Please fix it so that this can never happen.
>>
> 
> Actually I have never thought the race condition issue since the crash I met is
> not caused by race condition. It's caused due to Bluetooth driver call
> ttyport_close() after ttyport_open() failed. This two action happen one after
> another in one thread and it seems impossible to have race condition. And with
> my fix the crash doesn't happen again in several test of same case.
> 
> Let me introduce the complete process for you:
>   1) hci_dev_open_sync()->
> hci_dev_init_sync()->hci_dev_setup_sync()->hdev->setup()(hci_uart_setup)->qca_setup(),
> here in qca_setup(), qca_read_soc_version() fails and goto out, then calls
> serdev_device_close() to close tty normally. And then call serdev_device_open()
> to retry.
>   2) serdev_device_open() fails due to tty_init_dev() fails, then tty gets
> released, which means this time the tty has been freed succesfully.
>   3) Return back to upper func  hci_dev_open_sync(),
> hdev->close()(hci_uart_close) is called. And hci_uart_close calls
> hci_uart_flush() and serdev_device_close(). serdev_device_close() tries to close
> tty again, it's calltrace is serdev_device_close()->ttyport_close()->tty_lock(),
> tty_unlock(), tty_release_struct(). The four funcs hci_uart_flush(), tty_lock(),
> tty_unlock(), tty_release_struct() read tty pointer's value, which is invalid
> and causes crash.
> 

Hi Greg, could you please take some time to review my reply?
Thanks very much!