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 <[email protected]>
---
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);
+ 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.
+ */
+ 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 */
--
2.54.0
_______________________________________________
Devel mailing list
[email protected]
https://lists.openvz.org/mailman/listinfo/devel
