Skip to content

Commit ca36960

Browse files
borkmannAlexei Starovoitov
borkmann
authored and
Alexei Starovoitov
committed
bpf: allow xadd only on aligned memory
The requirements around atomic_add() / atomic64_add() resp. their JIT implementations differ across architectures. E.g. while x86_64 seems just fine with BPF's xadd on unaligned memory, on arm64 it triggers via interpreter but also JIT the following crash: [ 830.864985] Unable to handle kernel paging request at virtual address ffff8097d7ed6703 [...] [ 830.916161] Internal error: Oops: 96000021 [#1] SMP [ 830.984755] CPU: 37 PID: 2788 Comm: test_verifier Not tainted 4.16.0-rc2+ #8 [ 830.991790] Hardware name: Huawei TaiShan 2280 /BC11SPCD, BIOS 1.29 07/17/2017 [ 830.998998] pstate: 80400005 (Nzcv daif +PAN -UAO) [ 831.003793] pc : __ll_sc_atomic_add+0x4/0x18 [ 831.008055] lr : ___bpf_prog_run+0x1198/0x1588 [ 831.012485] sp : ffff00001ccabc20 [ 831.015786] x29: ffff00001ccabc20 x28: ffff8017d56a0f00 [ 831.021087] x27: 0000000000000001 x26: 0000000000000000 [ 831.026387] x25: 000000c168d9db98 x24: 0000000000000000 [ 831.031686] x23: ffff000008203878 x22: ffff000009488000 [ 831.036986] x21: ffff000008b14e28 x20: ffff00001ccabcb0 [ 831.042286] x19: ffff0000097b5080 x18: 0000000000000a03 [ 831.047585] x17: 0000000000000000 x16: 0000000000000000 [ 831.052885] x15: 0000ffffaeca8000 x14: 0000000000000000 [ 831.058184] x13: 0000000000000000 x12: 0000000000000000 [ 831.063484] x11: 0000000000000001 x10: 0000000000000000 [ 831.068783] x9 : 0000000000000000 x8 : 0000000000000000 [ 831.074083] x7 : 0000000000000000 x6 : 000580d428000000 [ 831.079383] x5 : 0000000000000018 x4 : 0000000000000000 [ 831.084682] x3 : ffff00001ccabcb0 x2 : 0000000000000001 [ 831.089982] x1 : ffff8097d7ed6703 x0 : 0000000000000001 [ 831.095282] Process test_verifier (pid: 2788, stack limit = 0x0000000018370044) [ 831.102577] Call trace: [ 831.105012] __ll_sc_atomic_add+0x4/0x18 [ 831.108923] __bpf_prog_run32+0x4c/0x70 [ 831.112748] bpf_test_run+0x78/0xf8 [ 831.116224] bpf_prog_test_run_xdp+0xb4/0x120 [ 831.120567] SyS_bpf+0x77c/0x1110 [ 831.123873] el0_svc_naked+0x30/0x34 [ 831.127437] Code: 97fffe97 17ffffec 00000000 f9800031 (885f7c31) Reason for this is because memory is required to be aligned. In case of BPF, we always enforce alignment in terms of stack access, but not when accessing map values or packet data when the underlying arch (e.g. arm64) has CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS set. xadd on packet data that is local to us anyway is just wrong, so forbid this case entirely. The only place where xadd makes sense in fact are map values; xadd on stack is wrong as well, but it's been around for much longer. Specifically enforce strict alignment in case of xadd, so that we handle this case generically and avoid such crashes in the first place. Fixes: 17a5267 ("bpf: verifier (add verifier core)") Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
1 parent a5f7add commit ca36960

File tree

2 files changed

+84
-16
lines changed

2 files changed

+84
-16
lines changed

‎kernel/bpf/verifier.c

Lines changed: 26 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1356,6 +1356,13 @@ static bool is_ctx_reg(struct bpf_verifier_env *env, int regno)
13561356
return reg->type == PTR_TO_CTX;
13571357
}
13581358

1359+
static bool is_pkt_reg(struct bpf_verifier_env *env, int regno)
1360+
{
1361+
const struct bpf_reg_state *reg = cur_regs(env) + regno;
1362+
1363+
return type_is_pkt_pointer(reg->type);
1364+
}
1365+
13591366
static int check_pkt_ptr_alignment(struct bpf_verifier_env *env,
13601367
const struct bpf_reg_state *reg,
13611368
int off, int size, bool strict)
@@ -1416,10 +1423,10 @@ static int check_generic_ptr_alignment(struct bpf_verifier_env *env,
14161423
}
14171424

14181425
static int check_ptr_alignment(struct bpf_verifier_env *env,
1419-
const struct bpf_reg_state *reg,
1420-
int off, int size)
1426+
const struct bpf_reg_state *reg, int off,
1427+
int size, bool strict_alignment_once)
14211428
{
1422-
bool strict = env->strict_alignment;
1429+
bool strict = env->strict_alignment || strict_alignment_once;
14231430
const char *pointer_desc = "";
14241431

14251432
switch (reg->type) {
@@ -1576,9 +1583,9 @@ static void coerce_reg_to_size(struct bpf_reg_state *reg, int size)
15761583
* if t==write && value_regno==-1, some unknown value is stored into memory
15771584
* if t==read && value_regno==-1, don't care what we read from memory
15781585
*/
1579-
static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regno, int off,
1580-
int bpf_size, enum bpf_access_type t,
1581-
int value_regno)
1586+
static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regno,
1587+
int off, int bpf_size, enum bpf_access_type t,
1588+
int value_regno, bool strict_alignment_once)
15821589
{
15831590
struct bpf_reg_state *regs = cur_regs(env);
15841591
struct bpf_reg_state *reg = regs + regno;
@@ -1590,7 +1597,7 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
15901597
return size;
15911598

15921599
/* alignment checks will add in reg->off themselves */
1593-
err = check_ptr_alignment(env, reg, off, size);
1600+
err = check_ptr_alignment(env, reg, off, size, strict_alignment_once);
15941601
if (err)
15951602
return err;
15961603

@@ -1735,21 +1742,23 @@ static int check_xadd(struct bpf_verifier_env *env, int insn_idx, struct bpf_ins
17351742
return -EACCES;
17361743
}
17371744

1738-
if (is_ctx_reg(env, insn->dst_reg)) {
1739-
verbose(env, "BPF_XADD stores into R%d context is not allowed\n",
1740-
insn->dst_reg);
1745+
if (is_ctx_reg(env, insn->dst_reg) ||
1746+
is_pkt_reg(env, insn->dst_reg)) {
1747+
verbose(env, "BPF_XADD stores into R%d %s is not allowed\n",
1748+
insn->dst_reg, is_ctx_reg(env, insn->dst_reg) ?
1749+
"context" : "packet");
17411750
return -EACCES;
17421751
}
17431752

17441753
/* check whether atomic_add can read the memory */
17451754
err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
1746-
BPF_SIZE(insn->code), BPF_READ, -1);
1755+
BPF_SIZE(insn->code), BPF_READ, -1, true);
17471756
if (err)
17481757
return err;
17491758

17501759
/* check whether atomic_add can write into the same memory */
17511760
return check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
1752-
BPF_SIZE(insn->code), BPF_WRITE, -1);
1761+
BPF_SIZE(insn->code), BPF_WRITE, -1, true);
17531762
}
17541763

17551764
/* when register 'regno' is passed into function that will read 'access_size'
@@ -2388,7 +2397,8 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
23882397
* is inferred from register state.
23892398
*/
23902399
for (i = 0; i < meta.access_size; i++) {
2391-
err = check_mem_access(env, insn_idx, meta.regno, i, BPF_B, BPF_WRITE, -1);
2400+
err = check_mem_access(env, insn_idx, meta.regno, i, BPF_B,
2401+
BPF_WRITE, -1, false);
23922402
if (err)
23932403
return err;
23942404
}
@@ -4632,7 +4642,7 @@ static int do_check(struct bpf_verifier_env *env)
46324642
*/
46334643
err = check_mem_access(env, insn_idx, insn->src_reg, insn->off,
46344644
BPF_SIZE(insn->code), BPF_READ,
4635-
insn->dst_reg);
4645+
insn->dst_reg, false);
46364646
if (err)
46374647
return err;
46384648

@@ -4684,7 +4694,7 @@ static int do_check(struct bpf_verifier_env *env)
46844694
/* check that memory (dst_reg + off) is writeable */
46854695
err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
46864696
BPF_SIZE(insn->code), BPF_WRITE,
4687-
insn->src_reg);
4697+
insn->src_reg, false);
46884698
if (err)
46894699
return err;
46904700

@@ -4719,7 +4729,7 @@ static int do_check(struct bpf_verifier_env *env)
47194729
/* check that memory (dst_reg + off) is writeable */
47204730
err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
47214731
BPF_SIZE(insn->code), BPF_WRITE,
4722-
-1);
4732+
-1, false);
47234733
if (err)
47244734
return err;
47254735

‎tools/testing/selftests/bpf/test_verifier.c

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11163,6 +11163,64 @@ static struct bpf_test tests[] = {
1116311163
.result = REJECT,
1116411164
.prog_type = BPF_PROG_TYPE_TRACEPOINT,
1116511165
},
11166+
{
11167+
"xadd/w check unaligned stack",
11168+
.insns = {
11169+
BPF_MOV64_IMM(BPF_REG_0, 1),
11170+
BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_0, -8),
11171+
BPF_STX_XADD(BPF_W, BPF_REG_10, BPF_REG_0, -7),
11172+
BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_10, -8),
11173+
BPF_EXIT_INSN(),
11174+
},
11175+
.result = REJECT,
11176+
.errstr = "misaligned stack access off",
11177+
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
11178+
},
11179+
{
11180+
"xadd/w check unaligned map",
11181+
.insns = {
11182+
BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
11183+
BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
11184+
BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
11185+
BPF_LD_MAP_FD(BPF_REG_1, 0),
11186+
BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
11187+
BPF_FUNC_map_lookup_elem),
11188+
BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
11189+
BPF_EXIT_INSN(),
11190+
BPF_MOV64_IMM(BPF_REG_1, 1),
11191+
BPF_STX_XADD(BPF_W, BPF_REG_0, BPF_REG_1, 3),
11192+
BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_0, 3),
11193+
BPF_EXIT_INSN(),
11194+
},
11195+
.fixup_map1 = { 3 },
11196+
.result = REJECT,
11197+
.errstr = "misaligned value access off",
11198+
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
11199+
},
11200+
{
11201+
"xadd/w check unaligned pkt",
11202+
.insns = {
11203+
BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
11204+
offsetof(struct xdp_md, data)),
11205+
BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
11206+
offsetof(struct xdp_md, data_end)),
11207+
BPF_MOV64_REG(BPF_REG_1, BPF_REG_2),
11208+
BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, 8),
11209+
BPF_JMP_REG(BPF_JLT, BPF_REG_1, BPF_REG_3, 2),
11210+
BPF_MOV64_IMM(BPF_REG_0, 99),
11211+
BPF_JMP_IMM(BPF_JA, 0, 0, 6),
11212+
BPF_MOV64_IMM(BPF_REG_0, 1),
11213+
BPF_ST_MEM(BPF_W, BPF_REG_2, 0, 0),
11214+
BPF_ST_MEM(BPF_W, BPF_REG_2, 3, 0),
11215+
BPF_STX_XADD(BPF_W, BPF_REG_2, BPF_REG_0, 1),
11216+
BPF_STX_XADD(BPF_W, BPF_REG_2, BPF_REG_0, 2),
11217+
BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_2, 1),
11218+
BPF_EXIT_INSN(),
11219+
},
11220+
.result = REJECT,
11221+
.errstr = "BPF_XADD stores into R2 packet",
11222+
.prog_type = BPF_PROG_TYPE_XDP,
11223+
},
1116611224
};
1116711225

1116811226
static int probe_filter_length(const struct bpf_insn *fp)

0 commit comments

Comments
 (0)