Issue in permissions checking in https://github.com/containers/crun/blob/main/src/libcrun/ebpf.c#L220 ?

[oOraph]

Hello :); My question is the same as the one reported here

NVIDIA/libnvidia-container#227

Since the code there looks like this one:
https://github.com/containers/crun/blob/main/src/libcrun/ebpf.c#L195

I'm most likely missing something because there's little chance there would be the same mistake in both codes. If someone can explain it to me that would be great :)

As commented in the code.

if ((request.access & device.access) == 0)
      goto next_block:

the current code denies device access (at least stops checking the current device) only if the intersection between the requested access and the device permissions is empty
https://github.com/containers/crun/blob/main/src/libcrun/ebpf.c#L220

BPF_MOV32_REG (BPF_REG_1, BPF_REG_3),
BPF_ALU32_IMM (BPF_AND, BPF_REG_1, bpf_access),
BPF_JMP_IMM (BPF_JEQ, BPF_REG_1, 0, number_instructions - 2),

But is it enough ?

example:
device access permissions: r _ _ (bpf_access == BPF_DEVCG_ACC_READ)
device access request: r w _ (BPF_REG_3 = BPF_DEVCG_ACC_READ | BPF_DEVCG_ACC_WRITE)

then BPF_REG_3 & bpf_access != 0, so we keep on checking other fields and it makes the device a potential candidate to be accepted for write permissions despite the device access being only read ?

Shouldn't we rather have something like this ?

if (BPF_REG_R3 & BPF_DEVCG_ACC_READ != 0) && (bpf_access & BPF_DEVCG_ACC_READ == 0) goto next
if (BPF_REG_R3 & BPF_DEVCG_ACC_WRITE != 0) && (bpf_access & BPF_DEVCG_ACC_WRITE == 0) goto next
if (BPF_REG_R3 & BPF_DEVCG_ACC_MKNOD != 0) && (bpf_access & BPF_DEVCG_ACC_MKNOD == 0) goto next

Thanks

[giuseppe]

the code has the check:

    if ((request.access & device.access) != request.access)
      goto next_block:

that means, the device is ignored (and the goto path is taken) if it requests any bit that is not set also in the access permissions list.

In your example:

device access permissions: r _ _ (bpf_access == BPF_DEVCG_ACC_READ)
device access request: r w _ (BPF_REG_3 = BPF_DEVCG_ACC_READ | BPF_DEVCG_ACC_WRITE)

So we have:

if ("r" &  "rw")  != "rw" {
    goto next_block;
}

that becomes:

if "r"  != "rw" {
    goto next_block;
}

and the current block is skipped.

[oOraph]

Thanks !

You're right, I just realized my comment was outdated in regard to the main branch, it was related to an old version https://github.com/containers/crun/blob/0.10.2/src/libcrun/ebpf.c#L219

And the check if ((request.access & device.access) == 0) got replaced by if ((request.access & device.access) != request.access) eversince !

Closing the issue :)