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

[Joerg Vehlow]

Hi Richard,

Am 1/13/2022 um 8:48 AM schrieb 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 was looking for kernel config options, that could modify bpf behavior
and also unprivileged_bpf_disable (set to 0 for me), but nothing stood out.

> 
>> 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.

Ok, I get that.


I did some more digging and bisecting. First I bisected the mainline
kernel and found, that the commit 2c78ee898 ("bpf: Implement CAP_BPF")
makes the check run successful. This is only in linux >= 5.8.
But my 5.4 ubuntu kernel also successfully ran the check, so I also
bisected ubuntu sources [1] and found this commit to be the fix here:
2fa9ab45c ("bpf: No need to simulate speculative domain for immediates")
This commit is also in the mainline kernel, but only in >= 5.13.

I guess the check you implemented now disables the test for a lot of
kernels, that do not have a patch like this... I will stop here and just
accept, that the test is not running successfully in my case. But still
wanted to share this information.

Joerg


[1]
https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/focal/commit/?id=2fa9ab45c53e8b104ba8f7d3a953131cc818fcc0