Re: [PATCH] tty: pty: preserve open slaves after rejected locked open

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] tty: NULL pointer dereference in pty_open and pty_close
- [Medium] tty: logic error tearing down active kernel PTY clients
--

commit f8d6871959bd3ab3965cd6aaf18087ecb7ef411f
Author: Akira Patafio <kokokoala4211@gmail.com>

tty: pty: preserve open slaves after rejected locked open

This commit tracks which PTY slave files were successfully opened to avoid
spurious IO errors or premature master closures during locked slave opens.
A selftest is also added to verify this behavior.

> diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
> index cc7f7091ed9a2..3cc149445993e 100644
> --- a/drivers/tty/pty.c
> +++ b/drivers/tty/pty.c
> @@ -46,12 +46,29 @@ static DEFINE_MUTEX(devpts_mutex);
>  
>  static void pty_close(struct tty_struct *tty, struct file *filp)
>  {
> +	struct tty_file_private *priv = filp->private_data;

[Severity: High]
Can this unconditionally dereference a NULL pointer? In-kernel TTY clients
such as speakup or serdev can manually invoke the driver's close callback
using tty->ops->close(tty, NULL). 

If an in-kernel client opens a PTY, calling pty_close() with filp == NULL
will cause a kernel panic here.

> +	struct tty_file_private *other;
> +	bool another_open = false;
> +
>  	if (tty->driver->subtype == PTY_TYPE_MASTER)
>  		WARN_ON(tty->count > 1);
>  	else {
> -		if (tty_io_error(tty))
> +		/* tty_release() also calls close after an unsuccessful open. */
> +		if (!priv->pty_opened)
> +			return;
> +		/* tty->count includes opens that have not succeeded yet. */
> +		spin_lock(&tty->files_lock);
> +		priv->pty_opened = false;
> +		list_for_each_entry(other, &tty->tty_files, list) {
> +			if (other->pty_opened) {
> +				another_open = true;
> +				break;
> +			}
> +		}

[Severity: Medium]
Will this loop correctly account for in-kernel PTY clients? 

If a client like speakup has the PTY open, its open is not backed by a
struct file and is absent from tty->tty_files.

> +		spin_unlock(&tty->files_lock);
> +		if (another_open)
>  			return;

If an in-kernel client is the only remaining open, another_open will
evaluate to false here. Could this prematurely tear down the master
connection despite the kernel client still actively using the slave?

[ ... ]

> @@ -223,14 +240,23 @@ static int pty_open(struct tty_struct *tty, struct file *filp)
>  
>  	if (test_bit(TTY_OTHER_CLOSED, &tty->flags))
>  		goto out;
> +	/* A rejected open must not disrupt already-open slave descriptors. */
>  	if (test_bit(TTY_PTY_LOCK, &tty->link->flags))
> -		goto out;
> +		return -EIO;
>  	if (tty->driver->subtype == PTY_TYPE_SLAVE && tty->link->count != 1)
>  		goto out;
>  
>  	clear_bit(TTY_IO_ERROR, &tty->flags);
>  	clear_bit(TTY_OTHER_CLOSED, &tty->link->flags);
>  	set_bit(TTY_THROTTLED, &tty->flags);
> +	/* Record a successful open before another slave can close. */
> +	if (tty->driver->subtype == PTY_TYPE_SLAVE) {
> +		struct tty_file_private *priv = filp->private_data;

[Severity: High]
Can filp be NULL here? 

Similar to pty_close(), in-kernel clients can call tty->ops->open(tty, NULL).
This could cause a kernel panic due to a NULL pointer dereference.

> +
> +		spin_lock(&tty->files_lock);
> +		priv->pty_opened = true;
> +		spin_unlock(&tty->files_lock);
> +	}
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001165552.2439-1-kokokoala4211@gmail.com?part=1