netbird/gvisor
0
0
Fork 0
mirror of https://github.com/netbirdio/gvisor.git synced 2026-05-22 17:12:49 -07:00
Code Issues Packages Projects Releases Wiki Activity

Clean up tty.CheckChange and call it in SetForegroundProcessGroup.

Browse Source 
Previously, CheckChange (corresponding to Linux's tty/tty_check_change()) was
only used the host TTY implementation, not the devpts implementation.

Furthermore, ThreadGroup.SetForegroundProcessGroup() duplicated some of the
logic in CheckChange, notably sending SIGTTOU to background tasks. This means
that, for host TTYs, we could send SIGTTOU multiple times. In some
circumstances, this leads the ioctl returning ERESTARTSYS in an infinite loop.

PiperOrigin-RevId: 735934036
This commit is contained in:
Nicolas Lacasse
2025-03-11 16:46:55 -07:00
committed by gVisor bot
parent 93536059ca
commit f9b1ce2f7d
5 changed files with 89 additions and 101 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/sentry/fsimpl/devpts/master.go
+1 -1
View File
@@ -198,7 +198,7 @@ func (mfd *masterFileDescription) Ioctl(ctx context.Context, io usermem.IO, sysn
if _, err := pgid.CopyIn(t, args[2].Pointer()); err != nil {
return 0, err
}
return 0, t.ThreadGroup().SetForegroundProcessGroupID(mfd.t.masterKTTY, kernel.ProcessGroupID(pgid))
return 0, t.ThreadGroup().SetForegroundProcessGroupID(ctx, mfd.t.masterKTTY, kernel.ProcessGroupID(pgid))
default:
maybeEmitUnimplementedEvent(ctx, sysno, cmd)
return 0, linuxerr.ENOTTY
pkg/sentry/fsimpl/devpts/replica.go
+1 -1
View File
@@ -188,7 +188,7 @@ func (rfd *replicaFileDescription) Ioctl(ctx context.Context, io usermem.IO, sys
if _, err := pgid.CopyIn(t, args[2].Pointer()); err != nil {
return 0, err
}
return 0, t.ThreadGroup().SetForegroundProcessGroupID(rfd.inode.t.replicaKTTY, kernel.ProcessGroupID(pgid))
return 0, t.ThreadGroup().SetForegroundProcessGroupID(ctx, rfd.inode.t.replicaKTTY, kernel.ProcessGroupID(pgid))
default:
maybeEmitUnimplementedEvent(ctx, sysno, cmd)
return 0, linuxerr.ENOTTY
pkg/sentry/fsimpl/host/tty.go
+6 -86
View File
@@ -95,7 +95,7 @@ func (t *TTYFileDescription) PRead(ctx context.Context, dst usermem.IOSequence,
// Are we allowed to do the read?
// drivers/tty/n_tty.c:n_tty_read()=>job_control()=>tty_check_change().
if err := t.checkChange(ctx, linux.SIGTTIN); err != nil {
if err := t.tty.CheckChange(ctx, linux.SIGTTIN); err != nil {
return 0, err
}
@@ -113,7 +113,7 @@ func (t *TTYFileDescription) Read(ctx context.Context, dst usermem.IOSequence, o
// Are we allowed to do the read?
// drivers/tty/n_tty.c:n_tty_read()=>job_control()=>tty_check_change().
if err := t.checkChange(ctx, linux.SIGTTIN); err != nil {
if err := t.tty.CheckChange(ctx, linux.SIGTTIN); err != nil {
return 0, err
}
@@ -129,7 +129,7 @@ func (t *TTYFileDescription) PWrite(ctx context.Context, src usermem.IOSequence,
// Check whether TOSTOP is enabled. This corresponds to the check in
// drivers/tty/n_tty.c:n_tty_write().
if t.termios.LEnabled(linux.TOSTOP) {
if err := t.checkChange(ctx, linux.SIGTTOU); err != nil {
if err := t.tty.CheckChange(ctx, linux.SIGTTOU); err != nil {
return 0, err
}
}
@@ -144,7 +144,7 @@ func (t *TTYFileDescription) Write(ctx context.Context, src usermem.IOSequence,
// Check whether TOSTOP is enabled. This corresponds to the check in
// drivers/tty/n_tty.c:n_tty_write().
if t.termios.LEnabled(linux.TOSTOP) {
if err := t.checkChange(ctx, linux.SIGTTOU); err != nil {
if err := t.tty.CheckChange(ctx, linux.SIGTTOU); err != nil {
return 0, err
}
}
@@ -185,7 +185,7 @@ func (t *TTYFileDescription) Ioctl(ctx context.Context, io usermem.IO, sysno uin
t.mu.Lock()
defer t.mu.Unlock()
if err := t.checkChange(ctx, linux.SIGTTOU); err != nil {
if err := t.tty.CheckChange(ctx, linux.SIGTTOU); err != nil {
return 0, err
}
@@ -231,27 +231,12 @@ func (t *TTYFileDescription) Ioctl(ctx context.Context, io usermem.IO, sysno uin
t.mu.Lock()
defer t.mu.Unlock()
// Check that we are allowed to set the process group.
if err := t.checkChange(ctx, linux.SIGTTOU); err != nil {
// drivers/tty/tty_io.c:tiocspgrp() converts -EIO from tty_check_change()
// to -ENOTTY.
if linuxerr.Equals(linuxerr.EIO, err) {
return 0, linuxerr.ENOTTY
}
return 0, err
}
// Check that calling task's process group is in the TTY session.
if task.ThreadGroup().Session() != t.tty.ThreadGroup().Session() {
return 0, linuxerr.ENOTTY
}
var pgIDP primitive.Int32
if _, err := pgIDP.CopyIn(task, args[2].Pointer()); err != nil {
return 0, err
}
pgID := kernel.ProcessGroupID(pgIDP)
if err := t.tty.ThreadGroup().SetForegroundProcessGroupID(t.tty, pgID); err != nil {
if err := t.tty.ThreadGroup().SetForegroundProcessGroupID(ctx, t.tty, pgID); err != nil {
return 0, err
}
@@ -314,68 +299,3 @@ func (t *TTYFileDescription) Ioctl(ctx context.Context, io usermem.IO, sysno uin
return 0, linuxerr.ENOTTY
}
}
// checkChange checks that the process group is allowed to read, write, or
// change the state of the TTY.
//
// This corresponds to Linux drivers/tty/tty_io.c:tty_check_change(). The logic
// is a bit convoluted, but documented inline.
//
// Preconditions: t.mu must be held.
func (t *TTYFileDescription) checkChange(ctx context.Context, sig linux.Signal) error {
task := kernel.TaskFromContext(ctx)
if task == nil {
// No task? Linux does not have an analog for this case, but
// tty_check_change only blocks specific cases and is
// surprisingly permissive. Allowing the change seems
// appropriate.
return nil
}
tg := task.ThreadGroup()
pg := tg.ProcessGroup()
ttyTg := t.tty.ThreadGroup()
// If the session for the task is different than the session for the
// controlling TTY, then the change is allowed. Seems like a bad idea,
// but that's exactly what linux does.
if ttyTg == nil || tg.Session() != ttyTg.Session() {
return nil
}
// If we are the foreground process group, then the change is allowed.
if fgpg, _ := t.tty.ThreadGroup().ForegroundProcessGroup(t.tty); pg == fgpg {
return nil
}
// We are not the foreground process group.
// Is the provided signal blocked or ignored?
if (task.SignalMask()&linux.SignalSetOf(sig) != 0) || tg.SignalHandlers().IsIgnored(sig) {
// If the signal is SIGTTIN, then we are attempting to read
// from the TTY. Don't send the signal and return EIO.
if sig == linux.SIGTTIN {
return linuxerr.EIO
}
// Otherwise, we are writing or changing terminal state. This is allowed.
return nil
}
// If the process group is an orphan, return EIO.
if pg.IsOrphan() {
return linuxerr.EIO
}
// Otherwise, send the signal to the process group and return ERESTARTSYS.
//
// Note that Linux also unconditionally sets TIF_SIGPENDING on current,
// but this isn't necessary in gVisor because the rationale given in
// 040b6362d58f "tty: fix leakage of -ERESTARTSYS to userland" doesn't
// apply: the sentry will handle -ERESTARTSYS in
// kernel.runApp.execute() even if the kernel.Task isn't interrupted.
//
// Linux ignores the result of kill_pgrp().
_ = pg.SendSignal(kernel.SignalInfoPriv(sig))
return linuxerr.ERESTARTSYS
}
pkg/sentry/kernel/thread_group.go
+17 -13
View File
@@ -558,8 +558,18 @@ func (tg *ThreadGroup) ForegroundProcessGroupID(tty *TTY) (ProcessGroupID, error
}
// SetForegroundProcessGroupID sets the foreground process group of tty to
// pgid.
func (tg *ThreadGroup) SetForegroundProcessGroupID(tty *TTY, pgid ProcessGroupID) error {
// pgid. It corresponds to Linux's drivers/tty/tty_io.c:tiocspgrp().
func (tg *ThreadGroup) SetForegroundProcessGroupID(ctx context.Context, tty *TTY, pgid ProcessGroupID) error {
// First check that the change is allowed.
if err := tty.CheckChange(ctx, linux.SIGTTOU); err != nil {
// tiocspgrp() converts -EIO from tty_check_change() to
// -ENOTTY.
if linuxerr.Equals(linuxerr.EIO, err) {
return linuxerr.ENOTTY
}
return err
}
tty.mu.Lock()
defer tty.mu.Unlock()
@@ -573,6 +583,11 @@ func (tg *ThreadGroup) SetForegroundProcessGroupID(tty *TTY, pgid ProcessGroupID
return linuxerr.ENOTTY
}
// Calling task's process group must be in the TTY session.
if tty.tg == nil || tty.tg.processGroup.session != tg.processGroup.session {
return linuxerr.ENOTTY
}
// pgid must be positive.
if pgid < 0 {
return linuxerr.EINVAL
@@ -590,17 +605,6 @@ func (tg *ThreadGroup) SetForegroundProcessGroupID(tty *TTY, pgid ProcessGroupID
return linuxerr.EPERM
}
signalAction := tg.signalHandlers.actions[linux.SIGTTOU]
// If the calling process is a member of a background group, a SIGTTOU
// signal is sent to all members of this background process group.
// We need also need to check whether it is ignoring or blocking SIGTTOU.
ignored := signalAction.Handler == linux.SIG_IGN
blocked := (linux.SignalSet(tg.leader.signalMask.RacyLoad()) & linux.SignalSetOf(linux.SIGTTOU)) != 0
if tg.processGroup.id != tg.processGroup.session.foreground.id && !ignored && !blocked {
tg.leader.sendSignalLocked(SignalInfoPriv(linux.SIGTTOU), true)
return linuxerr.ERESTARTSYS
}
tg.processGroup.session.foreground = pg
return nil
}
pkg/sentry/kernel/tty.go
+64
View File
@@ -17,6 +17,7 @@ package kernel
import (
"gvisor.dev/gvisor/pkg/abi/linux"
"gvisor.dev/gvisor/pkg/context"
"gvisor.dev/gvisor/pkg/errors/linuxerr"
"gvisor.dev/gvisor/pkg/log"
"gvisor.dev/gvisor/pkg/sentry/vfs"
"gvisor.dev/gvisor/pkg/sync"
@@ -94,3 +95,66 @@ func (tty *TTY) SignalForegroundProcessGroup(info *linux.SignalInfo) {
log.Warningf("failed to signal foreground process group (pgid=%d): %v", fg.id, err)
}
}
// CheckChange checks that the calling tash is allowed to read, write, or
// change the state of the TTY.
//
// This corresponds to Linux drivers/tty/tty_io.c:tty_check_change().
func (tty *TTY) CheckChange(ctx context.Context, sig linux.Signal) error {
task := TaskFromContext(ctx)
if task == nil {
// No task? Linux does not have an analog for this case, but
// tty_check_change only blocks specific cases and is
// surprisingly permissive. Allowing the change seems
// appropriate.
return nil
}
tg := task.ThreadGroup()
pg := tg.ProcessGroup()
ttyTG := tty.ThreadGroup()
// If the session for the task is different than the session for the
// controlling TTY, then the change is allowed. Seems like a bad idea,
// but that's exactly what linux does.
if ttyTG == nil || tg.Session() != ttyTG.Session() {
return nil
}
// If we are the foreground process group, then the change is allowed.
if fgpg, _ := ttyTG.ForegroundProcessGroup(tty); pg == fgpg {
return nil
}
// We are not the foreground process group.
// Is the provided signal blocked or ignored?
if (task.SignalMask()&linux.SignalSetOf(sig) != 0) || tg.SignalHandlers().IsIgnored(sig) {
// If the signal is SIGTTIN, then we are attempting to read
// from the TTY. Don't send the signal and return EIO.
if sig == linux.SIGTTIN {
return linuxerr.EIO
}
// Otherwise, we are writing or changing terminal state. This is allowed.
return nil
}
// If the process group is an orphan, return EIO.
if pg.IsOrphan() {
return linuxerr.EIO
}
// Otherwise, send the signal to the process group and return ERESTARTSYS.
//
// Note that Linux also unconditionally sets TIF_SIGPENDING on current,
// but this isn't necessary in gVisor because the rationale given in
// https://github.com/torvalds/linux/commit/040b6362d58f "tty: fix
// leakage of -ERESTARTSYS to userland" doesn't apply: the sentry will
// handle -ERESTARTSYS in kernel.runApp.execute() even if the
// kernel.Task isn't interrupted.
//
// Linux ignores the result of kill_pgrp().
_ = pg.SendSignal(SignalInfoPriv(sig))
return linuxerr.ERESTARTSYS
}