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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2025043022-rumbling-guy-26fb@gregkh>
Date: Wed, 30 Apr 2025 13:40:17 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Xin Chen <quic_cxin@...cinc.com>
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 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@...cinc.com>
> Signed-off-by: Panicker Harish <quic_pharish@...cinc.com>
> Signed-off-by: Xin Chen <quic_cxin@...cinc.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