From de5a590ee203b4ee217da68dbec8e58a7753e520 Mon Sep 17 00:00:00 2001 From: newmanwang Date: Fri, 14 Sep 2018 17:38:16 -0700 Subject: Avoid reuse of pending SignalInfo objects runApp.execute -> Task.SendSignal -> sendSignalLocked -> sendSignalTimerLocked -> pendingSignals.enqueue assumes that it owns the arch.SignalInfo returned from platform.Context.Switch. On the other hand, ptrace.context.Switch assumes that it owns the returned SignalInfo and can safely reuse it on the next call to Switch. The KVM platform always returns a unique SignalInfo. This becomes a problem when the returned signal is not immediately delivered, allowing a future signal in Switch to change the previous pending SignalInfo. This is noticeable in #38 when external SIGINTs are delivered from the PTY slave FD. Note that the ptrace stubs are in the same process group as the sentry, so they are eligible to receive the PTY signals. This should probably change, but is not the only possible cause of this bug. Updates #38 Original change by newmanwang , updated by Michael Pratt . Change-Id: I5383840272309df70a29f67b25e8221f933622cd PiperOrigin-RevId: 213071072 --- pkg/sentry/platform/platform.go | 3 ++- pkg/sentry/platform/ptrace/ptrace.go | 7 +++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/pkg/sentry/platform/platform.go b/pkg/sentry/platform/platform.go index 6eb2acbd7..8a1620d93 100644 --- a/pkg/sentry/platform/platform.go +++ b/pkg/sentry/platform/platform.go @@ -133,7 +133,8 @@ type Context interface { // - ErrContextSignal: The Context was interrupted by a signal. The // returned *arch.SignalInfo contains information about the signal. If // arch.SignalInfo.Signo == SIGSEGV, the returned usermem.AccessType - // contains the access type of the triggering fault. + // contains the access type of the triggering fault. The caller owns + // the returned SignalInfo. // // - ErrContextInterrupt: The Context was interrupted by a call to // Interrupt(). Switch() may return ErrContextInterrupt spuriously. In diff --git a/pkg/sentry/platform/ptrace/ptrace.go b/pkg/sentry/platform/ptrace/ptrace.go index a44f549a2..4f20716f7 100644 --- a/pkg/sentry/platform/ptrace/ptrace.go +++ b/pkg/sentry/platform/ptrace/ptrace.go @@ -142,9 +142,12 @@ func (c *context) Switch(as platform.AddressSpace, ac arch.Context, cpu int32) ( if isSyscall { return nil, usermem.NoAccess, nil } + + si := c.signalInfo + if faultSP == nil { // Non-fault signal. - return &c.signalInfo, usermem.NoAccess, platform.ErrContextSignal + return &si, usermem.NoAccess, platform.ErrContextSignal } // Got a page fault. Ideally, we'd get real fault type here, but ptrace @@ -168,7 +171,7 @@ func (c *context) Switch(as platform.AddressSpace, ac arch.Context, cpu int32) ( // here, in case this fault was generated by a CPUID exception. There // is no way to distinguish between CPUID-generated faults and regular // page faults. - return &c.signalInfo, at, platform.ErrContextSignalCPUID + return &si, at, platform.ErrContextSignalCPUID } // Interrupt interrupts the running guest application associated with this context. -- cgit v1.2.3