Message-ID: <c531923d-ddb7-4512-b0b3-ac0998167b75@quicinc.com>
Date: Thu, 5 Jun 2025 16:13:34 +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/31/2025 3:20 PM, Greg Kroah-Hartman wrote:
> On Fri, May 30, 2025 at 04:34:49PM +0800, Xin Chen wrote:
>>
>>
>> On 5/29/2025 5:41 PM, Greg Kroah-Hartman wrote:
>>> On Thu, May 29, 2025 at 11:07:25AM +0200, 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.
>>>
>>> Wait, what?  Why is qca_read_soc_version() failing?  
>>
>> Actually I have not root cause why qca_read_soc_version() fails of
>> __hci_cmd_sync_ev(). It may be relative to FW issue.
> 
> Please start there, don't you want to know why things are failing?
> 
>>> Why are you retrying multiple times until either you run out of attempts?  
>>
>> This is a retry mechanism. I find the reason in the change commit message
>> "Currently driver only retries to download FW if FW downloading
>> is failed. Sometimes observed command timeout for version request
>> command, if this happen on some platforms during boot time, then
>> a reboot is needed to turn ON BT. Instead to avoid a reboot, now
>> extended retry logic for version request command too."
>>
>>> Why are you closing the port and then opening it again right away? 
>>
>> This is a retry mechanism as above said. Do you mean there should be a gap
>> between close and open? The change owner maybe don't think about this issue.
> 
> Why are you calling close/open at all?  Why does that do anything?
> Doesn't that feel wrong?
> 
> Again, please root-cause the failure, don't try to paper over it by
> loads of looping and odd open/close attempts that are not understood and
> seem to actually cause other types of crashes :)
> 
>>> What close/open pair seems totally unnecessary, why do that at all?
>>>
>>> If I read that function qca_setup(), it can NEVER detect if a failure
>>> really happened (i.e. if it does run out of retries, you just plow on
>>> and keep going and keep on registering things and THEN return an error
>>> for some reason.
>>>
>>> In other words, the error handling in qca_setup() is very suspect, why
>>> not fix all of that up first?
>>>
>>
>> qca_read_soc_version() in qca_setup() can detect whether the hci_dev is set up
>> successfully. If if fails then a failure happens.
>> You mean I should fix why qca_read_soc_version() fails?
> 
> Yes, why wouldn't you want to do that?
> 

Yeah you are right. I will try to root cause the qca read version issue and fix
it first. Thanks very much!

Xin

Powered by blists - more mailing lists