Skip to content

Commit

Permalink
seccomp: Implement non-negative FD check as a ValueMatcher.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@EtiennePerot @gvisor-bot
EtiennePerot authored and gvisor-bot committed Nov 1, 2023
1 parent 260873b commit 5f75371
Show file tree
Hide file tree
Showing 6 changed files with 146 additions and 69 deletions.
7 changes: 0 additions & 7 deletions pkg/seccomp/seccomp.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
25 changes: 25 additions & 0 deletions pkg/seccomp/seccomp_rules.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
62 changes: 62 additions & 0 deletions pkg/seccomp/seccomp_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
13 changes: 6 additions & 7 deletions pkg/sentry/devices/accel/seccomp_filters.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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),
},
},
Expand Down
75 changes: 37 additions & 38 deletions pkg/sentry/devices/nvproxy/seccomp_filters.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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),
},
},
Expand Down
Loading

0 comments on commit 5f75371

Please sign in to comment.