netdev - [PATCH bpf v2 1/2] bpf: Load zeros for narrow loads beyond target field

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20200723095953.1003302-2-jakub@cloudflare.com>
Date:   Thu, 23 Jul 2020 11:59:52 +0200
From:   Jakub Sitnicki <jakub@...udflare.com>
To:     bpf@...r.kernel.org
Cc:     netdev@...r.kernel.org, kernel-team@...udflare.com,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Yonghong Song <yhs@...com>
Subject: [PATCH bpf v2 1/2] bpf: Load zeros for narrow loads beyond target field

For narrow loads from context that are:

  1) as big in size as the target field, and
  2) at an offset beyond the target field,

the verifier does not emit the shift-and-mask instruction sequence
following the target field load instruction, as it happens for narrow loads
smaller in size than the target field width.

This has an unexpected effect of loading the same data, no matter what the
offset. While, arguably, the expected behavior is to load zeros for offsets
that beyond the target field.

For instance, 2-byte load from a 4-byte context field, backed by a 2-byte
target field at an offset of 2 bytes results in:

  $ cat progs/test_narrow_load.c
  [...]
  SEC("sk_reuseport/narrow_load_half_word")
  int narrow_load_half_word(struct sk_reuseport_md *ctx)
  {
  	__u16 *half;

  	half = (__u16 *)&ctx->ip_protocol;
  	if (half[0] != IPPROTO_UDP)
  		return SK_DROP;
  	if (half[1] != 0)
  		return SK_DROP;
  	return SK_PASS;
  }

  $ llvm-objdump -S --no-show-raw-insn ...
  [...]
  0000000000000000 narrow_load_half_word:
  ; {
         0:       w0 = 0
  ;       if (half[0] != IPPROTO_UDP)
         1:       r2 = *(u16 *)(r1 + 24)
         2:       if w2 != 17 goto +4 <LBB1_3>
  ;       if (half[1] != 0)
         3:       r1 = *(u16 *)(r1 + 26)
         4:       w0 = 1
         5:       if w1 == 0 goto +1 <LBB1_3>
         6:       w0 = 0

  0000000000000038 LBB1_3:
  ; }
         7:       exit

  $ bpftool prog dump xlated ...
  int narrow_load_half_word(struct sk_reuseport_md * ctx):
  ; int narrow_load_half_word(struct sk_reuseport_md *ctx)
     0: (b4) w0 = 0
  ; if (half[0] != IPPROTO_UDP)
     1: (79) r2 = *(u64 *)(r1 +8)
     2: (69) r2 = *(u16 *)(r2 +924)
  ; if (half[0] != IPPROTO_UDP)
     3: (56) if w2 != 0x11 goto pc+5
  ; if (half[1] != 0)
     4: (79) r1 = *(u64 *)(r1 +8)
     5: (69) r1 = *(u16 *)(r1 +924)
     6: (b4) w0 = 1
  ; if (half[1] != 0)
     7: (16) if w1 == 0x0 goto pc+1
     8: (b4) w0 = 0
  ; }
     9: (95) exit

In this case half[0] == half[1] == sk->sk_protocol, which is the target
field for the ctx->ip_protocol.

Fix it by emitting 'wX = 0' or 'rX = 0' instruction for all narrow loads
from an offset that is beyond the target field.

Going back to the example, with the fix in place, the upper half load from
ctx->ip_protocol yields zero:

  int narrow_load_half_word(struct sk_reuseport_md * ctx):
  ; int narrow_load_half_word(struct sk_reuseport_md *ctx)
     0: (b4) w0 = 0
  ; if (half[0] != IPPROTO_UDP)
     1: (79) r2 = *(u64 *)(r1 +8)
     2: (69) r2 = *(u16 *)(r2 +924)
  ; if (half[0] != IPPROTO_UDP)
     3: (56) if w2 != 0x11 goto pc+4
  ; if (half[1] != 0)
     4: (b4) w1 = 0
     5: (b4) w0 = 1
  ; if (half[1] != 0)
     6: (16) if w1 == 0x0 goto pc+1
     7: (b4) w0 = 0
  ; }
     8: (95) exit

Fixes: f96da09473b5 ("bpf: simplify narrower ctx access")
Suggested-by: Yonghong Song <yhs@...com>
Signed-off-by: Jakub Sitnicki <jakub@...udflare.com>
---
 kernel/bpf/verifier.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 94cead5a43e5..0a9dbcdd6341 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -9614,11 +9614,11 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
  */
 static int convert_ctx_accesses(struct bpf_verifier_env *env)
 {
+	u32 target_size, size_default, off, access_off;
 	const struct bpf_verifier_ops *ops = env->ops;
 	int i, cnt, size, ctx_field_size, delta = 0;
 	const int insn_cnt = env->prog->len;
 	struct bpf_insn insn_buf[16], *insn;
-	u32 target_size, size_default, off;
 	struct bpf_prog *new_prog;
 	enum bpf_access_type type;
 	bool is_narrower_load;
@@ -9760,7 +9760,26 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
 			return -EINVAL;
 		}
 
-		if (is_narrower_load && size < target_size) {
+		/* When context field is wider than the target field,
+		 * narrow load from an offset beyond the target field
+		 * can be reduced to loading zero because there is
+		 * nothing to load from memory.
+		 */
+		access_off = off & (size_default - 1);
+		if (is_narrower_load && access_off >= target_size) {
+			cnt = 0;
+			if (ctx_field_size <= 4)
+				insn_buf[cnt++] = BPF_MOV32_IMM(insn->dst_reg, 0);
+			else
+				insn_buf[cnt++] = BPF_MOV64_IMM(insn->dst_reg, 0);
+		}
+		/* Narrow load from an offset within the target field,
+		 * smaller in size than the target field, needs
+		 * shifting and masking because convert_ctx_access
+		 * always emits full-size target field load.
+		 */
+		if (is_narrower_load && access_off < target_size &&
+		    size < target_size) {
 			u8 shift = bpf_ctx_narrow_access_offset(
 				off, size, size_default) * 8;
 			if (ctx_field_size <= 4) {
-- 
2.25.4