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

[Pavel Tikhomirov]

On 6/28/26 23:30, 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 detecting a dead pair in vtty_open_master(): if both vttym
> and vttys counts are zero the pair is dying, so clear the vtty map entry
> and fall through to create a fresh pair.  The concurrent release_tty
> will find driver_data == NULL in vtty_shutdown and harmlessly skip the
> already-cleared map.
> 
> Fixes: dfe187803cf9 ("ve/vtty: Don't close unread master peer if slave is nonzero")
> Signed-off-by: Eva Kurchatova <eva.kurchatova at virtuozzo.com>
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-136511
> Feature: fix vtty panic
> ---
>  drivers/tty/pty.c    | 10 ++++++++++
>  drivers/tty/tty_io.c | 15 +++++++++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
> index f8610c77817a..46cb744e85b1 100644
> --- a/drivers/tty/pty.c
> +++ b/drivers/tty/pty.c
> @@ -1062,6 +1062,16 @@ int vtty_open_master(envid_t veid, int idx)
>  		goto err_install;
>  	}
>  
> +	/*
> +	 * If both master and slave counts are zero, the pair is dying;
> +	 * A concurrent tty_release_struct is about to free it.
> +	 * Clear the map so release_tty's vtty_shutdown sees driver_data == NULL
> +	 * and skips, then fall through to create a fresh pair.
> +	 */
> +	if (tty && tty->count == 0 && tty->link->count == 0) {
> +		vtty_map_clear(tty->link);
> +		tty = NULL;
> +	}

Would returning error (-EBUSY) here be a simpler fix?

Making alternative point where one does vtty_map_clear() is a tricky change
one should make sure that they don't clear new map twice accidentally.
So this can be tricky to rebase in future.

>  	if (!tty) {
>  		tty = tty_init_dev(vttys_driver, idx);
>  		if (IS_ERR(tty)) {
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 797a9d0ddd1e..e9ba53cdfe7f 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -1864,6 +1864,21 @@ 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.
> +	 */
> +	if (final && !o_tty && tty->link && !vtty_is_master(tty) && tty->link->count > 0)
> +		final = 0;

Let's consider this order of events:

vtty_open_master - vtty_open_master
close - tty_release

Initial state master=0, slave=1 (no console attached, one getty fd on the slave).
Precisely the commit's "attach vs getty respawn" case. getty closes its slave fd while vzctl opens the master:

vtty_open_master  @pty.c:1071      read slave==1, master==0     → "alive", not dying
tty_release       @tty_io.c:1833   slave-- : 1 → 0
tty_release       @tty_io.c:1878   read master==0               → Hunk-1 does NOT fire → final = TRUE
vtty_open_master  @pty.c:1090      EBUSY check: master==0       → passes
vtty_open_master  @pty.c:1102      master++ : 0 → 1
vtty_open_master  @pty.c:1103      slave++  : 0 → 1   + fd_install   (hands out fd)
vtty_open_master  @pty.c:1107      mutex_unlock
tty_release       @tty_io.c:1894   tty_release_struct → tty_mutex → release_tty → frees the pair

Looks like use after free. Functions above use completely different locks so the above event order seems plausible.

p.s. Would not it be easier to add extra count from vtty master to the slave tty, so that slave can only check it's own counter? Proper locking surely still needed.

> +#endif
> +
>  	tty_unlock_slave(o_tty);
>  	tty_unlock(tty);
>  

-- 
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.