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 +}