CDI spec does not allow empty permissions for device nodes

[oOraph]

When creating a container it can make sense to not allow any rwm operation at first on specified devices: the device node will still be created and the associated shared libraries and tools can still be mounted, eg everything will be prepared for a later usage but the device won't be 'open' yet

This use case is close to the one nvidia-container-toolkit supported in legacy mode with the no-cgroups = true flag (at least this was the way I was using it, I may have misunderstood its purpose, cc @elezar here, will probably be able to clarify :) )

But in the spec, it's explicitely specified

// Cgroups permissions of the device, candidates are one or more of [... r, w, m]

And we can indeed see in the implem here that an empty string is translated to "rwm"

container-device-interface/pkg/cdi/container-edits.go

Line 109 in 98a7d73

if access == "" {

Proposal: change this spec to

// Cgroups permissions of the device, candidates are an empty string or any combination of one or more of [... r, w, m] 

What do you think ?

side note: so far, containerd users are lucky because containerd seems quite flexible and discards unknown chars in the allow string of the oci spec. This statement, coupled with the fact that the cdi spec implem above is flexible enough to pass through any invalid permission string to the oci spec without raising any error, setting the allow string to any unknown char of the cdi spec luckily creates the intented effect, but this is more a flaw than a spec respect, if I understand correctly

[klihub]

When creating a container it can make sense to not allow any rwm operation at first on specified devices: the device node will still be created and the associated shared libraries and tools can still be mounted, eg everything will be prepared for a later usage but the device won't be 'open' yet

@oOraph Once the container is created/started without any cgroup-based access permissions for the device, how is it supposed to gain those permissions later ? At least with K8s/CRI containers, I don't think you could update the container in that respect dynamically afterwards using a CRI protocol request.

[klihub]

coupled with the fact that the cdi spec implem above is flexible enough to pass through any invalid permission string to the oci spec without raising any error, setting the allow string to any unknown char of the cdi spec luckily creates the intented effect, but this is more a flaw than a spec respect, if I understand correctly

Yes. Definitely not a behavior that we'd like users to start relying on.

[klihub]

Also the current behavior is intentional. Omitted cgroup permissions will give you a default of full cgroup-based permissions. Changing it so that it would result in empty permissions would be a backward incompatible changes. So if there is justification for that an empty permissions set, then we'll probably need to use a dedicated non-zero-value const for it, for instance "none".

[oOraph]

Also the current behavior is intentional. Omitted cgroup permissions will give you a default of full cgroup-based permissions. Changing it so that it would result in empty permissions would be a backward incompatible changes.

Indeed, thanks for pointing this drawback. Changing the behaviour of the empty string would be incompatible

Once the container is created/started without any cgroup-based access permissions for the device, how is it supposed to gain those permissions later ? At least with K8s/CRI containers, I don't think you could update the container in that respect dynamically afterwards using a CRI protocol request.

It cannot be done with K8s/CRI but the CDI is OCI-runtime agnostic right ? For example we use a side piece of software that opens the gpu accesses after container creation. This does not conflict with kubelet at all, nor containerd(+runc), that does not perform any reconciliation after creation.

[klihub]

Also the current behavior is intentional. Omitted cgroup permissions will give you a default of full cgroup-based permissions. Changing it so that it would result in empty permissions would be a backward incompatible changes.

Indeed, thanks for pointing this drawback. Changing the behaviour of the empty string would be incompatible

Once the container is created/started without any cgroup-based access permissions for the device, how is it supposed to gain those permissions later ? At least with K8s/CRI containers, I don't think you could update the container in that respect dynamically afterwards using a CRI protocol request.

It cannot be done with K8s/CRI but the CDI is OCI-runtime agnostic right ? For example we use a side piece of software that opens the gpu accesses after container creation. This does not conflict with kubelet at all, nor containerd(+runc), that does not perform any reconciliation after creation.

Okay, so yours is a non-K8s environment, where this can very well be a useful case. So, I think the easiest way to codify the desired behavior would be to add an exported const, maybe a const NoPermissions = "none", which would then get substituted by "".

[klihub]

This statement, coupled with the fact that the cdi spec implem above is flexible enough to pass through any invalid permission string to the oci spec without raising any error

@oOraph Hmm, why do you think that the current CDI implementation would pass any unknown permission string through unmodified ? I think we don't. Instead we validate device permissions strings and treat anything else than 'r', 'w', or 'm' as an error:

container-device-interface/pkg/cdi/container-edits.go

Line 357 in 98a7d73

for _, bit := range d.Permissions {

[klihub]

Filed PR #301 to implement this.

[oOraph]

This statement, coupled with the fact that the cdi spec implem above is flexible enough to pass through any invalid permission string to the oci spec without raising any error

@oOraph Hmm, why do you think that the current CDI implementation would pass any unknown permission string through unmodified ? I think we don't. Instead we validate device permissions strings and treat anything else than 'r', 'w', or 'm' as an error:

container-device-interface/pkg/cdi/container-edits.go

Line 357 in 98a7d73
for _, bit := range d.Permissions {

Interesting I did not check the implem I directly made the experiment here:
https://github.com/NVIDIA/nvidia-container-toolkit/blob/v1.18.1/internal/edits/device.go#L57

-> I added Permissions with string "_" and it ended up injected as such into the OCI spec (and containerd did not complain either with this spec). Since it uses this repo as a dependency to generate the CDI spec I thought the flaw was here

(dependency:
tags.cncf.io/container-device-interface v1.0.2-0.20251114135136-1b24d969689f
tags.cncf.io/container-device-interface/specs-go v1.0.0
)

-> I'll rerun asap to check I can reproduce. But I don't see Validate as being called, just Apply (and the validation is not done there)

-> I'll reproduce to confirm :)

[klihub]

Interesting I did not check the implem I directly made the experiment here:
https://github.com/NVIDIA/nvidia-container-toolkit/blob/v1.18.1/internal/edits/device.go#L57

Yeah, we only validate a Spec when it is read/loaded (or when it is written if an external validator has been also set). If you manually construct one, then you can bypass validation.

[oOraph]

Filed PR #301 to implement this.

Thanks :)

Yeah, we only validate a Spec when it is read/loaded (or when it is written if an external validator has been also set). If you manually construct one, then you can bypass validation.

Ok so let's forget about this wrong side note statement I initially made about the cdi implem flaw then. Maybe it's more an issue on nvidia container runtime then that should call validate (or not, don't know if this is really an issue).

Side note:

I'll reproduce to confirm :)

I confirm I was able to reproduce the invalid permission string test with the following mini fork: https://github.com/oOraph/nvidia-container-toolkit/tree/reproduce1 and the oci spec ends up with the _ in spec, containerd (2.1.5) seems happy with it and what's even more surprising is that mknod seems always granted in the cgroup device bpf program for all block and char devices, no matter what this rwm permission string is (valid or invalid) ! But let's not talk about this here, I'll dig and report/ask on containerd or runc repo, not sure which one yet :) )

[klihub]

containerd (2.1.5) seems happy with it and what's even more surprising is that mknod seems always granted in the cgroup device bpf program for all block and char devices, no matter what this rwm permission string is (valid or invalid) !

And you are sure that you were not testing with a privileged container...

[klihub]

And mostly for reference, you can always look at the attached eBPF programs of the container cgroup and try to understand it to see what rules you ended up running with: https://dropbear.xyz/2023/05/23/devices-with-cgroup-v2

[oOraph]

no privileged container (read and write choices are properly enforced). And yes I checked the bpf programs (both the containerd and the runc one) -> the mknod is explicitely enabled in there for all char and block devices

41: (bc) w1 = w3 
42: (54) w1 &= 1 
43: (5d) if r1 != r3 goto pc+2 
44: (55) if r2 != 0x2 goto pc+1 
45: (05) goto pc+7 
46: (bc) w1 = w3 
47: (54) w1 &= 1 
48: (5d) if r1 != r3 goto pc+2 
49: (55) if r2 != 0x1 goto pc+1 
50: (05) goto pc+2 
51: (b7) r0 = 0 
52: (05) goto pc+1 
53: (b7) r0 = 1 
54: (95) exit

(I can reproduce with a minimal oci spec + runc btw, reported here: opencontainers/runc#5073, let's wait to see what is said on this because if expected this empty string "none" permission is not relevant, "m" being the minimum one)

[klihub]

Hmm, okay. But I wonder if that is really a problem. Ideally form the least privilege perspective mknod would be good to block if a container should not have access to a device. But if the cgroup access rules are otherwise correct wrt. read/write access, having the ability to create a device node alone won't do much good for the container (sans kernel bugs), since it shouldn't then be able to do much with that device node.