Re: [Devel] [PATCH vz10 v2] ve/vtty: fix use-after-free on concurrent tty close and reopen

[Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>]

On 6/30/26 6:06 PM, Eva Kurchatova wrote:
> Two races in the vtty subsystem lead to use-after-free of tty_struct
> objects when vzctl console attach/detach cycles run concurrently with
> container-side tty open/close (e.g. SAK-triggered getty respawn):
>
> Race 1: double-final between concurrent vtty master and slave close.
>
> In tty_release(), the vttys (slave) side has o_tty == NULL because its
> driver subtype is PTY_TYPE_SLAVE, so the final-close check depends
> solely on !slave->count.  When master and slave close concurrently,
> both sides can independently determine final == true: the slave sees
> slave->count == 0, the master sees both counts == 0 (after the slave
> count underflows to -1 and gets reset).  Both then call
> tty_release_struct -> release_tty, and the second caller hits a
> use-after-free.
>
> Fix this by adding a vtty-specific check after computing final: for
> vttys closes, also verify that the peer vttym count is not positive.
> This is safe without holding the vttym lock because the vttym side of
> tty_release holds tty_lock_slave(vttys) while decrementing vttym->count,
> so while we hold tty_lock(vttys) the vttym cannot have decremented yet.
>
> Race 2: vtty_open_master reopens a dying tty pair.
>
> After tty_release() sets final == true and releases tty_lock, there is
> a window before release_tty() runs under tty_mutex.  During this window
> vtty_open_master() can find the old vttym in the vtty map with count == 0,
> pass the ">= 1" check (which was designed as a "one vttym at a time"
> guard, not a liveness check), re-increment the counts, and hand out a
> file descriptor pointing to a tty_struct that is about to be freed.
>
> Fix this by taking tty_lock(vttys) in vtty_open_master() to serialize
> with a concurrent tty_release() on the vttys side, so we read the slave
> count after any in-flight close has decremented it.  If vttys->count
> has reached zero the pair is dying, so return -EBUSY.  Incrementing
> vttys->count under the same tty_lock prevents a concurrent tty_release()
> from seeing zero and entering the final-close path.
>
> The lock nesting pattern of tty_mutex -> tty_lock is preserved, which
> prevents any kind of A->B, B->A circular locking dependency.
>
> Fixes: dfe187803cf9 ("ve/vtty: Don't close unread master peer if slave is nonzero")
> Signed-off-by: Eva Kurchatova <eva.kurchatova@virtuozzo.com>
> ---
>   drivers/tty/pty.c    | 31 ++++++++++++++++++++++++++-----
>   drivers/tty/tty_io.c | 17 +++++++++++++++++
>   include/linux/ve.h   |  1 +
>   3 files changed, 44 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
> index f8610c77817a..2336204265ab 100644
> --- a/drivers/tty/pty.c
> +++ b/drivers/tty/pty.c
> @@ -628,6 +628,11 @@ bool vtty_is_master(struct tty_struct *tty)
>   	return tty->driver == vttym_driver;
>   }
>   
> +bool vtty_is_slave(struct tty_struct *tty)
> +{
> +	return tty->driver == vttys_driver;
> +}
> +
>   typedef struct {
>   	envid_t			veid;
>   	struct tty_struct	*vttys[MAX_NR_VTTY_CONSOLES];
> @@ -1082,15 +1087,31 @@ int vtty_open_master(envid_t veid, int idx)
>   		goto err_install;
>   	}
>   
> -	vtty_drop_context();
> -
>   	/*
> -	 * We're the master peer so increment
> -	 * slave counter as well.
> +	 * Serialize with a concurrent tty_release() on the vttys side.
> +	 * tty_lock guarantees we see the slave count after any in-flight
> +	 * close has decremented it.  If the slave has reached zero the
> +	 * pair is dying; return -EBUSY and let the caller retry; the
> +	 * old pair will be freed once we drop tty_mutex below, and the
> +	 * next attempt will create a fresh one.
> +	 *
> +	 * Incrementing slave->count under the same tty_lock prevents a
> +	 * concurrent tty_release() from seeing zero and starting the
> +	 * final-close path that would cause use-after-free.
>   	 */
> +	tty_lock(tty->link);
This check breaks attach to a console with no container-size opener. For 
fresh tty above you, decremented count to 0 and because of that opening 
with break with EBUSY.
> +	if (tty->link->count == 0) {
> +		tty_unlock(tty->link);
> +		ret = -EBUSY;
> +		goto err_install;
> +	}
> +	tty->link->count++;
> +	tty_unlock(tty->link);
> +
> +	vtty_drop_context();
> +
>   	tty_add_file(tty, file);
>   	tty->count++;
> -	tty->link->count++;
>   	fd_install(fd, file);
>   	vtty_open(tty, file);
>   
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 797a9d0ddd1e..507da79a24fa 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -1864,6 +1864,23 @@ int tty_release(struct inode *inode, struct file *filp)
>   	/* check whether both sides are closing ... */
>   	final = !tty->count && !(o_tty && o_tty->count);
>   
> +#ifdef CONFIG_VE
> +	/*
> +	 * vtty: prevent double-final between concurrent master and slave close.
> +	 *
> +	 * For vtty slaves o_tty is NULL (PTY_TYPE_SLAVE), so the standard final
> +	 * check only looks at slave->count.
> +	 *
> +	 * When the master is closing concurrently, it holds tty_lock_slave(slave)
> +	 * while decrementing master->count, so while we hold tty_lock(slave),
> +	 * the master cannot have decremented yet - master->count is still > 0.
> +	 * If master->count is already 0 here, the master already returned
> +	 * from tty_release with final = false, therefore we are closing it.
> +	 */
Can this really be reached now and be true? Since the ref increment and 
decrement happens under the same slave lock, can there be a case that 
slave->count == 0 and master->count > 0 were final was true? After 
adding locking in vtty_open_master this looks kinda impossible to me.

1) Slave closes first, master second:
 ? ?slave:? S 2->1, final = 0, check not reached
 ? ?master: S 1->0, M 1->0, final = 1, master frees the pair

2) Master closes first, slave second:
 ? ?master: S 2->1, M 1->0, final = 0
 ? ?slave:? S 1->0, final = 1, check reached: link->count == 0,
 ? ? ? ? ? ?condition false, slave frees the pair

3) Concurrent close, master's block takes tty_lock(slave) first:
 ? ?same as 2.

4) Concurrent close, slave's block takes the lock first:
 ? ?same as 1.

5) Several slave fds (M=1, S=N+1):
 ? ?slave closes can only bring S down to 1 while the master ref
 ? ?is present; S reaches 0 either inside the master's block
 ? ?(master frees) or in a slave's block after the master already
 ? ?closed (M == 0, condition false). Fallsback to cases 1-4.

6) No master attached (M=0, S=nfds):
 ? ?last slave close: S 1->0, final = 1, link->count == 0,
 ? ?condition false, slave frees the pair.

7) Master attached, no slave fd ever opened (M=1, S=1):
 ? ?detach: S 1->0, M 1->0, final = 1, master frees the pair.
 ? ?No slave release ever runs, check never executes.

8) Slave close racing vtty_open_master():
 ? ?attach order is: lock slave, check S != 0, S++, unlock, then
 ? ?M++. If the attach's locked section runs first, the closing
 ? ?slave sees S 2->1 and final = 0. If the close's block runs
 ? ?first, the attach cannot have done M++ yet (it comes strictly
 ? ?after its locked section, which is still waiting on the lock
 ? ?the closer holds), so the check reads M == 0, the slave frees
 ? ?the pair, and the attach then sees S == 0 and takes the
 ? ?dying-pair exit.

FYI: I asked AI to format these cases so they are more readable.


> +	if (final && !o_tty && tty->link && vtty_is_slave(tty) && tty->link->count > 0)
> +		final = 0;
> +#endif
> +
>   	tty_unlock_slave(o_tty);
>   	tty_unlock(tty);
>   
> diff --git a/include/linux/ve.h b/include/linux/ve.h
> index b037f60225bb..bf5d9acb964c 100644
> --- a/include/linux/ve.h
> +++ b/include/linux/ve.h
> @@ -236,6 +236,7 @@ extern int vtty_open_master(envid_t veid, int idx);
>   extern void vtty_release(struct tty_struct *tty, struct tty_struct *o_tty,
>   			int *tty_closing, int *o_tty_closing);
>   extern bool vtty_is_master(struct tty_struct *tty);
> +extern bool vtty_is_slave(struct tty_struct *tty);
>   extern void vtty_alloc_tty_struct(const struct tty_driver *driver,
>   				  struct tty_struct *o_tty);
>   #endif /* CONFIG_TTY */

-- 
Best regards, Vasileios Almpanis
Software Developer, Virtuozzo.