Re: [PATCH v2 2/2] tty: fix break race

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The lockless checks of `tty_hung_up_p(file)` introduce a Time-of-Check to Time-of-Use (TOCTOU) race against `__tty_hangup`, failing to fully resolve the Use-After-Free/NULL pointer dereference.
--

commit 8d56321f205bd51f6c216651862e6a270f2637ea
Author: Adriano Cordova <adrianox@gmail.com>

tty: fix break race

This commit attempts to fix a race condition where a timed break can call
break_ctl() on a tty whose driver data is gone, causing a use-after-free
or NULL-pointer dereference. It adds checks for the tty hung-up state
before each break_ctl() call.

Link: https://syzkaller.appspot.com/bug?extid=473d7477c523b41d4046

> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 48569035da56e..272f2839acdba 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
[ ... ]
> @@ -2438,13 +2439,16 @@ static int tiocgetd(struct tty_struct *tty, int __user *p)
>   * Locking:
>   *	@tty->atomic_write_lock serializes
>   */
> -static int send_break(struct tty_struct *tty, unsigned int duration)
> +static int send_break(struct file *file, struct tty_struct *tty, unsigned int duration)
>  {
>  	int retval;
>  
>  	if (tty->ops->break_ctl == NULL)
>  		return 0;
>  
> +	if (tty_hung_up_p(file))
> +		return -EIO;
> +

[Severity: High]
This isn't a bug introduced by this patch, but does this lockless check fully
resolve the Time-of-Check to Time-of-Use (TOCTOU) race? If a concurrent thread
executes __tty_hangup() immediately after tty_hung_up_p() evaluates to false,
could __tty_hangup() tear down the driver data before tty->ops->break_ctl()
is executed on the hardware break path, leading to a use-after-free?

>  	if (tty->driver->flags & TTY_DRIVER_HARDWARE_BREAK)
>  		return tty->ops->break_ctl(tty, duration);
>  
> @@ -2455,7 +2459,10 @@ static int send_break(struct tty_struct *tty, unsigned int duration)
>  	retval = tty->ops->break_ctl(tty, -1);
>  	if (!retval) {
>  		msleep_interruptible(duration);
> -		retval = tty->ops->break_ctl(tty, 0);
> +		if (tty_hung_up_p(file))
> +			retval = -EIO;
> +		else
> +			retval = tty->ops->break_ctl(tty, 0);

[Severity: High]
This is a pre-existing issue, but does checking tty_hung_up_p() here under
atomic_write_lock provide mutual exclusion against __tty_hangup()? Could a
concurrent hangup occur right after this check, causing the subsequent call
to tty->ops->break_ctl() to dereference freed driver data?

>  	} else if (retval == -EOPNOTSUPP) {
>  		/* some drivers can tell only dynamically */
>  		retval = 0;
[ ... ]
> @@ -2714,10 +2721,14 @@ long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  	 * Break handling
>  	 */
>  	case TIOCSBRK:	/* Turn break on, unconditionally */
> +		if (tty_hung_up_p(file))
> +			return -EIO;
>  		if (tty->ops->break_ctl)
>  			return tty->ops->break_ctl(tty, -1);

[Severity: High]
This isn't a bug introduced by this patch, but do these unconditional break
paths suffer from the same TOCTOU race? Since they do not hold tty_lock,
could a concurrent __tty_hangup() free the driver data between the
tty_hung_up_p() check and the break_ctl() call?

>  		return 0;
>  	case TIOCCBRK:	/* Turn break off, unconditionally */
> +		if (tty_hung_up_p(file))
> +			return -EIO;
>  		if (tty->ops->break_ctl)
>  			return tty->ops->break_ctl(tty, 0);

[Severity: High]
This is a pre-existing issue, but similar to TIOCSBRK, is this unprotected
check before unconditional break off vulnerable to a concurrent hangup?

>  		return 0;

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