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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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?

thanks,

greg k-h