cgroup: devices updates appear to be broken

[cyphar]

This affects both versions, but in quite different ways:

• For cgroupv1, Don't deny all devices when update cgroup resource #2205 highlighted that on device cgroup updates, we temporarily block all devices. This results in spurious errors in the container (such as programs being unable to open /dev/null). We've seen this happen on customer systems under Kubernetes, so this is definitely a real issue.

  • This is actually a more complicated issue than it first appears because runc actually incorrectly implements the spec here -- technically runc actually is a black-list by default and users have to convert runc to be a white-list. Aside from not following the spec this is a worrying security stance.

• For cgroupv2, devices cgroup updates are implemented by appending a new BPF program to the cgroup. This means that only new denials have an effect, and thus it's incorrectly implemented. (EDIT: This also means that we "leak" eBPF programs and thus after 64+ applications we start getting errors -- see api, cgroupv2: skip setting the devices cgroup #2474.)

  • Unfortunately this is a bit complicated to fix, but I have figured out how to do it. We need to make an eBPF map of type BPF_MAP_TYPE_PROG_ARRAYand then tail-call into it in a small stub eBPF program which we attach to the actual cgroup. This which will allow us to atomically update the devices cgroup rules (there is no way to atomically replace an eBPF program with BPF_F_ALLOW_MULTI -- and without any program, all device accesses would be permitted).
  • Ignore the above -- you cannot bpf_tail_call from cgroup programs. So we will need to instead implement it through an eBPF map (which we can atomically replace by mis-using BPF_MAP_TYPE_ARRAY_OF_ARRAY).
  • This is all slightly complicated by the fact that the entire API is fd-based (and we don't have our own monitor process so we can't stash away the fd). But luckily there is a lookup-by-id system which we can use to get the file descriptor, though the ids can be recycled so we'll need to be careful to make sure we don't start touching the wrong eBPF map -- and unlike eBPF programs, eBPF maps don't store information about when they were created.

Part of #2315.

[cyphar]

Fixing the white-list problem actually isn't too bad -- we just need to re-calculate the device list (merging entries which overlap with each other) rather than blindly applying the provided list of devices in-order. Basically we should pre-calculate the minimal devices.allow and devices.deny lists.

[cyphar]

And actually this does bring up a different question -- what does runc update mean for devices? Does it mean that we should replace the list, or update the existing list? This is a slightly open question because devices is the only cgroup with this behaviour. I think replacing the list is simpler because it'll be harder to implement for cgroupv2 (we'd need to fetch the resource limits saved in the container state file -- which is something I want to die eventually, so adding a new requirement for it seems like a bad idea).

[giuseppe]

I've played with it in crun and I think we can use pinning to the bpf file system to get back a fd to the previous eBPF program.

What do you think of the solution here: containers/crun#352 ?

[cyphar]

Pinning is another fine solution to the issue (as much as I'm not a fan of how ugly the pinning system is). Another choice would be to use the lookup-by-id system but then we'd have to deal with IDs being able to overflow.

But while BPF_F_REPLACE is a good solution for 5.6+ kernels, for older kernels we need another solution -- my idea was to use a BPF_MAP_TYPE_ARRAY_OF_ARRAY so that we can atomically swap the rules. That way you can actually install the same program for all cgroups, and then they just look up the rules for their cgroup in a per-container eBPF map. A nice benefit is that if you do it that way, userspace can actually read the current set of rules.

[cyphar]

The cgroupv1 portion of this issue was fixed by #2391.

[AkihiroSuda]

Changed the milestone from rc92 to 1.0

[AkihiroSuda]

@cyphar Can we postpone this to v1.1.0?