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

[Pavel Tikhomirov]

On 6/29/26 16:35, Pavel Tikhomirov wrote:
> 
> 
> 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;

By the way, the check seems to broad, maybe restrict it to vtty driver?
E.g.: "&& tty->driver == vttys_driver"

> 
> 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.