Clean up and re-enable process_vm_readv/writev

Some fixes:

* First argument of Task.CopyContext should always be the context.Context
  derived from the currently running task, because it is used to get a
  CopyScratchBuffer, which must be from the current task. This solved a bunch
  of data races.

* Fix logic around which process is remote and which is local. These were
  getting mixed up.

* Always read iovec structs (local and remote) from the local process's address
  space, since they are syscall arguments. Only use the remote process address
  space to read the memory pointed to by the remote iovecs.

* Added ptrace permissions check, per linux.

* Delete unused code from kernel/task_usermem.go

* Rewrote tests so that we read to (write from) a subprocess, rather than the
  other way around. So we don't need CAP_PTRACE to run the tests.

* Also make tests async-signal-safe after call to fork(). I think this was the
  source of the flakyness on linux previously.

PiperOrigin-RevId: 570506366

pkg/sentry/kernel/task_usermem.go

@@ -131,24 +131,18 @@ func (t *Task) CopyInVector(addr hostarch.Addr, maxElemSize, maxTotalSize int) (
 //   - The caller must be running on the task goroutine.
 //   - t's AddressSpace must be active.
 func (t *Task) CopyOutIovecs(addr hostarch.Addr, src hostarch.AddrRangeSeq) error {
-	return copyOutIovecs(t, t, addr, src)
-}
-
-// copyOutIovecs converts src to an array of struct iovecs and copies it to the
-// memory mapped at addr.
-func copyOutIovecs(ctx marshal.CopyContext, t *Task, addr hostarch.Addr, src hostarch.AddrRangeSeq) error {
 	switch t.Arch().Width() {
 	case 8:
 		if _, ok := addr.AddLength(uint64(src.NumRanges()) * iovecLength); !ok {
 			return linuxerr.EFAULT
 		}
 
-		b := ctx.CopyScratchBuffer(iovecLength)
+		b := t.CopyScratchBuffer(iovecLength)
 		for ; !src.IsEmpty(); src = src.Tail() {
 			ar := src.Head()
 			hostarch.ByteOrder.PutUint64(b[0:8], uint64(ar.Start))
 			hostarch.ByteOrder.PutUint64(b[8:16], uint64(ar.Length()))
-			if _, err := ctx.CopyOutBytes(addr, b); err != nil {
+			if _, err := t.CopyOutBytes(addr, b); err != nil {
 				return err
 			}
 			addr += iovecLength
@@ -178,6 +172,15 @@ func (t *Task) CopyInIovecs(addr hostarch.Addr, numIovecs int) (hostarch.AddrRan
 	return hostarch.AddrRangeSeqFromSlice(iovecs), nil
 }
 
+// CopyInIovecsAsSlice copies in IoVecs and returns them in a slice.
+//
+// Preconditions: Same as usermem.IO.CopyIn, plus:
+//   - The caller must be running on the task goroutine or hold t.mu.
+//   - t's AddressSpace must be active.
+func (t *Task) CopyInIovecsAsSlice(addr hostarch.Addr, numIovecs int) ([]hostarch.AddrRange, error) {
+	return copyInIovecs(t, t, addr, numIovecs)
+}
+
 func copyInIovec(ctx marshal.CopyContext, t *Task, addr hostarch.Addr) (hostarch.AddrRangeSeq, error) {
 	if err := checkArch(t); err != nil {
 		return hostarch.AddrRangeSeq{}, err
@@ -322,10 +325,9 @@ func (t *Task) IovecsIOSequence(addr hostarch.Addr, iovcnt int, opts usermem.IOO
 }
 
 type taskCopyContext struct {
-	ctx                context.Context
-	t                  *Task
-	opts               usermem.IOOpts
-	allocateNewBuffers bool
+	ctx  context.Context
+	t    *Task
+	opts usermem.IOOpts
 }
 
 // CopyContext returns a marshal.CopyContext that copies to/from t's address
@@ -340,7 +342,7 @@ func (t *Task) CopyContext(ctx context.Context, opts usermem.IOOpts) *taskCopyCo
 
 // CopyScratchBuffer implements marshal.CopyContext.CopyScratchBuffer.
 func (cc *taskCopyContext) CopyScratchBuffer(size int) []byte {
-	if ctxTask, ok := cc.ctx.(*Task); ok && !cc.allocateNewBuffers {
+	if ctxTask, ok := cc.ctx.(*Task); ok {
 		return ctxTask.CopyScratchBuffer(size)
 	}
 	return make([]byte, size)
@@ -357,13 +359,6 @@ func (cc *taskCopyContext) getMemoryManager() (*mm.MemoryManager, error) {
 	return tmm, nil
 }
 
-// WithTaskMutexLocked runs the given function with the task's mutex locked.
-func (cc *taskCopyContext) WithTaskMutexLocked(fn func() error) error {
-	cc.t.mu.Lock()
-	defer cc.t.mu.Unlock()
-	return fn()
-}
-
 // CopyInBytes implements marshal.CopyContext.CopyInBytes.
 //
 // Preconditions: Same as usermem.IO.CopyIn, plus:
@@ -392,25 +387,6 @@ func (cc *taskCopyContext) CopyOutBytes(addr hostarch.Addr, src []byte) (int, er
 	return tmm.CopyOut(cc.ctx, addr, src, cc.opts)
 }
 
-// CopyOutIovecs converts src to an array of struct iovecs and copies it to the
-// memory mapped at addr for Task.
-//
-// Preconditions: Same as usermem.IO.CopyOut, plus:
-//   - The caller must be running on the task goroutine or hold the cc.t.mu
-//   - t's AddressSpace must be active.
-func (cc *taskCopyContext) CopyOutIovecs(addr hostarch.Addr, src hostarch.AddrRangeSeq) error {
-	return copyOutIovecs(cc, cc.t, addr, src)
-}
-
-// CopyInIovecs copies in IoVecs for taskCopyContext.
-//
-// Preconditions: Same as usermem.IO.CopyIn, plus:
-//   - The caller must be running on the task goroutine or hold the cc.t.mu
-//   - t's AddressSpace must be active.
-func (cc *taskCopyContext) CopyInIovecs(addr hostarch.Addr, numIovecs int) ([]hostarch.AddrRange, error) {
-	return copyInIovecs(cc, cc.t, addr, numIovecs)
-}
-
 type ownTaskCopyContext struct {
 	t    *Task
 	opts usermem.IOOpts

pkg/sentry/strace/strace.go

@@ -392,7 +392,7 @@ func (i *SyscallInfo) pre(t *kernel.Task, args arch.SyscallArguments, maximumBlo
 			output = append(output, dump(t, args[arg].Pointer(), args[arg+1].SizeT(), maximumBlobSize, LogAppDataAllowed /* content */))
 		case WriteIOVec:
 			output = append(output, iovecs(t, args[arg].Pointer(), int(args[arg+1].Int()), LogAppDataAllowed /* content */, uint64(maximumBlobSize)))
-		case IOVec:
+		case ReadIOVec, IOVec:
 			output = append(output, iovecs(t, args[arg].Pointer(), int(args[arg+1].Int()), false /* content */, uint64(maximumBlobSize)))
 		case SendMsgHdr:
 			output = append(output, msghdr(t, args[arg].Pointer(), LogAppDataAllowed /* content */, uint64(maximumBlobSize)))

pkg/sentry/syscalls/linux/linux64.go

@@ -362,8 +362,8 @@ var AMD64 = &kernel.SyscallTable{
 		307: syscalls.Supported("sendmmsg", SendMMsg),
 		308: syscalls.Supported("setns", Setns),
 		309: syscalls.Supported("getcpu", Getcpu),
-		310: syscalls.ErrorWithEvent("process_vm_readv", linuxerr.ENOSYS, "", []string{"gvisor.dev/issue/158"}),  // TODO(b/260724654)
-		311: syscalls.ErrorWithEvent("process_vm_writev", linuxerr.ENOSYS, "", []string{"gvisor.dev/issue/158"}), // TODO(b/260724654)
+		310: syscalls.Supported("process_vm_readv", ProcessVMReadv),
+		311: syscalls.Supported("process_vm_writev", ProcessVMWritev),
 		312: syscalls.CapError("kcmp", linux.CAP_SYS_PTRACE, "", nil),
 		313: syscalls.CapError("finit_module", linux.CAP_SYS_MODULE, "", nil),
 		314: syscalls.ErrorWithEvent("sched_setattr", linuxerr.ENOSYS, "gVisor does not implement a scheduler.", []string{"gvisor.dev/issue/264"}), // TODO(b/118902272)
@@ -685,8 +685,8 @@ var ARM64 = &kernel.SyscallTable{
 		267: syscalls.Supported("syncfs", Syncfs),
 		268: syscalls.Supported("setns", Setns),
 		269: syscalls.Supported("sendmmsg", SendMMsg),
-		270: syscalls.ErrorWithEvent("process_vm_readv", linuxerr.ENOSYS, "", []string{"gvisor.dev/issue/158"}),  // TODO(b/260724654)
-		271: syscalls.ErrorWithEvent("process_vm_writev", linuxerr.ENOSYS, "", []string{"gvisor.dev/issue/158"}), // TODO(b/260724654)
+		270: syscalls.Supported("process_vm_readv", ProcessVMReadv),
+		271: syscalls.Supported("process_vm_writev", ProcessVMWritev),
 		272: syscalls.CapError("kcmp", linux.CAP_SYS_PTRACE, "", nil),
 		273: syscalls.CapError("finit_module", linux.CAP_SYS_MODULE, "", nil),
 		274: syscalls.ErrorWithEvent("sched_setattr", linuxerr.ENOSYS, "gVisor does not implement a scheduler.", []string{"gvisor.dev/issue/264"}), // TODO(b/118902272)

pkg/sentry/syscalls/linux/sys_process_vm.go

@@ -15,33 +15,35 @@
 package linux
 
 import (
+	"fmt"
+
 	"gvisor.dev/gvisor/pkg/abi/linux"
 	"gvisor.dev/gvisor/pkg/errors/linuxerr"
 	"gvisor.dev/gvisor/pkg/hostarch"
+	"gvisor.dev/gvisor/pkg/marshal"
 	"gvisor.dev/gvisor/pkg/sentry/arch"
 	"gvisor.dev/gvisor/pkg/sentry/kernel"
 	"gvisor.dev/gvisor/pkg/usermem"
 )
 
-type vmReadWriteOp int
+type processVMOpType int
 
 const (
-	localReadLocalWrite vmReadWriteOp = iota
-	remoteReadLocalWrite
-	localReadRemoteWrite
+	processVMOpRead = iota
+	processVMOpWrite
 )
 
 // ProcessVMReadv implements process_vm_readv(2).
 func ProcessVMReadv(t *kernel.Task, sysno uintptr, args arch.SyscallArguments) (uintptr, *kernel.SyscallControl, error) {
-	return processVMRW(t, args, false /*isWrite*/)
+	return processVMOp(t, args, processVMOpRead)
 }
 
 // ProcessVMWritev implements process_vm_writev(2).
 func ProcessVMWritev(t *kernel.Task, sysno uintptr, args arch.SyscallArguments) (uintptr, *kernel.SyscallControl, error) {
-	return processVMRW(t, args, true /*isWrite*/)
+	return processVMOp(t, args, processVMOpWrite)
 }
 
-func processVMRW(t *kernel.Task, args arch.SyscallArguments, isWrite bool) (uintptr, *kernel.SyscallControl, error) {
+func processVMOp(t *kernel.Task, args arch.SyscallArguments, op processVMOpType) (uintptr, *kernel.SyscallControl, error) {
 	pid := kernel.ThreadID(args[0].Int())
 	lvec := hostarch.Addr(args[1].Pointer())
 	liovcnt := int(args[2].Int64())
@@ -49,6 +51,7 @@ func processVMRW(t *kernel.Task, args arch.SyscallArguments, isWrite bool) (uint
 	riovcnt := int(args[4].Int64())
 	flags := args[5].Int()
 
+	// Parse the flags.
 	switch {
 	case flags != 0 ||
 		liovcnt < 0 ||
@@ -56,132 +59,159 @@ func processVMRW(t *kernel.Task, args arch.SyscallArguments, isWrite bool) (uint
 		liovcnt > linux.UIO_MAXIOV ||
 		riovcnt > linux.UIO_MAXIOV:
 		return 0, nil, linuxerr.EINVAL
-	case lvec == 0 || rvec == 0:
-		return 0, nil, linuxerr.EFAULT
 	case liovcnt == 0 || riovcnt == 0:
 		return 0, nil, nil
+	case lvec == 0 || rvec == 0:
+		return 0, nil, linuxerr.EFAULT
 	}
 
-	localProcess := t.ThreadGroup().Leader()
-	if localProcess == nil {
+	// Determine local and remote processes.
+	// Local process is always the caller.
+	localTask := t.ThreadGroup().Leader()
+	if localTask == nil {
 		return 0, nil, linuxerr.ESRCH
 	}
-	remoteThreadGroup := localProcess.PIDNamespace().ThreadGroupWithID(pid)
+	// Remote process is the pid specified in the syscall arguments. It is
+	// allowed to be the same as the caller process.
+	remoteThreadGroup := localTask.PIDNamespace().ThreadGroupWithID(pid)
 	if remoteThreadGroup == nil {
 		return 0, nil, linuxerr.ESRCH
 	}
-	remoteProcess := remoteThreadGroup.Leader()
+	remoteTask := remoteThreadGroup.Leader()
+	if remoteTask.ExitState() >= kernel.TaskExitInitiated {
+		return 0, nil, linuxerr.ESRCH
+	}
 
-	isRemote := localProcess == remoteProcess
+	// man 2 process_vm_read: "Permission to read from or write to another
+	// process is governed by a ptrace access mode
+	// PTRACE_MODE_ATTACH_REALCREDS check; see ptrace(2)."
+	if !localTask.CanTrace(remoteTask, true /* attach */) {
+		return 0, nil, linuxerr.EPERM
+	}
 
-	// For the write case, we read from the local process and write to the remote process.
-	op := localReadLocalWrite
-	if isWrite {
-		if isRemote {
-			op = remoteReadLocalWrite
+	// Figure out which processes and arguments (local or remote) are for
+	// writing and which are for reading, based on the operation.
+	var opArgs processVMOpArgs
+	switch op {
+	case processVMOpRead:
+		// Read from remote process and write into local.
+		opArgs = processVMOpArgs{
+			readCtx:         remoteTask.CopyContext(t, usermem.IOOpts{}),
+			readAddr:        rvec,
+			readIovecCount:  riovcnt,
+			writeCtx:        localTask.CopyContext(t, usermem.IOOpts{AddressSpaceActive: true}),
+			writeAddr:       lvec,
+			writeIovecCount: liovcnt,
 		}
-		return doProcessVMReadWrite(localProcess, remoteProcess, lvec, rvec, liovcnt, riovcnt, op)
+	case processVMOpWrite:
+		// Read from local process and write into remote.
+		opArgs = processVMOpArgs{
+			readCtx:         localTask.CopyContext(t, usermem.IOOpts{AddressSpaceActive: true}),
+			readAddr:        lvec,
+			readIovecCount:  liovcnt,
+			writeCtx:        remoteTask.CopyContext(t, usermem.IOOpts{}),
+			writeAddr:       rvec,
+			writeIovecCount: riovcnt,
+		}
+	default:
+		panic(fmt.Sprintf("unknown process vm op type: %v", op))
 	}
-	// For the read case, we read from the remote process and write to the local process.
-	if isRemote {
-		op = localReadRemoteWrite
+
+	var (
+		n   int
+		err error
+	)
+	if localTask == remoteTask {
+		// No need to lock remote process's task mutex since it is the
+		// same as this process.
+		n, err = doProcessVMOpMaybeLocked(t, opArgs)
+	} else {
+		// Need to take remote process's task mutex.
+		remoteTask.WithMuLocked(func(*kernel.Task) {
+			n, err = doProcessVMOpMaybeLocked(t, opArgs)
+		})
 	}
-	return doProcessVMReadWrite(remoteProcess, localProcess, rvec, lvec, riovcnt, liovcnt, op)
+	if err != nil {
+		return 0, nil, err
+	}
+	return uintptr(n), nil, nil
 }
 
-func doProcessVMReadWrite(rProcess, wProcess *kernel.Task, rAddr, wAddr hostarch.Addr, rIovecCount, wIovecCount int, op vmReadWriteOp) (uintptr, *kernel.SyscallControl, error) {
-	rCtx := rProcess.CopyContext(rProcess, usermem.IOOpts{})
-	wCtx := wProcess.CopyContext(wProcess, usermem.IOOpts{})
+type processVMOpArgs struct {
+	readCtx         marshal.CopyContext
+	readAddr        hostarch.Addr
+	readIovecCount  int
+	writeCtx        marshal.CopyContext
+	writeAddr       hostarch.Addr
+	writeIovecCount int
+}
 
-	var wCount int
-	doProcessVMReadWriteMaybeLocked := func() error {
-		rIovecs, err := rCtx.CopyInIovecs(rAddr, rIovecCount)
+func doProcessVMOpMaybeLocked(t *kernel.Task, args processVMOpArgs) (int, error) {
+	// Copy IOVecs in to kernel.
+	readIovecs, err := t.CopyInIovecsAsSlice(args.readAddr, args.readIovecCount)
+	if err != nil {
+		return 0, err
+	}
+	writeIovecs, err := t.CopyInIovecsAsSlice(args.writeAddr, args.writeIovecCount)
+	if err != nil {
+		return 0, err
+	}
+
+	// Get scratch buffer from the calling task.
+	// Size should be max be size of largest read iovec.
+	var bufSize int
+	for _, readIovec := range readIovecs {
+		if int(readIovec.Length()) > bufSize {
+			bufSize = int(readIovec.Length())
+		}
+	}
+	buf := t.CopyScratchBuffer(bufSize)
+
+	// Number of bytes written.
+	var n int
+	for _, readIovec := range readIovecs {
+		if len(writeIovecs) == 0 {
+			break
+		}
+
+		buf = buf[0:int(readIovec.Length())]
+		bytes, err := args.readCtx.CopyInBytes(readIovec.Start, buf)
+		if linuxerr.Equals(linuxerr.EFAULT, err) {
+			return n, nil
+		}
 		if err != nil {
-			return err
+			return n, err
 		}
-		wIovecs, err := wCtx.CopyInIovecs(wAddr, wIovecCount)
-		if err != nil {
-			return err
+		if bytes != int(readIovec.Length()) {
+			return n, nil
 		}
 
-		bufSize := 0
-		for _, rIovec := range rIovecs {
-			if int(rIovec.Length()) > bufSize {
-				bufSize = int(rIovec.Length())
+		start := 0
+		for bytes > start && 0 < len(writeIovecs) {
+			writeLength := int(writeIovecs[0].Length())
+			if writeLength > (bytes - start) {
+				writeLength = bytes - start
 			}
-		}
-
-		var buf []byte
-		// We need to copy the called task's scratch buffer so we don't get a data race. If we are
-		// reading a remote process's memory, then we are on the writer's task goroutine, so use
-		// the write context's scratch buffer.
-		if op == remoteReadLocalWrite {
-			buf = wCtx.CopyScratchBuffer(bufSize)
-		} else {
-			buf = rCtx.CopyScratchBuffer(bufSize)
-		}
-
-		for _, rIovec := range rIovecs {
-			if len(wIovecs) <= 0 {
-				break
-			}
-
-			buf = buf[0:int(rIovec.Length())]
-			bytes, err := rCtx.CopyInBytes(rIovec.Start, buf)
+			out, err := args.writeCtx.CopyOutBytes(writeIovecs[0].Start, buf[start:writeLength+start])
+			n += out
+			start += out
 			if linuxerr.Equals(linuxerr.EFAULT, err) {
-				return nil
+				return n, nil
 			}
 			if err != nil {
-				return err
+				return n, err
 			}
-			if bytes != int(rIovec.Length()) {
-				return nil
+			if out != writeLength {
+				return n, nil
 			}
-			start := 0
-			for bytes > start && 0 < len(wIovecs) {
-				writeLength := int(wIovecs[0].Length())
-				if writeLength > (bytes - start) {
-					writeLength = bytes - start
-				}
-				out, err := wCtx.CopyOutBytes(wIovecs[0].Start, buf[start:writeLength+start])
-				wCount += out
-				start += out
-				if linuxerr.Equals(linuxerr.EFAULT, err) {
-					return nil
-				}
-				if err != nil {
-					return err
-				}
-				if out != writeLength {
-					return nil
-				}
-				wIovecs[0].Start += hostarch.Addr(out)
-				if !wIovecs[0].WellFormed() {
-					return err
-				}
-				if wIovecs[0].Length() == 0 {
-					wIovecs = wIovecs[1:]
-				}
+			writeIovecs[0].Start += hostarch.Addr(out)
+			if !writeIovecs[0].WellFormed() {
+				return n, err
+			}
+			if writeIovecs[0].Length() == 0 {
+				writeIovecs = writeIovecs[1:]
 			}
 		}
-		return nil
 	}
-
-	var err error
-
-	switch op {
-	case remoteReadLocalWrite:
-		err = rCtx.WithTaskMutexLocked(doProcessVMReadWriteMaybeLocked)
-	case localReadRemoteWrite:
-		err = wCtx.WithTaskMutexLocked(doProcessVMReadWriteMaybeLocked)
-
-	case localReadLocalWrite:
-		// in the case of local reads/writes, we don't have to lock the task mutex, because we are
-		// running on the top of the task's goroutine already.
-		err = doProcessVMReadWriteMaybeLocked()
-	default:
-		panic("unsupported operation passed")
-	}
-
-	return uintptr(wCount), nil, err
+	return n, nil
 }

test/syscalls/BUILD

@@ -1,5 +1,5 @@
-load("//tools:defs.bzl", "more_shards", "most_shards")
 load("//test/runner:defs.bzl", "syscall_test")
+load("//tools:defs.bzl", "more_shards", "most_shards")
 
 package(
     default_applicable_licenses = ["//:license"],
@@ -1189,6 +1189,10 @@ syscall_test(
     test = "//test/syscalls/linux:processes_test",
 )
 
+syscall_test(
+    test = "//test/syscalls/linux:process_vm_read_write_test",
+)
+
 syscall_test(
     test = "//test/syscalls/linux:deleted_test",
 )

test/syscalls/linux/BUILD

@@ -4632,18 +4632,17 @@ cc_binary(
 )
 
 cc_binary(
-    name = "process_vm_read_write",
+    name = "process_vm_read_write_test",
     testonly = 1,
     srcs = ["process_vm_read_write.cc"],
     linkstatic = 1,
     deps = [
         gtest,
-        "//test/util:capability_util",
         "//test/util:logging",
-        "//test/util:multiprocess_util",
         "//test/util:posix_error",
         "//test/util:test_main",
         "//test/util:test_util",
+        "@com_google_absl//absl/cleanup",
         "@com_google_absl//absl/strings",
     ],
 )