From 98a3c0e4dbb68021e3f7c4cddfb43d4240ee8717 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Wed, 12 May 2021 15:56:14 +1000 Subject: [PATCH] cgroup2: devices: switch to emulator for cgroupv1 parity There were several issues with the previous cgroupv2 devices filter generator implementation, stemming from the previous implementation using a few too many tricks to implement the correct cgroup behaviour (rules were handled in reverse order, with wildcards having particularly special interpretations). As a result, some slightly odd configurations with rules in specific orders could result in incorrect filters being generated. By switching to the emulator which is already used by cgroupv1, we can guarantee that the behaviour of filters in both cgroup versions will be identical, as well as making use of the hardenings in the emulator (not allowing users to add deny rules the kernel will ignore). (Note that because the ordering of the devices emulator rules is deterministic and based on the rule value, the existing test rules had to be reordered slightly.) Signed-off-by: Aleksa Sarai --- .../cgroups/ebpf/devicefilter/devicefilter.go | 106 ++++++++----- .../ebpf/devicefilter/devicefilter_test.go | 144 +++++++++--------- libcontainer/cgroups/fs2/devices.go | 3 - 3 files changed, 139 insertions(+), 114 deletions(-) diff --git a/libcontainer/cgroups/ebpf/devicefilter/devicefilter.go b/libcontainer/cgroups/ebpf/devicefilter/devicefilter.go index fcd3746e0..10e575d26 100644 --- a/libcontainer/cgroups/ebpf/devicefilter/devicefilter.go +++ b/libcontainer/cgroups/ebpf/devicefilter/devicefilter.go @@ -11,6 +11,7 @@ import ( "strconv" "github.com/cilium/ebpf/asm" + devicesemulator "github.com/opencontainers/runc/libcontainer/cgroups/devices" "github.com/opencontainers/runc/libcontainer/devices" "github.com/pkg/errors" "golang.org/x/sys/unix" @@ -22,11 +23,44 @@ const ( ) // DeviceFilter returns eBPF device filter program and its license string -func DeviceFilter(devices []*devices.Rule) (asm.Instructions, string, error) { - p := &program{} +func DeviceFilter(rules []*devices.Rule) (asm.Instructions, string, error) { + // Generate the minimum ruleset for the device rules we are given. While we + // don't care about minimum transitions in cgroupv2, using the emulator + // gives us a guarantee that the behaviour of devices filtering is the same + // as cgroupv1, including security hardenings to avoid misconfiguration + // (such as punching holes in wildcard rules). + emu := new(devicesemulator.Emulator) + for _, rule := range rules { + if err := emu.Apply(*rule); err != nil { + return nil, "", err + } + } + cleanRules, err := emu.Rules() + if err != nil { + return nil, "", err + } + + p := &program{ + defaultAllow: emu.IsBlacklist(), + } p.init() - for i := len(devices) - 1; i >= 0; i-- { - if err := p.appendDevice(devices[i]); err != nil { + + for idx, rule := range cleanRules { + if rule.Type == devices.WildcardDevice { + // We can safely skip over wildcard entries because there should + // only be one (at most) at the very start to instruct cgroupv1 to + // go into allow-list mode. However we do double-check this here. + if idx != 0 || rule.Allow != emu.IsBlacklist() { + return nil, "", errors.Errorf("[internal error] emulated cgroupv2 devices ruleset had bad wildcard at idx %v (%s)", idx, rule.CgroupString()) + } + continue + } + if rule.Allow == p.defaultAllow { + // There should be no rules which have an action equal to the + // default action, the emulator removes those. + return nil, "", errors.Errorf("[internal error] emulated cgroupv2 devices ruleset had no-op rule at idx %v (%s)", idx, rule.CgroupString()) + } + if err := p.appendRule(rule); err != nil { return nil, "", err } } @@ -35,9 +69,9 @@ func DeviceFilter(devices []*devices.Rule) (asm.Instructions, string, error) { } type program struct { - insts asm.Instructions - hasWildCard bool - blockID int + insts asm.Instructions + defaultAllow bool + blockID int } func (p *program) init() { @@ -67,39 +101,36 @@ func (p *program) init() { asm.LoadMem(asm.R5, asm.R1, 8, asm.Word)) } -// appendDevice needs to be called from the last element of OCI linux.resources.devices to the head element. -func (p *program) appendDevice(dev *devices.Rule) error { +// appendRule rule converts an OCI rule to the relevant eBPF block and adds it +// to the in-progress filter program. In order to operate properly, it must be +// called with a "clean" rule list (generated by devices.Emulator.Rules() -- +// with any "a" rules removed). +func (p *program) appendRule(rule *devices.Rule) error { if p.blockID < 0 { return errors.New("the program is finalized") } - if p.hasWildCard { - // All entries after wildcard entry are ignored - return nil - } bpfType := int32(-1) hasType := true - switch dev.Type { - case 'c': + switch rule.Type { + case devices.CharDevice: bpfType = int32(unix.BPF_DEVCG_DEV_CHAR) - case 'b': + case devices.BlockDevice: bpfType = int32(unix.BPF_DEVCG_DEV_BLOCK) - case 'a': - hasType = false default: - // if not specified in OCI json, typ is set to DeviceTypeAll - return errors.Errorf("invalid Type %q", string(dev.Type)) + // We do not permit 'a', nor any other types we don't know about. + return errors.Errorf("invalid Type %q", string(rule.Type)) } - if dev.Major > math.MaxUint32 { - return errors.Errorf("invalid major %d", dev.Major) + if rule.Major > math.MaxUint32 { + return errors.Errorf("invalid major %d", rule.Major) } - if dev.Minor > math.MaxUint32 { - return errors.Errorf("invalid minor %d", dev.Major) + if rule.Minor > math.MaxUint32 { + return errors.Errorf("invalid minor %d", rule.Major) } - hasMajor := dev.Major >= 0 // if not specified in OCI json, major is set to -1 - hasMinor := dev.Minor >= 0 + hasMajor := rule.Major >= 0 // if not specified in OCI json, major is set to -1 + hasMinor := rule.Minor >= 0 bpfAccess := int32(0) - for _, r := range dev.Permissions { + for _, r := range rule.Permissions { switch r { case 'r': bpfAccess |= unix.BPF_DEVCG_ACC_READ @@ -136,19 +167,16 @@ func (p *program) appendDevice(dev *devices.Rule) error { if hasMajor { p.insts = append(p.insts, // if (R4 != major) goto next - asm.JNE.Imm(asm.R4, int32(dev.Major), nextBlockSym), + asm.JNE.Imm(asm.R4, int32(rule.Major), nextBlockSym), ) } if hasMinor { p.insts = append(p.insts, // if (R5 != minor) goto next - asm.JNE.Imm(asm.R5, int32(dev.Minor), nextBlockSym), + asm.JNE.Imm(asm.R5, int32(rule.Minor), nextBlockSym), ) } - if !hasType && !hasAccess && !hasMajor && !hasMinor { - p.hasWildCard = true - } - p.insts = append(p.insts, acceptBlock(dev.Allow)...) + p.insts = append(p.insts, acceptBlock(rule.Allow)...) // set blockSym to the first instruction we added in this iteration p.insts[prevBlockLastIdx+1] = p.insts[prevBlockLastIdx+1].Sym(blockSym) p.blockID++ @@ -156,14 +184,14 @@ func (p *program) appendDevice(dev *devices.Rule) error { } func (p *program) finalize() (asm.Instructions, error) { - if p.hasWildCard { - // acceptBlock with asm.Return() is already inserted - return p.insts, nil + var v int32 + if p.defaultAllow { + v = 1 } blockSym := "block-" + strconv.Itoa(p.blockID) p.insts = append(p.insts, - // R0 <- 0 - asm.Mov.Imm32(asm.R0, 0).Sym(blockSym), + // R0 <- v + asm.Mov.Imm32(asm.R0, v).Sym(blockSym), asm.Return(), ) p.blockID = -1 @@ -171,7 +199,7 @@ func (p *program) finalize() (asm.Instructions, error) { } func acceptBlock(accept bool) asm.Instructions { - v := int32(0) + var v int32 if accept { v = 1 } diff --git a/libcontainer/cgroups/ebpf/devicefilter/devicefilter_test.go b/libcontainer/cgroups/ebpf/devicefilter/devicefilter_test.go index cfcaa2030..a8fc562d0 100644 --- a/libcontainer/cgroups/ebpf/devicefilter/devicefilter_test.go +++ b/libcontainer/cgroups/ebpf/devicefilter/devicefilter_test.go @@ -55,81 +55,81 @@ block-0: func TestDeviceFilter_BuiltInAllowList(t *testing.T) { expected := ` // load parameters into registers - 0: LdXMemW dst: r2 src: r1 off: 0 imm: 0 - 1: And32Imm dst: r2 imm: 65535 - 2: LdXMemW dst: r3 src: r1 off: 0 imm: 0 - 3: RSh32Imm dst: r3 imm: 16 - 4: LdXMemW dst: r4 src: r1 off: 4 imm: 0 - 5: LdXMemW dst: r5 src: r1 off: 8 imm: 0 + 0: LdXMemW dst: r2 src: r1 off: 0 imm: 0 + 1: And32Imm dst: r2 imm: 65535 + 2: LdXMemW dst: r3 src: r1 off: 0 imm: 0 + 3: RSh32Imm dst: r3 imm: 16 + 4: LdXMemW dst: r4 src: r1 off: 4 imm: 0 + 5: LdXMemW dst: r5 src: r1 off: 8 imm: 0 block-0: -// tuntap (c, 10, 200, rwm, allow) - 6: JNEImm dst: r2 off: -1 imm: 2 - 7: JNEImm dst: r4 off: -1 imm: 10 - 8: JNEImm dst: r5 off: -1 imm: 200 - 9: Mov32Imm dst: r0 imm: 1 - 10: Exit -block-1: - 11: JNEImm dst: r2 off: -1 imm: 2 - 12: JNEImm dst: r4 off: -1 imm: 5 - 13: JNEImm dst: r5 off: -1 imm: 2 - 14: Mov32Imm dst: r0 imm: 1 - 15: Exit -block-2: -// /dev/pts (c, 136, wildcard, rwm, true) - 16: JNEImm dst: r2 off: -1 imm: 2 - 17: JNEImm dst: r4 off: -1 imm: 136 - 18: Mov32Imm dst: r0 imm: 1 - 19: Exit -block-3: - 20: JNEImm dst: r2 off: -1 imm: 2 - 21: JNEImm dst: r4 off: -1 imm: 1 - 22: JNEImm dst: r5 off: -1 imm: 9 - 23: Mov32Imm dst: r0 imm: 1 - 24: Exit -block-4: - 25: JNEImm dst: r2 off: -1 imm: 2 - 26: JNEImm dst: r4 off: -1 imm: 1 - 27: JNEImm dst: r5 off: -1 imm: 5 - 28: Mov32Imm dst: r0 imm: 1 - 29: Exit -block-5: - 30: JNEImm dst: r2 off: -1 imm: 2 - 31: JNEImm dst: r4 off: -1 imm: 5 - 32: JNEImm dst: r5 off: -1 imm: 0 - 33: Mov32Imm dst: r0 imm: 1 - 34: Exit -block-6: - 35: JNEImm dst: r2 off: -1 imm: 2 - 36: JNEImm dst: r4 off: -1 imm: 1 - 37: JNEImm dst: r5 off: -1 imm: 7 - 38: Mov32Imm dst: r0 imm: 1 - 39: Exit -block-7: - 40: JNEImm dst: r2 off: -1 imm: 2 - 41: JNEImm dst: r4 off: -1 imm: 1 - 42: JNEImm dst: r5 off: -1 imm: 8 - 43: Mov32Imm dst: r0 imm: 1 - 44: Exit -block-8: - 45: JNEImm dst: r2 off: -1 imm: 2 - 46: JNEImm dst: r4 off: -1 imm: 1 - 47: JNEImm dst: r5 off: -1 imm: 3 - 48: Mov32Imm dst: r0 imm: 1 - 49: Exit -block-9: // (b, wildcard, wildcard, m, true) - 50: JNEImm dst: r2 off: -1 imm: 1 - 51: Mov32Reg dst: r1 src: r3 - 52: And32Imm dst: r1 imm: 1 - 53: JNEReg dst: r1 off: -1 src: r3 - 54: Mov32Imm dst: r0 imm: 1 - 55: Exit -block-10: + 6: JNEImm dst: r2 off: -1 imm: 1 + 7: Mov32Reg dst: r1 src: r3 + 8: And32Imm dst: r1 imm: 1 + 9: JNEReg dst: r1 off: -1 src: r3 + 10: Mov32Imm dst: r0 imm: 1 + 11: Exit +block-1: // (c, wildcard, wildcard, m, true) - 56: JNEImm dst: r2 off: -1 imm: 2 - 57: Mov32Reg dst: r1 src: r3 - 58: And32Imm dst: r1 imm: 1 - 59: JNEReg dst: r1 off: -1 src: r3 + 12: JNEImm dst: r2 off: -1 imm: 2 + 13: Mov32Reg dst: r1 src: r3 + 14: And32Imm dst: r1 imm: 1 + 15: JNEReg dst: r1 off: -1 src: r3 + 16: Mov32Imm dst: r0 imm: 1 + 17: Exit +block-2: + 18: JNEImm dst: r2 off: -1 imm: 2 + 19: JNEImm dst: r4 off: -1 imm: 1 + 20: JNEImm dst: r5 off: -1 imm: 3 + 21: Mov32Imm dst: r0 imm: 1 + 22: Exit +block-3: + 23: JNEImm dst: r2 off: -1 imm: 2 + 24: JNEImm dst: r4 off: -1 imm: 1 + 25: JNEImm dst: r5 off: -1 imm: 5 + 26: Mov32Imm dst: r0 imm: 1 + 27: Exit +block-4: + 28: JNEImm dst: r2 off: -1 imm: 2 + 29: JNEImm dst: r4 off: -1 imm: 1 + 30: JNEImm dst: r5 off: -1 imm: 7 + 31: Mov32Imm dst: r0 imm: 1 + 32: Exit +block-5: + 33: JNEImm dst: r2 off: -1 imm: 2 + 34: JNEImm dst: r4 off: -1 imm: 1 + 35: JNEImm dst: r5 off: -1 imm: 8 + 36: Mov32Imm dst: r0 imm: 1 + 37: Exit +block-6: + 38: JNEImm dst: r2 off: -1 imm: 2 + 39: JNEImm dst: r4 off: -1 imm: 1 + 40: JNEImm dst: r5 off: -1 imm: 9 + 41: Mov32Imm dst: r0 imm: 1 + 42: Exit +block-7: + 43: JNEImm dst: r2 off: -1 imm: 2 + 44: JNEImm dst: r4 off: -1 imm: 5 + 45: JNEImm dst: r5 off: -1 imm: 0 + 46: Mov32Imm dst: r0 imm: 1 + 47: Exit +block-8: + 48: JNEImm dst: r2 off: -1 imm: 2 + 49: JNEImm dst: r4 off: -1 imm: 5 + 50: JNEImm dst: r5 off: -1 imm: 2 + 51: Mov32Imm dst: r0 imm: 1 + 52: Exit +block-9: +// tuntap (c, 10, 200, rwm, allow) + 53: JNEImm dst: r2 off: -1 imm: 2 + 54: JNEImm dst: r4 off: -1 imm: 10 + 55: JNEImm dst: r5 off: -1 imm: 200 + 56: Mov32Imm dst: r0 imm: 1 + 57: Exit +block-10: +// /dev/pts (c, 136, wildcard, rwm, true) + 58: JNEImm dst: r2 off: -1 imm: 2 + 59: JNEImm dst: r4 off: -1 imm: 136 60: Mov32Imm dst: r0 imm: 1 61: Exit block-11: diff --git a/libcontainer/cgroups/fs2/devices.go b/libcontainer/cgroups/fs2/devices.go index fa9194b55..de1930a80 100644 --- a/libcontainer/cgroups/fs2/devices.go +++ b/libcontainer/cgroups/fs2/devices.go @@ -58,9 +58,6 @@ func setDevices(dirPath string, r *configs.Resources) error { if r.SkipDevices { return nil } - // XXX: This is currently a white-list (but all callers pass a blacklist of - // devices). This is bad for a whole variety of reasons, but will need - // to be fixed with co-ordinated effort with downstreams. insts, license, err := devicefilter.DeviceFilter(r.Devices) if err != nil { return err