[LTP] [PATCH v2 4/4] bpf_prog05: Drop CAP_BPF and check if ptr arithmetic is allowed

[Richard Palethorpe]

Hello Joerg,

Joerg Vehlow <lkml@jv-coder.de> writes:

> Hi Richard,
>
> Am 1/11/2022 um 3:36 PM schrieb Richard Palethorpe:
>>>> +	if (log[0] != 0)
>>>> +		tst_brk(TCONF | TERRNO, "No pointer arithmetic:\n %s", log);
>>> This check now fails for me with the following output, where the test
>>> was successful, before this patch. The kernel is a non-standard suse
>>> 5.3.18 with realtime patches.
>>>
>>> bpf_prog05.c:100: TCONF: No pointer arithmetic:
>>>   0: (bf) r2 = r10
>>> 1: (b7) r3 = -1
>>> 2: (0f) r2 += r3
>>> 3: (72) *(u8 *)(r2 +0) = 0
>>> 4: (b7) r0 = 0
>>> 5: (95) exit
>>>
>>> from 2 to 3 (speculative execution): R1=ctx(id=0,off=0,imm=0) R2_w=fp0
>>> R3_w=invP-1 R10=fp0
>>> 3: (72) *(u8 *)(r2 +0) = 0
>>> invalid stack off=0 size=1
>>> processed 7 insns (limit 1000000) max_states_per_insn 0 total_states 0
>>> peak_states 0 mark_read 0
>>> : EACCES (13)
>>>
>>>
>>> Is this too old? But then again, the test was successful before this
>>> patch and your commit message states, that it was not successful on
>>> "older kernels".
>> Are you testing on ARM? eBPF adopted the ARM behavior when dividing
>> by
>> zero.
>> 
>
> No this is x86 and I was expecting 5.3 to not be "very old". IIRC
> patches for the bug tested here are partly already integrated into
> 4.19, so I guess 5.3 shouldn't be "very old" in that context?
>
> I am not sure what you tried to test, I guess accessing the stack
> using indirect pointer arithmetic? Because changing the store to "*(u8
> *)(r10 - 1) = 0", works. But if this indirection is required for the
> actually test, it should fail, but it doesn't -> I think the check
> tests something, that is not actually required for the actual test.

The actual test just checks what the result of modulo and division by
zero are against the 'arbitrary' values chosen by ARM/upstream (simlar
to the selftests BTW). This is because there are multiple different
values, which can be exploited depending on what patches are applied.

However they require different logic to detect if pointer arithmetic
resulted in an OOB read or write. Note that even if you can do pointer
arithmetic and it results in the wrong value being accessed. It doesn't
matter if it's not OOB or if the validator blocks it because it has
calculated the offset in the wrong direction. It may still be
exploitable in these cases, but the test will pass.

I can't remember what the exact problems were; it got very complicated
so I just checked that the results of division/modulo by zero matched
upstream. This resulted in a false positive on some kernel which in
theory would be vulnerable if unprivileged pointer arithmetic were
possible. So I added the ptr arithmetic check to prevent the test from
running on kernels like this.

This also appears to prevent the test from running on your kernel which
is not vulnerable in any case. Thinking about it, this may be due to a
configuration option you have set
(i.e. sys/kernel/unprivileged_bpf_disabled). Perhaps the problem is this
is not mentioned in the test description or TCONF message?

IIRC We chose TCONF instead of TPASS to give a hint that we're not
running the full test. You could also have a vulnerable kernel, but with
unpriveleged BPF disabled by a config option. In this case the test
should not return TCONF because a user could change the option and make
themselves vulnerable.

> I guess what you wanted to check is what BPF_MAP_ARRAY_STX does. There
> is one major difference between your implementation of the check and 
> BPF_MAP_ARRAY_STX: BPF_MAP_ARRAY_STX uses an immediate value, to add
> to r2 compared to a register in your check:
>
>  * r2 = fp
>  * r2 += r2 - 4
>
> vs
>
>  * r2 = r10
>  * r3 = -1
>  * r2 += r3
>
> That is actually what makes the difference here...

No, BPF_MAP_ARRAY_STX is just used to output the result of div/mod by
zero and I don't think it needs testing.

>
> If I modify the check like this it works:
>
> --- a/testcases/kernel/syscalls/bpf/bpf_prog05.c
> +++ b/testcases/kernel/syscalls/bpf/bpf_prog05.c
> @@ -67,13 +67,11 @@ static void ensure_ptr_arithmetic(void)
>  {
>         const struct bpf_insn prog_insn[] = {
>                 /* r2 = r10
> -                * r3 = -1
> -                * r2 += r3
> +                * r2 += -1
>                  * *(char *)r2 = 0
>                  */
>                 BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> -               BPF_MOV64_IMM(BPF_REG_3, -1),
> -               BPF_ALU64_REG(BPF_ADD, BPF_REG_2, BPF_REG_3),
> +               BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -1),
>                 BPF_ST_MEM(BPF_B, BPF_REG_2, 0, 0),
>
>                 /* exit(0) */
>
>
> Is this still checking what it is supposed to? Then I would send it
> post this as a patch
>
> Joerg

This will reintroduce the false positive.

-- 
Thank you,
Richard.