Skip to content

Commit 82abbf8

Browse files
Alexei Starovoitovborkmann
Alexei Starovoitov
authored and
borkmann
committed
bpf: do not allow root to mangle valid pointers
Do not allow root to convert valid pointers into unknown scalars. In particular disallow: ptr &= reg ptr <<= reg ptr += ptr and explicitly allow: ptr -= ptr since pkt_end - pkt == length 1. This minimizes amount of address leaks root can do. In the future may need to further tighten the leaks with kptr_restrict. 2. If program has such pointer math it's likely a user mistake and when verifier complains about it right away instead of many instructions later on invalid memory access it's easier for users to fix their progs. 3. when register holding a pointer cannot change to scalar it allows JITs to optimize better. Like 32-bit archs could use single register for pointers instead of a pair required to hold 64-bit scalars. 4. reduces architecture dependent behavior. Since code: r1 = r10; r1 &= 0xff; if (r1 ...) will behave differently arm64 vs x64 and offloaded vs native. A significant chunk of ptr mangling was allowed by commit f1174f7 ("bpf/verifier: rework value tracking") yet some of it was allowed even earlier. Signed-off-by: Alexei Starovoitov <ast@kernel.org> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
1 parent 3db9128 commit 82abbf8

File tree

2 files changed

+63
-95
lines changed

2 files changed

+63
-95
lines changed

‎kernel/bpf/verifier.c

Lines changed: 34 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -1890,29 +1890,25 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
18901890

18911891
if (BPF_CLASS(insn->code) != BPF_ALU64) {
18921892
/* 32-bit ALU ops on pointers produce (meaningless) scalars */
1893-
if (!env->allow_ptr_leaks)
1894-
verbose(env,
1895-
"R%d 32-bit pointer arithmetic prohibited\n",
1896-
dst);
1893+
verbose(env,
1894+
"R%d 32-bit pointer arithmetic prohibited\n",
1895+
dst);
18971896
return -EACCES;
18981897
}
18991898

19001899
if (ptr_reg->type == PTR_TO_MAP_VALUE_OR_NULL) {
1901-
if (!env->allow_ptr_leaks)
1902-
verbose(env, "R%d pointer arithmetic on PTR_TO_MAP_VALUE_OR_NULL prohibited, null-check it first\n",
1903-
dst);
1900+
verbose(env, "R%d pointer arithmetic on PTR_TO_MAP_VALUE_OR_NULL prohibited, null-check it first\n",
1901+
dst);
19041902
return -EACCES;
19051903
}
19061904
if (ptr_reg->type == CONST_PTR_TO_MAP) {
1907-
if (!env->allow_ptr_leaks)
1908-
verbose(env, "R%d pointer arithmetic on CONST_PTR_TO_MAP prohibited\n",
1909-
dst);
1905+
verbose(env, "R%d pointer arithmetic on CONST_PTR_TO_MAP prohibited\n",
1906+
dst);
19101907
return -EACCES;
19111908
}
19121909
if (ptr_reg->type == PTR_TO_PACKET_END) {
1913-
if (!env->allow_ptr_leaks)
1914-
verbose(env, "R%d pointer arithmetic on PTR_TO_PACKET_END prohibited\n",
1915-
dst);
1910+
verbose(env, "R%d pointer arithmetic on PTR_TO_PACKET_END prohibited\n",
1911+
dst);
19161912
return -EACCES;
19171913
}
19181914

@@ -1979,19 +1975,17 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
19791975
case BPF_SUB:
19801976
if (dst_reg == off_reg) {
19811977
/* scalar -= pointer. Creates an unknown scalar */
1982-
if (!env->allow_ptr_leaks)
1983-
verbose(env, "R%d tried to subtract pointer from scalar\n",
1984-
dst);
1978+
verbose(env, "R%d tried to subtract pointer from scalar\n",
1979+
dst);
19851980
return -EACCES;
19861981
}
19871982
/* We don't allow subtraction from FP, because (according to
19881983
* test_verifier.c test "invalid fp arithmetic", JITs might not
19891984
* be able to deal with it.
19901985
*/
19911986
if (ptr_reg->type == PTR_TO_STACK) {
1992-
if (!env->allow_ptr_leaks)
1993-
verbose(env, "R%d subtraction from stack pointer prohibited\n",
1994-
dst);
1987+
verbose(env, "R%d subtraction from stack pointer prohibited\n",
1988+
dst);
19951989
return -EACCES;
19961990
}
19971991
if (known && (ptr_reg->off - smin_val ==
@@ -2040,19 +2034,14 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
20402034
case BPF_AND:
20412035
case BPF_OR:
20422036
case BPF_XOR:
2043-
/* bitwise ops on pointers are troublesome, prohibit for now.
2044-
* (However, in principle we could allow some cases, e.g.
2045-
* ptr &= ~3 which would reduce min_value by 3.)
2046-
*/
2047-
if (!env->allow_ptr_leaks)
2048-
verbose(env, "R%d bitwise operator %s on pointer prohibited\n",
2049-
dst, bpf_alu_string[opcode >> 4]);
2037+
/* bitwise ops on pointers are troublesome, prohibit. */
2038+
verbose(env, "R%d bitwise operator %s on pointer prohibited\n",
2039+
dst, bpf_alu_string[opcode >> 4]);
20502040
return -EACCES;
20512041
default:
20522042
/* other operators (e.g. MUL,LSH) produce non-pointer results */
2053-
if (!env->allow_ptr_leaks)
2054-
verbose(env, "R%d pointer arithmetic with %s operator prohibited\n",
2055-
dst, bpf_alu_string[opcode >> 4]);
2043+
verbose(env, "R%d pointer arithmetic with %s operator prohibited\n",
2044+
dst, bpf_alu_string[opcode >> 4]);
20562045
return -EACCES;
20572046
}
20582047

@@ -2308,7 +2297,6 @@ static int adjust_reg_min_max_vals(struct bpf_verifier_env *env,
23082297
struct bpf_reg_state *regs = cur_regs(env), *dst_reg, *src_reg;
23092298
struct bpf_reg_state *ptr_reg = NULL, off_reg = {0};
23102299
u8 opcode = BPF_OP(insn->code);
2311-
int rc;
23122300

23132301
dst_reg = &regs[insn->dst_reg];
23142302
src_reg = NULL;
@@ -2319,43 +2307,29 @@ static int adjust_reg_min_max_vals(struct bpf_verifier_env *env,
23192307
if (src_reg->type != SCALAR_VALUE) {
23202308
if (dst_reg->type != SCALAR_VALUE) {
23212309
/* Combining two pointers by any ALU op yields
2322-
* an arbitrary scalar.
2310+
* an arbitrary scalar. Disallow all math except
2311+
* pointer subtraction
23232312
*/
2324-
if (!env->allow_ptr_leaks) {
2325-
verbose(env, "R%d pointer %s pointer prohibited\n",
2326-
insn->dst_reg,
2327-
bpf_alu_string[opcode >> 4]);
2328-
return -EACCES;
2313+
if (opcode == BPF_SUB){
2314+
mark_reg_unknown(env, regs, insn->dst_reg);
2315+
return 0;
23292316
}
2330-
mark_reg_unknown(env, regs, insn->dst_reg);
2331-
return 0;
2317+
verbose(env, "R%d pointer %s pointer prohibited\n",
2318+
insn->dst_reg,
2319+
bpf_alu_string[opcode >> 4]);
2320+
return -EACCES;
23322321
} else {
23332322
/* scalar += pointer
23342323
* This is legal, but we have to reverse our
23352324
* src/dest handling in computing the range
23362325
*/
2337-
rc = adjust_ptr_min_max_vals(env, insn,
2338-
src_reg, dst_reg);
2339-
if (rc == -EACCES && env->allow_ptr_leaks) {
2340-
/* scalar += unknown scalar */
2341-
__mark_reg_unknown(&off_reg);
2342-
return adjust_scalar_min_max_vals(
2343-
env, insn,
2344-
dst_reg, off_reg);
2345-
}
2346-
return rc;
2326+
return adjust_ptr_min_max_vals(env, insn,
2327+
src_reg, dst_reg);
23472328
}
23482329
} else if (ptr_reg) {
23492330
/* pointer += scalar */
2350-
rc = adjust_ptr_min_max_vals(env, insn,
2351-
dst_reg, src_reg);
2352-
if (rc == -EACCES && env->allow_ptr_leaks) {
2353-
/* unknown scalar += scalar */
2354-
__mark_reg_unknown(dst_reg);
2355-
return adjust_scalar_min_max_vals(
2356-
env, insn, dst_reg, *src_reg);
2357-
}
2358-
return rc;
2331+
return adjust_ptr_min_max_vals(env, insn,
2332+
dst_reg, src_reg);
23592333
}
23602334
} else {
23612335
/* Pretend the src is a reg with a known value, since we only
@@ -2364,17 +2338,9 @@ static int adjust_reg_min_max_vals(struct bpf_verifier_env *env,
23642338
off_reg.type = SCALAR_VALUE;
23652339
__mark_reg_known(&off_reg, insn->imm);
23662340
src_reg = &off_reg;
2367-
if (ptr_reg) { /* pointer += K */
2368-
rc = adjust_ptr_min_max_vals(env, insn,
2369-
ptr_reg, src_reg);
2370-
if (rc == -EACCES && env->allow_ptr_leaks) {
2371-
/* unknown scalar += K */
2372-
__mark_reg_unknown(dst_reg);
2373-
return adjust_scalar_min_max_vals(
2374-
env, insn, dst_reg, off_reg);
2375-
}
2376-
return rc;
2377-
}
2341+
if (ptr_reg) /* pointer += K */
2342+
return adjust_ptr_min_max_vals(env, insn,
2343+
ptr_reg, src_reg);
23782344
}
23792345

23802346
/* Got here implies adding two SCALAR_VALUEs */

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

Lines changed: 29 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -422,9 +422,7 @@ static struct bpf_test tests[] = {
422422
BPF_STX_MEM(BPF_DW, BPF_REG_1, BPF_REG_0, 0),
423423
BPF_EXIT_INSN(),
424424
},
425-
.errstr_unpriv = "R1 subtraction from stack pointer",
426-
.result_unpriv = REJECT,
427-
.errstr = "R1 invalid mem access",
425+
.errstr = "R1 subtraction from stack pointer",
428426
.result = REJECT,
429427
},
430428
{
@@ -1859,9 +1857,8 @@ static struct bpf_test tests[] = {
18591857
BPF_MOV64_IMM(BPF_REG_0, 0),
18601858
BPF_EXIT_INSN(),
18611859
},
1862-
.result = ACCEPT,
1863-
.result_unpriv = REJECT,
1864-
.errstr_unpriv = "R1 pointer += pointer",
1860+
.result = REJECT,
1861+
.errstr = "R1 pointer += pointer",
18651862
},
18661863
{
18671864
"unpriv: neg pointer",
@@ -2589,7 +2586,8 @@ static struct bpf_test tests[] = {
25892586
BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
25902587
offsetof(struct __sk_buff, data)),
25912588
BPF_ALU64_REG(BPF_ADD, BPF_REG_3, BPF_REG_4),
2592-
BPF_MOV64_REG(BPF_REG_2, BPF_REG_1),
2589+
BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
2590+
offsetof(struct __sk_buff, len)),
25932591
BPF_ALU64_IMM(BPF_LSH, BPF_REG_2, 49),
25942592
BPF_ALU64_IMM(BPF_RSH, BPF_REG_2, 49),
25952593
BPF_ALU64_REG(BPF_ADD, BPF_REG_3, BPF_REG_2),
@@ -2896,7 +2894,7 @@ static struct bpf_test tests[] = {
28962894
BPF_MOV64_IMM(BPF_REG_0, 0),
28972895
BPF_EXIT_INSN(),
28982896
},
2899-
.errstr = "invalid access to packet",
2897+
.errstr = "R3 pointer arithmetic on PTR_TO_PACKET_END",
29002898
.result = REJECT,
29012899
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
29022900
},
@@ -3882,9 +3880,7 @@ static struct bpf_test tests[] = {
38823880
BPF_EXIT_INSN(),
38833881
},
38843882
.fixup_map2 = { 3, 11 },
3885-
.errstr_unpriv = "R0 pointer += pointer",
3886-
.errstr = "R0 invalid mem access 'inv'",
3887-
.result_unpriv = REJECT,
3883+
.errstr = "R0 pointer += pointer",
38883884
.result = REJECT,
38893885
.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
38903886
},
@@ -3925,7 +3921,7 @@ static struct bpf_test tests[] = {
39253921
BPF_EXIT_INSN(),
39263922
},
39273923
.fixup_map1 = { 4 },
3928-
.errstr = "R4 invalid mem access",
3924+
.errstr = "R4 pointer arithmetic on PTR_TO_MAP_VALUE_OR_NULL",
39293925
.result = REJECT,
39303926
.prog_type = BPF_PROG_TYPE_SCHED_CLS
39313927
},
@@ -3946,7 +3942,7 @@ static struct bpf_test tests[] = {
39463942
BPF_EXIT_INSN(),
39473943
},
39483944
.fixup_map1 = { 4 },
3949-
.errstr = "R4 invalid mem access",
3945+
.errstr = "R4 pointer arithmetic on PTR_TO_MAP_VALUE_OR_NULL",
39503946
.result = REJECT,
39513947
.prog_type = BPF_PROG_TYPE_SCHED_CLS
39523948
},
@@ -3967,7 +3963,7 @@ static struct bpf_test tests[] = {
39673963
BPF_EXIT_INSN(),
39683964
},
39693965
.fixup_map1 = { 4 },
3970-
.errstr = "R4 invalid mem access",
3966+
.errstr = "R4 pointer arithmetic on PTR_TO_MAP_VALUE_OR_NULL",
39713967
.result = REJECT,
39723968
.prog_type = BPF_PROG_TYPE_SCHED_CLS
39733969
},
@@ -5192,10 +5188,8 @@ static struct bpf_test tests[] = {
51925188
BPF_EXIT_INSN(),
51935189
},
51945190
.fixup_map2 = { 3 },
5195-
.errstr_unpriv = "R0 bitwise operator &= on pointer",
5196-
.errstr = "invalid mem access 'inv'",
5191+
.errstr = "R0 bitwise operator &= on pointer",
51975192
.result = REJECT,
5198-
.result_unpriv = REJECT,
51995193
},
52005194
{
52015195
"map element value illegal alu op, 2",
@@ -5211,10 +5205,8 @@ static struct bpf_test tests[] = {
52115205
BPF_EXIT_INSN(),
52125206
},
52135207
.fixup_map2 = { 3 },
5214-
.errstr_unpriv = "R0 32-bit pointer arithmetic prohibited",
5215-
.errstr = "invalid mem access 'inv'",
5208+
.errstr = "R0 32-bit pointer arithmetic prohibited",
52165209
.result = REJECT,
5217-
.result_unpriv = REJECT,
52185210
},
52195211
{
52205212
"map element value illegal alu op, 3",
@@ -5230,10 +5222,8 @@ static struct bpf_test tests[] = {
52305222
BPF_EXIT_INSN(),
52315223
},
52325224
.fixup_map2 = { 3 },
5233-
.errstr_unpriv = "R0 pointer arithmetic with /= operator",
5234-
.errstr = "invalid mem access 'inv'",
5225+
.errstr = "R0 pointer arithmetic with /= operator",
52355226
.result = REJECT,
5236-
.result_unpriv = REJECT,
52375227
},
52385228
{
52395229
"map element value illegal alu op, 4",
@@ -6016,8 +6006,7 @@ static struct bpf_test tests[] = {
60166006
BPF_EXIT_INSN(),
60176007
},
60186008
.fixup_map_in_map = { 3 },
6019-
.errstr = "R1 type=inv expected=map_ptr",
6020-
.errstr_unpriv = "R1 pointer arithmetic on CONST_PTR_TO_MAP prohibited",
6009+
.errstr = "R1 pointer arithmetic on CONST_PTR_TO_MAP prohibited",
60216010
.result = REJECT,
60226011
},
60236012
{
@@ -7644,6 +7633,19 @@ static struct bpf_test tests[] = {
76447633
.result = REJECT,
76457634
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
76467635
},
7636+
{
7637+
"pkt_end - pkt_start is allowed",
7638+
.insns = {
7639+
BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_1,
7640+
offsetof(struct __sk_buff, data_end)),
7641+
BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
7642+
offsetof(struct __sk_buff, data)),
7643+
BPF_ALU64_REG(BPF_SUB, BPF_REG_0, BPF_REG_2),
7644+
BPF_EXIT_INSN(),
7645+
},
7646+
.result = ACCEPT,
7647+
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
7648+
},
76477649
{
76487650
"XDP pkt read, pkt_end mangling, bad access 1",
76497651
.insns = {
@@ -7659,7 +7661,7 @@ static struct bpf_test tests[] = {
76597661
BPF_MOV64_IMM(BPF_REG_0, 0),
76607662
BPF_EXIT_INSN(),
76617663
},
7662-
.errstr = "R1 offset is outside of the packet",
7664+
.errstr = "R3 pointer arithmetic on PTR_TO_PACKET_END",
76637665
.result = REJECT,
76647666
.prog_type = BPF_PROG_TYPE_XDP,
76657667
},
@@ -7678,7 +7680,7 @@ static struct bpf_test tests[] = {
76787680
BPF_MOV64_IMM(BPF_REG_0, 0),
76797681
BPF_EXIT_INSN(),
76807682
},
7681-
.errstr = "R1 offset is outside of the packet",
7683+
.errstr = "R3 pointer arithmetic on PTR_TO_PACKET_END",
76827684
.result = REJECT,
76837685
.prog_type = BPF_PROG_TYPE_XDP,
76847686
},

0 commit comments

Comments
 (0)