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

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

[Eva Kurchatova]

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 <eva.kurchatova@virtuozzo.com>
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);
-- 
2.55.0

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

[Vasileios Almpanis]

LGTM
Reviewed-by: Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>

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 <eva.kurchatova@virtuozzo.com>
> 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.

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

[Konstantin Khorenko]

The commit is pushed to "branch-rh10-6.12.0-211.39.1.16.x.vz10-ovz" and will appear at git at bitbucket.org:openvz/vzkernel.git
after rh10-6.12.0-211.39.1.16.3.vz10
------>
commit c5028cd61a694555ec2af47b2e6e2b4f8e013de9
Author: Eva Kurchatova <eva.kurchatova@virtuozzo.com>
Date:   Mon Aug 10 04:39:52 2026 +0300

    ve/vtty: fix use-after-free on concurrent tty close and reopen
    
    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.
    
    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
    Signed-off-by: Eva Kurchatova <eva.kurchatova@virtuozzo.com>
    Reviewed-by: Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>
---
 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 f8610c77817a5..14d46bb9c293d 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);