From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vasileios Almpanis Date: Mon, 10 Aug 2026 10:26:46 +0200 Subject: Re: [Devel] [PATCH vz10 v3] ve/vtty: fix use-after-free on concurrent tty close and reopen In-Reply-To: <20260810014009.81168-1-eva.kurchatova@virtuozzo.com> References: <20260810014009.81168-1-eva.kurchatova@virtuozzo.com> Message-ID: <442e2d40-a84c-4701-a668-837a521361b3@virtuozzo.com> List-Id: LGTM Reviewed-by: Vasileios Almpanis On 8/10/26 3:39 AM, Eva Kurchatova wrote: > A race in the vtty subsystem leads 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): > 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. > > vtty_open_master() moreover increments both counts with only tty_mutex > held, while tty_release() decrements them under tty_lock of the vttys > peer, so the two paths do not exclude each other at all. The resulting > data race underflows the counts, and lets both peers assume final == true: > the slave sees slave->count == 0, the master sees both counts == 0 (after > the slave count underflowed and got reset). > Both then call tty_release_struct -> release_tty, and the second caller > hits a use-after-free. > > 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. > > A pair vtty_open_master() has just allocated itself is exempt from the > zero check: it is not reachable by anyone else yet and its zero count is > merely the initial state, vtty_install() hands that reference over to the > master peer below. Treating it as dying would break every attach to a > console which has no container-side opener. > > The vttym count is manipulated under the same tty_lock as well, so that > every reference of a vtty pair is only ever touched under tty_lock of its > vttys peer. > > The lock nesting pattern of tty_mutex -> tty_lock is preserved, which > prevents the kind of A->B, B->A circular locking dependency. > > Signed-off-by: Eva Kurchatova > Fixes: 62dce02f0c99 ("ve/tty: vt -- Implement per VE support for console and terminals") > > https://virtuozzo.atlassian.net/browse/VSTOR-136511 > Feature: tty: virtual Container console > --- > > Changes since v2: > > - Do not fail an attach to freshly created console. > v2 tested vttys->count == 0 unconditionally, but a pair allocated > by vtty_open_master() always has a zero slave count at that point: > tty_init_dev() -> vtty_install() sets it to 1 and vtty_open_master() > drops it right away. > So every attach to a container which has not opened /dev/console > returned -EBUSY. > > - Drop the tty_release() hunk and vtty_is_slave() altogether. > Once vtty_open_master() manipulates the counts under tty_lock(vttys), > every count of a vtty pair is only ever touched under that lock, > so a vttys close observing count == 0 can never see a positive vttym > count: a live vttym always holds a reference on its vttys peer, and > the master close decrements both under the same lock. > > - Manipulate the vttym count under tty_lock(vttys) as well. > v2 left the "one vttym at a time" test and tty->count++ outside > the lock, yet the argument above depends on both counts being > consistent. > > drivers/tty/pty.c | 41 +++++++++++++++++++++++++++++++++++++---- > 1 file changed, 37 insertions(+), 4 deletions(-) > > diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c > index f8610c77817a..a8f1cddecd0e 100644 > --- a/drivers/tty/pty.c > +++ b/drivers/tty/pty.c > @@ -1022,9 +1022,10 @@ void vtty_alloc_tty_struct(const struct tty_driver *driver, > > int vtty_open_master(envid_t veid, int idx) > { > - struct tty_struct *tty; > + struct tty_struct *tty, *slave; > struct file *file; > char devname[64]; > + bool created = false; > int fd, ret; > > if (!vtty_match_index(idx)) > @@ -1074,15 +1075,41 @@ int vtty_open_master(envid_t veid, int idx) > tty_set_lock_subclass(tty); > #endif > tty = tty->link; > + created = true; > } > > + slave = tty->link; > + > + /* > + * Every reference of a vtty pair is manipulated under the tty_lock > + * of its slave peer: tty_release() takes it for both peers (on the > + * master side via tty_lock_slave()) and tty_open() takes it before > + * tty_reopen(). Take it here as well so that we neither race with > + * an in-flight close nor resurrect a pair which is already gone. > + */ > + tty_lock(slave); > + > /* One master at a time */ > if (tty->count >= 1) { > ret = -EBUSY; > - goto err_install; > + goto err_unlock; > } > > - vtty_drop_context(); > + /* > + * A zero slave count on a pair looked up in the map means the last > + * user is gone and tty_release() has already committed to the final > + * close, release_tty() is only waiting for tty_mutex to free it. > + * Never hand out a reference to a tty which is about to be freed, > + * report -EBUSY so that the next attempt allocates a fresh pair. > + * > + * A pair we've just created above is exempt: it is not reachable by > + * anyone else yet and its zero count is merely the initial state, > + * vtty_install() gives its reference away to the master peer below. > + */ > + if (!created && slave->count == 0) { > + ret = -EBUSY; > + goto err_unlock; > + } > > /* > * We're the master peer so increment > @@ -1090,7 +1117,11 @@ int vtty_open_master(envid_t veid, int idx) > */ > tty_add_file(tty, file); > tty->count++; > - tty->link->count++; > + slave->count++; > + tty_unlock(slave); > + > + vtty_drop_context(); > + > fd_install(fd, file); > vtty_open(tty, file); > > @@ -1099,6 +1130,8 @@ int vtty_open_master(envid_t veid, int idx) > out: > return ret; > > +err_unlock: > + tty_unlock(slave); > err_install: > vtty_drop_context(); > mutex_unlock(&tty_mutex); -- Best regards, Vasileios Almpanis Software Developer, Virtuozzo.