Skip to content

Commit 6f16101

Browse files
borkmannAlexei Starovoitov
borkmann
authored and
Alexei Starovoitov
committed
bpf: mark dst unknown on inconsistent {s, u}bounds adjustments
syzkaller generated a BPF proglet and triggered a warning with the following: 0: (b7) r0 = 0 1: (d5) if r0 s<= 0x0 goto pc+0 R0=inv0 R1=ctx(id=0,off=0,imm=0) R10=fp0 2: (1f) r0 -= r1 R0=inv0 R1=ctx(id=0,off=0,imm=0) R10=fp0 verifier internal error: known but bad sbounds What happens is that in the first insn, r0's min/max value are both 0 due to the immediate assignment, later in the jsle test the bounds are updated for the min value in the false path, meaning, they yield smin_val = 1, smax_val = 0, and when ctx pointer is subtracted from r0, verifier bails out with the internal error and throwing a WARN since smin_val != smax_val for the known constant. For min_val > max_val scenario it means that reg_set_min_max() and reg_set_min_max_inv() (which both refine existing bounds) demonstrated that such branch cannot be taken at runtime. In above scenario for the case where it will be taken, the existing [0, 0] bounds are kept intact. Meaning, the rejection is not due to a verifier internal error, and therefore the WARN() is not necessary either. We could just reject such cases in adjust_{ptr,scalar}_min_max_vals() when either known scalars have smin_val != smax_val or umin_val != umax_val or any scalar reg with bounds smin_val > smax_val or umin_val > umax_val. However, there may be a small risk of breakage of buggy programs, so handle this more gracefully and in adjust_{ptr,scalar}_min_max_vals() just taint the dst reg as unknown scalar when we see ops with such kind of src reg. Reported-by: [email protected] Signed-off-by: Daniel Borkmann <[email protected]> Signed-off-by: Alexei Starovoitov <[email protected]>
1 parent f37a8cb commit 6f16101

File tree

2 files changed

+138
-12
lines changed

2 files changed

+138
-12
lines changed

‎kernel/bpf/verifier.c

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1895,17 +1895,13 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
18951895

18961896
dst_reg = &regs[dst];
18971897

1898-
if (WARN_ON_ONCE(known && (smin_val != smax_val))) {
1899-
print_verifier_state(env, env->cur_state);
1900-
verbose(env,
1901-
"verifier internal error: known but bad sbounds\n");
1902-
return -EINVAL;
1903-
}
1904-
if (WARN_ON_ONCE(known && (umin_val != umax_val))) {
1905-
print_verifier_state(env, env->cur_state);
1906-
verbose(env,
1907-
"verifier internal error: known but bad ubounds\n");
1908-
return -EINVAL;
1898+
if ((known && (smin_val != smax_val || umin_val != umax_val)) ||
1899+
smin_val > smax_val || umin_val > umax_val) {
1900+
/* Taint dst register if offset had invalid bounds derived from
1901+
* e.g. dead branches.
1902+
*/
1903+
__mark_reg_unknown(dst_reg);
1904+
return 0;
19091905
}
19101906

19111907
if (BPF_CLASS(insn->code) != BPF_ALU64) {
@@ -2097,6 +2093,15 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
20972093
src_known = tnum_is_const(src_reg.var_off);
20982094
dst_known = tnum_is_const(dst_reg->var_off);
20992095

2096+
if ((src_known && (smin_val != smax_val || umin_val != umax_val)) ||
2097+
smin_val > smax_val || umin_val > umax_val) {
2098+
/* Taint dst register if offset had invalid bounds derived from
2099+
* e.g. dead branches.
2100+
*/
2101+
__mark_reg_unknown(dst_reg);
2102+
return 0;
2103+
}
2104+
21002105
if (!src_known &&
21012106
opcode != BPF_ADD && opcode != BPF_SUB && opcode != BPF_AND) {
21022107
__mark_reg_unknown(dst_reg);

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

Lines changed: 122 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6732,7 +6732,7 @@ static struct bpf_test tests[] = {
67326732
BPF_JMP_IMM(BPF_JA, 0, 0, -7),
67336733
},
67346734
.fixup_map1 = { 4 },
6735-
.errstr = "unbounded min value",
6735+
.errstr = "R0 invalid mem access 'inv'",
67366736
.result = REJECT,
67376737
},
67386738
{
@@ -8633,6 +8633,127 @@ static struct bpf_test tests[] = {
86338633
.prog_type = BPF_PROG_TYPE_XDP,
86348634
.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
86358635
},
8636+
{
8637+
"check deducing bounds from const, 1",
8638+
.insns = {
8639+
BPF_MOV64_IMM(BPF_REG_0, 1),
8640+
BPF_JMP_IMM(BPF_JSGE, BPF_REG_0, 1, 0),
8641+
BPF_ALU64_REG(BPF_SUB, BPF_REG_0, BPF_REG_1),
8642+
BPF_EXIT_INSN(),
8643+
},
8644+
.result = REJECT,
8645+
.errstr = "R0 tried to subtract pointer from scalar",
8646+
},
8647+
{
8648+
"check deducing bounds from const, 2",
8649+
.insns = {
8650+
BPF_MOV64_IMM(BPF_REG_0, 1),
8651+
BPF_JMP_IMM(BPF_JSGE, BPF_REG_0, 1, 1),
8652+
BPF_EXIT_INSN(),
8653+
BPF_JMP_IMM(BPF_JSLE, BPF_REG_0, 1, 1),
8654+
BPF_EXIT_INSN(),
8655+
BPF_ALU64_REG(BPF_SUB, BPF_REG_1, BPF_REG_0),
8656+
BPF_EXIT_INSN(),
8657+
},
8658+
.result = ACCEPT,
8659+
},
8660+
{
8661+
"check deducing bounds from const, 3",
8662+
.insns = {
8663+
BPF_MOV64_IMM(BPF_REG_0, 0),
8664+
BPF_JMP_IMM(BPF_JSLE, BPF_REG_0, 0, 0),
8665+
BPF_ALU64_REG(BPF_SUB, BPF_REG_0, BPF_REG_1),
8666+
BPF_EXIT_INSN(),
8667+
},
8668+
.result = REJECT,
8669+
.errstr = "R0 tried to subtract pointer from scalar",
8670+
},
8671+
{
8672+
"check deducing bounds from const, 4",
8673+
.insns = {
8674+
BPF_MOV64_IMM(BPF_REG_0, 0),
8675+
BPF_JMP_IMM(BPF_JSLE, BPF_REG_0, 0, 1),
8676+
BPF_EXIT_INSN(),
8677+
BPF_JMP_IMM(BPF_JSGE, BPF_REG_0, 0, 1),
8678+
BPF_EXIT_INSN(),
8679+
BPF_ALU64_REG(BPF_SUB, BPF_REG_1, BPF_REG_0),
8680+
BPF_EXIT_INSN(),
8681+
},
8682+
.result = ACCEPT,
8683+
},
8684+
{
8685+
"check deducing bounds from const, 5",
8686+
.insns = {
8687+
BPF_MOV64_IMM(BPF_REG_0, 0),
8688+
BPF_JMP_IMM(BPF_JSGE, BPF_REG_0, 0, 1),
8689+
BPF_ALU64_REG(BPF_SUB, BPF_REG_0, BPF_REG_1),
8690+
BPF_EXIT_INSN(),
8691+
},
8692+
.result = REJECT,
8693+
.errstr = "R0 tried to subtract pointer from scalar",
8694+
},
8695+
{
8696+
"check deducing bounds from const, 6",
8697+
.insns = {
8698+
BPF_MOV64_IMM(BPF_REG_0, 0),
8699+
BPF_JMP_IMM(BPF_JSGE, BPF_REG_0, 0, 1),
8700+
BPF_EXIT_INSN(),
8701+
BPF_ALU64_REG(BPF_SUB, BPF_REG_0, BPF_REG_1),
8702+
BPF_EXIT_INSN(),
8703+
},
8704+
.result = REJECT,
8705+
.errstr = "R0 tried to subtract pointer from scalar",
8706+
},
8707+
{
8708+
"check deducing bounds from const, 7",
8709+
.insns = {
8710+
BPF_MOV64_IMM(BPF_REG_0, ~0),
8711+
BPF_JMP_IMM(BPF_JSGE, BPF_REG_0, 0, 0),
8712+
BPF_ALU64_REG(BPF_SUB, BPF_REG_1, BPF_REG_0),
8713+
BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_1,
8714+
offsetof(struct __sk_buff, mark)),
8715+
BPF_EXIT_INSN(),
8716+
},
8717+
.result = REJECT,
8718+
.errstr = "dereference of modified ctx ptr",
8719+
},
8720+
{
8721+
"check deducing bounds from const, 8",
8722+
.insns = {
8723+
BPF_MOV64_IMM(BPF_REG_0, ~0),
8724+
BPF_JMP_IMM(BPF_JSGE, BPF_REG_0, 0, 1),
8725+
BPF_ALU64_REG(BPF_ADD, BPF_REG_1, BPF_REG_0),
8726+
BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_1,
8727+
offsetof(struct __sk_buff, mark)),
8728+
BPF_EXIT_INSN(),
8729+
},
8730+
.result = REJECT,
8731+
.errstr = "dereference of modified ctx ptr",
8732+
},
8733+
{
8734+
"check deducing bounds from const, 9",
8735+
.insns = {
8736+
BPF_MOV64_IMM(BPF_REG_0, 0),
8737+
BPF_JMP_IMM(BPF_JSGE, BPF_REG_0, 0, 0),
8738+
BPF_ALU64_REG(BPF_SUB, BPF_REG_0, BPF_REG_1),
8739+
BPF_EXIT_INSN(),
8740+
},
8741+
.result = REJECT,
8742+
.errstr = "R0 tried to subtract pointer from scalar",
8743+
},
8744+
{
8745+
"check deducing bounds from const, 10",
8746+
.insns = {
8747+
BPF_MOV64_IMM(BPF_REG_0, 0),
8748+
BPF_JMP_IMM(BPF_JSLE, BPF_REG_0, 0, 0),
8749+
/* Marks reg as unknown. */
8750+
BPF_ALU64_IMM(BPF_NEG, BPF_REG_0, 0),
8751+
BPF_ALU64_REG(BPF_SUB, BPF_REG_0, BPF_REG_1),
8752+
BPF_EXIT_INSN(),
8753+
},
8754+
.result = REJECT,
8755+
.errstr = "math between ctx pointer and register with unbounded min value is not allowed",
8756+
},
86368757
{
86378758
"bpf_exit with invalid return code. test1",
86388759
.insns = {

0 commit comments

Comments
 (0)