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 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
> set to NULL, leading to a use-after-free scenario in ttyport_close().
> 
> To fix this, initialize serport->tty to NULL upon failure and check its
> value before reading.
> 
> Call trace1:
> release_tty
> tty_init_dev
> ttyport_open
> serdev_device_open
> qca_setup[hci_uart]
> hci_uart_setup[hci_uart]
> hci_dev_open_sync[bluetooth]
> hci_dev_do_open[bluetooth]
> hci_dev_open[bluetooth]
> hci_sock_bind[bluetooth]
> 
> Call trace2:
> refcount_warn_saturate
> tty_lock
> ttyport_close
> serdev_device_close
> hci_uart_close[hci_uart]
> hci_dev_open_sync[bluetooth]
> hci_dev_do_open[bluetooth]
> hci_dev_open[bluetooth]
> hci_sock_bind[bluetooth]
> 
> Co-developed-by: Panicker Harish <quic_pharish@quicinc.com>
> Signed-off-by: Panicker Harish <quic_pharish@quicinc.com>
> Signed-off-by: Xin Chen <quic_cxin@quicinc.com>
> ---
>  drivers/tty/serdev/serdev-ttyport.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
> index 3d7ae7fa5018..287908f2009b 100644
> --- 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?

And why print out that message, what can userspace do with it?

> 
>  	tty_driver_flush_buffer(tty);
>  }
> @@ -108,8 +112,10 @@ static int ttyport_open(struct serdev_controller *ctrl)
>  	int ret;
> 
>  	tty = tty_init_dev(serport->tty_drv, serport->tty_idx);
> -	if (IS_ERR(tty))
> +	if (IS_ERR(tty)) {
> +		serport->tty = NULL;
>  		return PTR_ERR(tty);
> +	}
>  	serport->tty = tty;
> 
>  	if (!tty->ops->open || !tty->ops->close) {
> @@ -156,6 +162,11 @@ static void ttyport_close(struct serdev_controller *ctrl)
> 
>  	clear_bit(SERPORT_ACTIVE, &serport->flags);
> 
> +	if (!tty) {
> +		dev_err(&ctrl->dev, "tty is null\n");
> +		return;
> +	}

Again, what prevents it from changing right after you just checked it?

thanks,

greg k-h