From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
To: stable@vger.kernel.org
Cc: bpf@vger.kernel.org, Salvatore Bonaccorso <carnil@debian.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Alexei Starovoitov <ast@kernel.org>,
John Fastabend <john.fastabend@gmail.com>,
Pavel Machek <pavel@denx.de>,
Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Subject: [PATCH 4.19 2/3] bpf: Fix 32 bit src register truncation on div/mod
Date: Fri, 27 Aug 2021 10:55:32 -0300 [thread overview]
Message-ID: <20210827135533.146070-3-cascardo@canonical.com> (raw)
In-Reply-To: <20210827135533.146070-1-cascardo@canonical.com>
From: Daniel Borkmann <daniel@iogearbox.net>
Commit e88b2c6e5a4d9ce30d75391e4d950da74bb2bd90 upstream.
While reviewing a different fix, John and I noticed an oddity in one of the
BPF program dumps that stood out, for example:
# bpftool p d x i 13
0: (b7) r0 = 808464450
1: (b4) w4 = 808464432
2: (bc) w0 = w0
3: (15) if r0 == 0x0 goto pc+1
4: (9c) w4 %= w0
[...]
In line 2 we noticed that the mov32 would 32 bit truncate the original src
register for the div/mod operation. While for the two operations the dst
register is typically marked unknown e.g. from adjust_scalar_min_max_vals()
the src register is not, and thus verifier keeps tracking original bounds,
simplified:
0: R1=ctx(id=0,off=0,imm=0) R10=fp0
0: (b7) r0 = -1
1: R0_w=invP-1 R1=ctx(id=0,off=0,imm=0) R10=fp0
1: (b7) r1 = -1
2: R0_w=invP-1 R1_w=invP-1 R10=fp0
2: (3c) w0 /= w1
3: R0_w=invP(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R1_w=invP-1 R10=fp0
3: (77) r1 >>= 32
4: R0_w=invP(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R1_w=invP4294967295 R10=fp0
4: (bf) r0 = r1
5: R0_w=invP4294967295 R1_w=invP4294967295 R10=fp0
5: (95) exit
processed 6 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
Runtime result of r0 at exit is 0 instead of expected -1. Remove the
verifier mov32 src rewrite in div/mod and replace it with a jmp32 test
instead. After the fix, we result in the following code generation when
having dividend r1 and divisor r6:
div, 64 bit: div, 32 bit:
0: (b7) r6 = 8 0: (b7) r6 = 8
1: (b7) r1 = 8 1: (b7) r1 = 8
2: (55) if r6 != 0x0 goto pc+2 2: (56) if w6 != 0x0 goto pc+2
3: (ac) w1 ^= w1 3: (ac) w1 ^= w1
4: (05) goto pc+1 4: (05) goto pc+1
5: (3f) r1 /= r6 5: (3c) w1 /= w6
6: (b7) r0 = 0 6: (b7) r0 = 0
7: (95) exit 7: (95) exit
mod, 64 bit: mod, 32 bit:
0: (b7) r6 = 8 0: (b7) r6 = 8
1: (b7) r1 = 8 1: (b7) r1 = 8
2: (15) if r6 == 0x0 goto pc+1 2: (16) if w6 == 0x0 goto pc+1
3: (9f) r1 %= r6 3: (9c) w1 %= w6
4: (b7) r0 = 0 4: (b7) r0 = 0
5: (95) exit 5: (95) exit
x86 in particular can throw a 'divide error' exception for div
instruction not only for divisor being zero, but also for the case
when the quotient is too large for the designated register. For the
edx:eax and rdx:rax dividend pair it is not an issue in x86 BPF JIT
since we always zero edx (rdx). Hence really the only protection
needed is against divisor being zero.
Fixes: 68fda450a7df ("bpf: fix 32-bit divide by zero")
Co-developed-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
[Salvatore Bonaccorso: This is an earlier version of the patch provided
by Daniel Borkmann which does not rely on availability of the BPF_JMP32
instruction class. This means it is not even strictly a backport of the
upstream commit mentioned but based on Daniel's and John's work to
address the issue.]
Tested-by: Salvatore Bonaccorso <carnil@debian.org>
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
---
include/linux/filter.h | 24 ++++++++++++++++++++++++
kernel/bpf/verifier.c | 22 +++++++++++-----------
2 files changed, 35 insertions(+), 11 deletions(-)
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 117f9380069a..7c84762cb59e 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -77,6 +77,14 @@ struct sock_reuseport;
/* ALU ops on registers, bpf_add|sub|...: dst_reg += src_reg */
+#define BPF_ALU_REG(CLASS, OP, DST, SRC) \
+ ((struct bpf_insn) { \
+ .code = CLASS | BPF_OP(OP) | BPF_X, \
+ .dst_reg = DST, \
+ .src_reg = SRC, \
+ .off = 0, \
+ .imm = 0 })
+
#define BPF_ALU64_REG(OP, DST, SRC) \
((struct bpf_insn) { \
.code = BPF_ALU64 | BPF_OP(OP) | BPF_X, \
@@ -123,6 +131,14 @@ struct sock_reuseport;
/* Short form of mov, dst_reg = src_reg */
+#define BPF_MOV_REG(CLASS, DST, SRC) \
+ ((struct bpf_insn) { \
+ .code = CLASS | BPF_MOV | BPF_X, \
+ .dst_reg = DST, \
+ .src_reg = SRC, \
+ .off = 0, \
+ .imm = 0 })
+
#define BPF_MOV64_REG(DST, SRC) \
((struct bpf_insn) { \
.code = BPF_ALU64 | BPF_MOV | BPF_X, \
@@ -157,6 +173,14 @@ struct sock_reuseport;
.off = 0, \
.imm = IMM })
+#define BPF_RAW_REG(insn, DST, SRC) \
+ ((struct bpf_insn) { \
+ .code = (insn).code, \
+ .dst_reg = DST, \
+ .src_reg = SRC, \
+ .off = (insn).off, \
+ .imm = (insn).imm })
+
/* BPF_LD_IMM64 macro encodes single 'load 64-bit immediate' insn */
#define BPF_LD_IMM64(DST, IMM) \
BPF_LD_IMM64_RAW(DST, 0, IMM)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2bf83305e5ab..a346ecfe6241 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6177,28 +6177,28 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
insn->code == (BPF_ALU | BPF_DIV | BPF_X)) {
bool is64 = BPF_CLASS(insn->code) == BPF_ALU64;
struct bpf_insn mask_and_div[] = {
- BPF_MOV32_REG(insn->src_reg, insn->src_reg),
+ BPF_MOV_REG(BPF_CLASS(insn->code), BPF_REG_AX, insn->src_reg),
/* Rx div 0 -> 0 */
- BPF_JMP_IMM(BPF_JNE, insn->src_reg, 0, 2),
- BPF_ALU32_REG(BPF_XOR, insn->dst_reg, insn->dst_reg),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_AX, 0, 2),
+ BPF_RAW_REG(*insn, insn->dst_reg, BPF_REG_AX),
BPF_JMP_IMM(BPF_JA, 0, 0, 1),
- *insn,
+ BPF_ALU_REG(BPF_CLASS(insn->code), BPF_XOR, insn->dst_reg, insn->dst_reg),
};
struct bpf_insn mask_and_mod[] = {
- BPF_MOV32_REG(insn->src_reg, insn->src_reg),
+ BPF_MOV_REG(BPF_CLASS(insn->code), BPF_REG_AX, insn->src_reg),
/* Rx mod 0 -> Rx */
- BPF_JMP_IMM(BPF_JEQ, insn->src_reg, 0, 1),
- *insn,
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_AX, 0, 1),
+ BPF_RAW_REG(*insn, insn->dst_reg, BPF_REG_AX),
};
struct bpf_insn *patchlet;
if (insn->code == (BPF_ALU64 | BPF_DIV | BPF_X) ||
insn->code == (BPF_ALU | BPF_DIV | BPF_X)) {
- patchlet = mask_and_div + (is64 ? 1 : 0);
- cnt = ARRAY_SIZE(mask_and_div) - (is64 ? 1 : 0);
+ patchlet = mask_and_div;
+ cnt = ARRAY_SIZE(mask_and_div);
} else {
- patchlet = mask_and_mod + (is64 ? 1 : 0);
- cnt = ARRAY_SIZE(mask_and_mod) - (is64 ? 1 : 0);
+ patchlet = mask_and_mod;
+ cnt = ARRAY_SIZE(mask_and_mod);
}
new_prog = bpf_patch_insn_data(env, i + delta, patchlet, cnt);
--
2.30.2
next prev parent reply other threads:[~2021-08-27 13:56 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-27 13:55 [PATCH 4.19 0/3] BPF fixes for CVE-2021-3444 and CVE-2021-3600 Thadeu Lima de Souza Cascardo
2021-08-27 13:55 ` [PATCH 4.19 1/3] bpf: Do not use ax register in interpreter on div/mod Thadeu Lima de Souza Cascardo
2021-08-27 13:55 ` Thadeu Lima de Souza Cascardo [this message]
2021-08-27 13:55 ` [PATCH 4.19 3/3] bpf: Fix truncation handling for mod32 dst reg wrt zero Thadeu Lima de Souza Cascardo
2021-08-27 14:38 ` [PATCH 4.19 0/3] BPF fixes for CVE-2021-3444 and CVE-2021-3600 Greg KH
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210827135533.146070-3-cascardo@canonical.com \
--to=cascardo@canonical.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=carnil@debian.org \
--cc=daniel@iogearbox.net \
--cc=john.fastabend@gmail.com \
--cc=pavel@denx.de \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).