From cc48969bb72e3efdc22746c5e7463b79b1942c2b Mon Sep 17 00:00:00 2001 From: Michael Pratt Date: Wed, 10 Apr 2019 17:59:02 -0700 Subject: Internal change PiperOrigin-RevId: 242978508 Change-Id: I0ea59ac5ba1dd499e87c53f2e24709371048679b --- pkg/sentry/arch/arch_x86.go | 4 ++-- pkg/sentry/arch/signal_amd64.go | 11 ++++++----- pkg/sentry/kernel/syscalls.go | 4 ++-- pkg/sentry/loader/loader.go | 6 +++--- pkg/sentry/loader/vdso.go | 5 +++-- pkg/sentry/mm/syscalls.go | 10 +++++----- pkg/sentry/strace/strace.go | 4 ++-- pkg/sentry/syscalls/linux/error.go | 4 ++-- test/syscalls/linux/32bit.cc | 4 ++-- test/syscalls/linux/exec_binary.cc | 10 +++++----- vdso/vdso.lds | 4 ++-- 11 files changed, 34 insertions(+), 32 deletions(-) diff --git a/pkg/sentry/arch/arch_x86.go b/pkg/sentry/arch/arch_x86.go index e50a76083..c8bf0e7f2 100644 --- a/pkg/sentry/arch/arch_x86.go +++ b/pkg/sentry/arch/arch_x86.go @@ -306,8 +306,8 @@ func (s *State) ptraceGetRegs() syscall.PtraceRegs { // FS/GS_TLS_SEL when fs_base/gs_base is a 64-bit value. (We do the // same in PtraceSetRegs.) // - // TODO: Remove this fixup since newer Linux doesn't have - // this behavior anymore. + // TODO: Remove this fixup since newer Linux + // doesn't have this behavior anymore. if regs.Fs == 0 && regs.Fs_base <= 0xffffffff { regs.Fs = _FS_TLS_SEL } diff --git a/pkg/sentry/arch/signal_amd64.go b/pkg/sentry/arch/signal_amd64.go index f7f054b0b..c9de36897 100644 --- a/pkg/sentry/arch/signal_amd64.go +++ b/pkg/sentry/arch/signal_amd64.go @@ -392,15 +392,16 @@ func (c *context64) SignalSetup(st *Stack, act *SignalAct, info *SignalInfo, alt Sigset: sigset, } - // TODO: Set SignalContext64.Err, Trapno, and Cr2 based on - // the fault that caused the signal. For now, leave Err and Trapno - // unset and assume CR2 == info.Addr() for SIGSEGVs and SIGBUSes. + // TODO: Set SignalContext64.Err, Trapno, and Cr2 + // based on the fault that caused the signal. For now, leave Err and + // Trapno unset and assume CR2 == info.Addr() for SIGSEGVs and + // SIGBUSes. if linux.Signal(info.Signo) == linux.SIGSEGV || linux.Signal(info.Signo) == linux.SIGBUS { uc.MContext.Cr2 = info.Addr() } - // "... the value (%rsp+8) is always a multiple of 16 (...) when control is - // transferred to the function entry point." - AMD64 ABI + // "... the value (%rsp+8) is always a multiple of 16 (...) when + // control is transferred to the function entry point." - AMD64 ABI ucSize := binary.Size(uc) if ucSize < 0 { // This can only happen if we've screwed up the definition of diff --git a/pkg/sentry/kernel/syscalls.go b/pkg/sentry/kernel/syscalls.go index 19b711e9c..7eb99718d 100644 --- a/pkg/sentry/kernel/syscalls.go +++ b/pkg/sentry/kernel/syscalls.go @@ -165,8 +165,8 @@ type Stracer interface { // // The returned private data is passed to SyscallExit. // - // TODO: remove kernel imports from the strace package so - // that the type can be used directly. + // TODO: remove kernel imports from the strace + // package so that the type can be used directly. SyscallEnter(t *Task, sysno uintptr, args arch.SyscallArguments, flags uint32) interface{} // SyscallExit is called on syscall exit. diff --git a/pkg/sentry/loader/loader.go b/pkg/sentry/loader/loader.go index deb8892f6..80ad59dde 100644 --- a/pkg/sentry/loader/loader.go +++ b/pkg/sentry/loader/loader.go @@ -70,9 +70,9 @@ func openPath(ctx context.Context, mm *fs.MountNamespace, root, wd *fs.Dirent, m defer d.DecRef() perms := fs.PermMask{ - // TODO: Linux requires only execute permission, - // not read. However, our backing filesystems may prevent us - // from reading the file without read permission. + // TODO: Linux requires only execute + // permission, not read. However, our backing filesystems may + // prevent us from reading the file without read permission. // // Additionally, a task with a non-readable executable has // additional constraints on access via ptrace and procfs. diff --git a/pkg/sentry/loader/vdso.go b/pkg/sentry/loader/vdso.go index 273f6b5b9..fabf0cbe4 100644 --- a/pkg/sentry/loader/vdso.go +++ b/pkg/sentry/loader/vdso.go @@ -261,8 +261,9 @@ func PrepareVDSO(mfp pgalloc.MemoryFileProvider) (*VDSO, error) { return &VDSO{ ParamPage: mm.NewSpecialMappable("[vvar]", mfp, paramPage), - // TODO: Don't advertise the VDSO, as some applications may - // not be able to handle multiple [vdso] hints. + // TODO: Don't advertise the VDSO, as + // some applications may not be able to handle multiple [vdso] + // hints. vdso: mm.NewSpecialMappable("", mfp, vdso), phdrs: info.phdrs, }, nil diff --git a/pkg/sentry/mm/syscalls.go b/pkg/sentry/mm/syscalls.go index f8f095fed..cc7eb76d2 100644 --- a/pkg/sentry/mm/syscalls.go +++ b/pkg/sentry/mm/syscalls.go @@ -698,11 +698,11 @@ func (mm *MemoryManager) Brk(ctx context.Context, addr usermem.Addr) (usermem.Ad return mm.brk.End, syserror.EINVAL } - // TODO: This enforces RLIMIT_DATA, but is slightly more - // permissive than the usual data limit. In particular, this only - // limits the size of the heap; a true RLIMIT_DATA limits the size of - // heap + data + bss. The segment sizes need to be plumbed from the - // loader package to fully enforce RLIMIT_DATA. + // TODO: This enforces RLIMIT_DATA, but is + // slightly more permissive than the usual data limit. In particular, + // this only limits the size of the heap; a true RLIMIT_DATA limits the + // size of heap + data + bss. The segment sizes need to be plumbed from + // the loader package to fully enforce RLIMIT_DATA. if uint64(addr-mm.brk.Start) > limits.FromContext(ctx).Get(limits.Data).Cur { mm.mappingMu.Unlock() return mm.brk.End, syserror.ENOMEM diff --git a/pkg/sentry/strace/strace.go b/pkg/sentry/strace/strace.go index 6c93d7de7..a7e9df268 100644 --- a/pkg/sentry/strace/strace.go +++ b/pkg/sentry/strace/strace.go @@ -686,8 +686,8 @@ func (s SyscallMap) Name(sysno uintptr) string { // N.B. This is not in an init function because we can't be sure all syscall // tables are registered with the kernel when init runs. // -// TODO: remove kernel package dependencies from this package and -// have the kernel package self-initialize all syscall tables. +// TODO: remove kernel package dependencies from this +// package and have the kernel package self-initialize all syscall tables. func Initialize() { for _, table := range kernel.SyscallTables() { // Is this known? diff --git a/pkg/sentry/syscalls/linux/error.go b/pkg/sentry/syscalls/linux/error.go index e86bed313..8759e5e32 100644 --- a/pkg/sentry/syscalls/linux/error.go +++ b/pkg/sentry/syscalls/linux/error.go @@ -89,8 +89,8 @@ func handleIOError(t *kernel.Task, partialResult bool, err, intr error, op strin // side is gone. The partial write is returned. EPIPE will be // returned on the next call. // - // TODO: In some cases SIGPIPE should also be sent - // to the application. + // TODO: In some cases SIGPIPE should + // also be sent to the application. return nil case syserror.ErrWouldBlock: // Syscall would block, but completed a partial read/write. diff --git a/test/syscalls/linux/32bit.cc b/test/syscalls/linux/32bit.cc index b8d5f0355..230648c9b 100644 --- a/test/syscalls/linux/32bit.cc +++ b/test/syscalls/linux/32bit.cc @@ -84,8 +84,8 @@ TEST(Syscall32Bit, Int80) { // disabled). return; case Platform::kPtrace: - // TODO: The ptrace platform does not have a consistent story - // here. + // TODO: The ptrace platform does not have a + // consistent story here. return; case Platform::kNative: break; diff --git a/test/syscalls/linux/exec_binary.cc b/test/syscalls/linux/exec_binary.cc index cfc898699..cb5c7ae51 100644 --- a/test/syscalls/linux/exec_binary.cc +++ b/test/syscalls/linux/exec_binary.cc @@ -285,9 +285,9 @@ ElfBinary<64> StandardElf() { elf.header.e_phoff = sizeof(elf.header); elf.header.e_phentsize = sizeof(decltype(elf)::ElfPhdr); - // TODO: Always include a PT_GNU_STACK segment to disable - // executable stacks. With this omitted the stack (and all PROT_READ) mappings - // should be executable, but gVisor doesn't support that. + // TODO: Always include a PT_GNU_STACK segment to + // disable executable stacks. With this omitted the stack (and all PROT_READ) + // mappings should be executable, but gVisor doesn't support that. decltype(elf)::ElfPhdr phdr = {}; phdr.p_type = PT_GNU_STACK; phdr.p_flags = PF_R | PF_W; @@ -1005,8 +1005,8 @@ TEST(ElfTest, NoExecute) { // Execute, but no read permissions on the binary works just fine. TEST(ElfTest, NoRead) { - // TODO: gVisor's backing filesystem may prevent the sentry from - // reading the executable. + // TODO: gVisor's backing filesystem may prevent the + // sentry from reading the executable. SKIP_IF(IsRunningOnGvisor()); ElfBinary<64> elf = StandardElf(); diff --git a/vdso/vdso.lds b/vdso/vdso.lds index 97bb6d0c1..166779931 100644 --- a/vdso/vdso.lds +++ b/vdso/vdso.lds @@ -56,8 +56,8 @@ SECTIONS { .altinstr_replacement : { *(.altinstr_replacement) } /* - * TODO: Remove this alignment? Then the VDSO would fit in a - * single page. + * TODO: Remove this alignment? Then the VDSO would fit + * in a single page. */ . = ALIGN(0x1000); .text : { *(.text*) } :text =0x90909090 -- cgit v1.2.3