From 8b314b0bf402da58f90ccaac852a880d375f0885 Mon Sep 17 00:00:00 2001
From: Fabricio Voznika <fvoznika@google.com>
Date: Tue, 20 Nov 2018 15:07:12 -0800
Subject: Fix recursive read lock taken on TaskSet

SyncSyscallFiltersToThreadGroup and Task.TheadID() both acquired TaskSet RWLock
in R mode and could deadlock if a writer comes in between.

PiperOrigin-RevId: 222313551
Change-Id: I4221057d8d46fec544cbfa55765c9a284fe7ebfa
---
 pkg/sentry/kernel/seccomp.go             | 48 +++++++++++---------------------
 pkg/sentry/kernel/task.go                |  2 +-
 pkg/sentry/syscalls/linux/sys_seccomp.go |  7 +----
 3 files changed, 19 insertions(+), 38 deletions(-)

diff --git a/pkg/sentry/kernel/seccomp.go b/pkg/sentry/kernel/seccomp.go
index 37dd3e4c9..433b900c7 100644
--- a/pkg/sentry/kernel/seccomp.go
+++ b/pkg/sentry/kernel/seccomp.go
@@ -179,20 +179,19 @@ func (t *Task) evaluateSyscallFilters(sysno int32, args arch.SyscallArguments, i
 // AppendSyscallFilter adds BPF program p as a system call filter.
 //
 // Preconditions: The caller must be running on the task goroutine.
-func (t *Task) AppendSyscallFilter(p bpf.Program) error {
+func (t *Task) AppendSyscallFilter(p bpf.Program, syncAll bool) error {
+	// While syscallFilters are an atomic.Value we must take the mutex to prevent
+	// our read-copy-update from happening while another task is syncing syscall
+	// filters to us, this keeps the filters in a consistent state.
+	t.tg.signalHandlers.mu.Lock()
+	defer t.tg.signalHandlers.mu.Unlock()
+
 	// Cap the combined length of all syscall filters (plus a penalty of 4
-	// instructions per filter beyond the first) to
-	// maxSyscallFilterInstructions. (This restriction is inherited from
-	// Linux.)
+	// instructions per filter beyond the first) to maxSyscallFilterInstructions.
+	// This restriction is inherited from Linux.
 	totalLength := p.Length()
 	var newFilters []bpf.Program
 
-	// While syscallFilters are an atomic.Value we must take the mutex to
-	// prevent our read-copy-update from happening while another task
-	// is syncing syscall filters to us, this keeps the filters in a
-	// consistent state.
-	t.mu.Lock()
-	defer t.mu.Unlock()
 	if sf := t.syscallFilters.Load(); sf != nil {
 		oldFilters := sf.([]bpf.Program)
 		for _, f := range oldFilters {
@@ -207,31 +206,18 @@ func (t *Task) AppendSyscallFilter(p bpf.Program) error {
 
 	newFilters = append(newFilters, p)
 	t.syscallFilters.Store(newFilters)
-	return nil
-}
 
-// SyncSyscallFiltersToThreadGroup will copy this task's filters to all other
-// threads in our thread group.
-func (t *Task) SyncSyscallFiltersToThreadGroup() error {
-	f := t.syscallFilters.Load()
-
-	t.tg.pidns.owner.mu.RLock()
-	defer t.tg.pidns.owner.mu.RUnlock()
-
-	// Note: No new privs is always assumed to be set.
-	for ot := t.tg.tasks.Front(); ot != nil; ot = ot.Next() {
-		if ot.ThreadID() != t.ThreadID() {
-			// We must take the other task's mutex to prevent it from
-			// appending to its own syscall filters while we're syncing.
-			ot.mu.Lock()
-			var copiedFilters []bpf.Program
-			if f != nil {
-				copiedFilters = append(copiedFilters, f.([]bpf.Program)...)
+	if syncAll {
+		// Note: No new privs is always assumed to be set.
+		for ot := t.tg.tasks.Front(); ot != nil; ot = ot.Next() {
+			if ot != t {
+				var copiedFilters []bpf.Program
+				copiedFilters = append(copiedFilters, newFilters...)
+				ot.syscallFilters.Store(copiedFilters)
 			}
-			ot.syscallFilters.Store(copiedFilters)
-			ot.mu.Unlock()
 		}
 	}
+
 	return nil
 }
 
diff --git a/pkg/sentry/kernel/task.go b/pkg/sentry/kernel/task.go
index 73ba8bee9..2982bc5d1 100644
--- a/pkg/sentry/kernel/task.go
+++ b/pkg/sentry/kernel/task.go
@@ -392,7 +392,7 @@ type Task struct {
 
 	// syscallFilters is all seccomp-bpf syscall filters applicable to the
 	// task, in the order in which they were installed. The type of the atomic
-	// is []bpf.Program. Writing needs to be protected by mu.
+	// is []bpf.Program. Writing needs to be protected by the signal mutex.
 	//
 	// syscallFilters is owned by the task goroutine.
 	syscallFilters atomic.Value `state:".([]bpf.Program)"`
diff --git a/pkg/sentry/syscalls/linux/sys_seccomp.go b/pkg/sentry/syscalls/linux/sys_seccomp.go
index 969acaa36..f08fdf5cb 100644
--- a/pkg/sentry/syscalls/linux/sys_seccomp.go
+++ b/pkg/sentry/syscalls/linux/sys_seccomp.go
@@ -68,12 +68,7 @@ func seccomp(t *kernel.Task, mode, flags uint64, addr usermem.Addr) error {
 		return syscall.EINVAL
 	}
 
-	err = t.AppendSyscallFilter(compiledFilter)
-	if err == nil && tsync {
-		// Now we must copy this seccomp program to all other threads.
-		err = t.SyncSyscallFiltersToThreadGroup()
-	}
-	return err
+	return t.AppendSyscallFilter(compiledFilter, tsync)
 }
 
 // Seccomp implements linux syscall seccomp(2).
-- 
cgit v1.2.3