[PATCH bpf-next] bpf: Inherit system settings for CPU security mitigations

[PATCH bpf-next] bpf: Inherit system settings for CPU security mitigations

[Yafang Shao]

Currently, there exists a system-wide setting related to CPU security
mitigations, denoted as 'mitigations='. When set to 'mitigations=off', it
deactivates all optional CPU mitigations. Therefore, if we implement a
system-wide 'mitigations=off' setting, it should inherently bypass Spectre
v1 and Spectre v4 in the BPF subsystem.

Please note that there is also a 'nospectre_v1' setting on x86 and ppc
architectures, though it is not currently exported. For the time being,
let's disregard it.

This idea emerged during our discussion about potential Spectre v1 attacks
with Luis[1].

[1]. https://lore.kernel.org/bpf/b4fc15f7-b204-767e-ebb9-fdb4233961fb@iogearbox.net/

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Luis Gerhorst <gerhorst@cs.fau.de>
---
 include/linux/bpf.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index a82efd34b741..61bde4520f5c 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2164,12 +2164,12 @@ static inline bool bpf_allow_uninit_stack(void)
 
 static inline bool bpf_bypass_spec_v1(void)
 {
-	return perfmon_capable();
+	return perfmon_capable() || cpu_mitigations_off();
 }
 
 static inline bool bpf_bypass_spec_v4(void)
 {
-	return perfmon_capable();
+	return perfmon_capable() || cpu_mitigations_off();
 }
 
 int bpf_map_new_fd(struct bpf_map *map, int flags);
-- 
2.30.1 (Apple Git-130)

Re: [PATCH bpf-next] bpf: Inherit system settings for CPU security mitigations

[Stanislav Fomichev]

On 10/05, Yafang Shao wrote:
> Currently, there exists a system-wide setting related to CPU security
> mitigations, denoted as 'mitigations='. When set to 'mitigations=off', it
> deactivates all optional CPU mitigations. Therefore, if we implement a
> system-wide 'mitigations=off' setting, it should inherently bypass Spectre
> v1 and Spectre v4 in the BPF subsystem.
> 
> Please note that there is also a 'nospectre_v1' setting on x86 and ppc
> architectures, though it is not currently exported. For the time being,
> let's disregard it.
> 
> This idea emerged during our discussion about potential Spectre v1 attacks
> with Luis[1].
> 
> [1]. https://lore.kernel.org/bpf/b4fc15f7-b204-767e-ebb9-fdb4233961fb@iogearbox.net/

Based on the discussion from [1]:

Acked-by: Stanislav Fomichev <sdf@google.com>

> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Cc: Luis Gerhorst <gerhorst@cs.fau.de>
> ---
>  include/linux/bpf.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index a82efd34b741..61bde4520f5c 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -2164,12 +2164,12 @@ static inline bool bpf_allow_uninit_stack(void)
>  
>  static inline bool bpf_bypass_spec_v1(void)
>  {
> -	return perfmon_capable();
> +	return perfmon_capable() || cpu_mitigations_off();
>  }
>  
>  static inline bool bpf_bypass_spec_v4(void)
>  {
> -	return perfmon_capable();
> +	return perfmon_capable() || cpu_mitigations_off();
>  }
>  
>  int bpf_map_new_fd(struct bpf_map *map, int flags);
> -- 
> 2.30.1 (Apple Git-130)
> 

Re: [PATCH bpf-next] bpf: Inherit system settings for CPU security mitigations

[Song Liu]

On Thu, Oct 5, 2023 at 1:41 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> Currently, there exists a system-wide setting related to CPU security
> mitigations, denoted as 'mitigations='. When set to 'mitigations=off', it
> deactivates all optional CPU mitigations. Therefore, if we implement a
> system-wide 'mitigations=off' setting, it should inherently bypass Spectre
> v1 and Spectre v4 in the BPF subsystem.
>
> Please note that there is also a 'nospectre_v1' setting on x86 and ppc
> architectures, though it is not currently exported. For the time being,
> let's disregard it.
>
> This idea emerged during our discussion about potential Spectre v1 attacks
> with Luis[1].
>
> [1]. https://lore.kernel.org/bpf/b4fc15f7-b204-767e-ebb9-fdb4233961fb@iogearbox.net/
>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Cc: Luis Gerhorst <gerhorst@cs.fau.de>

Acked-by: Song Liu <song@kernel.org>

Re: [PATCH bpf-next] bpf: Inherit system settings for CPU security mitigations

[KP Singh]

On Thu, Oct 5, 2023 at 8:02 PM Song Liu <song@kernel.org> wrote:
>
> On Thu, Oct 5, 2023 at 1:41 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > Currently, there exists a system-wide setting related to CPU security
> > mitigations, denoted as 'mitigations='. When set to 'mitigations=off', it
> > deactivates all optional CPU mitigations. Therefore, if we implement a
> > system-wide 'mitigations=off' setting, it should inherently bypass Spectre
> > v1 and Spectre v4 in the BPF subsystem.
> >
> > Please note that there is also a 'nospectre_v1' setting on x86 and ppc
> > architectures, though it is not currently exported. For the time being,
> > let's disregard it.
> >
> > This idea emerged during our discussion about potential Spectre v1 attacks
> > with Luis[1].
> >
> > [1]. https://lore.kernel.org/bpf/b4fc15f7-b204-767e-ebb9-fdb4233961fb@iogearbox.net/
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > Cc: Luis Gerhorst <gerhorst@cs.fau.de>
>
> Acked-by: Song Liu <song@kernel.org>
>

Acked-by: KP Singh <kpsingh@kernel.org>

Re: [PATCH bpf-next] bpf: Inherit system settings for CPU security mitigations

[Daniel Borkmann]

On 10/6/23 1:30 AM, KP Singh wrote:
> On Thu, Oct 5, 2023 at 8:02 PM Song Liu <song@kernel.org> wrote:
>> On Thu, Oct 5, 2023 at 1:41 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>>>
>>> Currently, there exists a system-wide setting related to CPU security
>>> mitigations, denoted as 'mitigations='. When set to 'mitigations=off', it
>>> deactivates all optional CPU mitigations. Therefore, if we implement a
>>> system-wide 'mitigations=off' setting, it should inherently bypass Spectre
>>> v1 and Spectre v4 in the BPF subsystem.
>>>
>>> Please note that there is also a 'nospectre_v1' setting on x86 and ppc
>>> architectures, though it is not currently exported. For the time being,
>>> let's disregard it.

 From reading, the cpu_mitigations_off() is a more generic toggle to turn these
generally off, so going via cpu_mitigations_off() is fine in our case and does
not leave some corner cases behind. I presume you mean above that in future the
BPF side could respect some more fine-tuned settings, though it probably might
need some more coordination wrt archs to abstract sth generic out of it.

>>> This idea emerged during our discussion about potential Spectre v1 attacks
>>> with Luis[1].
>>>
>>> [1]. https://lore.kernel.org/bpf/b4fc15f7-b204-767e-ebb9-fdb4233961fb@iogearbox.net/
>>>
>>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>>> Cc: Luis Gerhorst <gerhorst@cs.fau.de>
>>
>> Acked-by: Song Liu <song@kernel.org>
>>
> 
> Acked-by: KP Singh <kpsingh@kernel.org>

Thanks,
Daniel

Re: [PATCH bpf-next] bpf: Inherit system settings for CPU security mitigations

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Thu,  5 Oct 2023 08:41:23 +0000 you wrote:
> Currently, there exists a system-wide setting related to CPU security
> mitigations, denoted as 'mitigations='. When set to 'mitigations=off', it
> deactivates all optional CPU mitigations. Therefore, if we implement a
> system-wide 'mitigations=off' setting, it should inherently bypass Spectre
> v1 and Spectre v4 in the BPF subsystem.
> 
> Please note that there is also a 'nospectre_v1' setting on x86 and ppc
> architectures, though it is not currently exported. For the time being,
> let's disregard it.
> 
> [...]

Here is the summary with links:
  - [bpf-next] bpf: Inherit system settings for CPU security mitigations
    https://git.kernel.org/bpf/bpf-next/c/bc5bc309db45

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH bpf-next] bpf: Inherit system settings for CPU security mitigations

[Andrii Nakryiko]

On Thu, Oct 5, 2023 at 1:41 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> Currently, there exists a system-wide setting related to CPU security
> mitigations, denoted as 'mitigations='. When set to 'mitigations=off', it
> deactivates all optional CPU mitigations. Therefore, if we implement a
> system-wide 'mitigations=off' setting, it should inherently bypass Spectre
> v1 and Spectre v4 in the BPF subsystem.
>
> Please note that there is also a 'nospectre_v1' setting on x86 and ppc
> architectures, though it is not currently exported. For the time being,
> let's disregard it.
>
> This idea emerged during our discussion about potential Spectre v1 attacks
> with Luis[1].
>
> [1]. https://lore.kernel.org/bpf/b4fc15f7-b204-767e-ebb9-fdb4233961fb@iogearbox.net/
>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Cc: Luis Gerhorst <gerhorst@cs.fau.de>
> ---
>  include/linux/bpf.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index a82efd34b741..61bde4520f5c 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -2164,12 +2164,12 @@ static inline bool bpf_allow_uninit_stack(void)
>
>  static inline bool bpf_bypass_spec_v1(void)
>  {
> -       return perfmon_capable();
> +       return perfmon_capable() || cpu_mitigations_off();

Should we check cpu_mitigations_off() first before perfmon_capable()
to avoid unnecessary capability checks, which generate audit messages?

>  }
>
>  static inline bool bpf_bypass_spec_v4(void)
>  {
> -       return perfmon_capable();
> +       return perfmon_capable() || cpu_mitigations_off();
>  }
>
>  int bpf_map_new_fd(struct bpf_map *map, int flags);
> --
> 2.30.1 (Apple Git-130)
>

Re: [PATCH bpf-next] bpf: Inherit system settings for CPU security mitigations

[Yafang Shao]

On Thu, Oct 12, 2023 at 6:53 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Oct 5, 2023 at 1:41 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > Currently, there exists a system-wide setting related to CPU security
> > mitigations, denoted as 'mitigations='. When set to 'mitigations=off', it
> > deactivates all optional CPU mitigations. Therefore, if we implement a
> > system-wide 'mitigations=off' setting, it should inherently bypass Spectre
> > v1 and Spectre v4 in the BPF subsystem.
> >
> > Please note that there is also a 'nospectre_v1' setting on x86 and ppc
> > architectures, though it is not currently exported. For the time being,
> > let's disregard it.
> >
> > This idea emerged during our discussion about potential Spectre v1 attacks
> > with Luis[1].
> >
> > [1]. https://lore.kernel.org/bpf/b4fc15f7-b204-767e-ebb9-fdb4233961fb@iogearbox.net/
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > Cc: Luis Gerhorst <gerhorst@cs.fau.de>
> > ---
> >  include/linux/bpf.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index a82efd34b741..61bde4520f5c 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -2164,12 +2164,12 @@ static inline bool bpf_allow_uninit_stack(void)
> >
> >  static inline bool bpf_bypass_spec_v1(void)
> >  {
> > -       return perfmon_capable();
> > +       return perfmon_capable() || cpu_mitigations_off();
>
> Should we check cpu_mitigations_off() first before perfmon_capable()
> to avoid unnecessary capability checks, which generate audit messages?

makes sense.
Should I send an additional patch, or you modify the original patch?

-- 
Regards
Yafang

Re: [PATCH bpf-next] bpf: Inherit system settings for CPU security mitigations

[Andrii Nakryiko]

On Wed, Oct 11, 2023 at 7:29 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Thu, Oct 12, 2023 at 6:53 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Thu, Oct 5, 2023 at 1:41 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> > >
> > > Currently, there exists a system-wide setting related to CPU security
> > > mitigations, denoted as 'mitigations='. When set to 'mitigations=off', it
> > > deactivates all optional CPU mitigations. Therefore, if we implement a
> > > system-wide 'mitigations=off' setting, it should inherently bypass Spectre
> > > v1 and Spectre v4 in the BPF subsystem.
> > >
> > > Please note that there is also a 'nospectre_v1' setting on x86 and ppc
> > > architectures, though it is not currently exported. For the time being,
> > > let's disregard it.
> > >
> > > This idea emerged during our discussion about potential Spectre v1 attacks
> > > with Luis[1].
> > >
> > > [1]. https://lore.kernel.org/bpf/b4fc15f7-b204-767e-ebb9-fdb4233961fb@iogearbox.net/
> > >
> > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > > Cc: Luis Gerhorst <gerhorst@cs.fau.de>
> > > ---
> > >  include/linux/bpf.h | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > index a82efd34b741..61bde4520f5c 100644
> > > --- a/include/linux/bpf.h
> > > +++ b/include/linux/bpf.h
> > > @@ -2164,12 +2164,12 @@ static inline bool bpf_allow_uninit_stack(void)
> > >
> > >  static inline bool bpf_bypass_spec_v1(void)
> > >  {
> > > -       return perfmon_capable();
> > > +       return perfmon_capable() || cpu_mitigations_off();
> >
> > Should we check cpu_mitigations_off() first before perfmon_capable()
> > to avoid unnecessary capability checks, which generate audit messages?
>
> makes sense.
> Should I send an additional patch, or you modify the original patch?

please send a patch

>
> --
> Regards
> Yafang

Re: [PATCH bpf-next] bpf: Inherit system settings for CPU security mitigations

[Alexei Starovoitov]

On Thu, Oct 5, 2023 at 1:41 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> Currently, there exists a system-wide setting related to CPU security
> mitigations, denoted as 'mitigations='. When set to 'mitigations=off', it
> deactivates all optional CPU mitigations. Therefore, if we implement a
> system-wide 'mitigations=off' setting, it should inherently bypass Spectre
> v1 and Spectre v4 in the BPF subsystem.
>
> Please note that there is also a 'nospectre_v1' setting on x86 and ppc
> architectures, though it is not currently exported. For the time being,
> let's disregard it.
>
> This idea emerged during our discussion about potential Spectre v1 attacks
> with Luis[1].
>
> [1]. https://lore.kernel.org/bpf/b4fc15f7-b204-767e-ebb9-fdb4233961fb@iogearbox.net/
>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Cc: Luis Gerhorst <gerhorst@cs.fau.de>
> ---
>  include/linux/bpf.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index a82efd34b741..61bde4520f5c 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -2164,12 +2164,12 @@ static inline bool bpf_allow_uninit_stack(void)
>
>  static inline bool bpf_bypass_spec_v1(void)
>  {
> -       return perfmon_capable();
> +       return perfmon_capable() || cpu_mitigations_off();
>  }
>
>  static inline bool bpf_bypass_spec_v4(void)
>  {
> -       return perfmon_capable();
> +       return perfmon_capable() || cpu_mitigations_off();
>  }

Yafang,

this patch breaks several
test_progs -t verifier

tests when system is booted with mitigations=off command line.

Please follow up with a patch to fix this.

As you noticed cpu_mitigations_off() is not quite right here.
The system might have booted without that command line, but
spec_v1 and spec_v4 mitigations are turned off.
Unfortunately there is no good way to check that atm.
Have you seen this patch set ?
https://lore.kernel.org/all/20231019181158.1982205-1-leitao@debian.org/
Please take a look at it and comment if you think it will help.

In the meantime please fix test_progs -t verifier

Thanks

Re: [PATCH bpf-next] bpf: Inherit system settings for CPU security mitigations

[Yafang Shao]

On Fri, Oct 20, 2023 at 8:42 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Oct 5, 2023 at 1:41 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > Currently, there exists a system-wide setting related to CPU security
> > mitigations, denoted as 'mitigations='. When set to 'mitigations=off', it
> > deactivates all optional CPU mitigations. Therefore, if we implement a
> > system-wide 'mitigations=off' setting, it should inherently bypass Spectre
> > v1 and Spectre v4 in the BPF subsystem.
> >
> > Please note that there is also a 'nospectre_v1' setting on x86 and ppc
> > architectures, though it is not currently exported. For the time being,
> > let's disregard it.
> >
> > This idea emerged during our discussion about potential Spectre v1 attacks
> > with Luis[1].
> >
> > [1]. https://lore.kernel.org/bpf/b4fc15f7-b204-767e-ebb9-fdb4233961fb@iogearbox.net/
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > Cc: Luis Gerhorst <gerhorst@cs.fau.de>
> > ---
> >  include/linux/bpf.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index a82efd34b741..61bde4520f5c 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -2164,12 +2164,12 @@ static inline bool bpf_allow_uninit_stack(void)
> >
> >  static inline bool bpf_bypass_spec_v1(void)
> >  {
> > -       return perfmon_capable();
> > +       return perfmon_capable() || cpu_mitigations_off();
> >  }
> >
> >  static inline bool bpf_bypass_spec_v4(void)
> >  {
> > -       return perfmon_capable();
> > +       return perfmon_capable() || cpu_mitigations_off();
> >  }
>
> Yafang,
>
> this patch breaks several
> test_progs -t verifier

Sorry, I miss that.

>
> tests when system is booted with mitigations=off command line.
>
> Please follow up with a patch to fix this.

will do it.

>
> As you noticed cpu_mitigations_off() is not quite right here.
> The system might have booted without that command line, but
> spec_v1 and spec_v4 mitigations are turned off.
> Unfortunately there is no good way to check that atm.
> Have you seen this patch set ?
> https://lore.kernel.org/all/20231019181158.1982205-1-leitao@debian.org/
> Please take a look at it and comment if you think it will help.

Thanks for your information. will take a look.

>
> In the meantime please fix test_progs -t verifier

sure

-- 
Regards
Yafang

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

[Yafang Shao]

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 as follows 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/948 PASSED, 388 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: 74/948 PASSED, 388 SKIPPED, 0 FAILED

Fixes: bc5bc309db45 ("bpf: Inherit system settings for CPU security mitigations")
Reported-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Link: 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 | 29 +++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/unpriv_helpers.c b/tools/testing/selftests/bpf/unpriv_helpers.c
index 2a6efbd0401e..2e756c89b37c 100644
--- a/tools/testing/selftests/bpf/unpriv_helpers.c
+++ b/tools/testing/selftests/bpf/unpriv_helpers.c
@@ -4,9 +4,36 @@
 #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;
+
+	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");
+		return false;
+	}
+
+	cmdline[sizeof(cmdline) - 1] = '\0';
+	for (c = strtok(cmdline, " \n"); c; c = strtok(NULL, " \n")) {
+		if (!strncmp(c, "mitigtions=off", strlen(c)))
+			return true;
+	}
+	return false;
+}
+
 bool get_unpriv_disabled(void)
 {
 	bool disabled;
@@ -22,5 +49,5 @@ bool get_unpriv_disabled(void)
 		disabled = true;
 	}
 
-	return disabled;
+	return disabled ? true : !get_mitigations_off();
 }
-- 
2.39.3

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

[Yafang Shao]

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 as follows 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/948 PASSED, 388 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: 74/948 PASSED, 388 SKIPPED, 0 FAILED

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 | 34 +++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

---
v1 -> v2: Fix leaked fd

diff --git a/tools/testing/selftests/bpf/unpriv_helpers.c b/tools/testing/selftests/bpf/unpriv_helpers.c
index 2a6efbd0401e..ca4760795f5d 100644
--- a/tools/testing/selftests/bpf/unpriv_helpers.c
+++ b/tools/testing/selftests/bpf/unpriv_helpers.c
@@ -4,9 +4,41 @@
 #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, "mitigtions=off", strlen(c))) {
+			ret = true;
+			break;
+		}
+	}
+
+out:
+	close(fd);
+	return ret;
+}
+
 bool get_unpriv_disabled(void)
 {
 	bool disabled;
@@ -22,5 +54,5 @@ bool get_unpriv_disabled(void)
 		disabled = true;
 	}
 
-	return disabled;
+	return disabled ? true : !get_mitigations_off();
 }
-- 
2.39.3

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

[Yafang Shao]

On Sun, Oct 22, 2023 at 5:49 PM Yafang Shao <laoar.shao@gmail.com> 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 as follows 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/948 PASSED, 388 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: 74/948 PASSED, 388 SKIPPED, 0 FAILED
>
> 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 | 34 +++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
>
> ---
> v1 -> v2: Fix leaked fd
>
> diff --git a/tools/testing/selftests/bpf/unpriv_helpers.c b/tools/testing/selftests/bpf/unpriv_helpers.c
> index 2a6efbd0401e..ca4760795f5d 100644
> --- a/tools/testing/selftests/bpf/unpriv_helpers.c
> +++ b/tools/testing/selftests/bpf/unpriv_helpers.c
> @@ -4,9 +4,41 @@
>  #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, "mitigtions=off", strlen(c))) {
> +                       ret = true;
> +                       break;
> +               }
> +       }
> +
> +out:
> +       close(fd);
> +       return ret;
> +}
> +
>  bool get_unpriv_disabled(void)
>  {
>         bool disabled;
> @@ -22,5 +54,5 @@ bool get_unpriv_disabled(void)
>                 disabled = true;
>         }
>
> -       return disabled;
> +       return disabled ? true : !get_mitigations_off();
>  }
> --
> 2.39.3
>

Pls. just igore this wrong patch. Sorry about the noise.
I must be in a sleep state currently. I will send a new one after I
get awake ...

-- 
Regards
Yafang

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

[Yafang Shao]

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(-)

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();
 }
-- 
2.39.3

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

[Yonghong Song]

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.

>   }

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

[Daniel Borkmann]

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!

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

[Yonghong Song]

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

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

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html