From f9b1ce2f7dab75c6d072fcff2f25f31186fe7329 Mon Sep 17 00:00:00 2001 From: Nicolas Lacasse Date: Tue, 11 Mar 2025 16:42:58 -0700 Subject: [PATCH] Clean up tty.CheckChange and call it in SetForegroundProcessGroup. 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 --- pkg/sentry/fsimpl/devpts/master.go | 2 +- pkg/sentry/fsimpl/devpts/replica.go | 2 +- pkg/sentry/fsimpl/host/tty.go | 92 ++--------------------------- pkg/sentry/kernel/thread_group.go | 30 ++++++---- pkg/sentry/kernel/tty.go | 64 ++++++++++++++++++++ 5 files changed, 89 insertions(+), 101 deletions(-) diff --git a/pkg/sentry/fsimpl/devpts/master.go b/pkg/sentry/fsimpl/devpts/master.go index 498026fa6..ed291f6b2 100644 --- a/pkg/sentry/fsimpl/devpts/master.go +++ b/pkg/sentry/fsimpl/devpts/master.go @@ -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 diff --git a/pkg/sentry/fsimpl/devpts/replica.go b/pkg/sentry/fsimpl/devpts/replica.go index e7af3512a..741999754 100644 --- a/pkg/sentry/fsimpl/devpts/replica.go +++ b/pkg/sentry/fsimpl/devpts/replica.go @@ -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 diff --git a/pkg/sentry/fsimpl/host/tty.go b/pkg/sentry/fsimpl/host/tty.go index 282266c8b..8dfac53f9 100644 --- a/pkg/sentry/fsimpl/host/tty.go +++ b/pkg/sentry/fsimpl/host/tty.go @@ -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 -} diff --git a/pkg/sentry/kernel/thread_group.go b/pkg/sentry/kernel/thread_group.go index b274c5b9b..fda92393d 100644 --- a/pkg/sentry/kernel/thread_group.go +++ b/pkg/sentry/kernel/thread_group.go @@ -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 } diff --git a/pkg/sentry/kernel/tty.go b/pkg/sentry/kernel/tty.go index 7b36548f4..16063daef 100644 --- a/pkg/sentry/kernel/tty.go +++ b/pkg/sentry/kernel/tty.go @@ -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 +}