[SRU][Xenial][PATCH 0/3] Use upstream Spectre variant 1 BPF mitigations

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU][Xenial][PATCH 0/3] Use upstream Spectre variant 1 BPF mitigations

Tyler Hicks-2
This patchset moves the Xenial kernel over to the upstream mitigations
for Spectre variant 1 (CVE-2017-5753). The upstream mitigations were
mostly already in place thanks to the following commits that we picked
up via linux-stable rebases:

b2157399cc98 ("bpf: prevent out-of-bounds speculation")
bbeb6e4323da ("bpf, array: fix overflow in max_entries and undefined behavior in index_mask")

However, a fix commit for b2157399cc98 was still missing:

c93552c443eb ("bpf: properly enforce index mask to prevent out-of-bounds speculation")

I've backported the missing patch and reverted the out-of-tree
mitigations for Spectre variant 1 in the BPF code now that all the
corresponding upstream commits are in place.

I tested these changes using the upstream kernel's test-verifier and
test-verifier-log BPF selftests. While there are many failures due to
the tests from Linus HEAD being used on a 4.4 based kernel, the test
results are the same with and without these patches applied. I ran the
tests as an unprivileged user and as root.

It is also worth mentioning that the backport of c93552c443eb matches
what SUSE has done in their 4.4 kernel:

 https://kernel.opensuse.org/cgit/kernel-source/tree/patches.fixes/bpf-properly-enforce-index-mask-to-prevent-out-of-bo.patch?h=SLE12-SP3

Tyler


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/3] bpf: properly enforce index mask to prevent out-of-bounds speculation

Tyler Hicks-2
From: Daniel Borkmann <[hidden email]>

While reviewing the verifier code, I recently noticed that the
following two program variants in relation to tail calls can be
loaded.

Variant 1:

  # bpftool p d x i 15
    0: (15) if r1 == 0x0 goto pc+3
    1: (18) r2 = map[id:5]
    3: (05) goto pc+2
    4: (18) r2 = map[id:6]
    6: (b7) r3 = 7
    7: (35) if r3 >= 0xa0 goto pc+2
    8: (54) (u32) r3 &= (u32) 255
    9: (85) call bpf_tail_call#12
   10: (b7) r0 = 1
   11: (95) exit

  # bpftool m s i 5
    5: prog_array  flags 0x0
        key 4B  value 4B  max_entries 4  memlock 4096B
  # bpftool m s i 6
    6: prog_array  flags 0x0
        key 4B  value 4B  max_entries 160  memlock 4096B

Variant 2:

  # bpftool p d x i 20
    0: (15) if r1 == 0x0 goto pc+3
    1: (18) r2 = map[id:8]
    3: (05) goto pc+2
    4: (18) r2 = map[id:7]
    6: (b7) r3 = 7
    7: (35) if r3 >= 0x4 goto pc+2
    8: (54) (u32) r3 &= (u32) 3
    9: (85) call bpf_tail_call#12
   10: (b7) r0 = 1
   11: (95) exit

  # bpftool m s i 8
    8: prog_array  flags 0x0
        key 4B  value 4B  max_entries 160  memlock 4096B
  # bpftool m s i 7
    7: prog_array  flags 0x0
        key 4B  value 4B  max_entries 4  memlock 4096B

In both cases the index masking inserted by the verifier in order
to control out of bounds speculation from a CPU via b2157399cc98
("bpf: prevent out-of-bounds speculation") seems to be incorrect
in what it is enforcing. In the 1st variant, the mask is applied
from the map with the significantly larger number of entries where
we would allow to a certain degree out of bounds speculation for
the smaller map, and in the 2nd variant where the mask is applied
from the map with the smaller number of entries, we get buggy
behavior since we truncate the index of the larger map.

The original intent from commit b2157399cc98 is to reject such
occasions where two or more different tail call maps are used
in the same tail call helper invocation. However, the check on
the BPF_MAP_PTR_POISON is never hit since we never poisoned the
saved pointer in the first place! We do this explicitly for map
lookups but in case of tail calls we basically used the tail
call map in insn_aux_data that was processed in the most recent
path which the verifier walked. Thus any prior path that stored
a pointer in insn_aux_data at the helper location was always
overridden.

Fix it by moving the map pointer poison logic into a small helper
that covers both BPF helpers with the same logic. After that in
fixup_bpf_calls() the poison check is then hit for tail calls
and the program rejected. Latter only happens in unprivileged
case since this is the *only* occasion where a rewrite needs to
happen, and where such rewrite is specific to the map (max_entries,
index_mask). In the privileged case the rewrite is generic for
the insn->imm / insn->code update so multiple maps from different
paths can be handled just fine since all the remaining logic
happens in the instruction processing itself. This is similar
to the case of map lookups: in case there is a collision of
maps in fixup_bpf_calls() we must skip the inlined rewrite since
this will turn the generic instruction sequence into a non-
generic one. Thus the patch_call_imm will simply update the
insn->imm location where the bpf_map_lookup_elem() will later
take care of the dispatch. Given we need this 'poison' state
as a check, the information of whether a map is an unpriv_array
gets lost, so enforcing it prior to that needs an additional
state. In general this check is needed since there are some
complex and tail call intensive BPF programs out there where
LLVM tends to generate such code occasionally. We therefore
convert the map_ptr rather into map_state to store all this
w/o extra memory overhead, and the bit whether one of the maps
involved in the collision was from an unpriv_array thus needs
to be retained as well there.

Fixes: b2157399cc98 ("bpf: prevent out-of-bounds speculation")
Signed-off-by: Daniel Borkmann <[hidden email]>
Acked-by: Alexei Starovoitov <[hidden email]>
Signed-off-by: Alexei Starovoitov <[hidden email]>

CVE-2017-5753

(backported from commit c93552c443ebc63b14e26e46d2e76941c88e0d71)
[tyhicks: Ignore pointer poison related changes since poisoning is not
          part of 4.4]
Signed-off-by: Tyler Hicks <[hidden email]>
---
 kernel/bpf/verifier.c | 57 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 46 insertions(+), 11 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index b8d06bb938a0..a0251bcf35e8 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -190,12 +190,28 @@ struct bpf_insn_aux_data {
  bool seen; /* this insn was processed by the verifier */
  union {
  enum bpf_reg_type ptr_type; /* pointer type for load/store insns */
- struct bpf_map *map_ptr; /* pointer for call insn into lookup_elem */
+ unsigned long map_state; /* pointer/poison value for maps */
  };
 };
 
 #define MAX_USED_MAPS 64 /* max number of maps accessed by one eBPF program */
 
+#define BPF_MAP_PTR_UNPRIV 1UL
+#define BPF_MAP_PTR(X) ((struct bpf_map *)((X) & ~BPF_MAP_PTR_UNPRIV))
+
+static bool bpf_map_ptr_unpriv(const struct bpf_insn_aux_data *aux)
+{
+ return aux->map_state & BPF_MAP_PTR_UNPRIV;
+}
+
+static void bpf_map_ptr_store(struct bpf_insn_aux_data *aux,
+      const struct bpf_map *map, bool unpriv)
+{
+ unpriv |= bpf_map_ptr_unpriv(aux);
+ aux->map_state = (unsigned long)map |
+ (unpriv ? BPF_MAP_PTR_UNPRIV : 0UL);
+}
+
 /* single container for all structs
  * one verifier_env per bpf_check() call
  */
@@ -967,6 +983,25 @@ error:
  return -EINVAL;
 }
 
+static int
+record_func_map(struct verifier_env *env, struct bpf_map *map_ptr,
+ int func_id, int insn_idx)
+{
+ struct bpf_insn_aux_data *aux = &env->insn_aux_data[insn_idx];
+
+ if (func_id != BPF_FUNC_tail_call &&
+    func_id != BPF_FUNC_map_lookup_elem)
+ return 0;
+ if (map_ptr == NULL) {
+ verbose("kernel subsystem misconfigured verifier\n");
+ return -EINVAL;
+ }
+
+ if (!BPF_MAP_PTR(aux->map_state))
+ bpf_map_ptr_store(aux, map_ptr, map_ptr->unpriv_array);
+ return 0;
+}
+
 static int check_call(struct verifier_env *env, int func_id, int insn_idx)
 {
  struct verifier_state *state = &env->cur_state;
@@ -1003,13 +1038,6 @@ static int check_call(struct verifier_env *env, int func_id, int insn_idx)
  err = check_func_arg(env, BPF_REG_2, fn->arg2_type, &map);
  if (err)
  return err;
- if (func_id == BPF_FUNC_tail_call) {
- if (map == NULL) {
- verbose("verifier bug\n");
- return -EINVAL;
- }
- env->insn_aux_data[insn_idx].map_ptr = map;
- }
  err = check_func_arg(env, BPF_REG_3, fn->arg3_type, &map);
  if (err)
  return err;
@@ -1020,6 +1048,10 @@ static int check_call(struct verifier_env *env, int func_id, int insn_idx)
  if (err)
  return err;
 
+ err = record_func_map(env, map, func_id, insn_idx);
+ if (err)
+ return err;
+
  /* reset caller saved regs */
  for (i = 0; i < CALLER_SAVED_REGS; i++) {
  reg = regs + caller_saved[i];
@@ -2262,6 +2294,7 @@ static int fixup_bpf_calls(struct verifier_env *env)
  struct bpf_insn *insn = prog->insnsi;
  const struct bpf_func_proto *fn;
  const int insn_cnt = prog->len;
+ struct bpf_insn_aux_data *aux;
  struct bpf_insn insn_buf[16];
  struct bpf_prog *new_prog;
  struct bpf_map *map_ptr;
@@ -2302,15 +2335,17 @@ static int fixup_bpf_calls(struct verifier_env *env)
  insn->imm = 0;
  insn->code |= BPF_X;
 
+ aux = &env->insn_aux_data[i + delta];
+ if (!bpf_map_ptr_unpriv(aux))
+ continue;
+
  /* 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
  */
- map_ptr = env->insn_aux_data[i + delta].map_ptr;
- if (!map_ptr->unpriv_array)
- continue;
+ 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,
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/3] Revert "UBUNTU: SAUCE: bpf: Use barrier_nospec() instead of osb()"

Tyler Hicks-2
In reply to this post by Tyler Hicks-2
This reverts commit 5b9ee25974f7e54c94e8b2ef307d9830d714f490 which was
part of an out-of-tree mitigation for CVE-2017-5753 (Spectre variant 1),
in the BPF subsystem, that was available at the time of the coordinated
release date. The Ubuntu kernel has since rebased on top of newer
linux-stable releases and picked up commit b2157399cc98 ("bpf: prevent
out-of-bounds speculation") which is upstream's mitigation of Spectre
variant 1 in the BPF code.

CVE-2017-5753

Signed-off-by: Tyler Hicks <[hidden email]>
---
 kernel/bpf/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 89e4f17b2c4d..2d77df1a22c9 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -27,9 +27,9 @@
 #include <linux/random.h>
 #include <linux/moduleloader.h>
 #include <linux/bpf.h>
-#include <linux/nospec.h>
 
 #include <asm/unaligned.h>
+#include <asm/barrier.h>
 
 /* Registers */
 #define BPF_R0 regs[BPF_REG_0]
@@ -632,7 +632,7 @@ select_insn:
  DST = IMM;
  CONT;
  LD_IMM_DW:
- barrier_nospec();
+ osb();
  DST = (u64) (u32) insn[0].imm | ((u64) (u32) insn[1].imm) << 32;
  insn++;
  CONT;
@@ -847,7 +847,7 @@ out:
  *(SIZE *)(unsigned long) (DST + insn->off) = IMM; \
  CONT; \
  LDX_MEM_##SIZEOP: \
- barrier_nospec(); \
+ osb(); \
  DST = *(SIZE *)(unsigned long) (SRC + insn->off); \
  CONT;
 
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 3/3] Revert "bpf: prevent speculative execution in eBPF interpreter"

Tyler Hicks-2
In reply to this post by Tyler Hicks-2
This reverts commit 81774d484f267526dd8aad449c9465b015e60e0c which was
part of an out-of-tree mitigation for CVE-2017-5753 (Spectre variant 1),
in the BPF subsystem, that was available at the time of the coordinated
release date. The Ubuntu kernel has since rebased on top of newer
linux-stable releases and picked up commit b2157399cc98 ("bpf: prevent
out-of-bounds speculation") which is upstream's mitigation of Spectre
variant 1 in the BPF code.

CVE-2017-5753

Signed-off-by: Tyler Hicks <[hidden email]>
---
 kernel/bpf/core.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 2d77df1a22c9..f853b2adadc5 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -29,7 +29,6 @@
 #include <linux/bpf.h>
 
 #include <asm/unaligned.h>
-#include <asm/barrier.h>
 
 /* Registers */
 #define BPF_R0 regs[BPF_REG_0]
@@ -632,7 +631,6 @@ select_insn:
  DST = IMM;
  CONT;
  LD_IMM_DW:
- osb();
  DST = (u64) (u32) insn[0].imm | ((u64) (u32) insn[1].imm) << 32;
  insn++;
  CONT;
@@ -847,7 +845,6 @@ out:
  *(SIZE *)(unsigned long) (DST + insn->off) = IMM; \
  CONT; \
  LDX_MEM_##SIZEOP: \
- osb(); \
  DST = *(SIZE *)(unsigned long) (SRC + insn->off); \
  CONT;
 
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU][Xenial][PATCH 0/3] Use upstream Spectre variant 1 BPF mitigations

Stefan Bader-2
In reply to this post by Tyler Hicks-2
On 11.09.2018 07:35, Tyler Hicks wrote:

> This patchset moves the Xenial kernel over to the upstream mitigations
> for Spectre variant 1 (CVE-2017-5753). The upstream mitigations were
> mostly already in place thanks to the following commits that we picked
> up via linux-stable rebases:
>
> b2157399cc98 ("bpf: prevent out-of-bounds speculation")
> bbeb6e4323da ("bpf, array: fix overflow in max_entries and undefined behavior in index_mask")
>
> However, a fix commit for b2157399cc98 was still missing:
>
> c93552c443eb ("bpf: properly enforce index mask to prevent out-of-bounds speculation")
>
> I've backported the missing patch and reverted the out-of-tree
> mitigations for Spectre variant 1 in the BPF code now that all the
> corresponding upstream commits are in place.
>
> I tested these changes using the upstream kernel's test-verifier and
> test-verifier-log BPF selftests. While there are many failures due to
> the tests from Linus HEAD being used on a 4.4 based kernel, the test
> results are the same with and without these patches applied. I ran the
> tests as an unprivileged user and as root.
>
> It is also worth mentioning that the backport of c93552c443eb matches
> what SUSE has done in their 4.4 kernel:
>
>  https://kernel.opensuse.org/cgit/kernel-source/tree/patches.fixes/bpf-properly-enforce-index-mask-to-prevent-out-of-bo.patch?h=SLE12-SP3
>
> Tyler
>
>
Acked-by: Stefan Bader <[hidden email]>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU][Xenial][PATCH 0/3] Use upstream Spectre variant 1 BPF mitigations

Kleber Souza
In reply to this post by Tyler Hicks-2
On 09/11/18 07:35, Tyler Hicks wrote:

> This patchset moves the Xenial kernel over to the upstream mitigations
> for Spectre variant 1 (CVE-2017-5753). The upstream mitigations were
> mostly already in place thanks to the following commits that we picked
> up via linux-stable rebases:
>
> b2157399cc98 ("bpf: prevent out-of-bounds speculation")
> bbeb6e4323da ("bpf, array: fix overflow in max_entries and undefined behavior in index_mask")
>
> However, a fix commit for b2157399cc98 was still missing:
>
> c93552c443eb ("bpf: properly enforce index mask to prevent out-of-bounds speculation")
>
> I've backported the missing patch and reverted the out-of-tree
> mitigations for Spectre variant 1 in the BPF code now that all the
> corresponding upstream commits are in place.
>
> I tested these changes using the upstream kernel's test-verifier and
> test-verifier-log BPF selftests. While there are many failures due to
> the tests from Linus HEAD being used on a 4.4 based kernel, the test
> results are the same with and without these patches applied. I ran the
> tests as an unprivileged user and as root.
>
> It is also worth mentioning that the backport of c93552c443eb matches
> what SUSE has done in their 4.4 kernel:
>
>  https://kernel.opensuse.org/cgit/kernel-source/tree/patches.fixes/bpf-properly-enforce-index-mask-to-prevent-out-of-bo.patch?h=SLE12-SP3
>
> Tyler
>
>

Acked-by: Kleber Sacilotto de Souza <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED: [SRU][Xenial][PATCH 0/3] Use upstream Spectre variant 1 BPF mitigations

Kleber Souza
In reply to this post by Tyler Hicks-2
On 09/11/18 07:35, Tyler Hicks wrote:

> This patchset moves the Xenial kernel over to the upstream mitigations
> for Spectre variant 1 (CVE-2017-5753). The upstream mitigations were
> mostly already in place thanks to the following commits that we picked
> up via linux-stable rebases:
>
> b2157399cc98 ("bpf: prevent out-of-bounds speculation")
> bbeb6e4323da ("bpf, array: fix overflow in max_entries and undefined behavior in index_mask")
>
> However, a fix commit for b2157399cc98 was still missing:
>
> c93552c443eb ("bpf: properly enforce index mask to prevent out-of-bounds speculation")
>
> I've backported the missing patch and reverted the out-of-tree
> mitigations for Spectre variant 1 in the BPF code now that all the
> corresponding upstream commits are in place.
>
> I tested these changes using the upstream kernel's test-verifier and
> test-verifier-log BPF selftests. While there are many failures due to
> the tests from Linus HEAD being used on a 4.4 based kernel, the test
> results are the same with and without these patches applied. I ran the
> tests as an unprivileged user and as root.
>
> It is also worth mentioning that the backport of c93552c443eb matches
> what SUSE has done in their 4.4 kernel:
>
>  https://kernel.opensuse.org/cgit/kernel-source/tree/patches.fixes/bpf-properly-enforce-index-mask-to-prevent-out-of-bo.patch?h=SLE12-SP3
>
> Tyler
>
>

Applied to xenial/master-next branch.

Thanks,
Kleber

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team