Re: [PATCH] tty: abort break signalling on hangup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing hangup check before initiating the break state leaves the tty in a permanent break state if the port is already hung up.
--

commit fb29de6b768e6b046a458b30129e8f3ab80e0b86
Author: Johan Hovold <johan@kernel.org>

tty: abort break signalling on hangup

This commit aborts TCSBRK and TCSBRKP break signalling on hangup by waking
up pending break ioctls and returning early. This prevents calling back into
the driver after a user-controlled timeout for a disconnected device.

> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 48569035da56e..1c30faae9ec13 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
[ ... ]
> @@ -2453,13 +2457,26 @@ static int send_break(struct tty_struct *tty, unsigned int duration)
>  		return -EINTR;
>  
>  	retval = tty->ops->break_ctl(tty, -1);
> -	if (!retval) {
> -		msleep_interruptible(duration);
> -		retval = tty->ops->break_ctl(tty, 0);
> -	} else if (retval == -EOPNOTSUPP) {
> -		/* some drivers can tell only dynamically */
> -		retval = 0;
> +	if (retval) {
> +		if (retval == -EOPNOTSUPP) {
> +			/* some drivers can tell only dynamically */
> +			retval = 0;
> +		}
> +		goto out_unlock;
> +	}
> +
> +	timeout = msecs_to_jiffies(duration);
> +	timeout = wait_event_interruptible_timeout(tty->break_wait,
> +						   tty_hung_up_p(file),
> +						   timeout);
> +	/* return early on hangup only */
> +	if (timeout > 0) {
> +		retval = -EIO;
> +		goto out_unlock;
>  	}

[Severity: High]
Does this leave the tty in a permanent break state if the port was already
hung up before calling send_break()?

If a thread is sleeping in tty_wait_until_sent() during an ioctl(TCSBRK)
and a hangup occurs, __tty_hangup() marks the file as hung up and wakes
the thread. The thread then proceeds to send_break(), unconditionally
asserting the break state via:

    retval = tty->ops->break_ctl(tty, -1);

Since the port is already hung up, the wait_event_interruptible_timeout()
condition tty_hung_up_p(file) evaluates to true immediately, returning the
initial timeout which is > 0.

Taking this early return path skips the cleanup call to
tty->ops->break_ctl(tty, 0), which can force the hardware TX line into a
continuous break state and corrupt outgoing data for any subsequent user.

> +
> +	retval = tty->ops->break_ctl(tty, 0);
> +out_unlock:
>  	tty_write_unlock(tty);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903163736.1499280-1-johan@kernel.org?part=1