| Message ID | 20231025031144.5508-1-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 399f6185a1c02f39bcadb8749bc2d9d48685816f |
| Delegated to: | BPF |
| Headers | show |
| Series | [v3,bpf-next] selftests/bpf: Fix selftests broken by mitigations=off | expand |
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> > --- > tools/testing/selftests/bpf/unpriv_helpers.c | 35 +++++++++++++++++++- > 1 file changed, 34 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/bpf/unpriv_helpers.c b/tools/testing/selftests/bpf/unpriv_helpers.c > index 2a6efbd0401e..7101e72ef4a3 100644 > --- a/tools/testing/selftests/bpf/unpriv_helpers.c > +++ b/tools/testing/selftests/bpf/unpriv_helpers.c > @@ -4,9 +4,42 @@ > #include <stdlib.h> > #include <error.h> > #include <stdio.h> > +#include <string.h> > +#include <unistd.h> > +#include <fcntl.h> > > #include "unpriv_helpers.h" > > [...] > bool get_unpriv_disabled(void) > { > bool disabled; > @@ -22,5 +55,5 @@ bool get_unpriv_disabled(void) > disabled = true; > } > > - 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. > }
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!
Hello: This patch was applied to bpf/bpf-next.git (master) by Daniel Borkmann <daniel@iogearbox.net>: On Wed, 25 Oct 2023 03:11:44 +0000 you 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: > > [...] Here is the summary with links: - [v3,bpf-next] selftests/bpf: Fix selftests broken by mitigations=off https://git.kernel.org/bpf/bpf-next/c/399f6185a1c0 You are awesome, thank you!
On 10/26/23 6:46 AM, Daniel Borkmann wrote: > 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 clang compiler is okay with '?:' change while gcc compiler issued errors. So yes, existing code is good for both compilers. Thanks! > > So it's okay as is, applied, thanks! >
diff --git a/tools/testing/selftests/bpf/unpriv_helpers.c b/tools/testing/selftests/bpf/unpriv_helpers.c index 2a6efbd0401e..7101e72ef4a3 100644 --- a/tools/testing/selftests/bpf/unpriv_helpers.c +++ b/tools/testing/selftests/bpf/unpriv_helpers.c @@ -4,9 +4,42 @@ #include <stdlib.h> #include <error.h> #include <stdio.h> +#include <string.h> +#include <unistd.h> +#include <fcntl.h> #include "unpriv_helpers.h" +static bool get_mitigations_off(void) +{ + char cmdline[4096], *c; + int fd, ret = false; + + fd = open("/proc/cmdline", O_RDONLY); + if (fd < 0) { + perror("open /proc/cmdline"); + return false; + } + + if (read(fd, cmdline, sizeof(cmdline) - 1) < 0) { + perror("read /proc/cmdline"); + goto out; + } + + cmdline[sizeof(cmdline) - 1] = '\0'; + for (c = strtok(cmdline, " \n"); c; c = strtok(NULL, " \n")) { + if (strncmp(c, "mitigations=off", strlen(c))) + continue; + + ret = true; + break; + } + +out: + close(fd); + return ret; +} + bool get_unpriv_disabled(void) { bool disabled; @@ -22,5 +55,5 @@ bool get_unpriv_disabled(void) disabled = true; } - return disabled; + return disabled ? true : get_mitigations_off(); }
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> --- tools/testing/selftests/bpf/unpriv_helpers.c | 35 +++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-)