Re: [PATCH v3 bpf-next] selftests/bpf: Fix selftests broken by mitigations=off

From: Daniel Borkmann <daniel@iogearbox.net>
To: Yonghong Song <yonghong.song@linux.dev>,
	Yafang Shao <laoar.shao@gmail.com>,
	alexei.starovoitov@gmail.com
Cc: andrii@kernel.org, ast@kernel.org, bpf@vger.kernel.org,
	gerhorst@cs.fau.de, haoluo@google.com, john.fastabend@gmail.com,
	jolsa@kernel.org, kpsingh@kernel.org, martin.lau@linux.dev,
	sdf@google.com, song@kernel.org
Subject: Re: [PATCH v3 bpf-next] selftests/bpf: Fix selftests broken by mitigations=off
Date: Thu, 26 Oct 2023 15:46:30 +0200	[thread overview]
Message-ID: <3f47542a-ec0f-c33c-4300-36b54858a79c@iogearbox.net> (raw)
In-Reply-To: <e6c950d7-bb81-4265-bbbe-0201694280b3@linux.dev>

On 10/25/23 6:56 AM, Yonghong Song wrote:
> On 10/24/23 8:11 PM, Yafang Shao wrote:
>> When we configure the kernel command line with 'mitigations=off' and set
>> the sysctl knob 'kernel.unprivileged_bpf_disabled' to 0, the commit
>> bc5bc309db45 ("bpf: Inherit system settings for CPU security mitigations")
>> causes issues in the execution of `test_progs -t verifier`. This is because
>> 'mitigations=off' bypasses Spectre v1 and Spectre v4 protections.
>>
>> Currently, when a program requests to run in unprivileged mode
>> (kernel.unprivileged_bpf_disabled = 0), the BPF verifier may prevent it
>> from running due to the following conditions not being enabled:
>>
>>    - bypass_spec_v1
>>    - bypass_spec_v4
>>    - allow_ptr_leaks
>>    - allow_uninit_stack
>>
>> While 'mitigations=off' enables the first two conditions, it does not
>> enable the latter two. As a result, some test cases in
>> 'test_progs -t verifier' that were expected to fail to run may run
>> successfully, while others still fail but with different error messages.
>> This makes it challenging to address them comprehensively.
>>
>> Moreover, in the future, we may introduce more fine-grained control over
>> CPU mitigations, such as enabling only bypass_spec_v1 or bypass_spec_v4.
>>
>> Given the complexity of the situation, rather than fixing each broken test
>> case individually, it's preferable to skip them when 'mitigations=off' is
>> in effect and introduce specific test cases for the new 'mitigations=off'
>> scenario. For instance, we can introduce new BTF declaration tags like
>> '__failure__nospec', '__failure_nospecv1' and '__failure_nospecv4'.
>>
>> In this patch, the approach is to simply skip the broken test cases when
>> 'mitigations=off' is enabled. The result of `test_progs -t verifier` as
>> follows after this commit,
>>
>> Before this commit
>> ==================
>> - without 'mitigations=off'
>>    - kernel.unprivileged_bpf_disabled = 2
>>      Summary: 74/948 PASSED, 388 SKIPPED, 0 FAILED
>>    - kernel.unprivileged_bpf_disabled = 0
>>      Summary: 74/1336 PASSED, 0 SKIPPED, 0 FAILED   <<<<
>> - with 'mitigations=off'
>>    - kernel.unprivileged_bpf_disabled = 2
>>      Summary: 74/948 PASSED, 388 SKIPPED, 0 FAILED
>>    - kernel.unprivileged_bpf_disabled = 0
>>      Summary: 63/1276 PASSED, 0 SKIPPED, 11 FAILED   <<<< 11 FAILED
>>
>> After this commit
>> =================
>> - without 'mitigations=off'
>>    - kernel.unprivileged_bpf_disabled = 2
>>      Summary: 74/948 PASSED, 388 SKIPPED, 0 FAILED
>>    - kernel.unprivileged_bpf_disabled = 0
>>      Summary: 74/1336 PASSED, 0 SKIPPED, 0 FAILED    <<<<
>> - with this patch, with 'mitigations=off'
>>    - kernel.unprivileged_bpf_disabled = 2
>>      Summary: 74/948 PASSED, 388 SKIPPED, 0 FAILED
>>    - kernel.unprivileged_bpf_disabled = 0
>>      Summary: 74/948 PASSED, 388 SKIPPED, 0 FAILED   <<<< SKIPPED
>>
>> Fixes: bc5bc309db45 ("bpf: Inherit system settings for CPU security mitigations")
>> Reported-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
>> Closes: https://lore.kernel.org/bpf/CAADnVQKUBJqg+hHtbLeeC2jhoJAWqnmRAzXW3hmUCNSV9kx4sQ@mail.gmail.com
>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> 
> Ack with a nit below.
> Acked-by: Yonghong Song <yonghong.song@linux.dev>
> 
[...]
>>       }
>> -    return disabled;
>> +    return disabled ? true : get_mitigations_off();
> 
> Above code is correct. But you could slightly simplify it with
>      return disabled ? : get_mitigations_off();
> 
> I guess maintainer can decide whether simplification is needed
> or not.

Turns out if you omit, then compiler will complain with a warning :)

   [...]
   GEN      vmlinux.h
unpriv_helpers.c: In function ‘get_unpriv_disabled’:
unpriv_helpers.c:56:27: error: the omitted middle operand in ‘?:’ will always be ‘true’, suggest explicit middle operand [-Werror=parentheses]
    56 |         return disabled ? : get_mitigations_off();
       |                           ^
cc1: all warnings being treated as errors
make: *** [Makefile:615: /root/linux/tools/testing/selftests/bpf/unpriv_helpers.o] Error 1

So it's okay as is, applied, thanks!