* [PATCH v4 bpf-next 00/11] BPF verifier retval logic fixes
@ 2023-12-01 18:33 Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 01/11] bpf: rearrange bpf_func_state fields to save a bit of memory Andrii Nakryiko
` (11 more replies)
0 siblings, 12 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2023-12-01 18:33 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team
This patch set fixes BPF verifier logic around validating and enforcing return
values for BPF programs that have specific range of expected return values.
Both sync and async callbacks have similar logic and are fixes as well.
A few tests are added that would fail without the fixes in this patch set.
Also, while at it, we update retval checking logic to use smin/smax range
instead of tnum, avoiding future potential issues if expected range cannot be
represented precisely by tnum (e.g., [0, 2] is not representable by tnum and
is treated as [0, 3]).
There is a little bit of refactoring to unify async callback and program exit
logic to avoid duplication of checks as much as possible.
v3->v4:
- add back bpf_func_state rearrangement patch;
- simplified patch #4 as suggested (Shung-Hsi);
v2->v3:
- more carefullly switch from umin/umax to smin/smax;
v1->v2:
- drop tnum from retval checks (Eduard);
- use smin/smax instead of umin/umax (Alexei).
Andrii Nakryiko (11):
bpf: rearrange bpf_func_state fields to save a bit of memory
bpf: provide correct register name for exception callback retval check
bpf: enforce precision of R0 on callback return
bpf: enforce exact retval range on subprog/callback exit
selftests/bpf: add selftest validating callback result is enforced
bpf: enforce precise retval range on program exit
bpf: unify async callback and program retval checks
bpf: enforce precision of R0 on program/async callback return
selftests/bpf: validate async callback return value check correctness
selftests/bpf: adjust global_func15 test to validate prog exit
precision
bpf: simplify tnum output if a fully known constant
include/linux/bpf_verifier.h | 9 +-
kernel/bpf/log.c | 13 ++
kernel/bpf/tnum.c | 6 -
kernel/bpf/verifier.c | 120 ++++++++++--------
.../selftests/bpf/progs/exceptions_assert.c | 2 +-
.../selftests/bpf/progs/exceptions_fail.c | 2 +-
.../selftests/bpf/progs/test_global_func15.c | 34 ++++-
.../selftests/bpf/progs/timer_failure.c | 36 ++++--
.../selftests/bpf/progs/user_ringbuf_fail.c | 2 +-
.../bpf/progs/verifier_cgroup_inv_retcode.c | 8 +-
.../bpf/progs/verifier_direct_packet_access.c | 2 +-
.../selftests/bpf/progs/verifier_int_ptr.c | 2 +-
.../bpf/progs/verifier_netfilter_retcode.c | 2 +-
.../selftests/bpf/progs/verifier_stack_ptr.c | 4 +-
.../bpf/progs/verifier_subprog_precision.c | 50 ++++++++
15 files changed, 212 insertions(+), 80 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 bpf-next 01/11] bpf: rearrange bpf_func_state fields to save a bit of memory
2023-12-01 18:33 [PATCH v4 bpf-next 00/11] BPF verifier retval logic fixes Andrii Nakryiko
@ 2023-12-01 18:33 ` Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 02/11] bpf: provide correct register name for exception callback retval check Andrii Nakryiko
` (10 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2023-12-01 18:33 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team, Eduard Zingerman
It's a trivial rearrangement saving 8 bytes. We have 4 bytes of padding
at the end which can be filled with another field without increasing
struct bpf_func_state.
copy_func_state() logic remains correct without any further changes.
BEFORE
======
struct bpf_func_state {
struct bpf_reg_state regs[11]; /* 0 1320 */
/* --- cacheline 20 boundary (1280 bytes) was 40 bytes ago --- */
int callsite; /* 1320 4 */
u32 frameno; /* 1324 4 */
u32 subprogno; /* 1328 4 */
u32 async_entry_cnt; /* 1332 4 */
bool in_callback_fn; /* 1336 1 */
/* XXX 7 bytes hole, try to pack */
/* --- cacheline 21 boundary (1344 bytes) --- */
struct tnum callback_ret_range; /* 1344 16 */
bool in_async_callback_fn; /* 1360 1 */
bool in_exception_callback_fn; /* 1361 1 */
/* XXX 2 bytes hole, try to pack */
int acquired_refs; /* 1364 4 */
struct bpf_reference_state * refs; /* 1368 8 */
int allocated_stack; /* 1376 4 */
/* XXX 4 bytes hole, try to pack */
struct bpf_stack_state * stack; /* 1384 8 */
/* size: 1392, cachelines: 22, members: 13 */
/* sum members: 1379, holes: 3, sum holes: 13 */
/* last cacheline: 48 bytes */
};
AFTER
=====
struct bpf_func_state {
struct bpf_reg_state regs[11]; /* 0 1320 */
/* --- cacheline 20 boundary (1280 bytes) was 40 bytes ago --- */
int callsite; /* 1320 4 */
u32 frameno; /* 1324 4 */
u32 subprogno; /* 1328 4 */
u32 async_entry_cnt; /* 1332 4 */
struct tnum callback_ret_range; /* 1336 16 */
/* --- cacheline 21 boundary (1344 bytes) was 8 bytes ago --- */
bool in_callback_fn; /* 1352 1 */
bool in_async_callback_fn; /* 1353 1 */
bool in_exception_callback_fn; /* 1354 1 */
/* XXX 1 byte hole, try to pack */
int acquired_refs; /* 1356 4 */
struct bpf_reference_state * refs; /* 1360 8 */
struct bpf_stack_state * stack; /* 1368 8 */
int allocated_stack; /* 1376 4 */
/* size: 1384, cachelines: 22, members: 13 */
/* sum members: 1379, holes: 1, sum holes: 1 */
/* padding: 4 */
/* last cacheline: 40 bytes */
};
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
include/linux/bpf_verifier.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index d99a636d36a7..0c0e1bccad45 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -297,8 +297,8 @@ struct bpf_func_state {
* void foo(void) { bpf_timer_set_callback(,foo); }
*/
u32 async_entry_cnt;
- bool in_callback_fn;
struct tnum callback_ret_range;
+ bool in_callback_fn;
bool in_async_callback_fn;
bool in_exception_callback_fn;
/* For callback calling functions that limit number of possible
@@ -316,8 +316,8 @@ struct bpf_func_state {
/* The following fields should be last. See copy_func_state() */
int acquired_refs;
struct bpf_reference_state *refs;
- int allocated_stack;
struct bpf_stack_state *stack;
+ int allocated_stack;
};
struct bpf_idx_pair {
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 bpf-next 02/11] bpf: provide correct register name for exception callback retval check
2023-12-01 18:33 [PATCH v4 bpf-next 00/11] BPF verifier retval logic fixes Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 01/11] bpf: rearrange bpf_func_state fields to save a bit of memory Andrii Nakryiko
@ 2023-12-01 18:33 ` Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 03/11] bpf: enforce precision of R0 on callback return Andrii Nakryiko
` (9 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2023-12-01 18:33 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau
Cc: andrii, kernel-team, Eduard Zingerman, Shung-Hsi Yu
bpf_throw() is checking R1, so let's report R1 in the log.
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Acked-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
kernel/bpf/verifier.c | 12 ++++++------
.../testing/selftests/bpf/progs/exceptions_assert.c | 2 +-
tools/testing/selftests/bpf/progs/exceptions_fail.c | 2 +-
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 8e7b6072e3f4..25b9d470957e 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11805,7 +11805,7 @@ static int fetch_kfunc_meta(struct bpf_verifier_env *env,
return 0;
}
-static int check_return_code(struct bpf_verifier_env *env, int regno);
+static int check_return_code(struct bpf_verifier_env *env, int regno, const char *reg_name);
static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
int *insn_idx_p)
@@ -11942,7 +11942,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
* to bpf_throw becomes the return value of the program.
*/
if (!env->exception_callback_subprog) {
- err = check_return_code(env, BPF_REG_1);
+ err = check_return_code(env, BPF_REG_1, "R1");
if (err < 0)
return err;
}
@@ -14972,7 +14972,7 @@ static int check_ld_abs(struct bpf_verifier_env *env, struct bpf_insn *insn)
return 0;
}
-static int check_return_code(struct bpf_verifier_env *env, int regno)
+static int check_return_code(struct bpf_verifier_env *env, int regno, const char *reg_name)
{
struct tnum enforce_attach_type_range = tnum_unknown;
const struct bpf_prog *prog = env->prog;
@@ -15026,7 +15026,7 @@ static int check_return_code(struct bpf_verifier_env *env, int regno)
}
if (!tnum_in(const_0, reg->var_off)) {
- verbose_invalid_scalar(env, reg, &const_0, "async callback", "R0");
+ verbose_invalid_scalar(env, reg, &const_0, "async callback", reg_name);
return -EINVAL;
}
return 0;
@@ -15126,7 +15126,7 @@ static int check_return_code(struct bpf_verifier_env *env, int regno)
}
if (!tnum_in(range, reg->var_off)) {
- verbose_invalid_scalar(env, reg, &range, "program exit", "R0");
+ verbose_invalid_scalar(env, reg, &range, "program exit", reg_name);
if (prog->expected_attach_type == BPF_LSM_CGROUP &&
prog_type == BPF_PROG_TYPE_LSM &&
!prog->aux->attach_func_proto->type)
@@ -17410,7 +17410,7 @@ static int do_check(struct bpf_verifier_env *env)
continue;
}
- err = check_return_code(env, BPF_REG_0);
+ err = check_return_code(env, BPF_REG_0, "R0");
if (err)
return err;
process_bpf_exit:
diff --git a/tools/testing/selftests/bpf/progs/exceptions_assert.c b/tools/testing/selftests/bpf/progs/exceptions_assert.c
index 49efaed143fc..575e7dd719c4 100644
--- a/tools/testing/selftests/bpf/progs/exceptions_assert.c
+++ b/tools/testing/selftests/bpf/progs/exceptions_assert.c
@@ -125,7 +125,7 @@ int check_assert_generic(struct __sk_buff *ctx)
}
SEC("?fentry/bpf_check")
-__failure __msg("At program exit the register R0 has value (0x40; 0x0)")
+__failure __msg("At program exit the register R1 has value (0x40; 0x0)")
int check_assert_with_return(void *ctx)
{
bpf_assert_with(!ctx, 64);
diff --git a/tools/testing/selftests/bpf/progs/exceptions_fail.c b/tools/testing/selftests/bpf/progs/exceptions_fail.c
index 8c0ef2742208..81ead7512ba2 100644
--- a/tools/testing/selftests/bpf/progs/exceptions_fail.c
+++ b/tools/testing/selftests/bpf/progs/exceptions_fail.c
@@ -308,7 +308,7 @@ int reject_set_exception_cb_bad_ret1(void *ctx)
}
SEC("?fentry/bpf_check")
-__failure __msg("At program exit the register R0 has value (0x40; 0x0) should")
+__failure __msg("At program exit the register R1 has value (0x40; 0x0) should")
int reject_set_exception_cb_bad_ret2(void *ctx)
{
bpf_throw(64);
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 bpf-next 03/11] bpf: enforce precision of R0 on callback return
2023-12-01 18:33 [PATCH v4 bpf-next 00/11] BPF verifier retval logic fixes Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 01/11] bpf: rearrange bpf_func_state fields to save a bit of memory Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 02/11] bpf: provide correct register name for exception callback retval check Andrii Nakryiko
@ 2023-12-01 18:33 ` Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 04/11] bpf: enforce exact retval range on subprog/callback exit Andrii Nakryiko
` (8 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2023-12-01 18:33 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau
Cc: andrii, kernel-team, Eduard Zingerman, Shung-Hsi Yu
Given verifier checks actual value, r0 has to be precise, so we need to
propagate precision properly. r0 also has to be marked as read,
otherwise subsequent state comparisons will ignore such register as
unimportant and precision won't really help here.
Fixes: 69c087ba6225 ("bpf: Add bpf_for_each_map_elem() helper")
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Acked-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
kernel/bpf/verifier.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 25b9d470957e..849fbf47b5f3 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -9590,6 +9590,13 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
verbose(env, "R0 not a scalar value\n");
return -EACCES;
}
+
+ /* we are going to rely on register's precise value */
+ err = mark_reg_read(env, r0, r0->parent, REG_LIVE_READ64);
+ err = err ?: mark_chain_precision(env, BPF_REG_0);
+ if (err)
+ return err;
+
if (!tnum_in(range, r0->var_off)) {
verbose_invalid_scalar(env, r0, &range, "callback return", "R0");
return -EINVAL;
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 bpf-next 04/11] bpf: enforce exact retval range on subprog/callback exit
2023-12-01 18:33 [PATCH v4 bpf-next 00/11] BPF verifier retval logic fixes Andrii Nakryiko
` (2 preceding siblings ...)
2023-12-01 18:33 ` [PATCH v4 bpf-next 03/11] bpf: enforce precision of R0 on callback return Andrii Nakryiko
@ 2023-12-01 18:33 ` Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 05/11] selftests/bpf: add selftest validating callback result is enforced Andrii Nakryiko
` (7 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2023-12-01 18:33 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau
Cc: andrii, kernel-team, Eduard Zingerman, Shung-Hsi Yu
Instead of relying on potentially imprecise tnum representation of
expected return value range for callbacks and subprogs, validate that
smin/smax range satisfy exact expected range of return values.
E.g., if callback would need to return [0, 2] range, tnum can't
represent this precisely and instead will allow [0, 3] range. By
checking smin/smax range, we can make sure that subprog/callback indeed
returns only valid [0, 2] range.
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Acked-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
include/linux/bpf_verifier.h | 7 ++++++-
kernel/bpf/verifier.c | 33 ++++++++++++++++++++++-----------
2 files changed, 28 insertions(+), 12 deletions(-)
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 0c0e1bccad45..3378cc753061 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -275,6 +275,11 @@ struct bpf_reference_state {
int callback_ref;
};
+struct bpf_retval_range {
+ s32 minval;
+ s32 maxval;
+};
+
/* state of the program:
* type of all registers and stack info
*/
@@ -297,7 +302,7 @@ struct bpf_func_state {
* void foo(void) { bpf_timer_set_callback(,foo); }
*/
u32 async_entry_cnt;
- struct tnum callback_ret_range;
+ struct bpf_retval_range callback_ret_range;
bool in_callback_fn;
bool in_async_callback_fn;
bool in_exception_callback_fn;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 849fbf47b5f3..f3d9d7de68da 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2305,6 +2305,11 @@ static void init_reg_state(struct bpf_verifier_env *env,
regs[BPF_REG_FP].frameno = state->frameno;
}
+static struct bpf_retval_range retval_range(s32 minval, s32 maxval)
+{
+ return (struct bpf_retval_range){ minval, maxval };
+}
+
#define BPF_MAIN_FUNC (-1)
static void init_func_state(struct bpf_verifier_env *env,
struct bpf_func_state *state,
@@ -2313,7 +2318,7 @@ static void init_func_state(struct bpf_verifier_env *env,
state->callsite = callsite;
state->frameno = frameno;
state->subprogno = subprogno;
- state->callback_ret_range = tnum_range(0, 0);
+ state->callback_ret_range = retval_range(0, 0);
init_reg_state(env, state);
mark_verifier_state_scratched(env);
}
@@ -9396,7 +9401,7 @@ static int set_map_elem_callback_state(struct bpf_verifier_env *env,
return err;
callee->in_callback_fn = true;
- callee->callback_ret_range = tnum_range(0, 1);
+ callee->callback_ret_range = retval_range(0, 1);
return 0;
}
@@ -9418,7 +9423,7 @@ static int set_loop_callback_state(struct bpf_verifier_env *env,
__mark_reg_not_init(env, &callee->regs[BPF_REG_5]);
callee->in_callback_fn = true;
- callee->callback_ret_range = tnum_range(0, 1);
+ callee->callback_ret_range = retval_range(0, 1);
return 0;
}
@@ -9448,7 +9453,7 @@ static int set_timer_callback_state(struct bpf_verifier_env *env,
__mark_reg_not_init(env, &callee->regs[BPF_REG_4]);
__mark_reg_not_init(env, &callee->regs[BPF_REG_5]);
callee->in_async_callback_fn = true;
- callee->callback_ret_range = tnum_range(0, 1);
+ callee->callback_ret_range = retval_range(0, 1);
return 0;
}
@@ -9476,7 +9481,7 @@ static int set_find_vma_callback_state(struct bpf_verifier_env *env,
__mark_reg_not_init(env, &callee->regs[BPF_REG_4]);
__mark_reg_not_init(env, &callee->regs[BPF_REG_5]);
callee->in_callback_fn = true;
- callee->callback_ret_range = tnum_range(0, 1);
+ callee->callback_ret_range = retval_range(0, 1);
return 0;
}
@@ -9499,7 +9504,7 @@ static int set_user_ringbuf_callback_state(struct bpf_verifier_env *env,
__mark_reg_not_init(env, &callee->regs[BPF_REG_5]);
callee->in_callback_fn = true;
- callee->callback_ret_range = tnum_range(0, 1);
+ callee->callback_ret_range = retval_range(0, 1);
return 0;
}
@@ -9531,7 +9536,7 @@ static int set_rbtree_add_callback_state(struct bpf_verifier_env *env,
__mark_reg_not_init(env, &callee->regs[BPF_REG_4]);
__mark_reg_not_init(env, &callee->regs[BPF_REG_5]);
callee->in_callback_fn = true;
- callee->callback_ret_range = tnum_range(0, 1);
+ callee->callback_ret_range = retval_range(0, 1);
return 0;
}
@@ -9560,6 +9565,11 @@ static bool in_rbtree_lock_required_cb(struct bpf_verifier_env *env)
return is_rbtree_lock_required_kfunc(kfunc_btf_id);
}
+static bool retval_range_within(struct bpf_retval_range range, const struct bpf_reg_state *reg)
+{
+ return range.minval <= reg->smin_value && reg->smax_value <= range.maxval;
+}
+
static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
{
struct bpf_verifier_state *state = env->cur_state, *prev_st;
@@ -9583,9 +9593,6 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
caller = state->frame[state->curframe - 1];
if (callee->in_callback_fn) {
- /* enforce R0 return value range [0, 1]. */
- struct tnum range = callee->callback_ret_range;
-
if (r0->type != SCALAR_VALUE) {
verbose(env, "R0 not a scalar value\n");
return -EACCES;
@@ -9597,7 +9604,11 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
if (err)
return err;
- if (!tnum_in(range, r0->var_off)) {
+ /* enforce R0 return value range */
+ if (!retval_range_within(callee->callback_ret_range, r0)) {
+ struct tnum range = tnum_range(callee->callback_ret_range.minval,
+ callee->callback_ret_range.maxval);
+
verbose_invalid_scalar(env, r0, &range, "callback return", "R0");
return -EINVAL;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 bpf-next 05/11] selftests/bpf: add selftest validating callback result is enforced
2023-12-01 18:33 [PATCH v4 bpf-next 00/11] BPF verifier retval logic fixes Andrii Nakryiko
` (3 preceding siblings ...)
2023-12-01 18:33 ` [PATCH v4 bpf-next 04/11] bpf: enforce exact retval range on subprog/callback exit Andrii Nakryiko
@ 2023-12-01 18:33 ` Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 06/11] bpf: enforce precise retval range on program exit Andrii Nakryiko
` (6 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2023-12-01 18:33 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau
Cc: andrii, kernel-team, Eduard Zingerman, Shung-Hsi Yu
BPF verifier expects callback subprogs to return values from specified
range (typically [0, 1]). This requires that r0 at exit is both precise
(because we rely on specific value range) and is marked as read
(otherwise state comparison will ignore such register as unimportant).
Add a simple test that validates that all these conditions are enforced.
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Acked-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
.../bpf/progs/verifier_subprog_precision.c | 50 +++++++++++++++++++
1 file changed, 50 insertions(+)
diff --git a/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c b/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c
index b5efcaeaa1ae..d41d2a8bb97e 100644
--- a/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c
+++ b/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c
@@ -117,6 +117,56 @@ __naked int global_subprog_result_precise(void)
);
}
+__naked __noinline __used
+static unsigned long loop_callback_bad()
+{
+ /* bpf_loop() callback that can return values outside of [0, 1] range */
+ asm volatile (
+ "call %[bpf_get_prandom_u32];"
+ "if r0 s> 1000 goto 1f;"
+ "r0 = 0;"
+ "1:"
+ "goto +0;" /* checkpoint */
+ /* bpf_loop() expects [0, 1] values, so branch above skipping
+ * r0 = 0; should lead to a failure, but if exit instruction
+ * doesn't enforce r0's precision, this callback will be
+ * successfully verified
+ */
+ "exit;"
+ :
+ : __imm(bpf_get_prandom_u32)
+ : __clobber_common
+ );
+}
+
+SEC("?raw_tp")
+__failure __log_level(2)
+__flag(BPF_F_TEST_STATE_FREQ)
+/* check that fallthrough code path marks r0 as precise */
+__msg("mark_precise: frame1: regs=r0 stack= before 11: (b7) r0 = 0")
+/* check that we have branch code path doing its own validation */
+__msg("from 10 to 12: frame1: R0=scalar(smin=umin=1001")
+/* check that branch code path marks r0 as precise, before failing */
+__msg("mark_precise: frame1: regs=r0 stack= before 9: (85) call bpf_get_prandom_u32#7")
+__msg("At callback return the register R0 has value (0x0; 0x7fffffffffffffff) should have been in (0x0; 0x1)")
+__naked int callback_precise_return_fail(void)
+{
+ asm volatile (
+ "r1 = 1;" /* nr_loops */
+ "r2 = %[loop_callback_bad];" /* callback_fn */
+ "r3 = 0;" /* callback_ctx */
+ "r4 = 0;" /* flags */
+ "call %[bpf_loop];"
+
+ "r0 = 0;"
+ "exit;"
+ :
+ : __imm_ptr(loop_callback_bad),
+ __imm(bpf_loop)
+ : __clobber_common
+ );
+}
+
SEC("?raw_tp")
__success __log_level(2)
/* First simulated path does not include callback body,
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 bpf-next 06/11] bpf: enforce precise retval range on program exit
2023-12-01 18:33 [PATCH v4 bpf-next 00/11] BPF verifier retval logic fixes Andrii Nakryiko
` (4 preceding siblings ...)
2023-12-01 18:33 ` [PATCH v4 bpf-next 05/11] selftests/bpf: add selftest validating callback result is enforced Andrii Nakryiko
@ 2023-12-01 18:33 ` Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 07/11] bpf: unify async callback and program retval checks Andrii Nakryiko
` (5 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2023-12-01 18:33 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau
Cc: andrii, kernel-team, Eduard Zingerman, Shung-Hsi Yu
Similarly to subprog/callback logic, enforce return value of BPF program
using more precise smin/smax range.
We need to adjust a bunch of tests due to a changed format of an error
message.
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Acked-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
kernel/bpf/verifier.c | 56 ++++++++++---------
.../selftests/bpf/progs/exceptions_assert.c | 2 +-
.../selftests/bpf/progs/exceptions_fail.c | 2 +-
.../selftests/bpf/progs/test_global_func15.c | 2 +-
.../selftests/bpf/progs/timer_failure.c | 2 +-
.../selftests/bpf/progs/user_ringbuf_fail.c | 2 +-
.../bpf/progs/verifier_cgroup_inv_retcode.c | 8 +--
.../bpf/progs/verifier_netfilter_retcode.c | 2 +-
.../bpf/progs/verifier_subprog_precision.c | 2 +-
9 files changed, 40 insertions(+), 38 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index f3d9d7de68da..9411c1046268 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -362,20 +362,23 @@ __printf(2, 3) static void verbose(void *private_data, const char *fmt, ...)
static void verbose_invalid_scalar(struct bpf_verifier_env *env,
struct bpf_reg_state *reg,
- struct tnum *range, const char *ctx,
+ struct bpf_retval_range range, const char *ctx,
const char *reg_name)
{
- char tn_buf[48];
+ bool unknown = true;
- verbose(env, "At %s the register %s ", ctx, reg_name);
- if (!tnum_is_unknown(reg->var_off)) {
- tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);
- verbose(env, "has value %s", tn_buf);
- } else {
- verbose(env, "has unknown scalar value");
+ verbose(env, "At %s the register %s has", ctx, reg_name);
+ if (reg->smin_value > S64_MIN) {
+ verbose(env, " smin=%lld", reg->smin_value);
+ unknown = false;
}
- tnum_strn(tn_buf, sizeof(tn_buf), *range);
- verbose(env, " should have been in %s\n", tn_buf);
+ if (reg->smax_value < S64_MAX) {
+ verbose(env, " smax=%lld", reg->smax_value);
+ unknown = false;
+ }
+ if (unknown)
+ verbose(env, " unknown scalar value");
+ verbose(env, " should have been in [%d, %d]\n", range.minval, range.maxval);
}
static bool type_may_be_null(u32 type)
@@ -9606,10 +9609,8 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
/* enforce R0 return value range */
if (!retval_range_within(callee->callback_ret_range, r0)) {
- struct tnum range = tnum_range(callee->callback_ret_range.minval,
- callee->callback_ret_range.maxval);
-
- verbose_invalid_scalar(env, r0, &range, "callback return", "R0");
+ verbose_invalid_scalar(env, r0, callee->callback_ret_range,
+ "callback return", "R0");
return -EINVAL;
}
if (!calls_callback(env, callee->callsite)) {
@@ -14995,7 +14996,8 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char
struct tnum enforce_attach_type_range = tnum_unknown;
const struct bpf_prog *prog = env->prog;
struct bpf_reg_state *reg;
- struct tnum range = tnum_range(0, 1), const_0 = tnum_const(0);
+ struct bpf_retval_range range = retval_range(0, 1);
+ struct bpf_retval_range const_0 = retval_range(0, 0);
enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
int err;
struct bpf_func_state *frame = env->cur_state->frame[0];
@@ -15043,8 +15045,8 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char
return -EINVAL;
}
- if (!tnum_in(const_0, reg->var_off)) {
- verbose_invalid_scalar(env, reg, &const_0, "async callback", reg_name);
+ if (!retval_range_within(const_0, reg)) {
+ verbose_invalid_scalar(env, reg, const_0, "async callback", reg_name);
return -EINVAL;
}
return 0;
@@ -15070,14 +15072,14 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char
env->prog->expected_attach_type == BPF_CGROUP_INET4_GETSOCKNAME ||
env->prog->expected_attach_type == BPF_CGROUP_INET6_GETSOCKNAME ||
env->prog->expected_attach_type == BPF_CGROUP_UNIX_GETSOCKNAME)
- range = tnum_range(1, 1);
+ range = retval_range(1, 1);
if (env->prog->expected_attach_type == BPF_CGROUP_INET4_BIND ||
env->prog->expected_attach_type == BPF_CGROUP_INET6_BIND)
- range = tnum_range(0, 3);
+ range = retval_range(0, 3);
break;
case BPF_PROG_TYPE_CGROUP_SKB:
if (env->prog->expected_attach_type == BPF_CGROUP_INET_EGRESS) {
- range = tnum_range(0, 3);
+ range = retval_range(0, 3);
enforce_attach_type_range = tnum_range(2, 3);
}
break;
@@ -15090,13 +15092,13 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char
case BPF_PROG_TYPE_RAW_TRACEPOINT:
if (!env->prog->aux->attach_btf_id)
return 0;
- range = tnum_const(0);
+ range = retval_range(0, 0);
break;
case BPF_PROG_TYPE_TRACING:
switch (env->prog->expected_attach_type) {
case BPF_TRACE_FENTRY:
case BPF_TRACE_FEXIT:
- range = tnum_const(0);
+ range = retval_range(0, 0);
break;
case BPF_TRACE_RAW_TP:
case BPF_MODIFY_RETURN:
@@ -15108,7 +15110,7 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char
}
break;
case BPF_PROG_TYPE_SK_LOOKUP:
- range = tnum_range(SK_DROP, SK_PASS);
+ range = retval_range(SK_DROP, SK_PASS);
break;
case BPF_PROG_TYPE_LSM:
@@ -15122,12 +15124,12 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char
/* Make sure programs that attach to void
* hooks don't try to modify return value.
*/
- range = tnum_range(1, 1);
+ range = retval_range(1, 1);
}
break;
case BPF_PROG_TYPE_NETFILTER:
- range = tnum_range(NF_DROP, NF_ACCEPT);
+ range = retval_range(NF_DROP, NF_ACCEPT);
break;
case BPF_PROG_TYPE_EXT:
/* freplace program can return anything as its return value
@@ -15143,8 +15145,8 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char
return -EINVAL;
}
- if (!tnum_in(range, reg->var_off)) {
- verbose_invalid_scalar(env, reg, &range, "program exit", reg_name);
+ if (!retval_range_within(range, reg)) {
+ verbose_invalid_scalar(env, reg, range, "program exit", reg_name);
if (prog->expected_attach_type == BPF_LSM_CGROUP &&
prog_type == BPF_PROG_TYPE_LSM &&
!prog->aux->attach_func_proto->type)
diff --git a/tools/testing/selftests/bpf/progs/exceptions_assert.c b/tools/testing/selftests/bpf/progs/exceptions_assert.c
index 575e7dd719c4..0ef81040da59 100644
--- a/tools/testing/selftests/bpf/progs/exceptions_assert.c
+++ b/tools/testing/selftests/bpf/progs/exceptions_assert.c
@@ -125,7 +125,7 @@ int check_assert_generic(struct __sk_buff *ctx)
}
SEC("?fentry/bpf_check")
-__failure __msg("At program exit the register R1 has value (0x40; 0x0)")
+__failure __msg("At program exit the register R1 has smin=64 smax=64")
int check_assert_with_return(void *ctx)
{
bpf_assert_with(!ctx, 64);
diff --git a/tools/testing/selftests/bpf/progs/exceptions_fail.c b/tools/testing/selftests/bpf/progs/exceptions_fail.c
index 81ead7512ba2..9cceb6521143 100644
--- a/tools/testing/selftests/bpf/progs/exceptions_fail.c
+++ b/tools/testing/selftests/bpf/progs/exceptions_fail.c
@@ -308,7 +308,7 @@ int reject_set_exception_cb_bad_ret1(void *ctx)
}
SEC("?fentry/bpf_check")
-__failure __msg("At program exit the register R1 has value (0x40; 0x0) should")
+__failure __msg("At program exit the register R1 has smin=64 smax=64 should")
int reject_set_exception_cb_bad_ret2(void *ctx)
{
bpf_throw(64);
diff --git a/tools/testing/selftests/bpf/progs/test_global_func15.c b/tools/testing/selftests/bpf/progs/test_global_func15.c
index b512d6a6c75e..f80207480e8a 100644
--- a/tools/testing/selftests/bpf/progs/test_global_func15.c
+++ b/tools/testing/selftests/bpf/progs/test_global_func15.c
@@ -13,7 +13,7 @@ __noinline int foo(unsigned int *v)
}
SEC("cgroup_skb/ingress")
-__failure __msg("At program exit the register R0 has value")
+__failure __msg("At program exit the register R0 has ")
int global_func15(struct __sk_buff *skb)
{
unsigned int v = 1;
diff --git a/tools/testing/selftests/bpf/progs/timer_failure.c b/tools/testing/selftests/bpf/progs/timer_failure.c
index 226d33b5a05c..9000da1e2120 100644
--- a/tools/testing/selftests/bpf/progs/timer_failure.c
+++ b/tools/testing/selftests/bpf/progs/timer_failure.c
@@ -30,7 +30,7 @@ static int timer_cb_ret1(void *map, int *key, struct bpf_timer *timer)
}
SEC("fentry/bpf_fentry_test1")
-__failure __msg("should have been in (0x0; 0x0)")
+__failure __msg("should have been in [0, 0]")
int BPF_PROG2(test_ret_1, int, a)
{
int key = 0;
diff --git a/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c b/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c
index 03ee946c6bf7..11ab25c42c36 100644
--- a/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c
+++ b/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c
@@ -184,7 +184,7 @@ invalid_drain_callback_return(struct bpf_dynptr *dynptr, void *context)
* not be able to write to that pointer.
*/
SEC("?raw_tp")
-__failure __msg("At callback return the register R0 has value")
+__failure __msg("At callback return the register R0 has ")
int user_ringbuf_callback_invalid_return(void *ctx)
{
bpf_user_ringbuf_drain(&user_ringbuf, invalid_drain_callback_return, NULL, 0);
diff --git a/tools/testing/selftests/bpf/progs/verifier_cgroup_inv_retcode.c b/tools/testing/selftests/bpf/progs/verifier_cgroup_inv_retcode.c
index d6c4a7f3f790..6e0f349f8f15 100644
--- a/tools/testing/selftests/bpf/progs/verifier_cgroup_inv_retcode.c
+++ b/tools/testing/selftests/bpf/progs/verifier_cgroup_inv_retcode.c
@@ -7,7 +7,7 @@
SEC("cgroup/sock")
__description("bpf_exit with invalid return code. test1")
-__failure __msg("R0 has value (0x0; 0xffffffff)")
+__failure __msg("smin=0 smax=4294967295 should have been in [0, 1]")
__naked void with_invalid_return_code_test1(void)
{
asm volatile (" \
@@ -30,7 +30,7 @@ __naked void with_invalid_return_code_test2(void)
SEC("cgroup/sock")
__description("bpf_exit with invalid return code. test3")
-__failure __msg("R0 has value (0x0; 0x3)")
+__failure __msg("smin=0 smax=3 should have been in [0, 1]")
__naked void with_invalid_return_code_test3(void)
{
asm volatile (" \
@@ -53,7 +53,7 @@ __naked void with_invalid_return_code_test4(void)
SEC("cgroup/sock")
__description("bpf_exit with invalid return code. test5")
-__failure __msg("R0 has value (0x2; 0x0)")
+__failure __msg("smin=2 smax=2 should have been in [0, 1]")
__naked void with_invalid_return_code_test5(void)
{
asm volatile (" \
@@ -75,7 +75,7 @@ __naked void with_invalid_return_code_test6(void)
SEC("cgroup/sock")
__description("bpf_exit with invalid return code. test7")
-__failure __msg("R0 has unknown scalar value")
+__failure __msg("R0 has unknown scalar value should have been in [0, 1]")
__naked void with_invalid_return_code_test7(void)
{
asm volatile (" \
diff --git a/tools/testing/selftests/bpf/progs/verifier_netfilter_retcode.c b/tools/testing/selftests/bpf/progs/verifier_netfilter_retcode.c
index 353ae6da00e1..e1ffa5d32ff0 100644
--- a/tools/testing/selftests/bpf/progs/verifier_netfilter_retcode.c
+++ b/tools/testing/selftests/bpf/progs/verifier_netfilter_retcode.c
@@ -39,7 +39,7 @@ __naked void with_valid_return_code_test3(void)
SEC("netfilter")
__description("bpf_exit with invalid return code. test4")
-__failure __msg("R0 has value (0x2; 0x0)")
+__failure __msg("R0 has smin=2 smax=2 should have been in [0, 1]")
__naked void with_invalid_return_code_test4(void)
{
asm volatile (" \
diff --git a/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c b/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c
index d41d2a8bb97e..0dfe3f8b69ac 100644
--- a/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c
+++ b/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c
@@ -148,7 +148,7 @@ __msg("mark_precise: frame1: regs=r0 stack= before 11: (b7) r0 = 0")
__msg("from 10 to 12: frame1: R0=scalar(smin=umin=1001")
/* check that branch code path marks r0 as precise, before failing */
__msg("mark_precise: frame1: regs=r0 stack= before 9: (85) call bpf_get_prandom_u32#7")
-__msg("At callback return the register R0 has value (0x0; 0x7fffffffffffffff) should have been in (0x0; 0x1)")
+__msg("At callback return the register R0 has smin=1001 should have been in [0, 1]")
__naked int callback_precise_return_fail(void)
{
asm volatile (
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 bpf-next 07/11] bpf: unify async callback and program retval checks
2023-12-01 18:33 [PATCH v4 bpf-next 00/11] BPF verifier retval logic fixes Andrii Nakryiko
` (5 preceding siblings ...)
2023-12-01 18:33 ` [PATCH v4 bpf-next 06/11] bpf: enforce precise retval range on program exit Andrii Nakryiko
@ 2023-12-01 18:33 ` Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 08/11] bpf: enforce precision of R0 on program/async callback return Andrii Nakryiko
` (4 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2023-12-01 18:33 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau
Cc: andrii, kernel-team, Eduard Zingerman, Shung-Hsi Yu
Use common logic to verify program return values and async callback
return values. This allows to avoid duplication of any extra steps
necessary, like precision marking, which will be added in the next
patch.
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Acked-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
kernel/bpf/verifier.c | 30 ++++++++++++------------------
1 file changed, 12 insertions(+), 18 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 9411c1046268..c54944af1bcc 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -367,7 +367,7 @@ static void verbose_invalid_scalar(struct bpf_verifier_env *env,
{
bool unknown = true;
- verbose(env, "At %s the register %s has", ctx, reg_name);
+ verbose(env, "%s the register %s has", ctx, reg_name);
if (reg->smin_value > S64_MIN) {
verbose(env, " smin=%lld", reg->smin_value);
unknown = false;
@@ -9610,7 +9610,7 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
/* enforce R0 return value range */
if (!retval_range_within(callee->callback_ret_range, r0)) {
verbose_invalid_scalar(env, r0, callee->callback_ret_range,
- "callback return", "R0");
+ "At callback return", "R0");
return -EINVAL;
}
if (!calls_callback(env, callee->callsite)) {
@@ -14993,11 +14993,11 @@ static int check_ld_abs(struct bpf_verifier_env *env, struct bpf_insn *insn)
static int check_return_code(struct bpf_verifier_env *env, int regno, const char *reg_name)
{
+ const char *exit_ctx = "At program exit";
struct tnum enforce_attach_type_range = tnum_unknown;
const struct bpf_prog *prog = env->prog;
struct bpf_reg_state *reg;
struct bpf_retval_range range = retval_range(0, 1);
- struct bpf_retval_range const_0 = retval_range(0, 0);
enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
int err;
struct bpf_func_state *frame = env->cur_state->frame[0];
@@ -15039,17 +15039,9 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char
if (frame->in_async_callback_fn) {
/* enforce return zero from async callbacks like timer */
- if (reg->type != SCALAR_VALUE) {
- verbose(env, "In async callback the register R%d is not a known value (%s)\n",
- regno, reg_type_str(env, reg->type));
- return -EINVAL;
- }
-
- if (!retval_range_within(const_0, reg)) {
- verbose_invalid_scalar(env, reg, const_0, "async callback", reg_name);
- return -EINVAL;
- }
- return 0;
+ exit_ctx = "At async callback return";
+ range = retval_range(0, 0);
+ goto enforce_retval;
}
if (is_subprog && !frame->in_exception_callback_fn) {
@@ -15139,15 +15131,17 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char
return 0;
}
+enforce_retval:
if (reg->type != SCALAR_VALUE) {
- verbose(env, "At program exit the register R%d is not a known value (%s)\n",
- regno, reg_type_str(env, reg->type));
+ verbose(env, "%s the register R%d is not a known value (%s)\n",
+ exit_ctx, regno, reg_type_str(env, reg->type));
return -EINVAL;
}
if (!retval_range_within(range, reg)) {
- verbose_invalid_scalar(env, reg, range, "program exit", reg_name);
- if (prog->expected_attach_type == BPF_LSM_CGROUP &&
+ verbose_invalid_scalar(env, reg, range, exit_ctx, reg_name);
+ if (!is_subprog &&
+ prog->expected_attach_type == BPF_LSM_CGROUP &&
prog_type == BPF_PROG_TYPE_LSM &&
!prog->aux->attach_func_proto->type)
verbose(env, "Note, BPF_LSM_CGROUP that attach to void LSM hooks can't modify return value!\n");
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 bpf-next 08/11] bpf: enforce precision of R0 on program/async callback return
2023-12-01 18:33 [PATCH v4 bpf-next 00/11] BPF verifier retval logic fixes Andrii Nakryiko
` (6 preceding siblings ...)
2023-12-01 18:33 ` [PATCH v4 bpf-next 07/11] bpf: unify async callback and program retval checks Andrii Nakryiko
@ 2023-12-01 18:33 ` Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 09/11] selftests/bpf: validate async callback return value check correctness Andrii Nakryiko
` (3 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2023-12-01 18:33 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team, Eduard Zingerman
Given we enforce a valid range for program and async callback return
value, we must mark R0 as precise to avoid incorrect state pruning.
Fixes: b5dc0163d8fd ("bpf: precise scalar_value tracking")
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
kernel/bpf/verifier.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index c54944af1bcc..2cd150d6d141 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -15138,6 +15138,10 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char
return -EINVAL;
}
+ err = mark_chain_precision(env, regno);
+ if (err)
+ return err;
+
if (!retval_range_within(range, reg)) {
verbose_invalid_scalar(env, reg, range, exit_ctx, reg_name);
if (!is_subprog &&
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 bpf-next 09/11] selftests/bpf: validate async callback return value check correctness
2023-12-01 18:33 [PATCH v4 bpf-next 00/11] BPF verifier retval logic fixes Andrii Nakryiko
` (7 preceding siblings ...)
2023-12-01 18:33 ` [PATCH v4 bpf-next 08/11] bpf: enforce precision of R0 on program/async callback return Andrii Nakryiko
@ 2023-12-01 18:33 ` Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 10/11] selftests/bpf: adjust global_func15 test to validate prog exit precision Andrii Nakryiko
` (2 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2023-12-01 18:33 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team, Eduard Zingerman
Adjust timer/timer_ret_1 test to validate more carefully verifier logic
of enforcing async callback return value. This test will pass only if
return result is marked precise and read.
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
.../selftests/bpf/progs/timer_failure.c | 36 ++++++++++++++-----
1 file changed, 28 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/bpf/progs/timer_failure.c b/tools/testing/selftests/bpf/progs/timer_failure.c
index 9000da1e2120..a9f717d4e269 100644
--- a/tools/testing/selftests/bpf/progs/timer_failure.c
+++ b/tools/testing/selftests/bpf/progs/timer_failure.c
@@ -21,17 +21,37 @@ struct {
__type(value, struct elem);
} timer_map SEC(".maps");
-static int timer_cb_ret1(void *map, int *key, struct bpf_timer *timer)
+__naked __noinline __used
+static unsigned long timer_cb_ret_bad()
{
- if (bpf_get_smp_processor_id() % 2)
- return 1;
- else
- return 0;
+ asm volatile (
+ "call %[bpf_get_prandom_u32];"
+ "if r0 s> 1000 goto 1f;"
+ "r0 = 0;"
+ "1:"
+ "goto +0;" /* checkpoint */
+ /* async callback is expected to return 0, so branch above
+ * skipping r0 = 0; should lead to a failure, but if exit
+ * instruction doesn't enforce r0's precision, this callback
+ * will be successfully verified
+ */
+ "exit;"
+ :
+ : __imm(bpf_get_prandom_u32)
+ : __clobber_common
+ );
}
SEC("fentry/bpf_fentry_test1")
-__failure __msg("should have been in [0, 0]")
-int BPF_PROG2(test_ret_1, int, a)
+__log_level(2)
+__flag(BPF_F_TEST_STATE_FREQ)
+__failure
+/* check that fallthrough code path marks r0 as precise */
+__msg("mark_precise: frame0: regs=r0 stack= before 22: (b4) w0 = 0")
+/* check that branch code path marks r0 as precise */
+__msg("mark_precise: frame0: regs=r0 stack= before 24: (85) call bpf_get_prandom_u32#7")
+__msg("should have been in [0, 0]")
+int BPF_PROG2(test_bad_ret, int, a)
{
int key = 0;
struct bpf_timer *timer;
@@ -39,7 +59,7 @@ int BPF_PROG2(test_ret_1, int, a)
timer = bpf_map_lookup_elem(&timer_map, &key);
if (timer) {
bpf_timer_init(timer, &timer_map, CLOCK_BOOTTIME);
- bpf_timer_set_callback(timer, timer_cb_ret1);
+ bpf_timer_set_callback(timer, timer_cb_ret_bad);
bpf_timer_start(timer, 1000, 0);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 bpf-next 10/11] selftests/bpf: adjust global_func15 test to validate prog exit precision
2023-12-01 18:33 [PATCH v4 bpf-next 00/11] BPF verifier retval logic fixes Andrii Nakryiko
` (8 preceding siblings ...)
2023-12-01 18:33 ` [PATCH v4 bpf-next 09/11] selftests/bpf: validate async callback return value check correctness Andrii Nakryiko
@ 2023-12-01 18:33 ` Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 11/11] bpf: simplify tnum output if a fully known constant Andrii Nakryiko
2023-12-01 20:11 ` [PATCH v4 bpf-next 00/11] BPF verifier retval logic fixes Andrii Nakryiko
11 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2023-12-01 18:33 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team, Eduard Zingerman
Add one more subtest to global_func15 selftest to validate that
verifier properly marks r0 as precise and avoids erroneous state pruning
of the branch that has return value outside of expected [0, 1] value.
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
.../selftests/bpf/progs/test_global_func15.c | 32 +++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/tools/testing/selftests/bpf/progs/test_global_func15.c b/tools/testing/selftests/bpf/progs/test_global_func15.c
index f80207480e8a..b4e089d6981d 100644
--- a/tools/testing/selftests/bpf/progs/test_global_func15.c
+++ b/tools/testing/selftests/bpf/progs/test_global_func15.c
@@ -22,3 +22,35 @@ int global_func15(struct __sk_buff *skb)
return v;
}
+
+SEC("cgroup_skb/ingress")
+__log_level(2) __flag(BPF_F_TEST_STATE_FREQ)
+__failure
+/* check that fallthrough code path marks r0 as precise */
+__msg("mark_precise: frame0: regs=r0 stack= before 2: (b7) r0 = 1")
+/* check that branch code path marks r0 as precise */
+__msg("mark_precise: frame0: regs=r0 stack= before 0: (85) call bpf_get_prandom_u32#7")
+__msg("At program exit the register R0 has ")
+__naked int global_func15_tricky_pruning(void)
+{
+ asm volatile (
+ "call %[bpf_get_prandom_u32];"
+ "if r0 s> 1000 goto 1f;"
+ "r0 = 1;"
+ "1:"
+ "goto +0;" /* checkpoint */
+ /* cgroup_skb/ingress program is expected to return [0, 1]
+ * values, so branch above makes sure that in a fallthrough
+ * case we have a valid 1 stored in R0 register, but in
+ * a branch case we assign some random value to R0. So if
+ * there is something wrong with precision tracking for R0 at
+ * program exit, we might erronenously prune branch case,
+ * because R0 in fallthrough case is imprecise (and thus any
+ * value is valid from POV of verifier is_state_equal() logic)
+ */
+ "exit;"
+ :
+ : __imm(bpf_get_prandom_u32)
+ : __clobber_common
+ );
+}
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 bpf-next 11/11] bpf: simplify tnum output if a fully known constant
2023-12-01 18:33 [PATCH v4 bpf-next 00/11] BPF verifier retval logic fixes Andrii Nakryiko
` (9 preceding siblings ...)
2023-12-01 18:33 ` [PATCH v4 bpf-next 10/11] selftests/bpf: adjust global_func15 test to validate prog exit precision Andrii Nakryiko
@ 2023-12-01 18:33 ` Andrii Nakryiko
2023-12-01 20:11 ` [PATCH v4 bpf-next 00/11] BPF verifier retval logic fixes Andrii Nakryiko
11 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2023-12-01 18:33 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team, Shung-Hsi Yu
Emit tnum representation as just a constant if all bits are known.
Use decimal-vs-hex logic to determine exact format of emitted
constant value, just like it's done for register range values.
For that move tnum_strn() to kernel/bpf/log.c to reuse decimal-vs-hex
determination logic and constants.
Acked-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
kernel/bpf/log.c | 13 +++++++++++++
kernel/bpf/tnum.c | 6 ------
.../bpf/progs/verifier_direct_packet_access.c | 2 +-
.../testing/selftests/bpf/progs/verifier_int_ptr.c | 2 +-
.../selftests/bpf/progs/verifier_stack_ptr.c | 4 ++--
5 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/kernel/bpf/log.c b/kernel/bpf/log.c
index 3505f3e5ae96..55d019f30e91 100644
--- a/kernel/bpf/log.c
+++ b/kernel/bpf/log.c
@@ -539,6 +539,19 @@ static void verbose_snum(struct bpf_verifier_env *env, s64 num)
verbose(env, "%#llx", num);
}
+int tnum_strn(char *str, size_t size, struct tnum a)
+{
+ /* print as a constant, if tnum is fully known */
+ if (a.mask == 0) {
+ if (is_unum_decimal(a.value))
+ return snprintf(str, size, "%llu", a.value);
+ else
+ return snprintf(str, size, "%#llx", a.value);
+ }
+ return snprintf(str, size, "(%#llx; %#llx)", a.value, a.mask);
+}
+EXPORT_SYMBOL_GPL(tnum_strn);
+
static void print_scalar_ranges(struct bpf_verifier_env *env,
const struct bpf_reg_state *reg,
const char **sep)
diff --git a/kernel/bpf/tnum.c b/kernel/bpf/tnum.c
index f4c91c9b27d7..9dbc31b25e3d 100644
--- a/kernel/bpf/tnum.c
+++ b/kernel/bpf/tnum.c
@@ -172,12 +172,6 @@ bool tnum_in(struct tnum a, struct tnum b)
return a.value == b.value;
}
-int tnum_strn(char *str, size_t size, struct tnum a)
-{
- return snprintf(str, size, "(%#llx; %#llx)", a.value, a.mask);
-}
-EXPORT_SYMBOL_GPL(tnum_strn);
-
int tnum_sbin(char *str, size_t size, struct tnum a)
{
size_t n;
diff --git a/tools/testing/selftests/bpf/progs/verifier_direct_packet_access.c b/tools/testing/selftests/bpf/progs/verifier_direct_packet_access.c
index 99a23dea8233..be95570ab382 100644
--- a/tools/testing/selftests/bpf/progs/verifier_direct_packet_access.c
+++ b/tools/testing/selftests/bpf/progs/verifier_direct_packet_access.c
@@ -411,7 +411,7 @@ l0_%=: r0 = 0; \
SEC("tc")
__description("direct packet access: test17 (pruning, alignment)")
-__failure __msg("misaligned packet access off 2+(0x0; 0x0)+15+-4 size 4")
+__failure __msg("misaligned packet access off 2+0+15+-4 size 4")
__flag(BPF_F_STRICT_ALIGNMENT)
__naked void packet_access_test17_pruning_alignment(void)
{
diff --git a/tools/testing/selftests/bpf/progs/verifier_int_ptr.c b/tools/testing/selftests/bpf/progs/verifier_int_ptr.c
index b054f9c48143..74d9cad469d9 100644
--- a/tools/testing/selftests/bpf/progs/verifier_int_ptr.c
+++ b/tools/testing/selftests/bpf/progs/verifier_int_ptr.c
@@ -67,7 +67,7 @@ __naked void ptr_to_long_half_uninitialized(void)
SEC("cgroup/sysctl")
__description("ARG_PTR_TO_LONG misaligned")
-__failure __msg("misaligned stack access off (0x0; 0x0)+-20+0 size 8")
+__failure __msg("misaligned stack access off 0+-20+0 size 8")
__naked void arg_ptr_to_long_misaligned(void)
{
asm volatile (" \
diff --git a/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c b/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c
index e0f77e3e7869..417c61cd4b19 100644
--- a/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c
+++ b/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c
@@ -37,7 +37,7 @@ __naked void ptr_to_stack_store_load(void)
SEC("socket")
__description("PTR_TO_STACK store/load - bad alignment on off")
-__failure __msg("misaligned stack access off (0x0; 0x0)+-8+2 size 8")
+__failure __msg("misaligned stack access off 0+-8+2 size 8")
__failure_unpriv
__naked void load_bad_alignment_on_off(void)
{
@@ -53,7 +53,7 @@ __naked void load_bad_alignment_on_off(void)
SEC("socket")
__description("PTR_TO_STACK store/load - bad alignment on reg")
-__failure __msg("misaligned stack access off (0x0; 0x0)+-10+8 size 8")
+__failure __msg("misaligned stack access off 0+-10+8 size 8")
__failure_unpriv
__naked void load_bad_alignment_on_reg(void)
{
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v4 bpf-next 00/11] BPF verifier retval logic fixes
2023-12-01 18:33 [PATCH v4 bpf-next 00/11] BPF verifier retval logic fixes Andrii Nakryiko
` (10 preceding siblings ...)
2023-12-01 18:33 ` [PATCH v4 bpf-next 11/11] bpf: simplify tnum output if a fully known constant Andrii Nakryiko
@ 2023-12-01 20:11 ` Andrii Nakryiko
11 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2023-12-01 20:11 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, ast, daniel, martin.lau, kernel-team
On Fri, Dec 1, 2023 at 10:34 AM Andrii Nakryiko <andrii@kernel.org> wrote:
>
> This patch set fixes BPF verifier logic around validating and enforcing return
> values for BPF programs that have specific range of expected return values.
> Both sync and async callbacks have similar logic and are fixes as well.
> A few tests are added that would fail without the fixes in this patch set.
>
> Also, while at it, we update retval checking logic to use smin/smax range
> instead of tnum, avoiding future potential issues if expected range cannot be
> represented precisely by tnum (e.g., [0, 2] is not representable by tnum and
> is treated as [0, 3]).
>
> There is a little bit of refactoring to unify async callback and program exit
> logic to avoid duplication of checks as much as possible.
>
> v3->v4:
> - add back bpf_func_state rearrangement patch;
> - simplified patch #4 as suggested (Shung-Hsi);
> v2->v3:
> - more carefullly switch from umin/umax to smin/smax;
> v1->v2:
> - drop tnum from retval checks (Eduard);
> - use smin/smax instead of umin/umax (Alexei).
This patch set must be cursed or something :) CI caught regression for
no-alu32 test_progs variant in test_bad_ret:
EXPECTED MSG: 'mark_precise: frame0: regs=r0 stack= before 22: (b4) w0 = 0'
I'll check, fix, and will try again, maybe v5 will be luckier.
>
> Andrii Nakryiko (11):
> bpf: rearrange bpf_func_state fields to save a bit of memory
> bpf: provide correct register name for exception callback retval check
> bpf: enforce precision of R0 on callback return
> bpf: enforce exact retval range on subprog/callback exit
> selftests/bpf: add selftest validating callback result is enforced
> bpf: enforce precise retval range on program exit
> bpf: unify async callback and program retval checks
> bpf: enforce precision of R0 on program/async callback return
> selftests/bpf: validate async callback return value check correctness
> selftests/bpf: adjust global_func15 test to validate prog exit
> precision
> bpf: simplify tnum output if a fully known constant
>
> include/linux/bpf_verifier.h | 9 +-
> kernel/bpf/log.c | 13 ++
> kernel/bpf/tnum.c | 6 -
> kernel/bpf/verifier.c | 120 ++++++++++--------
> .../selftests/bpf/progs/exceptions_assert.c | 2 +-
> .../selftests/bpf/progs/exceptions_fail.c | 2 +-
> .../selftests/bpf/progs/test_global_func15.c | 34 ++++-
> .../selftests/bpf/progs/timer_failure.c | 36 ++++--
> .../selftests/bpf/progs/user_ringbuf_fail.c | 2 +-
> .../bpf/progs/verifier_cgroup_inv_retcode.c | 8 +-
> .../bpf/progs/verifier_direct_packet_access.c | 2 +-
> .../selftests/bpf/progs/verifier_int_ptr.c | 2 +-
> .../bpf/progs/verifier_netfilter_retcode.c | 2 +-
> .../selftests/bpf/progs/verifier_stack_ptr.c | 4 +-
> .../bpf/progs/verifier_subprog_precision.c | 50 ++++++++
> 15 files changed, 212 insertions(+), 80 deletions(-)
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-12-01 20:11 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-01 18:33 [PATCH v4 bpf-next 00/11] BPF verifier retval logic fixes Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 01/11] bpf: rearrange bpf_func_state fields to save a bit of memory Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 02/11] bpf: provide correct register name for exception callback retval check Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 03/11] bpf: enforce precision of R0 on callback return Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 04/11] bpf: enforce exact retval range on subprog/callback exit Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 05/11] selftests/bpf: add selftest validating callback result is enforced Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 06/11] bpf: enforce precise retval range on program exit Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 07/11] bpf: unify async callback and program retval checks Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 08/11] bpf: enforce precision of R0 on program/async callback return Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 09/11] selftests/bpf: validate async callback return value check correctness Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 10/11] selftests/bpf: adjust global_func15 test to validate prog exit precision Andrii Nakryiko
2023-12-01 18:33 ` [PATCH v4 bpf-next 11/11] bpf: simplify tnum output if a fully known constant Andrii Nakryiko
2023-12-01 20:11 ` [PATCH v4 bpf-next 00/11] BPF verifier retval logic fixes Andrii Nakryiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox