From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vasileios Almpanis Date: Tue, 4 Aug 2026 13:15:00 +0200 Subject: Re: [Devel] [PATCH vz10 v2] ve/vtty: fix use-after-free on concurrent tty close and reopen In-Reply-To: <20260630160639.3043321-1-eva.kurchatova@virtuozzo.com> References: <20260630160639.3043321-1-eva.kurchatova@virtuozzo.com> Message-ID: <975d2435-67bb-418a-af22-fcd114e776ce@virtuozzo.com> List-Id: 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 > --- > 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.