[PATCH 1/2] ARM: multi_v7_defconfig: enable BPF related configs

[PATCH 1/2] ARM: multi_v7_defconfig: enable BPF related configs

[Peter Robinson]

The BPF components are getting more widely used by various components
so we should enable them in the ARMv7 multi config to ensure they
get wider testing and don't regress.

Signed-off-by: Peter Robinson <pbrobinson@gmail.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Olof Johansson <olof@lixom.net>
---
 arch/arm/configs/multi_v7_defconfig | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index 1c7616815a86..8067c882fa2e 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -3,7 +3,10 @@ CONFIG_NO_HZ=y
 CONFIG_HIGH_RES_TIMERS=y
 CONFIG_PREEMPT=y
 CONFIG_CGROUPS=y
+CONFIG_CGROUP_BPF=y
 CONFIG_BLK_DEV_INITRD=y
+CONFIG_BPF_SYSCALL=y
+CONFIG_BPF_JIT_ALWAYS_ON=y
 CONFIG_EMBEDDED=y
 CONFIG_PERF_EVENTS=y
 CONFIG_MODULES=y
@@ -156,6 +159,8 @@ CONFIG_IPV6_MIP6=m
 CONFIG_IPV6_TUNNEL=m
 CONFIG_IPV6_MULTIPLE_TABLES=y
 CONFIG_NET_DSA=m
+CONFIG_BPF_JIT=y
+CONFIG_BPF_STREAM_PARSER=y
 CONFIG_CAN=y
 CONFIG_CAN_AT91=m
 CONFIG_CAN_FLEXCAN=m
-- 
2.19.1

[PATCH 2/2] arm64: defconfig: enable BPF related configs

[Peter Robinson]

The BPF components are getting more widely used by various components
so we should enable them in the ARMv7 multi config to ensure they
get wider testing and don't regress.

Signed-off-by: Peter Robinson <pbrobinson@gmail.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Olof Johansson <olof@lixom.net>
---
 arch/arm64/configs/defconfig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index c9a57d11330b..8748f6cf9fcd 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -19,6 +19,7 @@ CONFIG_BLK_CGROUP=y
 CONFIG_CGROUP_PIDS=y
 CONFIG_CGROUP_HUGETLB=y
 CONFIG_CPUSETS=y
+CONFIG_CGROUP_BPF=y
 CONFIG_CGROUP_DEVICE=y
 CONFIG_CGROUP_CPUACCT=y
 CONFIG_CGROUP_PERF=y
@@ -157,7 +158,10 @@ CONFIG_VLAN_8021Q_MVRP=y
 CONFIG_QRTR=m
 CONFIG_QRTR_SMD=m
 CONFIG_QRTR_TUN=m
+CONFIG_BPF_SYSCALL=y
 CONFIG_BPF_JIT=y
+CONFIG_BPF_JIT_ALWAYS_ON=y
+CONFIG_BPF_STREAM_PARSER=y
 CONFIG_BT=m
 CONFIG_BT_HIDP=m
 # CONFIG_BT_HS is not set
-- 
2.19.1

[PATCH 2/2] arm64: defconfig: enable BPF related configs

[Will Deacon]

Hi Peter, [+Jann and Ard]

On Sun, Nov 11, 2018 at 06:10:48PM +0000, Peter Robinson wrote:
> The BPF components are getting more widely used by various components
> so we should enable them in the ARMv7 multi config to ensure they
> get wider testing and don't regress.

^^^ Looks like it needs updating for arm64?

> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index c9a57d11330b..8748f6cf9fcd 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -19,6 +19,7 @@ CONFIG_BLK_CGROUP=y
>  CONFIG_CGROUP_PIDS=y
>  CONFIG_CGROUP_HUGETLB=y
>  CONFIG_CPUSETS=y
> +CONFIG_CGROUP_BPF=y
>  CONFIG_CGROUP_DEVICE=y
>  CONFIG_CGROUP_CPUACCT=y
>  CONFIG_CGROUP_PERF=y
> @@ -157,7 +158,10 @@ CONFIG_VLAN_8021Q_MVRP=y
>  CONFIG_QRTR=m
>  CONFIG_QRTR_SMD=m
>  CONFIG_QRTR_TUN=m
> +CONFIG_BPF_SYSCALL=y
>  CONFIG_BPF_JIT=y
> +CONFIG_BPF_JIT_ALWAYS_ON=y

Have you considered the security impact of these two options at all? Whilst
there appears to be a sysctl to disable unprivileged BPF usage, it's a
one-way switch so it defaults to "allow unprivileged access" so we're
opening up a pretty big attack surface by default with this change. My
concerns are broadly:

  1. BPF has a track record of serious security problems. This isn't
     intended as a criticism of the developers, it's more that the nature of
     the beast means it's a bit of a sitting duck. It's also constantly growing
     new functionality which would then likely be enabled by default under
     these options, so I fully expect things to continue as they are until
     the feature set has started to stabilise.

  2. Whilst forcing the JIT to be "always on" removes the interpreter from the
     equation (and it's the interpreter which was heavily targetted by
     spectre-v2), it also has the nasty consequent of effectively allowing the
     modules area to be sprayed with executable code.

Now, this is only defconfig we're talking about here, but the impression I
get from your patch is that your motivation is purely (ARMv7?) regression
testing and I would prefer us at least to consider whether there are other
implications from enabling this stuff by default on arm64.

Have other architectures already made this leap?

Thanks,

Will

> +CONFIG_BPF_STREAM_PARSER=y
>  CONFIG_BT=m
>  CONFIG_BT_HIDP=m
>  # CONFIG_BT_HS is not set
> -- 
> 2.19.1
> 

[PATCH 2/2] arm64: defconfig: enable BPF related configs

[Ard Biesheuvel]

On Mon, 12 Nov 2018 at 10:36, Will Deacon <will.deacon@arm.com> wrote:
>
> Hi Peter, [+Jann and Ard]
>
> On Sun, Nov 11, 2018 at 06:10:48PM +0000, Peter Robinson wrote:
> > The BPF components are getting more widely used by various components
> > so we should enable them in the ARMv7 multi config to ensure they
> > get wider testing and don't regress.
>
> ^^^ Looks like it needs updating for arm64?
>
> > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> > index c9a57d11330b..8748f6cf9fcd 100644
> > --- a/arch/arm64/configs/defconfig
> > +++ b/arch/arm64/configs/defconfig
> > @@ -19,6 +19,7 @@ CONFIG_BLK_CGROUP=y
> >  CONFIG_CGROUP_PIDS=y
> >  CONFIG_CGROUP_HUGETLB=y
> >  CONFIG_CPUSETS=y
> > +CONFIG_CGROUP_BPF=y
> >  CONFIG_CGROUP_DEVICE=y
> >  CONFIG_CGROUP_CPUACCT=y
> >  CONFIG_CGROUP_PERF=y
> > @@ -157,7 +158,10 @@ CONFIG_VLAN_8021Q_MVRP=y
> >  CONFIG_QRTR=m
> >  CONFIG_QRTR_SMD=m
> >  CONFIG_QRTR_TUN=m
> > +CONFIG_BPF_SYSCALL=y
> >  CONFIG_BPF_JIT=y
> > +CONFIG_BPF_JIT_ALWAYS_ON=y
>
> Have you considered the security impact of these two options at all? Whilst
> there appears to be a sysctl to disable unprivileged BPF usage, it's a
> one-way switch so it defaults to "allow unprivileged access" so we're
> opening up a pretty big attack surface by default with this change. My
> concerns are broadly:
>
>   1. BPF has a track record of serious security problems. This isn't
>      intended as a criticism of the developers, it's more that the nature of
>      the beast means it's a bit of a sitting duck. It's also constantly growing
>      new functionality which would then likely be enabled by default under
>      these options, so I fully expect things to continue as they are until
>      the feature set has started to stabilise.
>
>   2. Whilst forcing the JIT to be "always on" removes the interpreter from the
>      equation (and it's the interpreter which was heavily targetted by
>      spectre-v2), it also has the nasty consequent of effectively allowing the
>      modules area to be sprayed with executable code.
>
> Now, this is only defconfig we're talking about here, but the impression I
> get from your patch is that your motivation is purely (ARMv7?) regression
> testing and I would prefer us at least to consider whether there are other
> implications from enabling this stuff by default on arm64.
>
> Have other architectures already made this leap?
>

$ git grep CONFIG_BPF_SYSCALL=y arch/
arch/arm/configs/aspeed_g4_defconfig:CONFIG_BPF_SYSCALL=y
arch/arm/configs/aspeed_g5_defconfig:CONFIG_BPF_SYSCALL=y
arch/mips/configs/generic_defconfig:CONFIG_BPF_SYSCALL=y
arch/powerpc/configs/44x/fsp2_defconfig:CONFIG_BPF_SYSCALL=y
arch/powerpc/configs/powernv_defconfig:CONFIG_BPF_SYSCALL=y
arch/powerpc/configs/ppc64_defconfig:CONFIG_BPF_SYSCALL=y
arch/powerpc/configs/pseries_defconfig:CONFIG_BPF_SYSCALL=y
arch/riscv/configs/defconfig:CONFIG_BPF_SYSCALL=y
arch/s390/configs/debug_defconfig:CONFIG_BPF_SYSCALL=y
arch/s390/configs/performance_defconfig:CONFIG_BPF_SYSCALL=y
arch/s390/defconfig:CONFIG_BPF_SYSCALL=y

but nobody seems to enable CONFIG_BPF_JIT_ALWAYS_ON.

I sent some patches to move the BPF JIT allocations out of the module
range. Whether that really improves things in terms of security is not
obvious to me, but at least we stop wasting module region space (and
potentially KASAN shadow pages) on BPF programs.

If this is mainly for coverage, it would indeed be nice if we could at
least make it root only by default. However, if the distros are
enabling this in their default configurations, I'd prefer it if we at
least have a config that will help us spot issues early on.

[PATCH 2/2] arm64: defconfig: enable BPF related configs

[Will Deacon]

On Sat, Nov 17, 2018 at 03:18:04PM -0800, Ard Biesheuvel wrote:
> On Mon, 12 Nov 2018 at 10:36, Will Deacon <will.deacon@arm.com> wrote:
> > On Sun, Nov 11, 2018 at 06:10:48PM +0000, Peter Robinson wrote:
> > > The BPF components are getting more widely used by various components
> > > so we should enable them in the ARMv7 multi config to ensure they
> > > get wider testing and don't regress.
> >
> > Have other architectures already made this leap?
> >
> 
> $ git grep CONFIG_BPF_SYSCALL=y arch/
> arch/arm/configs/aspeed_g4_defconfig:CONFIG_BPF_SYSCALL=y
> arch/arm/configs/aspeed_g5_defconfig:CONFIG_BPF_SYSCALL=y
> arch/mips/configs/generic_defconfig:CONFIG_BPF_SYSCALL=y
> arch/powerpc/configs/44x/fsp2_defconfig:CONFIG_BPF_SYSCALL=y
> arch/powerpc/configs/powernv_defconfig:CONFIG_BPF_SYSCALL=y
> arch/powerpc/configs/ppc64_defconfig:CONFIG_BPF_SYSCALL=y
> arch/powerpc/configs/pseries_defconfig:CONFIG_BPF_SYSCALL=y
> arch/riscv/configs/defconfig:CONFIG_BPF_SYSCALL=y
> arch/s390/configs/debug_defconfig:CONFIG_BPF_SYSCALL=y
> arch/s390/configs/performance_defconfig:CONFIG_BPF_SYSCALL=y
> arch/s390/defconfig:CONFIG_BPF_SYSCALL=y
> 
> but nobody seems to enable CONFIG_BPF_JIT_ALWAYS_ON.
> 
> I sent some patches to move the BPF JIT allocations out of the module
> range. Whether that really improves things in terms of security is not
> obvious to me, but at least we stop wasting module region space (and
> potentially KASAN shadow pages) on BPF programs.
> 
> If this is mainly for coverage, it would indeed be nice if we could at
> least make it root only by default. However, if the distros are
> enabling this in their default configurations, I'd prefer it if we at
> least have a config that will help us spot issues early on.

That's a fair point on the distros. Peter, as author of the patch, please
can you take a look at the arm64 kernel configs from some popular
distributions and see which of these options they tend to enable?

Thanks,

Will

[PATCH 2/2] arm64: defconfig: enable BPF related configs

[Russell King - ARM Linux]

On Mon, Nov 26, 2018 at 03:38:20PM +0000, Will Deacon wrote:
> On Sat, Nov 17, 2018 at 03:18:04PM -0800, Ard Biesheuvel wrote:
> > On Mon, 12 Nov 2018 at 10:36, Will Deacon <will.deacon@arm.com> wrote:
> > > On Sun, Nov 11, 2018 at 06:10:48PM +0000, Peter Robinson wrote:
> > > > The BPF components are getting more widely used by various components
> > > > so we should enable them in the ARMv7 multi config to ensure they
> > > > get wider testing and don't regress.
> > >
> > > Have other architectures already made this leap?
> > >
> > 
> > $ git grep CONFIG_BPF_SYSCALL=y arch/
> > arch/arm/configs/aspeed_g4_defconfig:CONFIG_BPF_SYSCALL=y
> > arch/arm/configs/aspeed_g5_defconfig:CONFIG_BPF_SYSCALL=y
> > arch/mips/configs/generic_defconfig:CONFIG_BPF_SYSCALL=y
> > arch/powerpc/configs/44x/fsp2_defconfig:CONFIG_BPF_SYSCALL=y
> > arch/powerpc/configs/powernv_defconfig:CONFIG_BPF_SYSCALL=y
> > arch/powerpc/configs/ppc64_defconfig:CONFIG_BPF_SYSCALL=y
> > arch/powerpc/configs/pseries_defconfig:CONFIG_BPF_SYSCALL=y
> > arch/riscv/configs/defconfig:CONFIG_BPF_SYSCALL=y
> > arch/s390/configs/debug_defconfig:CONFIG_BPF_SYSCALL=y
> > arch/s390/configs/performance_defconfig:CONFIG_BPF_SYSCALL=y
> > arch/s390/defconfig:CONFIG_BPF_SYSCALL=y
> > 
> > but nobody seems to enable CONFIG_BPF_JIT_ALWAYS_ON.
> > 
> > I sent some patches to move the BPF JIT allocations out of the module
> > range. Whether that really improves things in terms of security is not
> > obvious to me, but at least we stop wasting module region space (and
> > potentially KASAN shadow pages) on BPF programs.
> > 
> > If this is mainly for coverage, it would indeed be nice if we could at
> > least make it root only by default. However, if the distros are
> > enabling this in their default configurations, I'd prefer it if we at
> > least have a config that will help us spot issues early on.
> 
> That's a fair point on the distros. Peter, as author of the patch, please
> can you take a look at the arm64 kernel configs from some popular
> distributions and see which of these options they tend to enable?

Another point to consider is whether the BPF JIT creates code that
is safe from Spectre, or whether it opens up more Spectre issues.
If it isn't, maybe the better option is to disable kernel-side BPF
entirely until it is fixed up.

I've raised that point for 32-bit ARM already, and the 32-bit ARM
BPF JIT isn't going to have the Spectre mitigations applied (since
no one appears interested whenever I've raised it.)  So, for 32-bit
ARM, it's better to have kernel-side BPF entirely disabled if you
care about Spectre mitigations.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

[PATCH 2/2] arm64: defconfig: enable BPF related configs

[Will Deacon]

Hi Russell,

On Mon, Nov 26, 2018 at 03:52:47PM +0000, Russell King - ARM Linux wrote:
> On Mon, Nov 26, 2018 at 03:38:20PM +0000, Will Deacon wrote:
> > On Sat, Nov 17, 2018 at 03:18:04PM -0800, Ard Biesheuvel wrote:
> > > On Mon, 12 Nov 2018 at 10:36, Will Deacon <will.deacon@arm.com> wrote:
> > > > On Sun, Nov 11, 2018 at 06:10:48PM +0000, Peter Robinson wrote:
> > > > > The BPF components are getting more widely used by various components
> > > > > so we should enable them in the ARMv7 multi config to ensure they
> > > > > get wider testing and don't regress.
> > > >
> > > > Have other architectures already made this leap?
> > > >
> > > 
> > > $ git grep CONFIG_BPF_SYSCALL=y arch/
> > > arch/arm/configs/aspeed_g4_defconfig:CONFIG_BPF_SYSCALL=y
> > > arch/arm/configs/aspeed_g5_defconfig:CONFIG_BPF_SYSCALL=y
> > > arch/mips/configs/generic_defconfig:CONFIG_BPF_SYSCALL=y
> > > arch/powerpc/configs/44x/fsp2_defconfig:CONFIG_BPF_SYSCALL=y
> > > arch/powerpc/configs/powernv_defconfig:CONFIG_BPF_SYSCALL=y
> > > arch/powerpc/configs/ppc64_defconfig:CONFIG_BPF_SYSCALL=y
> > > arch/powerpc/configs/pseries_defconfig:CONFIG_BPF_SYSCALL=y
> > > arch/riscv/configs/defconfig:CONFIG_BPF_SYSCALL=y
> > > arch/s390/configs/debug_defconfig:CONFIG_BPF_SYSCALL=y
> > > arch/s390/configs/performance_defconfig:CONFIG_BPF_SYSCALL=y
> > > arch/s390/defconfig:CONFIG_BPF_SYSCALL=y
> > > 
> > > but nobody seems to enable CONFIG_BPF_JIT_ALWAYS_ON.
> > > 
> > > I sent some patches to move the BPF JIT allocations out of the module
> > > range. Whether that really improves things in terms of security is not
> > > obvious to me, but at least we stop wasting module region space (and
> > > potentially KASAN shadow pages) on BPF programs.
> > > 
> > > If this is mainly for coverage, it would indeed be nice if we could at
> > > least make it root only by default. However, if the distros are
> > > enabling this in their default configurations, I'd prefer it if we at
> > > least have a config that will help us spot issues early on.
> > 
> > That's a fair point on the distros. Peter, as author of the patch, please
> > can you take a look at the arm64 kernel configs from some popular
> > distributions and see which of these options they tend to enable?
> 
> Another point to consider is whether the BPF JIT creates code that
> is safe from Spectre, or whether it opens up more Spectre issues.
> If it isn't, maybe the better option is to disable kernel-side BPF
> entirely until it is fixed up.
> 
> I've raised that point for 32-bit ARM already, and the 32-bit ARM
> BPF JIT isn't going to have the Spectre mitigations applied (since
> no one appears interested whenever I've raised it.)  So, for 32-bit
> ARM, it's better to have kernel-side BPF entirely disabled if you
> care about Spectre mitigations.

I thought that the Spectre mitigations (at least for variant 1) were handled
by the core BPF JIT, as a result of b2157399cc98 ("bpf: prevent out-of-bounds
speculation")? I think we rely on randomization to protect against variant 2
for JITted code.

Did I miss something here?

Will

[PATCH 2/2] arm64: defconfig: enable BPF related configs

[Russell King - ARM Linux]

On Tue, Nov 27, 2018 at 07:31:18PM +0000, Will Deacon wrote:
> Hi Russell,
> 
> On Mon, Nov 26, 2018 at 03:52:47PM +0000, Russell King - ARM Linux wrote:
> > On Mon, Nov 26, 2018 at 03:38:20PM +0000, Will Deacon wrote:
> > > On Sat, Nov 17, 2018 at 03:18:04PM -0800, Ard Biesheuvel wrote:
> > > > On Mon, 12 Nov 2018 at 10:36, Will Deacon <will.deacon@arm.com> wrote:
> > > > > On Sun, Nov 11, 2018 at 06:10:48PM +0000, Peter Robinson wrote:
> > > > > > The BPF components are getting more widely used by various components
> > > > > > so we should enable them in the ARMv7 multi config to ensure they
> > > > > > get wider testing and don't regress.
> > > > >
> > > > > Have other architectures already made this leap?
> > > > >
> > > > 
> > > > $ git grep CONFIG_BPF_SYSCALL=y arch/
> > > > arch/arm/configs/aspeed_g4_defconfig:CONFIG_BPF_SYSCALL=y
> > > > arch/arm/configs/aspeed_g5_defconfig:CONFIG_BPF_SYSCALL=y
> > > > arch/mips/configs/generic_defconfig:CONFIG_BPF_SYSCALL=y
> > > > arch/powerpc/configs/44x/fsp2_defconfig:CONFIG_BPF_SYSCALL=y
> > > > arch/powerpc/configs/powernv_defconfig:CONFIG_BPF_SYSCALL=y
> > > > arch/powerpc/configs/ppc64_defconfig:CONFIG_BPF_SYSCALL=y
> > > > arch/powerpc/configs/pseries_defconfig:CONFIG_BPF_SYSCALL=y
> > > > arch/riscv/configs/defconfig:CONFIG_BPF_SYSCALL=y
> > > > arch/s390/configs/debug_defconfig:CONFIG_BPF_SYSCALL=y
> > > > arch/s390/configs/performance_defconfig:CONFIG_BPF_SYSCALL=y
> > > > arch/s390/defconfig:CONFIG_BPF_SYSCALL=y
> > > > 
> > > > but nobody seems to enable CONFIG_BPF_JIT_ALWAYS_ON.
> > > > 
> > > > I sent some patches to move the BPF JIT allocations out of the module
> > > > range. Whether that really improves things in terms of security is not
> > > > obvious to me, but at least we stop wasting module region space (and
> > > > potentially KASAN shadow pages) on BPF programs.
> > > > 
> > > > If this is mainly for coverage, it would indeed be nice if we could at
> > > > least make it root only by default. However, if the distros are
> > > > enabling this in their default configurations, I'd prefer it if we at
> > > > least have a config that will help us spot issues early on.
> > > 
> > > That's a fair point on the distros. Peter, as author of the patch, please
> > > can you take a look at the arm64 kernel configs from some popular
> > > distributions and see which of these options they tend to enable?
> > 
> > Another point to consider is whether the BPF JIT creates code that
> > is safe from Spectre, or whether it opens up more Spectre issues.
> > If it isn't, maybe the better option is to disable kernel-side BPF
> > entirely until it is fixed up.
> > 
> > I've raised that point for 32-bit ARM already, and the 32-bit ARM
> > BPF JIT isn't going to have the Spectre mitigations applied (since
> > no one appears interested whenever I've raised it.)  So, for 32-bit
> > ARM, it's better to have kernel-side BPF entirely disabled if you
> > care about Spectre mitigations.
> 
> I thought that the Spectre mitigations (at least for variant 1) were handled
> by the core BPF JIT, as a result of b2157399cc98 ("bpf: prevent out-of-bounds
> speculation")? I think we rely on randomization to protect against variant 2
> for JITted code.
> 
> Did I miss something here?

It depends - there's at least one case in eBPF that requires the JIT to
be mitigated, and that's the tail-call stuff.  That's prime Spectre
exploitation territory, because the pseudocode for that eBPF instruction
is:

	if (index >= array->map.max_entries)
		goto out;
	if (tail_call_cnt > MAX_TAIL_CALL_CNT)
		goto out;
	tail_call_cnt++;
	prog = array->ptrs[index];
	if (prog == NULL)
		goto out;
	goto *(prog->bpf_func + prologue_size);

If 'index' is larger than map.max_entries, and we speculate past the
goto, then out of bounds cache lines in the array->ptrs[] could be
loaded.

In cBPF, it seems to me that there is enough support to do something
like:

	if (val > max)		// BPF_JMP | BPF_JGT | BPF_K
		jump somewhere;
	addr += val;		// BPF_ALU | BPF_ADD | BPF_W
	val2 = *addr;		// BPF_LDX | BPF_MEM | BPF_W

which looks to me like it can fit the Spectre exploitation pattern,
but is much harder to work around than the eBPF tailcall (we would
need to detect the BPF instruction sequence.)  I haven't validated
that such a sequence is actually possible in cBPF, but as cBPF is
used for packet filtering, and there's a need for it to support
jumping over the headers which can be variable lengths, then I
would not be surprised if such BPF sequences were possible.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

[PATCH 2/2] arm64: defconfig: enable BPF related configs

[Will Deacon]

[+Daniel and Alexei]

On Tue, Nov 27, 2018 at 11:18:39PM +0000, Russell King - ARM Linux wrote:
> On Tue, Nov 27, 2018 at 07:31:18PM +0000, Will Deacon wrote:
> > On Mon, Nov 26, 2018 at 03:52:47PM +0000, Russell King - ARM Linux wrote:
> > > Another point to consider is whether the BPF JIT creates code that
> > > is safe from Spectre, or whether it opens up more Spectre issues.
> > > If it isn't, maybe the better option is to disable kernel-side BPF
> > > entirely until it is fixed up.
> > > 
> > > I've raised that point for 32-bit ARM already, and the 32-bit ARM
> > > BPF JIT isn't going to have the Spectre mitigations applied (since
> > > no one appears interested whenever I've raised it.)  So, for 32-bit
> > > ARM, it's better to have kernel-side BPF entirely disabled if you
> > > care about Spectre mitigations.
> > 
> > I thought that the Spectre mitigations (at least for variant 1) were handled
> > by the core BPF JIT, as a result of b2157399cc98 ("bpf: prevent out-of-bounds
> > speculation")? I think we rely on randomization to protect against variant 2
> > for JITted code.
> > 
> > Did I miss something here?
> 
> It depends - there's at least one case in eBPF that requires the JIT to
> be mitigated, and that's the tail-call stuff.  That's prime Spectre
> exploitation territory, because the pseudocode for that eBPF instruction
> is:
> 
> 	if (index >= array->map.max_entries)
> 		goto out;
> 	if (tail_call_cnt > MAX_TAIL_CALL_CNT)
> 		goto out;
> 	tail_call_cnt++;
> 	prog = array->ptrs[index];
> 	if (prog == NULL)
> 		goto out;
> 	goto *(prog->bpf_func + prologue_size);
> 
> If 'index' is larger than map.max_entries, and we speculate past the
> goto, then out of bounds cache lines in the array->ptrs[] could be
> loaded.

It looks like the verifier rewrites the BPF instructions so that the
index is masked here:


	/* instead of changing every JIT dealing with tail_call
	 * emit two extra insns:
	 * if (index >= max_entries) goto out;
	 * index &= array->index_mask;
	 * to avoid out-of-bounds cpu speculation
	 */
	if (bpf_map_ptr_poisoned(aux)) {
		verbose(env, "tail_call abusing map_ptr\n");
		return -EINVAL;
	}

	map_ptr = BPF_MAP_PTR(aux->map_state);
	insn_buf[0] = BPF_JMP_IMM(BPF_JGE, BPF_REG_3,
			map_ptr->max_entries, 2);
	insn_buf[1] = BPF_ALU32_IMM(BPF_AND, BPF_REG_3,
			container_of(map_ptr,
				struct bpf_array,
				map)->index_mask);
	insn_buf[2] = *insn;
	cnt = 3;
	new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);


so I think that should be ok. In the worst case, we have some redundant
bounds checking of the index in the arch code.

> In cBPF, it seems to me that there is enough support to do something
> like:
> 
> 	if (val > max)		// BPF_JMP | BPF_JGT | BPF_K
> 		jump somewhere;
> 	addr += val;		// BPF_ALU | BPF_ADD | BPF_W
> 	val2 = *addr;		// BPF_LDX | BPF_MEM | BPF_W
> 
> which looks to me like it can fit the Spectre exploitation pattern,
> but is much harder to work around than the eBPF tailcall (we would
> need to detect the BPF instruction sequence.)  I haven't validated
> that such a sequence is actually possible in cBPF, but as cBPF is
> used for packet filtering, and there's a need for it to support
> jumping over the headers which can be variable lengths, then I
> would not be surprised if such BPF sequences were possible.

I was under the impression that the BPF type system didn't allow you to
construct arbitrary addresses like that, but I must defer to the BPF
developers on the details.

Will