seccomp: Implement non-negative FD check as a ValueMatcher.

This uses fewer (6 -> 5) and simpler instructions (equality and AND),
and avoids having a helper function returning a different matcher type.

Also added tests for it.

PiperOrigin-RevId: 578365467

pkg/seccomp/seccomp.go

@@ -38,13 +38,6 @@ const (
 	vsyscallPageIPMask = 1 << 31
 )
 
-// NonNegativeFDCheck ensures an FD argument is a non-negative int.
-func NonNegativeFDCheck() LessThanOrEqual {
-	// Negative int32 has the MSB (31st bit) set. So the raw uint FD value must
-	// be less than or equal to 0x7fffffff.
-	return LessThanOrEqual(0x7fffffff)
-}
-
 // Install generates BPF code based on the set of syscalls provided. It only
 // allows syscalls that conform to the specification. Syscalls that violate the
 // specification will trigger RET_KILL_PROCESS. If RET_KILL_PROCESS is not

pkg/seccomp/seccomp_rules.go

@@ -267,6 +267,31 @@ func (le LessThanOrEqual) Render(program *syscallProgram, labelSet *labelSet, va
 	program.JumpTo(labelSet.Matched())
 }
 
+// NonNegativeFD ensures that an FD argument is a non-negative int32.
+type NonNegativeFD struct{}
+
+// String implements `ValueMatcher.String`.
+func (NonNegativeFD) String() string {
+	return fmt.Sprintf("NonNegativeFD")
+}
+
+// Repr implements `ValueMatcher.Repr`.
+func (NonNegativeFD) Repr() string {
+	return NonNegativeFD{}.String()
+}
+
+// Render implements `ValueMatcher.Render`.
+func (NonNegativeFD) Render(program *syscallProgram, labelSet *labelSet, value matchedValue) {
+	// FDs are 32 bits, so the high 32 bits must all be zero.
+	value.LoadHigh32Bits()
+	program.IfNot(bpf.Jmp|bpf.Jeq|bpf.K, 0, labelSet.Mismatched())
+	// Negative int32 has the MSB (31st bit) set.
+	// So the raw uint FD value must not have the 31st bit set.
+	value.LoadLow32Bits()
+	program.If(bpf.Jmp|bpf.Jset|bpf.K, 1<<31, labelSet.Mismatched())
+	program.JumpTo(labelSet.Matched())
+}
+
 // MaskedEqual specifies a value that matches the input after the input is
 // masked (bitwise &) against the given mask. It implements `ValueMatcher`.
 type maskedEqual struct {

pkg/seccomp/seccomp_test.go

@@ -901,6 +901,68 @@ func TestBasic(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "NonNegativeFD",
+			ruleSets: []RuleSet{
+				{
+					Rules: MakeSyscallRules(map[uintptr]SyscallRule{
+						1: PerArg{
+							NonNegativeFD{},
+						},
+					}),
+					Action: linux.SECCOMP_RET_ALLOW,
+				},
+			},
+			defaultAction: linux.SECCOMP_RET_TRAP,
+			badArchAction: linux.SECCOMP_RET_KILL_THREAD,
+			specs: []spec{
+				{
+					desc: "zero allowed",
+					data: linux.SeccompData{Nr: 1, Arch: LINUX_AUDIT_ARCH, Args: [6]uint64{0x0}},
+					want: linux.SECCOMP_RET_ALLOW,
+				},
+				{
+					desc: "one allowed",
+					data: linux.SeccompData{Nr: 1, Arch: LINUX_AUDIT_ARCH, Args: [6]uint64{0x0}},
+					want: linux.SECCOMP_RET_ALLOW,
+				},
+				{
+					desc: "seven allowed",
+					data: linux.SeccompData{Nr: 1, Arch: LINUX_AUDIT_ARCH, Args: [6]uint64{0x7}},
+					want: linux.SECCOMP_RET_ALLOW,
+				},
+				{
+					desc: "largest int32 allowed",
+					data: linux.SeccompData{Nr: 1, Arch: LINUX_AUDIT_ARCH, Args: [6]uint64{0x7fffffff}},
+					want: linux.SECCOMP_RET_ALLOW,
+				},
+				{
+					desc: "negative 1 not allowed",
+					data: linux.SeccompData{Nr: 1, Arch: LINUX_AUDIT_ARCH, Args: [6]uint64{0x80000000}},
+					want: linux.SECCOMP_RET_TRAP,
+				},
+				{
+					desc: "largest uint32 not allowed",
+					data: linux.SeccompData{Nr: 1, Arch: LINUX_AUDIT_ARCH, Args: [6]uint64{0xffffffff}},
+					want: linux.SECCOMP_RET_TRAP,
+				},
+				{
+					desc: "a positive int64 larger than max uint32 is not allowed",
+					data: linux.SeccompData{Nr: 1, Arch: LINUX_AUDIT_ARCH, Args: [6]uint64{0x100000000}},
+					want: linux.SECCOMP_RET_TRAP,
+				},
+				{
+					desc: "largest int64 not allowed",
+					data: linux.SeccompData{Nr: 1, Arch: LINUX_AUDIT_ARCH, Args: [6]uint64{0x7fffffffffffffff}},
+					want: linux.SECCOMP_RET_TRAP,
+				},
+				{
+					desc: "largest uint64 not allowed",
+					data: linux.SeccompData{Nr: 1, Arch: LINUX_AUDIT_ARCH, Args: [6]uint64{0xffffffffffffffff}},
+					want: linux.SECCOMP_RET_TRAP,
+				},
+			},
+		},
 		{
 			name: "Instruction Pointer",
 			ruleSets: []RuleSet{

pkg/sentry/devices/accel/seccomp_filters.go

@@ -23,7 +23,6 @@ import (
 
 // Filters returns seccomp-bpf filters for this package.
 func Filters() seccomp.SyscallRules {
-	nonNegativeFD := seccomp.NonNegativeFDCheck()
 	return seccomp.MakeSyscallRules(map[uintptr]seccomp.SyscallRule{
 		unix.SYS_OPENAT: seccomp.PerArg{
 			// All paths that we openat() are absolute, so we pass a dirfd
@@ -38,27 +37,27 @@ func Filters() seccomp.SyscallRules {
 		unix.SYS_GETDENTS64: seccomp.MatchAll{},
 		unix.SYS_IOCTL: seccomp.Or{
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(gasket.GASKET_IOCTL_RESET),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(gasket.GASKET_IOCTL_MAP_BUFFER),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(gasket.GASKET_IOCTL_UNMAP_BUFFER),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(gasket.GASKET_IOCTL_CLEAR_INTERRUPT_COUNTS),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(gasket.GASKET_IOCTL_REGISTER_INTERRUPT),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(gasket.GASKET_IOCTL_UNREGISTER_INTERRUPT),
 			},
 		},

pkg/sentry/devices/nvproxy/seccomp_filters.go

@@ -23,7 +23,6 @@ import (
 
 // Filters returns seccomp-bpf filters for this package.
 func Filters() seccomp.SyscallRules {
-	nonNegativeFD := seccomp.NonNegativeFDCheck()
 	notIocSizeMask := ^(((uintptr(1) << linux.IOC_SIZEBITS) - 1) << linux.IOC_SIZESHIFT) // for ioctls taking arbitrary size
 	return seccomp.MakeSyscallRules(map[uintptr]seccomp.SyscallRule{
 		unix.SYS_OPENAT: seccomp.PerArg{
@@ -38,153 +37,153 @@ func Filters() seccomp.SyscallRules {
 		},
 		unix.SYS_IOCTL: seccomp.Or{
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.MaskedEqual(notIocSizeMask, frontendIoctlCmd(nvgpu.NV_ESC_CARD_INFO, 0)),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_CHECK_VERSION_STR, nvgpu.SizeofRMAPIVersion)),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_REGISTER_FD, nvgpu.SizeofIoctlRegisterFD)),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_ALLOC_OS_EVENT, nvgpu.SizeofIoctlAllocOSEvent)),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_FREE_OS_EVENT, nvgpu.SizeofIoctlFreeOSEvent)),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_SYS_PARAMS, nvgpu.SizeofIoctlSysParams)),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_RM_ALLOC_MEMORY, nvgpu.SizeofIoctlNVOS02ParametersWithFD)),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_RM_FREE, nvgpu.SizeofNVOS00Parameters)),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_RM_CONTROL, nvgpu.SizeofNVOS54Parameters)),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_RM_ALLOC, nvgpu.SizeofNVOS21Parameters)),
 			},
 			// Note that we don't need to add one for NVOS21ParametersV535, because
 			// SizeofNVOS21ParametersV535 == SizeofNVOS21Parameters. We test this.
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_RM_ALLOC, nvgpu.SizeofNVOS64Parameters)),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_RM_ALLOC, nvgpu.SizeofNVOS64ParametersV535)),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_RM_DUP_OBJECT, nvgpu.SizeofNVOS55Parameters)),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_RM_SHARE, nvgpu.SizeofNVOS57Parameters)),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_RM_VID_HEAP_CONTROL, nvgpu.SizeofNVOS32Parameters)),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_RM_MAP_MEMORY, nvgpu.SizeofIoctlNVOS33ParametersWithFD)),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_RM_UNMAP_MEMORY, nvgpu.SizeofNVOS34Parameters)),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_RM_UPDATE_DEVICE_MAPPING_INFO, nvgpu.SizeofNVOS56Parameters)),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(nvgpu.UVM_INITIALIZE),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(nvgpu.UVM_MM_INITIALIZE),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(nvgpu.UVM_DEINITIALIZE),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(nvgpu.UVM_CREATE_RANGE_GROUP),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(nvgpu.UVM_DESTROY_RANGE_GROUP),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(nvgpu.UVM_REGISTER_GPU_VASPACE),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(nvgpu.UVM_UNREGISTER_GPU_VASPACE),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(nvgpu.UVM_REGISTER_CHANNEL),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(nvgpu.UVM_UNREGISTER_CHANNEL),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(nvgpu.UVM_MAP_EXTERNAL_ALLOCATION),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(nvgpu.UVM_FREE),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(nvgpu.UVM_REGISTER_GPU),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(nvgpu.UVM_UNREGISTER_GPU),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(nvgpu.UVM_PAGEABLE_MEM_ACCESS),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(nvgpu.UVM_DISABLE_READ_DUPLICATION),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(nvgpu.UVM_MAP_DYNAMIC_PARALLELISM_REGION),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(nvgpu.UVM_ALLOC_SEMAPHORE_POOL),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(nvgpu.UVM_VALIDATE_VA_RANGE),
 			},
 			seccomp.PerArg{
-				nonNegativeFD,
+				seccomp.NonNegativeFD{},
 				seccomp.EqualTo(nvgpu.UVM_CREATE_EXTERNAL_RANGE),
 			},
 		},

runsc/boot/filter/config.go

@@ -340,83 +340,82 @@ func hostFilesystemFilters() seccomp.SyscallRules {
 	// negative FD values (like AT_FDCWD or invalid FD numbers). We try to be as
 	// restrictive as possible because any restriction here improves security. We
 	// don't know what set of arguments will trigger a future vulnerability.
-	validFDCheck := seccomp.NonNegativeFDCheck()
 	return seccomp.MakeSyscallRules(map[uintptr]seccomp.SyscallRule{
 		unix.SYS_FCHOWNAT: seccomp.PerArg{
-			validFDCheck,
+			seccomp.NonNegativeFD{},
 			seccomp.AnyValue{},
 			seccomp.AnyValue{},
 			seccomp.AnyValue{},
 			seccomp.EqualTo(unix.AT_EMPTY_PATH | unix.AT_SYMLINK_NOFOLLOW),
 		},
 		unix.SYS_FCHMODAT: seccomp.PerArg{
-			validFDCheck,
+			seccomp.NonNegativeFD{},
 			seccomp.AnyValue{},
 			seccomp.AnyValue{},
 		},
 		unix.SYS_UNLINKAT: seccomp.PerArg{
-			validFDCheck,
+			seccomp.NonNegativeFD{},
 			seccomp.AnyValue{},
 			seccomp.AnyValue{},
 		},
 		unix.SYS_GETDENTS64: seccomp.PerArg{
-			validFDCheck,
+			seccomp.NonNegativeFD{},
 			seccomp.AnyValue{},
 			seccomp.AnyValue{},
 		},
 		unix.SYS_OPENAT: seccomp.PerArg{
-			validFDCheck,
+			seccomp.NonNegativeFD{},
 			seccomp.AnyValue{},
 			seccomp.MaskedEqual(unix.O_NOFOLLOW, unix.O_NOFOLLOW),
 			seccomp.AnyValue{},
 		},
 		unix.SYS_LINKAT: seccomp.PerArg{
-			validFDCheck,
+			seccomp.NonNegativeFD{},
 			seccomp.AnyValue{},
-			validFDCheck,
+			seccomp.NonNegativeFD{},
 			seccomp.AnyValue{},
 			seccomp.EqualTo(0),
 		},
 		unix.SYS_MKDIRAT: seccomp.PerArg{
-			validFDCheck,
+			seccomp.NonNegativeFD{},
 			seccomp.AnyValue{},
 			seccomp.AnyValue{},
 		},
 		unix.SYS_MKNODAT: seccomp.PerArg{
-			validFDCheck,
+			seccomp.NonNegativeFD{},
 			seccomp.AnyValue{},
 			seccomp.AnyValue{},
 			seccomp.AnyValue{},
 		},
 		unix.SYS_SYMLINKAT: seccomp.PerArg{
 			seccomp.AnyValue{},
-			validFDCheck,
+			seccomp.NonNegativeFD{},
 			seccomp.AnyValue{},
 		},
 		unix.SYS_FSTATFS: seccomp.PerArg{
-			validFDCheck,
+			seccomp.NonNegativeFD{},
 			seccomp.AnyValue{},
 		},
 		unix.SYS_READLINKAT: seccomp.PerArg{
-			validFDCheck,
+			seccomp.NonNegativeFD{},
 			seccomp.AnyValue{},
 			seccomp.AnyValue{},
 			seccomp.AnyValue{},
 		},
 		unix.SYS_UTIMENSAT: seccomp.PerArg{
-			validFDCheck,
+			seccomp.NonNegativeFD{},
 			seccomp.AnyValue{},
 			seccomp.AnyValue{},
 			seccomp.AnyValue{},
 		},
 		unix.SYS_RENAMEAT: seccomp.PerArg{
-			validFDCheck,
+			seccomp.NonNegativeFD{},
 			seccomp.AnyValue{},
-			validFDCheck,
+			seccomp.NonNegativeFD{},
 			seccomp.AnyValue{},
 		},
 		archFstatAtSysNo(): seccomp.PerArg{
-			validFDCheck,
+			seccomp.NonNegativeFD{},
 			seccomp.AnyValue{},
 			seccomp.AnyValue{},
 			seccomp.AnyValue{},