From 03a28d158e54aae89149f5b5b580f9d113aaf9e0 Mon Sep 17 00:00:00 2001 From: Andrei Vagin Date: Mon, 18 Nov 2024 10:29:40 -0800 Subject: [PATCH] platform/systrap: return memory access type based on a page fault error code Now we don't need to trigger a second fault to figure out whether it was write or read access. Fixes #11008 Co-developed-by: Jamie Liu PiperOrigin-RevId: 697677262 --- pkg/abi/linux/mm_amd64.go | 9 +++ pkg/hostarch/hostarch_arm64.go | 33 ++++++++ pkg/sentry/platform/kvm/machine_arm64.go | 25 +----- pkg/sentry/platform/systrap/subprocess.go | 42 ++++------ .../platform/systrap/subprocess_amd64.go | 14 ++++ .../platform/systrap/subprocess_arm64.go | 5 ++ .../systrap/sysmsg/sighandler_amd64.c | 11 ++- .../systrap/sysmsg/sighandler_arm64.c | 18 ++++- pkg/sentry/platform/systrap/sysmsg/sysmsg.go | 2 + pkg/sentry/platform/systrap/sysmsg/sysmsg.h | 1 + pkg/sentry/platform/systrap/systrap.go | 76 +------------------ 11 files changed, 106 insertions(+), 130 deletions(-) diff --git a/pkg/abi/linux/mm_amd64.go b/pkg/abi/linux/mm_amd64.go index 3fede2f74..5faa2fa7c 100644 --- a/pkg/abi/linux/mm_amd64.go +++ b/pkg/abi/linux/mm_amd64.go @@ -22,3 +22,12 @@ package linux // // The array has to be sorted in decreasing order. var feasibleTaskSizes = []uintptr{0xfffffffffff000, 0x7ffffffff000} + +// Page fault error codes +const ( + X86_PF_PROT = 1 << iota + X86_PF_WRITE + X86_PF_USER + X86_PF_RSVD + X86_PF_INSTR +) diff --git a/pkg/hostarch/hostarch_arm64.go b/pkg/hostarch/hostarch_arm64.go index 635f0e475..8ec71eb6d 100644 --- a/pkg/hostarch/hostarch_arm64.go +++ b/pkg/hostarch/hostarch_arm64.go @@ -53,6 +53,39 @@ var ( ByteOrder = binary.LittleEndian ) +// Arm64: Exception Syndrome Register EL1. +const ( + _ESR_ELx_EC_SHIFT = 26 + _ESR_ELx_EC_MASK = 0x3F << _ESR_ELx_EC_SHIFT + + _ESR_ELx_EC_IABT_LOW = 0x20 + _ESR_ELx_EC_DABT_LOW = 0x24 + + _ESR_ELx_WNR = 1 << 6 + _ESR_ELx_CM = 1 << 8 +) + +// ESRAccessType returns the memory access type for the given ESR (Exception +// Syndrome Register) code. If code does not represent an invalid memory +// access from a lower exception level, ESRAccessType returns NoAccess. +// +//go:nosplit +func ESRAccessType(code uint64) AccessType { + switch (code & _ESR_ELx_EC_MASK) >> _ESR_ELx_EC_SHIFT { + case _ESR_ELx_EC_IABT_LOW: + return Execute + case _ESR_ELx_EC_DABT_LOW: + // For faults on cache maintenance and address translation + // instructions, _ESR_ELx_WNR is always set. + if code&(_ESR_ELx_WNR|_ESR_ELx_CM) == _ESR_ELx_WNR { + return Write + } + return Read + default: + return NoAccess + } +} + func init() { // Make sure the page size is 4K on arm64 platform. if size := unix.Getpagesize(); size != PageSize { diff --git a/pkg/sentry/platform/kvm/machine_arm64.go b/pkg/sentry/platform/kvm/machine_arm64.go index ee770f36b..bcc7fb776 100644 --- a/pkg/sentry/platform/kvm/machine_arm64.go +++ b/pkg/sentry/platform/kvm/machine_arm64.go @@ -148,25 +148,6 @@ func nonCanonical(addr uint64, signal int32, info *linux.SignalInfo) (hostarch.A return hostarch.NoAccess, platform.ErrContextSignal } -// isInstructionAbort returns true if it is an instruction abort. -// -//go:nosplit -func isInstructionAbort(code uint64) bool { - value := (code & _ESR_ELx_EC_MASK) >> _ESR_ELx_EC_SHIFT - return value == _ESR_ELx_EC_IABT_LOW -} - -// isWriteFault returns whether it is a write fault. -// -//go:nosplit -func isWriteFault(code uint64) bool { - if isInstructionAbort(code) { - return false - } - - return (code & _ESR_ELx_WNR) != 0 -} - // fault generates an appropriate fault return. // //go:nosplit @@ -186,11 +167,7 @@ func (c *vCPU) fault(signal int32, info *linux.SignalInfo) (hostarch.AccessType, info.SetAddr(uint64(faultAddr)) accessType := hostarch.AccessType{} if signal == int32(unix.SIGSEGV) { - accessType = hostarch.AccessType{ - Read: !isWriteFault(uint64(code)), - Write: isWriteFault(uint64(code)), - Execute: isInstructionAbort(uint64(code)), - } + accessType = hostarch.ESRAccessType(uint64(code)) } ret := code & _ESR_ELx_FSC diff --git a/pkg/sentry/platform/systrap/subprocess.go b/pkg/sentry/platform/systrap/subprocess.go index f1fdd15f1..09491f8bb 100644 --- a/pkg/sentry/platform/systrap/subprocess.go +++ b/pkg/sentry/platform/systrap/subprocess.go @@ -782,7 +782,7 @@ func (s *subprocess) decAwakeContexts() { // This function returns true on a system call, false on a signal. // The second return value is true if a syscall instruction can be replaced on // a function call. -func (s *subprocess) switchToApp(c *platformContext, ac *arch.Context64) (isSyscall bool, shouldPatchSyscall bool, err *platform.ContextError) { +func (s *subprocess) switchToApp(c *platformContext, ac *arch.Context64) (isSyscall bool, shouldPatchSyscall bool, at hostarch.AccessType, err *platform.ContextError) { // Reset necessary registers. regs := &ac.StateData().Regs s.resetSysemuRegs(regs) @@ -795,7 +795,7 @@ func (s *subprocess) switchToApp(c *platformContext, ac *arch.Context64) (isSysc // Pending interrupt; simulate. ctx.clearInterrupt() c.signalInfo = linux.SignalInfo{Signo: int32(platform.SignalInterrupt)} - return false, false, nil + return false, false, hostarch.NoAccess, nil } defer func() { ctx.clearInterrupt() @@ -811,20 +811,20 @@ func (s *subprocess) switchToApp(c *platformContext, ac *arch.Context64) (isSysc } ctx.setState(sysmsg.ContextStateNone) if err := s.contextQueue.add(ctx); err != nil { - return false, false, err + return false, false, hostarch.NoAccess, err } if err := s.waitOnState(ctx); err != nil { - return false, false, corruptedSharedMemoryErr(err.Error()) + return false, false, hostarch.NoAccess, corruptedSharedMemoryErr(err.Error()) } // Check if there's been an error. threadID := ctx.threadID() if threadID != invalidThreadID { if sysThread, ok := s.sysmsgThreads[threadID]; ok && sysThread.msg.Err != 0 { - return false, false, sysThread.msg.ConvertSysmsgErr() + return false, false, hostarch.NoAccess, sysThread.msg.ConvertSysmsgErr() } - return false, false, corruptedSharedMemoryErr(fmt.Sprintf("found unexpected ThreadContext.ThreadID field, expected %d found %d", invalidThreadID, threadID)) + return false, false, hostarch.NoAccess, corruptedSharedMemoryErr(fmt.Sprintf("found unexpected ThreadContext.ThreadID field, expected %d found %d", invalidThreadID, threadID)) } // Copy register state locally. @@ -843,15 +843,19 @@ func (s *subprocess) switchToApp(c *platformContext, ac *arch.Context64) (isSysc if ctxState == sysmsg.ContextStateSyscall || ctxState == sysmsg.ContextStateSyscallTrap { if maybePatchSignalInfo(regs, &c.signalInfo) { - return false, false, nil + return false, false, hostarch.Execute, nil } updateSyscallRegs(regs) - return true, shouldPatchSyscall, nil + return true, shouldPatchSyscall, hostarch.NoAccess, nil } else if ctxState != sysmsg.ContextStateFault { - return false, false, corruptedSharedMemoryErr(fmt.Sprintf("unknown context state: %v", ctxState)) + return false, false, hostarch.NoAccess, corruptedSharedMemoryErr(fmt.Sprintf("unknown context state: %v", ctxState)) } - return false, false, nil + at = hostarch.NoAccess + if c.signalInfo.Signo == int32(linux.SIGSEGV) { + at = sigErrorToAccessType(ctx.shared.SigError) + } + return false, false, at, nil } func (s *subprocess) waitOnState(ctx *sharedContext) error { @@ -989,24 +993,6 @@ func (s *subprocess) MapFile(addr hostarch.Addr, f memmap.File, fr memmap.FileRa // Unmap implements platform.AddressSpace.Unmap. func (s *subprocess) Unmap(addr hostarch.Addr, length uint64) { - ar, ok := addr.ToRange(length) - if !ok { - panic(fmt.Sprintf("addr %#x + length %#x overflows", addr, length)) - } - s.mu.Lock() - for c := range s.faultedContexts { - c.mu.Lock() - if c.lastFaultSP == s && ar.Contains(c.lastFaultAddr) { - // Forget the last fault so that if c faults again, the fault isn't - // incorrectly reported as a write fault. If this is being called - // due to munmap() of the corresponding vma, handling of the second - // fault will fail anyway. - c.lastFaultSP = nil - delete(s.faultedContexts, c) - } - c.mu.Unlock() - } - s.mu.Unlock() _, err := s.syscall( unix.SYS_MUNMAP, arch.SyscallArgument{Value: uintptr(addr)}, diff --git a/pkg/sentry/platform/systrap/subprocess_amd64.go b/pkg/sentry/platform/systrap/subprocess_amd64.go index 3456103be..e4f589a72 100644 --- a/pkg/sentry/platform/systrap/subprocess_amd64.go +++ b/pkg/sentry/platform/systrap/subprocess_amd64.go @@ -23,6 +23,7 @@ import ( "golang.org/x/sys/unix" "gvisor.dev/gvisor/pkg/abi/linux" + "gvisor.dev/gvisor/pkg/hostarch" "gvisor.dev/gvisor/pkg/hostsyscall" "gvisor.dev/gvisor/pkg/seccomp" "gvisor.dev/gvisor/pkg/sentry/arch" @@ -223,3 +224,16 @@ func retrieveArchSpecificState(ctx *sysmsg.ThreadContext, ac *arch.Context64) { func archSpecificSysmsgThreadInit(sysThread *sysmsgThread) { } + +func sigErrorToAccessType(sigError uint64) hostarch.AccessType { + switch { + case sigError&linux.X86_PF_WRITE != 0: + return hostarch.Write + case sigError&linux.X86_PF_INSTR != 0: + return hostarch.Execute + case sigError&linux.X86_PF_USER != 0: + return hostarch.Read + default: + return hostarch.NoAccess + } +} diff --git a/pkg/sentry/platform/systrap/subprocess_arm64.go b/pkg/sentry/platform/systrap/subprocess_arm64.go index 86fad320f..0e553fca5 100644 --- a/pkg/sentry/platform/systrap/subprocess_arm64.go +++ b/pkg/sentry/platform/systrap/subprocess_arm64.go @@ -23,6 +23,7 @@ import ( "golang.org/x/sys/unix" "gvisor.dev/gvisor/pkg/abi/linux" + "gvisor.dev/gvisor/pkg/hostarch" "gvisor.dev/gvisor/pkg/hostsyscall" "gvisor.dev/gvisor/pkg/seccomp" "gvisor.dev/gvisor/pkg/sentry/arch" @@ -195,3 +196,7 @@ func archSpecificSysmsgThreadInit(sysThread *sysmsgThread) { panic(fmt.Sprintf("tkill failed: %v", e)) } } + +func sigErrorToAccessType(sigError uint64) hostarch.AccessType { + return hostarch.ESRAccessType(sigError) +} diff --git a/pkg/sentry/platform/systrap/sysmsg/sighandler_amd64.c b/pkg/sentry/platform/systrap/sysmsg/sighandler_amd64.c index b3d6e98d9..f211dd433 100644 --- a/pkg/sentry/platform/systrap/sysmsg/sighandler_amd64.c +++ b/pkg/sentry/platform/systrap/sysmsg/sighandler_amd64.c @@ -32,6 +32,10 @@ #include "sysmsg_offsets.h" #include "sysmsg_offsets_amd64.h" +#ifndef X86_TRAP_PF +#define X86_TRAP_PF 14 +#endif + // TODO(b/271631387): These globals are shared between AMD64 and ARM64; move to // sysmsg_lib.c. struct arch_state __export_arch_state; @@ -230,6 +234,7 @@ void __export_sighandler(int signo, siginfo_t *siginfo, void *_ucontext) { if (signo != SIGCHLD || ucontext->uc_mcontext.gregs[REG_RIP] < __export_stub_start) { ctx->ptregs.fs_base = fs_base; + ctx->err = 0; gregs_to_ptregs(ucontext, &ctx->ptregs); memcpy(ctx->fpstate, (uint8_t *)ucontext->uc_mcontext.fpregs, __export_arch_state.fp_len); @@ -307,8 +312,12 @@ void __export_sighandler(int signo, siginfo_t *siginfo, void *_ucontext) { ctx->ptregs.orig_rax += 0x86000000; break; } - case SIGCHLD: case SIGSEGV: + if (ucontext->uc_mcontext.gregs[REG_TRAPNO] == X86_TRAP_PF) { + ctx->err = ucontext->uc_mcontext.gregs[REG_ERR]; + } + // fallthrough + case SIGCHLD: case SIGBUS: case SIGFPE: case SIGTRAP: diff --git a/pkg/sentry/platform/systrap/sysmsg/sighandler_arm64.c b/pkg/sentry/platform/systrap/sysmsg/sighandler_arm64.c index 1173b8a94..570b93ed0 100644 --- a/pkg/sentry/platform/systrap/sysmsg/sighandler_arm64.c +++ b/pkg/sentry/platform/systrap/sysmsg/sighandler_arm64.c @@ -139,6 +139,7 @@ void __export_sighandler(int signo, siginfo_t *siginfo, void *_ucontext) { memcpy(ctx->fpstate, fpStatePointer, kFpsimdContextSize); ctx->tls = get_tls(); ctx->siginfo = *siginfo; + ctx->err = 0; switch (signo) { case SIGSYS: { ctx_state = CONTEXT_STATE_SYSCALL; @@ -151,9 +152,22 @@ void __export_sighandler(int signo, siginfo_t *siginfo, void *_ucontext) { } break; } - case SIGCHLD: - case SIGSEGV: + case SIGSEGV: { + unsigned char *base = &ucontext->uc_mcontext.__reserved[0]; + size_t offset = 0; + while (1) { + struct _aarch64_ctx *head = (struct _aarch64_ctx *)(base + offset); + if (head->magic == ESR_MAGIC) { + ctx->err = ((struct esr_context *)head)->esr; + break; + } + if (head->magic == 0 || head->magic == EXTRA_MAGIC) break; + offset += head->size; + } + } + // fallthrough case SIGBUS: + case SIGCHLD: case SIGFPE: case SIGTRAP: case SIGILL: diff --git a/pkg/sentry/platform/systrap/sysmsg/sysmsg.go b/pkg/sentry/platform/systrap/sysmsg/sysmsg.go index b8ab2a549..60f1796ac 100644 --- a/pkg/sentry/platform/systrap/sysmsg/sysmsg.go +++ b/pkg/sentry/platform/systrap/sysmsg/sysmsg.go @@ -269,6 +269,8 @@ type ThreadContext struct { TLS uint64 // Debug is a variable to use to get visibility into the stub from the sentry. Debug uint64 + // SigError is an error code that clarifies the nature of the signal. + SigError uint64 } // StubError are values that represent known stub-thread failure modes. diff --git a/pkg/sentry/platform/systrap/sysmsg/sysmsg.h b/pkg/sentry/platform/systrap/sysmsg/sysmsg.h index 07ba6f8bd..056d2d1e0 100644 --- a/pkg/sentry/platform/systrap/sysmsg/sysmsg.h +++ b/pkg/sentry/platform/systrap/sysmsg/sysmsg.h @@ -98,6 +98,7 @@ struct thread_context { uint64_t state_changed_time; uint64_t tls; uint64_t debug; + uint64_t err; }; enum stub_error { diff --git a/pkg/sentry/platform/systrap/systrap.go b/pkg/sentry/platform/systrap/systrap.go index 70cc19155..058b979b5 100644 --- a/pkg/sentry/platform/systrap/systrap.go +++ b/pkg/sentry/platform/systrap/systrap.go @@ -129,22 +129,6 @@ type platformContext struct { // mutex protected. sharedContext *sharedContext - // mu protects the following fields. - mu sync.Mutex - - // If lastFaultSP is non-nil, the last platformContext switch was due to a fault - // received while executing lastFaultSP. Only platformContext.Switch may set - // lastFaultSP to a non-nil value. - lastFaultSP *subprocess - - // lastFaultAddr is the last faulting address; this is only meaningful if - // lastFaultSP is non-nil. - lastFaultAddr hostarch.Addr - - // lastFaultIP is the address of the last faulting instruction; - // this is also only meaningful if lastFaultSP is non-nil. - lastFaultIP hostarch.Addr - // needRestoreFPState indicates that the FPU state has been changed by // the Sentry and has to be updated on the stub thread. needRestoreFPState bool @@ -182,7 +166,7 @@ func (c *platformContext) Switch(ctx pkgcontext.Context, mm platform.MemoryManag } restart: - isSyscall, needPatch, err := s.switchToApp(c, ac) + isSyscall, needPatch, at, err := s.switchToApp(c, ac) if err != nil { return nil, hostarch.NoAccess, err } @@ -199,76 +183,18 @@ restart: ctx.Warningf("usertrap.HandleFault failed: %v", err) } } - var ( - faultSP *subprocess - faultAddr hostarch.Addr - faultIP hostarch.Addr - ) - if !isSyscall && linux.Signal(c.signalInfo.Signo) == linux.SIGSEGV { - faultSP = s - faultAddr = hostarch.Addr(c.signalInfo.Addr()) - faultIP = hostarch.Addr(ac.IP()) - } - - // Update the platformContext to reflect the outcome of this context switch. - c.mu.Lock() - lastFaultSP := c.lastFaultSP - lastFaultAddr := c.lastFaultAddr - lastFaultIP := c.lastFaultIP - // At this point, c may not yet be in s.faultedContexts, so c.lastFaultSP won't - // be updated by s.Unmap(). This is fine; we only need to synchronize with - // calls to s.Unmap() that occur after the handling of this fault. - c.lastFaultSP = faultSP - c.lastFaultAddr = faultAddr - c.lastFaultIP = faultIP - c.mu.Unlock() - - // Update subprocesses to reflect the outcome of this context switch. - if lastFaultSP != faultSP { - if lastFaultSP != nil { - lastFaultSP.mu.Lock() - delete(lastFaultSP.faultedContexts, c) - lastFaultSP.mu.Unlock() - } - if faultSP != nil { - faultSP.mu.Lock() - faultSP.faultedContexts[c] = struct{}{} - faultSP.mu.Unlock() - } - } if isSyscall { return nil, hostarch.NoAccess, nil } si := c.signalInfo - if faultSP == nil { - // Non-fault signal. - return &si, hostarch.NoAccess, platform.ErrContextSignal - } // See if this can be handled as a CPUID exception. if linux.Signal(si.Signo) == linux.SIGSEGV && platform.TryCPUIDEmulate(ctx, mm, ac) { goto restart } - // Got a page fault. Ideally, we'd get real fault type here, but ptrace - // doesn't expose this information. Instead, we use a simple heuristic: - // - // It was an instruction fault iff the faulting addr == instruction - // pointer. - // - // It was a write fault if the fault is immediately repeated. - at := hostarch.Read - if faultAddr == faultIP { - at.Execute = true - } - if lastFaultSP == faultSP && - lastFaultAddr == faultAddr && - lastFaultIP == faultIP { - at.Write = true - } - // Handle as a signal. return &si, at, platform.ErrContextSignal }