Re: [PATCH v1] tty: serdev: serdev-ttyport: Fix use-after-free in ttyport_close() due to uninitialized serport->tty

[Xin Chen <quic_cxin@quicinc.com>]

On 5/29/2025 5:07 PM, Greg Kroah-Hartman wrote:
> On Fri, May 23, 2025 at 10:52:27AM +0800, Xin Chen wrote:
>>
>>
>> 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?
> 
> I am not disputing the fact that there is a bug here, I'm just saying
> that you can't test for a value and then act on it without a lock
> protecting that action because the value can be changed right after you
> test for it.
> 
> You might not see this in your testing, as you have narrowed the window
> that the value can change, but you have not solved the issue properly,
> right?
> 
> thanks,
> 
> greg k-h

From my analysis, I think there is only one thread operating the tty of
Bluetooth. So the case of tty changed after check will not happen.

Thanks,
Xin