* [PATCH bpf-next v2 00/11] Dynptr fixes
@ 2023-01-19 2:14 Kumar Kartikeya Dwivedi
2023-01-19 2:14 ` [PATCH bpf-next v2 01/11] bpf: Fix state pruning for STACK_DYNPTR stack slots Kumar Kartikeya Dwivedi
` (10 more replies)
0 siblings, 11 replies; 22+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2023-01-19 2:14 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, Joanne Koong, David Vernet, Eduard Zingerman
This is part 2 of https://lore.kernel.org/bpf/20221018135920.726360-1-memxor@gmail.com.
Changelog:
----------
v1 -> v2
v1: https://lore.kernel.org/bpf/20230101083403.332783-1-memxor@gmail.com
* Return error early in case of overwriting referenced dynptr slots (Andrii, Joanne)
* Rename destroy_stack_slots_dynptr to destroy_if_dynptr_stack_slot (Joanne)
* Invalidate dynptr slices associated with dynptr in destroy_if_dynptr_stack_slot (Joanne)
* Combine both dynptr_get_spi and is_spi_bounds_valid (Joanne)
* Compute spi once in process_dynptr_func and pass it as parameter instead of recomputing (Joanne)
* Add comments expanding REG_LIVE_WRITTEN marking in unmark_stack_slots_dynptr (Joanne)
* Add comments explaining why destroy_if_dynptr_stack_slot call needs to be done for both spi
and spi - 1 (Joanne)
* Port BPF assembly tests from test_verifier to test_progs framework (Andrii)
* Address misc feedback, rebase to bpf-next
Old v1 -> v1
Old v1: https://lore.kernel.org/bpf/20221018135920.726360-1-memxor@gmail.com
* Allow overwriting dynptr stack slots from dynptr init helpers
* Fix a bug in alignment check where reg->var_off.value was still not included
* Address other minor nits
Eduard Zingerman (1):
selftests/bpf: convenience macro for use with 'asm volatile' blocks
Kumar Kartikeya Dwivedi (10):
bpf: Fix state pruning for STACK_DYNPTR stack slots
bpf: Fix missing var_off check for ARG_PTR_TO_DYNPTR
bpf: Fix partial dynptr stack slot reads/writes
bpf: Allow reinitializing unreferenced dynptr stack slots
bpf: Combine dynptr_get_spi and is_spi_bounds_valid
bpf: Avoid recomputing spi in process_dynptr_func
selftests/bpf: Add dynptr pruning tests
selftests/bpf: Add dynptr var_off tests
selftests/bpf: Add dynptr partial slot overwrite tests
selftests/bpf: Add dynptr helper tests
kernel/bpf/verifier.c | 347 +++++++++++++++---
.../bpf/prog_tests/kfunc_dynptr_param.c | 2 +-
tools/testing/selftests/bpf/progs/bpf_misc.h | 7 +
.../testing/selftests/bpf/progs/dynptr_fail.c | 319 +++++++++++++++-
4 files changed, 611 insertions(+), 64 deletions(-)
base-commit: 92afc5329a5b23d876b215b783d200352d5aaea6
--
2.39.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH bpf-next v2 01/11] bpf: Fix state pruning for STACK_DYNPTR stack slots
2023-01-19 2:14 [PATCH bpf-next v2 00/11] Dynptr fixes Kumar Kartikeya Dwivedi
@ 2023-01-19 2:14 ` Kumar Kartikeya Dwivedi
2023-01-19 2:14 ` [PATCH bpf-next v2 02/11] bpf: Fix missing var_off check for ARG_PTR_TO_DYNPTR Kumar Kartikeya Dwivedi
` (9 subsequent siblings)
10 siblings, 0 replies; 22+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2023-01-19 2:14 UTC (permalink / raw)
To: bpf
Cc: Eduard Zingerman, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Martin KaFai Lau, Joanne Koong, David Vernet
The root of the problem is missing liveness marking for STACK_DYNPTR
slots. This leads to all kinds of problems inside stacksafe.
The verifier by default inside stacksafe ignores spilled_ptr in stack
slots which do not have REG_LIVE_READ marks. Since this is being checked
in the 'old' explored state, it must have already done clean_live_states
for this old bpf_func_state. Hence, it won't be receiving any more
liveness marks from to be explored insns (it has received REG_LIVE_DONE
marking from liveness point of view).
What this means is that verifier considers that it's safe to not compare
the stack slot if was never read by children states. While liveness
marks are usually propagated correctly following the parentage chain for
spilled registers (SCALAR_VALUE and PTR_* types), the same is not the
case for STACK_DYNPTR.
clean_live_states hence simply rewrites these stack slots to the type
STACK_INVALID since it sees no REG_LIVE_READ marks.
The end result is that we will never see STACK_DYNPTR slots in explored
state. Even if verifier was conservatively matching !REG_LIVE_READ
slots, very next check continuing the stacksafe loop on seeing
STACK_INVALID would again prevent further checks.
Now as long as verifier stores an explored state which we can compare to
when reaching a pruning point, we can abuse this bug to make verifier
prune search for obviously unsafe paths using STACK_DYNPTR slots
thinking they are never used hence safe.
Doing this in unprivileged mode is a bit challenging. add_new_state is
only set when seeing BPF_F_TEST_STATE_FREQ (which requires privileges)
or when jmps_processed difference is >= 2 and insn_processed difference
is >= 8. So coming up with the unprivileged case requires a little more
work, but it is still totally possible. The test case being discussed
below triggers the heuristic even in unprivileged mode.
However, it no longer works since commit
8addbfc7b308 ("bpf: Gate dynptr API behind CAP_BPF").
Let's try to study the test step by step.
Consider the following program (C style BPF ASM):
0 r0 = 0;
1 r6 = &ringbuf_map;
3 r1 = r6;
4 r2 = 8;
5 r3 = 0;
6 r4 = r10;
7 r4 -= -16;
8 call bpf_ringbuf_reserve_dynptr;
9 if r0 == 0 goto pc+1;
10 goto pc+1;
11 *(r10 - 16) = 0xeB9F;
12 r1 = r10;
13 r1 -= -16;
14 r2 = 0;
15 call bpf_ringbuf_discard_dynptr;
16 r0 = 0;
17 exit;
We know that insn 12 will be a pruning point, hence if we force
add_new_state for it, it will first verify the following path as
safe in straight line exploration:
0 1 3 4 5 6 7 8 9 -> 10 -> (12) 13 14 15 16 17
Then, when we arrive at insn 12 from the following path:
0 1 3 4 5 6 7 8 9 -> 11 (12)
We will find a state that has been verified as safe already at insn 12.
Since register state is same at this point, regsafe will pass. Next, in
stacksafe, for spi = 0 and spi = 1 (location of our dynptr) is skipped
seeing !REG_LIVE_READ. The rest matches, so stacksafe returns true.
Next, refsafe is also true as reference state is unchanged in both
states.
The states are considered equivalent and search is pruned.
Hence, we are able to construct a dynptr with arbitrary contents and use
the dynptr API to operate on this arbitrary pointer and arbitrary size +
offset.
To fix this, first define a mark_dynptr_read function that propagates
liveness marks whenever a valid initialized dynptr is accessed by dynptr
helpers. REG_LIVE_WRITTEN is marked whenever we initialize an
uninitialized dynptr. This is done in mark_stack_slots_dynptr. It allows
screening off mark_reg_read and not propagating marks upwards from that
point.
This ensures that we either set REG_LIVE_READ64 on both dynptr slots, or
none, so clean_live_states either sets both slots to STACK_INVALID or
none of them. This is the invariant the checks inside stacksafe rely on.
Next, do a complete comparison of both stack slots whenever they have
STACK_DYNPTR. Compare the dynptr type stored in the spilled_ptr, and
also whether both form the same first_slot. Only then is the later path
safe.
Fixes: 97e03f521050 ("bpf: Add verifier support for dynptrs")
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
kernel/bpf/verifier.c | 83 ++++++++++++++++++++++++++++++++++++++++---
1 file changed, 79 insertions(+), 4 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index ca7db2ce70b9..89de5bc46f27 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -781,6 +781,9 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
state->stack[spi - 1].spilled_ptr.ref_obj_id = id;
}
+ state->stack[spi].spilled_ptr.live |= REG_LIVE_WRITTEN;
+ state->stack[spi - 1].spilled_ptr.live |= REG_LIVE_WRITTEN;
+
return 0;
}
@@ -805,6 +808,31 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re
__mark_reg_not_init(env, &state->stack[spi].spilled_ptr);
__mark_reg_not_init(env, &state->stack[spi - 1].spilled_ptr);
+
+ /* Why do we need to set REG_LIVE_WRITTEN for STACK_INVALID slot?
+ *
+ * While we don't allow reading STACK_INVALID, it is still possible to
+ * do <8 byte writes marking some but not all slots as STACK_MISC. Then,
+ * helpers or insns can do partial read of that part without failing,
+ * but check_stack_range_initialized, check_stack_read_var_off, and
+ * check_stack_read_fixed_off will do mark_reg_read for all 8-bytes of
+ * the slot conservatively. Hence we need to prevent those liveness
+ * marking walks.
+ *
+ * This was not a problem before because STACK_INVALID is only set by
+ * default (where the default reg state has its reg->parent as NULL), or
+ * in clean_live_states after REG_LIVE_DONE (at which point
+ * mark_reg_read won't walk reg->parent chain), but not randomly during
+ * verifier state exploration (like we did above). Hence, for our case
+ * parentage chain will still be live (i.e. reg->parent may be
+ * non-NULL), while earlier reg->parent was NULL, so we need
+ * REG_LIVE_WRITTEN to screen off read marker propagation when it is
+ * done later on reads or by mark_dynptr_read as well to unnecessary
+ * mark registers in verifier state.
+ */
+ state->stack[spi].spilled_ptr.live |= REG_LIVE_WRITTEN;
+ state->stack[spi - 1].spilled_ptr.live |= REG_LIVE_WRITTEN;
+
return 0;
}
@@ -2390,6 +2418,30 @@ static int mark_reg_read(struct bpf_verifier_env *env,
return 0;
}
+static int mark_dynptr_read(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
+{
+ struct bpf_func_state *state = func(env, reg);
+ int spi, ret;
+
+ /* For CONST_PTR_TO_DYNPTR, it must have already been done by
+ * check_reg_arg in check_helper_call and mark_btf_func_reg_size in
+ * check_kfunc_call.
+ */
+ if (reg->type == CONST_PTR_TO_DYNPTR)
+ return 0;
+ spi = get_spi(reg->off);
+ /* Caller ensures dynptr is valid and initialized, which means spi is in
+ * bounds and spi is the first dynptr slot. Simply mark stack slot as
+ * read.
+ */
+ ret = mark_reg_read(env, &state->stack[spi].spilled_ptr,
+ state->stack[spi].spilled_ptr.parent, REG_LIVE_READ64);
+ if (ret)
+ return ret;
+ return mark_reg_read(env, &state->stack[spi - 1].spilled_ptr,
+ state->stack[spi - 1].spilled_ptr.parent, REG_LIVE_READ64);
+}
+
/* This function is supposed to be used by the following 32-bit optimization
* code only. It returns TRUE if the source or destination register operates
* on 64-bit, otherwise return FALSE.
@@ -5930,6 +5982,7 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno,
enum bpf_arg_type arg_type, struct bpf_call_arg_meta *meta)
{
struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno];
+ int err;
/* MEM_UNINIT and MEM_RDONLY are exclusive, when applied to an
* ARG_PTR_TO_DYNPTR (or ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_*):
@@ -6010,6 +6063,10 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno,
err_extra, regno);
return -EINVAL;
}
+
+ err = mark_dynptr_read(env, reg);
+ if (err)
+ return err;
}
return 0;
}
@@ -13174,6 +13231,7 @@ static bool regsafe(struct bpf_verifier_env *env, struct bpf_reg_state *rold,
static bool stacksafe(struct bpf_verifier_env *env, struct bpf_func_state *old,
struct bpf_func_state *cur, struct bpf_id_pair *idmap)
{
+ const struct bpf_reg_state *old_reg, *cur_reg;
int i, spi;
/* walk slots of the explored stack and ignore any additional
@@ -13215,10 +13273,9 @@ static bool stacksafe(struct bpf_verifier_env *env, struct bpf_func_state *old,
return false;
if (i % BPF_REG_SIZE != BPF_REG_SIZE - 1)
continue;
- if (!is_spilled_reg(&old->stack[spi]))
- continue;
- if (!regsafe(env, &old->stack[spi].spilled_ptr,
- &cur->stack[spi].spilled_ptr, idmap))
+ /* Both old and cur are having same slot_type */
+ switch (old->stack[spi].slot_type[BPF_REG_SIZE - 1]) {
+ case STACK_SPILL:
/* when explored and current stack slot are both storing
* spilled registers, check that stored pointers types
* are the same as well.
@@ -13229,7 +13286,25 @@ static bool stacksafe(struct bpf_verifier_env *env, struct bpf_func_state *old,
* such verifier states are not equivalent.
* return false to continue verification of this path
*/
+ if (!regsafe(env, &old->stack[spi].spilled_ptr,
+ &cur->stack[spi].spilled_ptr, idmap))
+ return false;
+ break;
+ case STACK_DYNPTR:
+ old_reg = &old->stack[spi].spilled_ptr;
+ cur_reg = &cur->stack[spi].spilled_ptr;
+ if (old_reg->dynptr.type != cur_reg->dynptr.type ||
+ old_reg->dynptr.first_slot != cur_reg->dynptr.first_slot ||
+ !check_ids(old_reg->ref_obj_id, cur_reg->ref_obj_id, idmap))
+ return false;
+ break;
+ case STACK_MISC:
+ case STACK_ZERO:
+ case STACK_INVALID:
+ continue;
+ default:
return false;
+ }
}
return true;
}
--
2.39.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH bpf-next v2 02/11] bpf: Fix missing var_off check for ARG_PTR_TO_DYNPTR
2023-01-19 2:14 [PATCH bpf-next v2 00/11] Dynptr fixes Kumar Kartikeya Dwivedi
2023-01-19 2:14 ` [PATCH bpf-next v2 01/11] bpf: Fix state pruning for STACK_DYNPTR stack slots Kumar Kartikeya Dwivedi
@ 2023-01-19 2:14 ` Kumar Kartikeya Dwivedi
2023-01-19 22:00 ` Joanne Koong
2023-01-19 2:14 ` [PATCH bpf-next v2 03/11] bpf: Fix partial dynptr stack slot reads/writes Kumar Kartikeya Dwivedi
` (8 subsequent siblings)
10 siblings, 1 reply; 22+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2023-01-19 2:14 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, Joanne Koong, David Vernet, Eduard Zingerman
Currently, the dynptr function is not checking the variable offset part
of PTR_TO_STACK that it needs to check. The fixed offset is considered
when computing the stack pointer index, but if the variable offset was
not a constant (such that it could not be accumulated in reg->off), we
will end up a discrepency where runtime pointer does not point to the
actual stack slot we mark as STACK_DYNPTR.
It is impossible to precisely track dynptr state when variable offset is
not constant, hence, just like bpf_timer, kptr, bpf_spin_lock, etc.
simply reject the case where reg->var_off is not constant. Then,
consider both reg->off and reg->var_off.value when computing the stack
pointer index.
A new helper dynptr_get_spi is introduced to hide over these details
since the dynptr needs to be located in multiple places outside the
process_dynptr_func checks, hence once we know it's a PTR_TO_STACK, we
need to enforce these checks in all places.
Note that it is disallowed for unprivileged users to have a non-constant
var_off, so this problem should only be possible to trigger from
programs having CAP_PERFMON. However, its effects can vary.
Without the fix, it is possible to replace the contents of the dynptr
arbitrarily by making verifier mark different stack slots than actual
location and then doing writes to the actual stack address of dynptr at
runtime.
Fixes: 97e03f521050 ("bpf: Add verifier support for dynptrs")
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
kernel/bpf/verifier.c | 83 +++++++++++++++----
.../bpf/prog_tests/kfunc_dynptr_param.c | 2 +-
.../testing/selftests/bpf/progs/dynptr_fail.c | 4 +-
3 files changed, 68 insertions(+), 21 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 89de5bc46f27..eeb6f1b2bd60 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -638,11 +638,34 @@ static void print_liveness(struct bpf_verifier_env *env,
verbose(env, "D");
}
-static int get_spi(s32 off)
+static int __get_spi(s32 off)
{
return (-off - 1) / BPF_REG_SIZE;
}
+static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
+{
+ int off, spi;
+
+ if (!tnum_is_const(reg->var_off)) {
+ verbose(env, "dynptr has to be at the constant offset\n");
+ return -EINVAL;
+ }
+
+ off = reg->off + reg->var_off.value;
+ if (off % BPF_REG_SIZE) {
+ verbose(env, "cannot pass in dynptr at an offset=%d\n", off);
+ return -EINVAL;
+ }
+
+ spi = __get_spi(off);
+ if (spi < 1) {
+ verbose(env, "cannot pass in dynptr at an offset=%d\n", off);
+ return -EINVAL;
+ }
+ return spi;
+}
+
static bool is_spi_bounds_valid(struct bpf_func_state *state, int spi, int nr_slots)
{
int allocated_slots = state->allocated_stack / BPF_REG_SIZE;
@@ -754,7 +777,9 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
enum bpf_dynptr_type type;
int spi, i, id;
- spi = get_spi(reg->off);
+ spi = dynptr_get_spi(env, reg);
+ if (spi < 0)
+ return spi;
if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS))
return -EINVAL;
@@ -792,7 +817,9 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re
struct bpf_func_state *state = func(env, reg);
int spi, i;
- spi = get_spi(reg->off);
+ spi = dynptr_get_spi(env, reg);
+ if (spi < 0)
+ return spi;
if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS))
return -EINVAL;
@@ -844,7 +871,11 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_
if (reg->type == CONST_PTR_TO_DYNPTR)
return false;
- spi = get_spi(reg->off);
+ spi = dynptr_get_spi(env, reg);
+ if (spi < 0)
+ return false;
+
+ /* We will do check_mem_access to check and update stack bounds later */
if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS))
return true;
@@ -860,14 +891,15 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_
static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
{
struct bpf_func_state *state = func(env, reg);
- int spi;
- int i;
+ int spi, i;
/* This already represents first slot of initialized bpf_dynptr */
if (reg->type == CONST_PTR_TO_DYNPTR)
return true;
- spi = get_spi(reg->off);
+ spi = dynptr_get_spi(env, reg);
+ if (spi < 0)
+ return false;
if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS) ||
!state->stack[spi].spilled_ptr.dynptr.first_slot)
return false;
@@ -896,7 +928,9 @@ static bool is_dynptr_type_expected(struct bpf_verifier_env *env, struct bpf_reg
if (reg->type == CONST_PTR_TO_DYNPTR) {
return reg->dynptr.type == dynptr_type;
} else {
- spi = get_spi(reg->off);
+ spi = dynptr_get_spi(env, reg);
+ if (spi < 0)
+ return false;
return state->stack[spi].spilled_ptr.dynptr.type == dynptr_type;
}
}
@@ -2429,7 +2463,9 @@ static int mark_dynptr_read(struct bpf_verifier_env *env, struct bpf_reg_state *
*/
if (reg->type == CONST_PTR_TO_DYNPTR)
return 0;
- spi = get_spi(reg->off);
+ spi = dynptr_get_spi(env, reg);
+ if (spi < 0)
+ return spi;
/* Caller ensures dynptr is valid and initialized, which means spi is in
* bounds and spi is the first dynptr slot. Simply mark stack slot as
* read.
@@ -5993,12 +6029,14 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno,
}
/* CONST_PTR_TO_DYNPTR already has fixed and var_off as 0 due to
* check_func_arg_reg_off's logic. We only need to check offset
- * alignment for PTR_TO_STACK.
+ * and its alignment for PTR_TO_STACK.
*/
- if (reg->type == PTR_TO_STACK && (reg->off % BPF_REG_SIZE)) {
- verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off);
- return -EINVAL;
+ if (reg->type == PTR_TO_STACK) {
+ err = dynptr_get_spi(env, reg);
+ if (err < 0)
+ return err;
}
+
/* MEM_UNINIT - Points to memory that is an appropriate candidate for
* constructing a mutable bpf_dynptr object.
*
@@ -6404,15 +6442,16 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env,
}
}
-static u32 dynptr_ref_obj_id(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
+static int dynptr_ref_obj_id(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
{
struct bpf_func_state *state = func(env, reg);
int spi;
if (reg->type == CONST_PTR_TO_DYNPTR)
return reg->ref_obj_id;
-
- spi = get_spi(reg->off);
+ spi = dynptr_get_spi(env, reg);
+ if (spi < 0)
+ return spi;
return state->stack[spi].spilled_ptr.ref_obj_id;
}
@@ -6486,7 +6525,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
* PTR_TO_STACK.
*/
if (reg->type == PTR_TO_STACK) {
- spi = get_spi(reg->off);
+ spi = dynptr_get_spi(env, reg);
+ if (spi < 0)
+ return spi;
if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS) ||
!state->stack[spi].spilled_ptr.ref_obj_id) {
verbose(env, "arg %d is an unacquired reference\n", regno);
@@ -7976,13 +8017,19 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) {
if (arg_type_is_dynptr(fn->arg_type[i])) {
struct bpf_reg_state *reg = ®s[BPF_REG_1 + i];
+ int ref_obj_id;
if (meta.ref_obj_id) {
verbose(env, "verifier internal error: meta.ref_obj_id already set\n");
return -EFAULT;
}
- meta.ref_obj_id = dynptr_ref_obj_id(env, reg);
+ ref_obj_id = dynptr_ref_obj_id(env, reg);
+ if (err < 0) {
+ verbose(env, "verifier internal error: failed to obtain dynptr ref_obj_id\n");
+ return err;
+ }
+ meta.ref_obj_id = ref_obj_id;
break;
}
}
diff --git a/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c b/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c
index a9229260a6ce..72800b1e8395 100644
--- a/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c
+++ b/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c
@@ -18,7 +18,7 @@ static struct {
const char *expected_verifier_err_msg;
int expected_runtime_err;
} kfunc_dynptr_tests[] = {
- {"not_valid_dynptr", "Expected an initialized dynptr as arg #1", 0},
+ {"not_valid_dynptr", "cannot pass in dynptr at an offset=-8", 0},
{"not_ptr_to_stack", "arg#0 expected pointer to stack or dynptr_ptr", 0},
{"dynptr_data_null", NULL, -EBADMSG},
};
diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
index 78debc1b3820..02d57b95cf6e 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
@@ -382,7 +382,7 @@ int invalid_helper1(void *ctx)
/* A dynptr can't be passed into a helper function at a non-zero offset */
SEC("?raw_tp")
-__failure __msg("Expected an initialized dynptr as arg #3")
+__failure __msg("cannot pass in dynptr at an offset=-8")
int invalid_helper2(void *ctx)
{
struct bpf_dynptr ptr;
@@ -584,7 +584,7 @@ int invalid_read4(void *ctx)
/* Initializing a dynptr on an offset should fail */
SEC("?raw_tp")
-__failure __msg("invalid write to stack")
+__failure __msg("cannot pass in dynptr at an offset=0")
int invalid_offset(void *ctx)
{
struct bpf_dynptr ptr;
--
2.39.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH bpf-next v2 03/11] bpf: Fix partial dynptr stack slot reads/writes
2023-01-19 2:14 [PATCH bpf-next v2 00/11] Dynptr fixes Kumar Kartikeya Dwivedi
2023-01-19 2:14 ` [PATCH bpf-next v2 01/11] bpf: Fix state pruning for STACK_DYNPTR stack slots Kumar Kartikeya Dwivedi
2023-01-19 2:14 ` [PATCH bpf-next v2 02/11] bpf: Fix missing var_off check for ARG_PTR_TO_DYNPTR Kumar Kartikeya Dwivedi
@ 2023-01-19 2:14 ` Kumar Kartikeya Dwivedi
2023-01-19 22:06 ` Joanne Koong
2023-01-19 2:14 ` [PATCH bpf-next v2 04/11] bpf: Allow reinitializing unreferenced dynptr stack slots Kumar Kartikeya Dwivedi
` (7 subsequent siblings)
10 siblings, 1 reply; 22+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2023-01-19 2:14 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, Joanne Koong, David Vernet, Eduard Zingerman
Currently, while reads are disallowed for dynptr stack slots, writes are
not. Reads don't work from both direct access and helpers, while writes
do work in both cases, but have the effect of overwriting the slot_type.
While this is fine, handling for a few edge cases is missing. Firstly,
a user can overwrite the stack slots of dynptr partially.
Consider the following layout:
spi: [d][d][?]
2 1 0
First slot is at spi 2, second at spi 1.
Now, do a write of 1 to 8 bytes for spi 1.
This will essentially either write STACK_MISC for all slot_types or
STACK_MISC and STACK_ZERO (in case of size < BPF_REG_SIZE partial write
of zeroes). The end result is that slot is scrubbed.
Now, the layout is:
spi: [d][m][?]
2 1 0
Suppose if user initializes spi = 1 as dynptr.
We get:
spi: [d][d][d]
2 1 0
But this time, both spi 2 and spi 1 have first_slot = true.
Now, when passing spi 2 to dynptr helper, it will consider it as
initialized as it does not check whether second slot has first_slot ==
false. And spi 1 should already work as normal.
This effectively replaced size + offset of first dynptr, hence allowing
invalid OOB reads and writes.
Make a few changes to protect against this:
When writing to PTR_TO_STACK using BPF insns, when we touch spi of a
STACK_DYNPTR type, mark both first and second slot (regardless of which
slot we touch) as STACK_INVALID. Reads are already prevented.
Second, prevent writing to stack memory from helpers if the range may
contain any STACK_DYNPTR slots. Reads are already prevented.
For helpers, we cannot allow it to destroy dynptrs from the writes as
depending on arguments, helper may take uninit_mem and dynptr both at
the same time. This would mean that helper may write to uninit_mem
before it reads the dynptr, which would be bad.
PTR_TO_MEM: [?????dd]
Depending on the code inside the helper, it may end up overwriting the
dynptr contents first and then read those as the dynptr argument.
Verifier would only simulate destruction when it does byte by byte
access simulation in check_helper_call for meta.access_size, and
fail to catch this case, as it happens after argument checks.
The same would need to be done for any other non-trivial objects created
on the stack in the future, such as bpf_list_head on stack, or
bpf_rb_root on stack.
A common misunderstanding in the current code is that MEM_UNINIT means
writes, but note that writes may also be performed even without
MEM_UNINIT in case of helpers, in that case the code after handling meta
&& meta->raw_mode will complain when it sees STACK_DYNPTR. So that
invalid read case also covers writes to potential STACK_DYNPTR slots.
The only loophole was in case of meta->raw_mode which simulated writes
through instructions which could overwrite them.
A future series sequenced after this will focus on the clean up of
helper access checks and bugs around that.
Fixes: 97e03f521050 ("bpf: Add verifier support for dynptrs")
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
kernel/bpf/verifier.c | 102 ++++++++++++++++++
.../testing/selftests/bpf/progs/dynptr_fail.c | 6 +-
2 files changed, 105 insertions(+), 3 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index eeb6f1b2bd60..09c09d9bfd89 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -769,6 +769,8 @@ static void mark_dynptr_cb_reg(struct bpf_reg_state *reg,
__mark_dynptr_reg(reg, type, true);
}
+static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env,
+ struct bpf_func_state *state, int spi);
static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
enum bpf_arg_type arg_type, int insn_idx)
@@ -863,6 +865,69 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re
return 0;
}
+static void __mark_reg_unknown(const struct bpf_verifier_env *env,
+ struct bpf_reg_state *reg);
+
+static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env,
+ struct bpf_func_state *state, int spi)
+{
+ int i;
+
+ /* We always ensure that STACK_DYNPTR is never set partially,
+ * hence just checking for slot_type[0] is enough. This is
+ * different for STACK_SPILL, where it may be only set for
+ * 1 byte, so code has to use is_spilled_reg.
+ */
+ if (state->stack[spi].slot_type[0] != STACK_DYNPTR)
+ return 0;
+
+ /* Reposition spi to first slot */
+ if (!state->stack[spi].spilled_ptr.dynptr.first_slot)
+ spi = spi + 1;
+
+ if (dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type)) {
+ verbose(env, "cannot overwrite referenced dynptr\n");
+ return -EINVAL;
+ }
+
+ mark_stack_slot_scratched(env, spi);
+ mark_stack_slot_scratched(env, spi - 1);
+
+ /* Writing partially to one dynptr stack slot destroys both. */
+ for (i = 0; i < BPF_REG_SIZE; i++) {
+ state->stack[spi].slot_type[i] = STACK_INVALID;
+ state->stack[spi - 1].slot_type[i] = STACK_INVALID;
+ }
+
+ /* Invalidate any slices associated with this dynptr */
+ if (dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type)) {
+ int ref_obj_id = state->stack[spi].spilled_ptr.ref_obj_id;
+ struct bpf_func_state *fstate;
+ struct bpf_reg_state *reg;
+
+ bpf_for_each_reg_in_vstate(env->cur_state, fstate, reg, ({
+ if (reg->ref_obj_id == ref_obj_id) {
+ if (!env->allow_ptr_leaks)
+ __mark_reg_not_init(env, reg);
+ else
+ __mark_reg_unknown(env, reg);
+ }
+ }));
+ }
+
+ /* Do not release reference state, we are destroying dynptr on stack,
+ * not using some helper to release it. Just reset register.
+ */
+ __mark_reg_not_init(env, &state->stack[spi].spilled_ptr);
+ __mark_reg_not_init(env, &state->stack[spi - 1].spilled_ptr);
+
+ /* Same reason as unmark_stack_slots_dynptr above */
+ state->stack[spi].spilled_ptr.live |= REG_LIVE_WRITTEN;
+ state->stack[spi - 1].spilled_ptr.live |= REG_LIVE_WRITTEN;
+
+ return 0;
+}
+
static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
{
struct bpf_func_state *state = func(env, reg);
@@ -3391,6 +3456,10 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
env->insn_aux_data[insn_idx].sanitize_stack_spill = true;
}
+ err = destroy_if_dynptr_stack_slot(env, state, spi);
+ if (err)
+ return err;
+
mark_stack_slot_scratched(env, spi);
if (reg && !(off % BPF_REG_SIZE) && register_is_bounded(reg) &&
!register_is_null(reg) && env->bpf_capable) {
@@ -3504,6 +3573,14 @@ static int check_stack_write_var_off(struct bpf_verifier_env *env,
if (err)
return err;
+ for (i = min_off; i < max_off; i++) {
+ int spi;
+
+ spi = __get_spi(i);
+ err = destroy_if_dynptr_stack_slot(env, state, spi);
+ if (err)
+ return err;
+ }
/* Variable offset writes destroy any spilled pointers in range. */
for (i = min_off; i < max_off; i++) {
@@ -5531,6 +5608,31 @@ static int check_stack_range_initialized(
}
if (meta && meta->raw_mode) {
+ /* Ensure we won't be overwriting dynptrs when simulating byte
+ * by byte access in check_helper_call using meta.access_size.
+ * This would be a problem if we have a helper in the future
+ * which takes:
+ *
+ * helper(uninit_mem, len, dynptr)
+ *
+ * Now, uninint_mem may overlap with dynptr pointer. Hence, it
+ * may end up writing to dynptr itself when touching memory from
+ * arg 1. This can be relaxed on a case by case basis for known
+ * safe cases, but reject due to the possibilitiy of aliasing by
+ * default.
+ */
+ for (i = min_off; i < max_off + access_size; i++) {
+ int stack_off = -i - 1;
+
+ spi = __get_spi(i);
+ /* raw_mode may write past allocated_stack */
+ if (state->allocated_stack <= stack_off)
+ continue;
+ if (state->stack[spi].slot_type[stack_off % BPF_REG_SIZE] == STACK_DYNPTR) {
+ verbose(env, "potential write to dynptr at off=%d disallowed\n", i);
+ return -EACCES;
+ }
+ }
meta->access_size = access_size;
meta->regno = regno;
return 0;
diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
index 02d57b95cf6e..9dc3f23a8270 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
@@ -420,7 +420,7 @@ int invalid_write1(void *ctx)
* offset
*/
SEC("?raw_tp")
-__failure __msg("Expected an initialized dynptr as arg #3")
+__failure __msg("cannot overwrite referenced dynptr")
int invalid_write2(void *ctx)
{
struct bpf_dynptr ptr;
@@ -444,7 +444,7 @@ int invalid_write2(void *ctx)
* non-const offset
*/
SEC("?raw_tp")
-__failure __msg("Expected an initialized dynptr as arg #1")
+__failure __msg("cannot overwrite referenced dynptr")
int invalid_write3(void *ctx)
{
struct bpf_dynptr ptr;
@@ -476,7 +476,7 @@ static int invalid_write4_callback(__u32 index, void *data)
* be invalidated as a dynptr
*/
SEC("?raw_tp")
-__failure __msg("arg 1 is an unacquired reference")
+__failure __msg("cannot overwrite referenced dynptr")
int invalid_write4(void *ctx)
{
struct bpf_dynptr ptr;
--
2.39.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH bpf-next v2 04/11] bpf: Allow reinitializing unreferenced dynptr stack slots
2023-01-19 2:14 [PATCH bpf-next v2 00/11] Dynptr fixes Kumar Kartikeya Dwivedi
` (2 preceding siblings ...)
2023-01-19 2:14 ` [PATCH bpf-next v2 03/11] bpf: Fix partial dynptr stack slot reads/writes Kumar Kartikeya Dwivedi
@ 2023-01-19 2:14 ` Kumar Kartikeya Dwivedi
2023-01-19 22:13 ` Joanne Koong
2023-01-19 2:14 ` [PATCH bpf-next v2 05/11] bpf: Combine dynptr_get_spi and is_spi_bounds_valid Kumar Kartikeya Dwivedi
` (6 subsequent siblings)
10 siblings, 1 reply; 22+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2023-01-19 2:14 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, Joanne Koong, David Vernet, Eduard Zingerman
Consider a program like below:
void prog(void)
{
{
struct bpf_dynptr ptr;
bpf_dynptr_from_mem(...);
}
...
{
struct bpf_dynptr ptr;
bpf_dynptr_from_mem(...);
}
}
Here, the C compiler based on lifetime rules in the C standard would be
well within in its rights to share stack storage for dynptr 'ptr' as
their lifetimes do not overlap in the two distinct scopes. Currently,
such an example would be rejected by the verifier, but this is too
strict. Instead, we should allow reinitializing over dynptr stack slots
and forget information about the old dynptr object.
The destroy_if_dynptr_stack_slot function already makes necessary checks
to avoid overwriting referenced dynptr slots. This is done to present a
better error message instead of forgetting dynptr information on stack
and preserving reference state, leading to an inevitable but
undecipherable error at the end about an unreleased reference which has
to be associated back to its allocating call instruction to make any
sense to the user.
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
kernel/bpf/verifier.c | 34 ++++++++++++++++++++++++++--------
1 file changed, 26 insertions(+), 8 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 09c09d9bfd89..4feaddd5d6dc 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -777,7 +777,7 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
{
struct bpf_func_state *state = func(env, reg);
enum bpf_dynptr_type type;
- int spi, i, id;
+ int spi, i, id, err;
spi = dynptr_get_spi(env, reg);
if (spi < 0)
@@ -786,6 +786,22 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS))
return -EINVAL;
+ /* We cannot assume both spi and spi - 1 belong to the same dynptr,
+ * hence we need to call destroy_if_dynptr_stack_slot twice for both,
+ * to ensure that for the following example:
+ * [d1][d1][d2][d2]
+ * spi 3 2 1 0
+ * So marking spi = 2 should lead to destruction of both d1 and d2. In
+ * case they do belong to same dynptr, second call won't see slot_type
+ * as STACK_DYNPTR and will simply skip destruction.
+ */
+ err = destroy_if_dynptr_stack_slot(env, state, spi);
+ if (err)
+ return err;
+ err = destroy_if_dynptr_stack_slot(env, state, spi - 1);
+ if (err)
+ return err;
+
for (i = 0; i < BPF_REG_SIZE; i++) {
state->stack[spi].slot_type[i] = STACK_DYNPTR;
state->stack[spi - 1].slot_type[i] = STACK_DYNPTR;
@@ -931,7 +947,7 @@ static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env,
static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
{
struct bpf_func_state *state = func(env, reg);
- int spi, i;
+ int spi;
if (reg->type == CONST_PTR_TO_DYNPTR)
return false;
@@ -944,12 +960,14 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_
if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS))
return true;
- for (i = 0; i < BPF_REG_SIZE; i++) {
- if (state->stack[spi].slot_type[i] == STACK_DYNPTR ||
- state->stack[spi - 1].slot_type[i] == STACK_DYNPTR)
- return false;
- }
-
+ /* We allow overwriting existing unreferenced STACK_DYNPTR slots, see
+ * mark_stack_slots_dynptr which calls destroy_if_dynptr_stack_slot to
+ * ensure dynptr objects at the slots we are touching are completely
+ * destructed before we reinitialize them for a new one. For referenced
+ * ones, destroy_if_dynptr_stack_slot returns an error early instead of
+ * delaying it until the end where the user will get "Unreleased
+ * reference" error.
+ */
return true;
}
--
2.39.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH bpf-next v2 05/11] bpf: Combine dynptr_get_spi and is_spi_bounds_valid
2023-01-19 2:14 [PATCH bpf-next v2 00/11] Dynptr fixes Kumar Kartikeya Dwivedi
` (3 preceding siblings ...)
2023-01-19 2:14 ` [PATCH bpf-next v2 04/11] bpf: Allow reinitializing unreferenced dynptr stack slots Kumar Kartikeya Dwivedi
@ 2023-01-19 2:14 ` Kumar Kartikeya Dwivedi
2023-01-19 22:30 ` Joanne Koong
2023-01-19 2:14 ` [PATCH bpf-next v2 06/11] bpf: Avoid recomputing spi in process_dynptr_func Kumar Kartikeya Dwivedi
` (5 subsequent siblings)
10 siblings, 1 reply; 22+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2023-01-19 2:14 UTC (permalink / raw)
To: bpf
Cc: Joanne Koong, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Martin KaFai Lau, David Vernet,
Eduard Zingerman
Currently, a check on spi resides in dynptr_get_spi, while others
checking its validity for being within the allocated stack slots happens
in is_spi_bounds_valid. Almost always barring a couple of cases (where
being beyond allocated stack slots is not an error as stack slots need
to be populated), both are used together to make checks. Hence, subsume
the is_spi_bounds_valid check in dynptr_get_spi, and return -ERANGE to
specially distinguish the case where spi is valid but not within
allocated slots in the stack state.
The is_spi_bounds_valid function is still kept around as it is a generic
helper that will be useful for other objects on stack similar to dynptr
in the future.
Suggested-by: Joanne Koong <joannelkoong@gmail.com>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
kernel/bpf/verifier.c | 75 +++++++++++++++++++------------------------
1 file changed, 33 insertions(+), 42 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 4feaddd5d6dc..18b54b219fac 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -643,6 +643,28 @@ static int __get_spi(s32 off)
return (-off - 1) / BPF_REG_SIZE;
}
+static struct bpf_func_state *func(struct bpf_verifier_env *env,
+ const struct bpf_reg_state *reg)
+{
+ struct bpf_verifier_state *cur = env->cur_state;
+
+ return cur->frame[reg->frameno];
+}
+
+static bool is_spi_bounds_valid(struct bpf_func_state *state, int spi, int nr_slots)
+{
+ int allocated_slots = state->allocated_stack / BPF_REG_SIZE;
+
+ /* We need to check that slots between [spi - nr_slots + 1, spi] are
+ * within [0, allocated_stack).
+ *
+ * Please note that the spi grows downwards. For example, a dynptr
+ * takes the size of two stack slots; the first slot will be at
+ * spi and the second slot will be at spi - 1.
+ */
+ return spi - nr_slots + 1 >= 0 && spi < allocated_slots;
+}
+
static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
{
int off, spi;
@@ -663,29 +685,10 @@ static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *re
verbose(env, "cannot pass in dynptr at an offset=%d\n", off);
return -EINVAL;
}
- return spi;
-}
-
-static bool is_spi_bounds_valid(struct bpf_func_state *state, int spi, int nr_slots)
-{
- int allocated_slots = state->allocated_stack / BPF_REG_SIZE;
- /* We need to check that slots between [spi - nr_slots + 1, spi] are
- * within [0, allocated_stack).
- *
- * Please note that the spi grows downwards. For example, a dynptr
- * takes the size of two stack slots; the first slot will be at
- * spi and the second slot will be at spi - 1.
- */
- return spi - nr_slots + 1 >= 0 && spi < allocated_slots;
-}
-
-static struct bpf_func_state *func(struct bpf_verifier_env *env,
- const struct bpf_reg_state *reg)
-{
- struct bpf_verifier_state *cur = env->cur_state;
-
- return cur->frame[reg->frameno];
+ if (!is_spi_bounds_valid(func(env, reg), spi, BPF_DYNPTR_NR_SLOTS))
+ return -ERANGE;
+ return spi;
}
static const char *kernel_type_name(const struct btf* btf, u32 id)
@@ -783,9 +786,6 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
if (spi < 0)
return spi;
- if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS))
- return -EINVAL;
-
/* We cannot assume both spi and spi - 1 belong to the same dynptr,
* hence we need to call destroy_if_dynptr_stack_slot twice for both,
* to ensure that for the following example:
@@ -839,9 +839,6 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re
if (spi < 0)
return spi;
- if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS))
- return -EINVAL;
-
for (i = 0; i < BPF_REG_SIZE; i++) {
state->stack[spi].slot_type[i] = STACK_INVALID;
state->stack[spi - 1].slot_type[i] = STACK_INVALID;
@@ -946,20 +943,18 @@ static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env,
static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
{
- struct bpf_func_state *state = func(env, reg);
int spi;
if (reg->type == CONST_PTR_TO_DYNPTR)
return false;
spi = dynptr_get_spi(env, reg);
+ /* For -ERANGE (i.e. spi not falling into allocated stack slots), we
+ * will do check_mem_access to check and update stack bounds later, so
+ * return true for that case.
+ */
if (spi < 0)
- return false;
-
- /* We will do check_mem_access to check and update stack bounds later */
- if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS))
- return true;
-
+ return spi == -ERANGE;
/* We allow overwriting existing unreferenced STACK_DYNPTR slots, see
* mark_stack_slots_dynptr which calls destroy_if_dynptr_stack_slot to
* ensure dynptr objects at the slots we are touching are completely
@@ -983,8 +978,7 @@ static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_re
spi = dynptr_get_spi(env, reg);
if (spi < 0)
return false;
- if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS) ||
- !state->stack[spi].spilled_ptr.dynptr.first_slot)
+ if (!state->stack[spi].spilled_ptr.dynptr.first_slot)
return false;
for (i = 0; i < BPF_REG_SIZE; i++) {
@@ -6153,7 +6147,7 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno,
*/
if (reg->type == PTR_TO_STACK) {
err = dynptr_get_spi(env, reg);
- if (err < 0)
+ if (err < 0 && err != -ERANGE)
return err;
}
@@ -6646,10 +6640,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
*/
if (reg->type == PTR_TO_STACK) {
spi = dynptr_get_spi(env, reg);
- if (spi < 0)
- return spi;
- if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS) ||
- !state->stack[spi].spilled_ptr.ref_obj_id) {
+ if (spi < 0 || !state->stack[spi].spilled_ptr.ref_obj_id) {
verbose(env, "arg %d is an unacquired reference\n", regno);
return -EINVAL;
}
--
2.39.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH bpf-next v2 06/11] bpf: Avoid recomputing spi in process_dynptr_func
2023-01-19 2:14 [PATCH bpf-next v2 00/11] Dynptr fixes Kumar Kartikeya Dwivedi
` (4 preceding siblings ...)
2023-01-19 2:14 ` [PATCH bpf-next v2 05/11] bpf: Combine dynptr_get_spi and is_spi_bounds_valid Kumar Kartikeya Dwivedi
@ 2023-01-19 2:14 ` Kumar Kartikeya Dwivedi
2023-01-19 22:35 ` Joanne Koong
2023-01-19 2:14 ` [PATCH bpf-next v2 07/11] selftests/bpf: convenience macro for use with 'asm volatile' blocks Kumar Kartikeya Dwivedi
` (4 subsequent siblings)
10 siblings, 1 reply; 22+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2023-01-19 2:14 UTC (permalink / raw)
To: bpf
Cc: Joanne Koong, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Martin KaFai Lau, David Vernet,
Eduard Zingerman
Currently, process_dynptr_func first calls dynptr_get_spi and then
is_dynptr_reg_valid_init and is_dynptr_reg_valid_uninit have to call it
again to obtain the spi value. Instead of doing this twice, reuse the
already obtained value (which is by default 0, and is only set for
PTR_TO_STACK, and only used in that case in aforementioned functions).
The input value for these two functions will either be -ERANGE or >= 1,
and can either be permitted or rejected based on the respective check.
Suggested-by: Joanne Koong <joannelkoong@gmail.com>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
kernel/bpf/verifier.c | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 18b54b219fac..7b8de84568a3 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -941,14 +941,12 @@ static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env,
return 0;
}
-static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
+static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
+ int spi)
{
- int spi;
-
if (reg->type == CONST_PTR_TO_DYNPTR)
return false;
- spi = dynptr_get_spi(env, reg);
/* For -ERANGE (i.e. spi not falling into allocated stack slots), we
* will do check_mem_access to check and update stack bounds later, so
* return true for that case.
@@ -966,16 +964,16 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_
return true;
}
-static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
+static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
+ int spi)
{
struct bpf_func_state *state = func(env, reg);
- int spi, i;
+ int i;
/* This already represents first slot of initialized bpf_dynptr */
if (reg->type == CONST_PTR_TO_DYNPTR)
return true;
- spi = dynptr_get_spi(env, reg);
if (spi < 0)
return false;
if (!state->stack[spi].spilled_ptr.dynptr.first_slot)
@@ -6132,7 +6130,7 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno,
enum bpf_arg_type arg_type, struct bpf_call_arg_meta *meta)
{
struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno];
- int err;
+ int err, spi = 0;
/* MEM_UNINIT and MEM_RDONLY are exclusive, when applied to an
* ARG_PTR_TO_DYNPTR (or ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_*):
@@ -6146,9 +6144,9 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno,
* and its alignment for PTR_TO_STACK.
*/
if (reg->type == PTR_TO_STACK) {
- err = dynptr_get_spi(env, reg);
- if (err < 0 && err != -ERANGE)
- return err;
+ spi = dynptr_get_spi(env, reg);
+ if (spi < 0 && spi != -ERANGE)
+ return spi;
}
/* MEM_UNINIT - Points to memory that is an appropriate candidate for
@@ -6167,7 +6165,7 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno,
* to.
*/
if (arg_type & MEM_UNINIT) {
- if (!is_dynptr_reg_valid_uninit(env, reg)) {
+ if (!is_dynptr_reg_valid_uninit(env, reg, spi)) {
verbose(env, "Dynptr has to be an uninitialized dynptr\n");
return -EINVAL;
}
@@ -6188,7 +6186,7 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno,
return -EINVAL;
}
- if (!is_dynptr_reg_valid_init(env, reg)) {
+ if (!is_dynptr_reg_valid_init(env, reg, spi)) {
verbose(env,
"Expected an initialized dynptr as arg #%d\n",
regno);
--
2.39.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH bpf-next v2 07/11] selftests/bpf: convenience macro for use with 'asm volatile' blocks
2023-01-19 2:14 [PATCH bpf-next v2 00/11] Dynptr fixes Kumar Kartikeya Dwivedi
` (5 preceding siblings ...)
2023-01-19 2:14 ` [PATCH bpf-next v2 06/11] bpf: Avoid recomputing spi in process_dynptr_func Kumar Kartikeya Dwivedi
@ 2023-01-19 2:14 ` Kumar Kartikeya Dwivedi
2023-01-19 2:14 ` [PATCH bpf-next v2 08/11] selftests/bpf: Add dynptr pruning tests Kumar Kartikeya Dwivedi
` (3 subsequent siblings)
10 siblings, 0 replies; 22+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2023-01-19 2:14 UTC (permalink / raw)
To: bpf
Cc: Andrii Nakryiko, Yonghong Song, Eduard Zingerman,
Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau,
Joanne Koong, David Vernet
From: Eduard Zingerman <eddyz87@gmail.com>
A set of macros useful for writing naked BPF functions using inline
assembly. E.g. as follows:
struct map_struct {
...
} map SEC(".maps");
SEC(...)
__naked int foo_test(void)
{
asm volatile(
"r0 = 0;"
"*(u64*)(r10 - 8) = r0;"
"r1 = %[map] ll;"
"r2 = r10;"
"r2 += -8;"
"call %[bpf_map_lookup_elem];"
"r0 = 0;"
"exit;"
:
: __imm(bpf_map_lookup_elem),
__imm_addr(map)
: __clobber_all);
}
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Acked-by: Yonghong Song <yhs@fb.com>
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
[ Kartikeya: Add acks, include __clobber_common from Andrii ]
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
tools/testing/selftests/bpf/progs/bpf_misc.h | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/tools/testing/selftests/bpf/progs/bpf_misc.h b/tools/testing/selftests/bpf/progs/bpf_misc.h
index 4a01ea9113bf..2d7b89b447b2 100644
--- a/tools/testing/selftests/bpf/progs/bpf_misc.h
+++ b/tools/testing/selftests/bpf/progs/bpf_misc.h
@@ -7,6 +7,13 @@
#define __success __attribute__((btf_decl_tag("comment:test_expect_success")))
#define __log_level(lvl) __attribute__((btf_decl_tag("comment:test_log_level="#lvl)))
+/* Convenience macro for use with 'asm volatile' blocks */
+#define __naked __attribute__((naked))
+#define __clobber_all "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r7", "r8", "r9", "memory"
+#define __clobber_common "r0", "r1", "r2", "r3", "r4", "r5", "memory"
+#define __imm(name) [name]"i"(name)
+#define __imm_addr(name) [name]"i"(&name)
+
#if defined(__TARGET_ARCH_x86)
#define SYSCALL_WRAPPER 1
#define SYS_PREFIX "__x64_"
--
2.39.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH bpf-next v2 08/11] selftests/bpf: Add dynptr pruning tests
2023-01-19 2:14 [PATCH bpf-next v2 00/11] Dynptr fixes Kumar Kartikeya Dwivedi
` (6 preceding siblings ...)
2023-01-19 2:14 ` [PATCH bpf-next v2 07/11] selftests/bpf: convenience macro for use with 'asm volatile' blocks Kumar Kartikeya Dwivedi
@ 2023-01-19 2:14 ` Kumar Kartikeya Dwivedi
2023-01-19 2:14 ` [PATCH bpf-next v2 09/11] selftests/bpf: Add dynptr var_off tests Kumar Kartikeya Dwivedi
` (2 subsequent siblings)
10 siblings, 0 replies; 22+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2023-01-19 2:14 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, Joanne Koong, David Vernet, Eduard Zingerman
Add verifier tests that verify the new pruning behavior for STACK_DYNPTR
slots, and ensure that state equivalence takes into account changes to
the old and current verifier state correctly. Also ensure that the
stacksafe changes are actually enabling pruning in case states are
equivalent from pruning PoV.
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
.../testing/selftests/bpf/progs/dynptr_fail.c | 141 ++++++++++++++++++
1 file changed, 141 insertions(+)
diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
index 9dc3f23a8270..023b3c36bc78 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
@@ -35,6 +35,13 @@ struct {
__type(value, __u32);
} array_map3 SEC(".maps");
+struct {
+ __uint(type, BPF_MAP_TYPE_ARRAY);
+ __uint(max_entries, 1);
+ __type(key, __u32);
+ __type(value, __u64);
+} array_map4 SEC(".maps");
+
struct sample {
int pid;
long value;
@@ -653,3 +660,137 @@ int dynptr_from_mem_invalid_api(void *ctx)
return 0;
}
+
+SEC("?tc")
+__failure __msg("cannot overwrite referenced dynptr") __log_level(2)
+int dynptr_pruning_overwrite(struct __sk_buff *ctx)
+{
+ asm volatile (
+ "r9 = 0xeB9F;"
+ "r6 = %[ringbuf] ll;"
+ "r1 = r6;"
+ "r2 = 8;"
+ "r3 = 0;"
+ "r4 = r10;"
+ "r4 += -16;"
+ "call %[bpf_ringbuf_reserve_dynptr];"
+ "if r0 == 0 goto pjmp1;"
+ "goto pjmp2;"
+ "pjmp1:"
+ "*(u64 *)(r10 - 16) = r9;"
+ "pjmp2:"
+ "r1 = r10;"
+ "r1 += -16;"
+ "r2 = 0;"
+ "call %[bpf_ringbuf_discard_dynptr];"
+ :
+ : __imm(bpf_ringbuf_reserve_dynptr),
+ __imm(bpf_ringbuf_discard_dynptr),
+ __imm_addr(ringbuf)
+ : __clobber_all
+ );
+ return 0;
+}
+
+SEC("?tc")
+__success __msg("12: safe") __log_level(2)
+int dynptr_pruning_stacksafe(struct __sk_buff *ctx)
+{
+ asm volatile (
+ "r9 = 0xeB9F;"
+ "r6 = %[ringbuf] ll;"
+ "r1 = r6;"
+ "r2 = 8;"
+ "r3 = 0;"
+ "r4 = r10;"
+ "r4 += -16;"
+ "call %[bpf_ringbuf_reserve_dynptr];"
+ "if r0 == 0 goto stjmp1;"
+ "goto stjmp2;"
+ "stjmp1:"
+ "r9 = r9;"
+ "stjmp2:"
+ "r1 = r10;"
+ "r1 += -16;"
+ "r2 = 0;"
+ "call %[bpf_ringbuf_discard_dynptr];"
+ :
+ : __imm(bpf_ringbuf_reserve_dynptr),
+ __imm(bpf_ringbuf_discard_dynptr),
+ __imm_addr(ringbuf)
+ : __clobber_all
+ );
+ return 0;
+}
+
+SEC("?tc")
+__failure __msg("cannot overwrite referenced dynptr") __log_level(2)
+int dynptr_pruning_type_confusion(struct __sk_buff *ctx)
+{
+ asm volatile (
+ "r6 = %[array_map4] ll;"
+ "r7 = %[ringbuf] ll;"
+ "r1 = r6;"
+ "r2 = r10;"
+ "r2 += -8;"
+ "r9 = 0;"
+ "*(u64 *)(r2 + 0) = r9;"
+ "r3 = r10;"
+ "r3 += -24;"
+ "r9 = 0xeB9FeB9F;"
+ "*(u64 *)(r10 - 16) = r9;"
+ "*(u64 *)(r10 - 24) = r9;"
+ "r9 = 0;"
+ "r4 = 0;"
+ "r8 = r2;"
+ "call %[bpf_map_update_elem];"
+ "r1 = r6;"
+ "r2 = r8;"
+ "call %[bpf_map_lookup_elem];"
+ "if r0 != 0 goto tjmp1;"
+ "exit;"
+ "tjmp1:"
+ "r8 = r0;"
+ "r1 = r7;"
+ "r2 = 8;"
+ "r3 = 0;"
+ "r4 = r10;"
+ "r4 += -16;"
+ "r0 = *(u64 *)(r0 + 0);"
+ "call %[bpf_ringbuf_reserve_dynptr];"
+ "if r0 == 0 goto tjmp2;"
+ "r8 = r8;"
+ "r8 = r8;"
+ "r8 = r8;"
+ "r8 = r8;"
+ "r8 = r8;"
+ "r8 = r8;"
+ "r8 = r8;"
+ "goto tjmp3;"
+ "tjmp2:"
+ "*(u64 *)(r10 - 8) = r9;"
+ "*(u64 *)(r10 - 16) = r9;"
+ "r1 = r8;"
+ "r1 += 8;"
+ "r2 = 0;"
+ "r3 = 0;"
+ "r4 = r10;"
+ "r4 += -16;"
+ "call %[bpf_dynptr_from_mem];"
+ "tjmp3:"
+ "r1 = r10;"
+ "r1 += -16;"
+ "r2 = 0;"
+ "call %[bpf_ringbuf_discard_dynptr];"
+ :
+ : __imm(bpf_map_update_elem),
+ __imm(bpf_map_lookup_elem),
+ __imm(bpf_ringbuf_reserve_dynptr),
+ __imm(bpf_dynptr_from_mem),
+ __imm(bpf_ringbuf_discard_dynptr),
+ __imm_addr(array_map4),
+ __imm_addr(ringbuf)
+ : __clobber_all
+ );
+ return 0;
+}
--
2.39.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH bpf-next v2 09/11] selftests/bpf: Add dynptr var_off tests
2023-01-19 2:14 [PATCH bpf-next v2 00/11] Dynptr fixes Kumar Kartikeya Dwivedi
` (7 preceding siblings ...)
2023-01-19 2:14 ` [PATCH bpf-next v2 08/11] selftests/bpf: Add dynptr pruning tests Kumar Kartikeya Dwivedi
@ 2023-01-19 2:14 ` Kumar Kartikeya Dwivedi
2023-01-19 22:49 ` Joanne Koong
2023-01-19 2:14 ` [PATCH bpf-next v2 10/11] selftests/bpf: Add dynptr partial slot overwrite tests Kumar Kartikeya Dwivedi
2023-01-19 2:14 ` [PATCH bpf-next v2 11/11] selftests/bpf: Add dynptr helper tests Kumar Kartikeya Dwivedi
10 siblings, 1 reply; 22+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2023-01-19 2:14 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, Joanne Koong, David Vernet, Eduard Zingerman
Ensure that variable offset is handled correctly, and verifier takes
both fixed and variable part into account. Also ensures that only
constant var_off is allowed.
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
.../testing/selftests/bpf/progs/dynptr_fail.c | 40 +++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
index 023b3c36bc78..063d351f327a 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
@@ -794,3 +794,43 @@ int dynptr_pruning_type_confusion(struct __sk_buff *ctx)
);
return 0;
}
+
+SEC("?tc")
+__failure __msg("dynptr has to be at the constant offset") __log_level(2)
+int dynptr_var_off_overwrite(struct __sk_buff *ctx)
+{
+ asm volatile (
+ "r9 = 16;"
+ "*(u32 *)(r10 - 4) = r9;"
+ "r8 = *(u32 *)(r10 - 4);"
+ "if r8 >= 0 goto vjmp1;"
+ "r0 = 1;"
+ "exit;"
+ "vjmp1:"
+ "if r8 <= 16 goto vjmp2;"
+ "r0 = 1;"
+ "exit;"
+ "vjmp2:"
+ "r8 &= 16;"
+ "r1 = %[ringbuf] ll;"
+ "r2 = 8;"
+ "r3 = 0;"
+ "r4 = r10;"
+ "r4 += -32;"
+ "r4 += r8;"
+ "call %[bpf_ringbuf_reserve_dynptr];"
+ "r9 = 0xeB9F;"
+ "*(u64 *)(r10 - 16) = r9;"
+ "r1 = r10;"
+ "r1 += -32;"
+ "r1 += r8;"
+ "r2 = 0;"
+ "call %[bpf_ringbuf_discard_dynptr];"
+ :
+ : __imm(bpf_ringbuf_reserve_dynptr),
+ __imm(bpf_ringbuf_discard_dynptr),
+ __imm_addr(ringbuf)
+ : __clobber_all
+ );
+ return 0;
+}
--
2.39.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH bpf-next v2 10/11] selftests/bpf: Add dynptr partial slot overwrite tests
2023-01-19 2:14 [PATCH bpf-next v2 00/11] Dynptr fixes Kumar Kartikeya Dwivedi
` (8 preceding siblings ...)
2023-01-19 2:14 ` [PATCH bpf-next v2 09/11] selftests/bpf: Add dynptr var_off tests Kumar Kartikeya Dwivedi
@ 2023-01-19 2:14 ` Kumar Kartikeya Dwivedi
2023-01-19 2:14 ` [PATCH bpf-next v2 11/11] selftests/bpf: Add dynptr helper tests Kumar Kartikeya Dwivedi
10 siblings, 0 replies; 22+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2023-01-19 2:14 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, Joanne Koong, David Vernet, Eduard Zingerman
Try creating a dynptr, then overwriting second slot with first slot of
another dynptr. Then, the first slot of first dynptr should also be
invalidated, but without our fix that does not happen. As a consequence,
the unfixed case allows passing first dynptr (as the kernel check only
checks for slot_type and then first_slot == true).
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
.../testing/selftests/bpf/progs/dynptr_fail.c | 66 +++++++++++++++++++
1 file changed, 66 insertions(+)
diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
index 063d351f327a..e63d25d82b05 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
@@ -834,3 +834,69 @@ int dynptr_var_off_overwrite(struct __sk_buff *ctx)
);
return 0;
}
+
+SEC("?tc")
+__failure __msg("cannot overwrite referenced dynptr") __log_level(2)
+int dynptr_partial_slot_invalidate(struct __sk_buff *ctx)
+{
+ asm volatile (
+ "r6 = %[ringbuf] ll;"
+ "r7 = %[array_map4] ll;"
+ "r1 = r7;"
+ "r2 = r10;"
+ "r2 += -8;"
+ "r9 = 0;"
+ "*(u64 *)(r2 + 0) = r9;"
+ "r3 = r2;"
+ "r4 = 0;"
+ "r8 = r2;"
+ "call %[bpf_map_update_elem];"
+ "r1 = r7;"
+ "r2 = r8;"
+ "call %[bpf_map_lookup_elem];"
+ "if r0 != 0 goto sjmp1;"
+ "exit;"
+ "sjmp1:"
+ "r7 = r0;"
+ "r1 = r6;"
+ "r2 = 8;"
+ "r3 = 0;"
+ "r4 = r10;"
+ "r4 += -24;"
+ "call %[bpf_ringbuf_reserve_dynptr];"
+ "*(u64 *)(r10 - 16) = r9;"
+ "r1 = r7;"
+ "r2 = 8;"
+ "r3 = 0;"
+ "r4 = r10;"
+ "r4 += -16;"
+ "call %[bpf_dynptr_from_mem];"
+ "r1 = r10;"
+ "r1 += -512;"
+ "r2 = 488;"
+ "r3 = r10;"
+ "r3 += -24;"
+ "r4 = 0;"
+ "r5 = 0;"
+ "call %[bpf_dynptr_read];"
+ "r8 = 1;"
+ "if r0 != 0 goto sjmp2;"
+ "r8 = 0;"
+ "sjmp2:"
+ "r1 = r10;"
+ "r1 += -24;"
+ "r2 = 0;"
+ "call %[bpf_ringbuf_reserve_dynptr];"
+ :
+ : __imm(bpf_map_update_elem),
+ __imm(bpf_map_lookup_elem),
+ __imm(bpf_ringbuf_reserve_dynptr),
+ __imm(bpf_ringbuf_discard_dynptr),
+ __imm(bpf_dynptr_from_mem),
+ __imm(bpf_dynptr_read),
+ __imm_addr(ringbuf),
+ __imm_addr(array_map4)
+ : __clobber_all
+ );
+ return 0;
+}
--
2.39.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH bpf-next v2 11/11] selftests/bpf: Add dynptr helper tests
2023-01-19 2:14 [PATCH bpf-next v2 00/11] Dynptr fixes Kumar Kartikeya Dwivedi
` (9 preceding siblings ...)
2023-01-19 2:14 ` [PATCH bpf-next v2 10/11] selftests/bpf: Add dynptr partial slot overwrite tests Kumar Kartikeya Dwivedi
@ 2023-01-19 2:14 ` Kumar Kartikeya Dwivedi
10 siblings, 0 replies; 22+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2023-01-19 2:14 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, Joanne Koong, David Vernet, Eduard Zingerman
First test that we allow overwriting dynptr slots and reinitializing
them in unreferenced case, and disallow overwriting for referenced case.
Next, test that MEM_UNINIT doesn't allow writing dynptr stack slots.
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
.../testing/selftests/bpf/progs/dynptr_fail.c | 62 +++++++++++++++++++
1 file changed, 62 insertions(+)
diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
index e63d25d82b05..94686366dcde 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
@@ -900,3 +900,65 @@ int dynptr_partial_slot_invalidate(struct __sk_buff *ctx)
);
return 0;
}
+
+SEC("?raw_tp")
+__success
+int dynptr_overwrite_unref(void *ctx)
+{
+ struct bpf_dynptr ptr;
+
+ get_map_val_dynptr(&ptr);
+ get_map_val_dynptr(&ptr);
+ get_map_val_dynptr(&ptr);
+
+ return 0;
+}
+
+SEC("?raw_tp")
+__failure __msg("cannot overwrite referenced dynptr")
+int dynptr_overwrite_ref(void *ctx)
+{
+ struct bpf_dynptr ptr;
+
+ bpf_ringbuf_reserve_dynptr(&ringbuf, 64, 0, &ptr);
+ if (get_map_val_dynptr(&ptr))
+ bpf_ringbuf_discard_dynptr(&ptr, 0);
+ return 0;
+}
+
+/* Reject writes to dynptr slot from bpf_dynptr_read */
+SEC("?raw_tp")
+__failure __msg("potential write to dynptr at off=-16")
+int dynptr_read_into_slot(void *ctx)
+{
+ union {
+ struct {
+ char _pad[48];
+ struct bpf_dynptr ptr;
+ };
+ char buf[64];
+ } data;
+
+ bpf_ringbuf_reserve_dynptr(&ringbuf, 64, 0, &data.ptr);
+ /* this should fail */
+ bpf_dynptr_read(data.buf, sizeof(data.buf), &data.ptr, 0, 0);
+
+ return 0;
+}
+
+/* Reject writes to dynptr slot for uninit arg */
+SEC("?raw_tp")
+__failure __msg("potential write to dynptr at off=-16")
+int uninit_write_into_slot(void *ctx)
+{
+ struct {
+ char buf[64];
+ struct bpf_dynptr ptr;
+ } data;
+
+ bpf_ringbuf_reserve_dynptr(&ringbuf, 80, 0, &data.ptr);
+ /* this should fail */
+ bpf_get_current_comm(data.buf, 80);
+
+ return 0;
+}
--
2.39.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 02/11] bpf: Fix missing var_off check for ARG_PTR_TO_DYNPTR
2023-01-19 2:14 ` [PATCH bpf-next v2 02/11] bpf: Fix missing var_off check for ARG_PTR_TO_DYNPTR Kumar Kartikeya Dwivedi
@ 2023-01-19 22:00 ` Joanne Koong
2023-01-19 22:14 ` Kumar Kartikeya Dwivedi
0 siblings, 1 reply; 22+ messages in thread
From: Joanne Koong @ 2023-01-19 22:00 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, David Vernet, Eduard Zingerman
On Wed, Jan 18, 2023 at 6:14 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> Currently, the dynptr function is not checking the variable offset part
> of PTR_TO_STACK that it needs to check. The fixed offset is considered
> when computing the stack pointer index, but if the variable offset was
> not a constant (such that it could not be accumulated in reg->off), we
> will end up a discrepency where runtime pointer does not point to the
> actual stack slot we mark as STACK_DYNPTR.
>
> It is impossible to precisely track dynptr state when variable offset is
> not constant, hence, just like bpf_timer, kptr, bpf_spin_lock, etc.
> simply reject the case where reg->var_off is not constant. Then,
> consider both reg->off and reg->var_off.value when computing the stack
> pointer index.
>
> A new helper dynptr_get_spi is introduced to hide over these details
> since the dynptr needs to be located in multiple places outside the
> process_dynptr_func checks, hence once we know it's a PTR_TO_STACK, we
> need to enforce these checks in all places.
>
> Note that it is disallowed for unprivileged users to have a non-constant
> var_off, so this problem should only be possible to trigger from
> programs having CAP_PERFMON. However, its effects can vary.
>
> Without the fix, it is possible to replace the contents of the dynptr
> arbitrarily by making verifier mark different stack slots than actual
> location and then doing writes to the actual stack address of dynptr at
> runtime.
>
> Fixes: 97e03f521050 ("bpf: Add verifier support for dynptrs")
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
> kernel/bpf/verifier.c | 83 +++++++++++++++----
> .../bpf/prog_tests/kfunc_dynptr_param.c | 2 +-
> .../testing/selftests/bpf/progs/dynptr_fail.c | 4 +-
> 3 files changed, 68 insertions(+), 21 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 89de5bc46f27..eeb6f1b2bd60 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -638,11 +638,34 @@ static void print_liveness(struct bpf_verifier_env *env,
> verbose(env, "D");
> }
>
> -static int get_spi(s32 off)
> +static int __get_spi(s32 off)
> {
> return (-off - 1) / BPF_REG_SIZE;
> }
>
> +static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> +{
> + int off, spi;
> +
> + if (!tnum_is_const(reg->var_off)) {
> + verbose(env, "dynptr has to be at the constant offset\n");
nit: "at a constant offset" instead of "at the constant offset"?
> + return -EINVAL;
> + }
> +
> + off = reg->off + reg->var_off.value;
> + if (off % BPF_REG_SIZE) {
> + verbose(env, "cannot pass in dynptr at an offset=%d\n", off);
> + return -EINVAL;
> + }
> +
> + spi = __get_spi(off);
> + if (spi < 1) {
> + verbose(env, "cannot pass in dynptr at an offset=%d\n", off);
> + return -EINVAL;
> + }
I still think this if (spi < 1) check should have the same logic
is_spi_bounds_valid() does (eg checking against total allocated slots
as well). I think we can combine is_spi_bounds_valid() with this
function and then remove every place we call is_spi_bounds_valid().
WDYT?
> + return spi;
> +}
> +
> static bool is_spi_bounds_valid(struct bpf_func_state *state, int spi, int nr_slots)
> {
> int allocated_slots = state->allocated_stack / BPF_REG_SIZE;
> @@ -754,7 +777,9 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
> enum bpf_dynptr_type type;
> int spi, i, id;
>
> - spi = get_spi(reg->off);
> + spi = dynptr_get_spi(env, reg);
> + if (spi < 0)
> + return spi;
>
> if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS))
> return -EINVAL;
> @@ -792,7 +817,9 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re
> struct bpf_func_state *state = func(env, reg);
> int spi, i;
>
> - spi = get_spi(reg->off);
> + spi = dynptr_get_spi(env, reg);
> + if (spi < 0)
> + return spi;
>
> if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS))
> return -EINVAL;
> @@ -844,7 +871,11 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_
> if (reg->type == CONST_PTR_TO_DYNPTR)
> return false;
>
> - spi = get_spi(reg->off);
> + spi = dynptr_get_spi(env, reg);
> + if (spi < 0)
> + return false;
> +
> + /* We will do check_mem_access to check and update stack bounds later */
> if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS))
> return true;
>
> @@ -860,14 +891,15 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_
> static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> {
> struct bpf_func_state *state = func(env, reg);
> - int spi;
> - int i;
> + int spi, i;
>
> /* This already represents first slot of initialized bpf_dynptr */
> if (reg->type == CONST_PTR_TO_DYNPTR)
> return true;
>
> - spi = get_spi(reg->off);
> + spi = dynptr_get_spi(env, reg);
> + if (spi < 0)
> + return false;
> if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS) ||
> !state->stack[spi].spilled_ptr.dynptr.first_slot)
> return false;
> @@ -896,7 +928,9 @@ static bool is_dynptr_type_expected(struct bpf_verifier_env *env, struct bpf_reg
> if (reg->type == CONST_PTR_TO_DYNPTR) {
> return reg->dynptr.type == dynptr_type;
> } else {
> - spi = get_spi(reg->off);
> + spi = dynptr_get_spi(env, reg);
> + if (spi < 0)
> + return false;
> return state->stack[spi].spilled_ptr.dynptr.type == dynptr_type;
> }
> }
> @@ -2429,7 +2463,9 @@ static int mark_dynptr_read(struct bpf_verifier_env *env, struct bpf_reg_state *
> */
> if (reg->type == CONST_PTR_TO_DYNPTR)
> return 0;
> - spi = get_spi(reg->off);
> + spi = dynptr_get_spi(env, reg);
> + if (spi < 0)
> + return spi;
> /* Caller ensures dynptr is valid and initialized, which means spi is in
> * bounds and spi is the first dynptr slot. Simply mark stack slot as
> * read.
> @@ -5993,12 +6029,14 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno,
> }
> /* CONST_PTR_TO_DYNPTR already has fixed and var_off as 0 due to
> * check_func_arg_reg_off's logic. We only need to check offset
> - * alignment for PTR_TO_STACK.
> + * and its alignment for PTR_TO_STACK.
> */
> - if (reg->type == PTR_TO_STACK && (reg->off % BPF_REG_SIZE)) {
> - verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off);
> - return -EINVAL;
> + if (reg->type == PTR_TO_STACK) {
> + err = dynptr_get_spi(env, reg);
> + if (err < 0)
> + return err;
> }
> +
> /* MEM_UNINIT - Points to memory that is an appropriate candidate for
> * constructing a mutable bpf_dynptr object.
> *
> @@ -6404,15 +6442,16 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env,
> }
> }
>
> -static u32 dynptr_ref_obj_id(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> +static int dynptr_ref_obj_id(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> {
> struct bpf_func_state *state = func(env, reg);
> int spi;
>
> if (reg->type == CONST_PTR_TO_DYNPTR)
> return reg->ref_obj_id;
> -
> - spi = get_spi(reg->off);
> + spi = dynptr_get_spi(env, reg);
> + if (spi < 0)
> + return spi;
> return state->stack[spi].spilled_ptr.ref_obj_id;
> }
>
> @@ -6486,7 +6525,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> * PTR_TO_STACK.
> */
> if (reg->type == PTR_TO_STACK) {
> - spi = get_spi(reg->off);
> + spi = dynptr_get_spi(env, reg);
> + if (spi < 0)
> + return spi;
> if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS) ||
> !state->stack[spi].spilled_ptr.ref_obj_id) {
> verbose(env, "arg %d is an unacquired reference\n", regno);
> @@ -7976,13 +8017,19 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) {
> if (arg_type_is_dynptr(fn->arg_type[i])) {
> struct bpf_reg_state *reg = ®s[BPF_REG_1 + i];
> + int ref_obj_id;
>
> if (meta.ref_obj_id) {
> verbose(env, "verifier internal error: meta.ref_obj_id already set\n");
> return -EFAULT;
> }
>
> - meta.ref_obj_id = dynptr_ref_obj_id(env, reg);
> + ref_obj_id = dynptr_ref_obj_id(env, reg);
> + if (err < 0) {
> + verbose(env, "verifier internal error: failed to obtain dynptr ref_obj_id\n");
> + return err;
> + }
> + meta.ref_obj_id = ref_obj_id;
> break;
> }
> }
> diff --git a/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c b/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c
> index a9229260a6ce..72800b1e8395 100644
> --- a/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c
> +++ b/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c
> @@ -18,7 +18,7 @@ static struct {
> const char *expected_verifier_err_msg;
> int expected_runtime_err;
> } kfunc_dynptr_tests[] = {
> - {"not_valid_dynptr", "Expected an initialized dynptr as arg #1", 0},
> + {"not_valid_dynptr", "cannot pass in dynptr at an offset=-8", 0},
> {"not_ptr_to_stack", "arg#0 expected pointer to stack or dynptr_ptr", 0},
> {"dynptr_data_null", NULL, -EBADMSG},
> };
> diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
> index 78debc1b3820..02d57b95cf6e 100644
> --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
> +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
> @@ -382,7 +382,7 @@ int invalid_helper1(void *ctx)
>
> /* A dynptr can't be passed into a helper function at a non-zero offset */
> SEC("?raw_tp")
> -__failure __msg("Expected an initialized dynptr as arg #3")
> +__failure __msg("cannot pass in dynptr at an offset=-8")
> int invalid_helper2(void *ctx)
> {
> struct bpf_dynptr ptr;
> @@ -584,7 +584,7 @@ int invalid_read4(void *ctx)
>
> /* Initializing a dynptr on an offset should fail */
> SEC("?raw_tp")
> -__failure __msg("invalid write to stack")
> +__failure __msg("cannot pass in dynptr at an offset=0")
> int invalid_offset(void *ctx)
> {
> struct bpf_dynptr ptr;
> --
> 2.39.1
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 03/11] bpf: Fix partial dynptr stack slot reads/writes
2023-01-19 2:14 ` [PATCH bpf-next v2 03/11] bpf: Fix partial dynptr stack slot reads/writes Kumar Kartikeya Dwivedi
@ 2023-01-19 22:06 ` Joanne Koong
2023-01-19 22:15 ` Kumar Kartikeya Dwivedi
0 siblings, 1 reply; 22+ messages in thread
From: Joanne Koong @ 2023-01-19 22:06 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, David Vernet, Eduard Zingerman
On Wed, Jan 18, 2023 at 6:14 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> Currently, while reads are disallowed for dynptr stack slots, writes are
> not. Reads don't work from both direct access and helpers, while writes
> do work in both cases, but have the effect of overwriting the slot_type.
>
> While this is fine, handling for a few edge cases is missing. Firstly,
> a user can overwrite the stack slots of dynptr partially.
>
> Consider the following layout:
> spi: [d][d][?]
> 2 1 0
>
> First slot is at spi 2, second at spi 1.
> Now, do a write of 1 to 8 bytes for spi 1.
>
> This will essentially either write STACK_MISC for all slot_types or
> STACK_MISC and STACK_ZERO (in case of size < BPF_REG_SIZE partial write
> of zeroes). The end result is that slot is scrubbed.
>
> Now, the layout is:
> spi: [d][m][?]
> 2 1 0
>
> Suppose if user initializes spi = 1 as dynptr.
> We get:
> spi: [d][d][d]
> 2 1 0
>
> But this time, both spi 2 and spi 1 have first_slot = true.
>
> Now, when passing spi 2 to dynptr helper, it will consider it as
> initialized as it does not check whether second slot has first_slot ==
> false. And spi 1 should already work as normal.
>
> This effectively replaced size + offset of first dynptr, hence allowing
> invalid OOB reads and writes.
>
> Make a few changes to protect against this:
> When writing to PTR_TO_STACK using BPF insns, when we touch spi of a
> STACK_DYNPTR type, mark both first and second slot (regardless of which
> slot we touch) as STACK_INVALID. Reads are already prevented.
>
> Second, prevent writing to stack memory from helpers if the range may
> contain any STACK_DYNPTR slots. Reads are already prevented.
>
> For helpers, we cannot allow it to destroy dynptrs from the writes as
> depending on arguments, helper may take uninit_mem and dynptr both at
> the same time. This would mean that helper may write to uninit_mem
> before it reads the dynptr, which would be bad.
>
> PTR_TO_MEM: [?????dd]
>
> Depending on the code inside the helper, it may end up overwriting the
> dynptr contents first and then read those as the dynptr argument.
>
> Verifier would only simulate destruction when it does byte by byte
> access simulation in check_helper_call for meta.access_size, and
> fail to catch this case, as it happens after argument checks.
>
> The same would need to be done for any other non-trivial objects created
> on the stack in the future, such as bpf_list_head on stack, or
> bpf_rb_root on stack.
>
> A common misunderstanding in the current code is that MEM_UNINIT means
> writes, but note that writes may also be performed even without
> MEM_UNINIT in case of helpers, in that case the code after handling meta
> && meta->raw_mode will complain when it sees STACK_DYNPTR. So that
> invalid read case also covers writes to potential STACK_DYNPTR slots.
> The only loophole was in case of meta->raw_mode which simulated writes
> through instructions which could overwrite them.
>
> A future series sequenced after this will focus on the clean up of
> helper access checks and bugs around that.
>
> Fixes: 97e03f521050 ("bpf: Add verifier support for dynptrs")
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
> kernel/bpf/verifier.c | 102 ++++++++++++++++++
> .../testing/selftests/bpf/progs/dynptr_fail.c | 6 +-
> 2 files changed, 105 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index eeb6f1b2bd60..09c09d9bfd89 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -769,6 +769,8 @@ static void mark_dynptr_cb_reg(struct bpf_reg_state *reg,
> __mark_dynptr_reg(reg, type, true);
> }
>
> +static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env,
> + struct bpf_func_state *state, int spi);
>
> static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> enum bpf_arg_type arg_type, int insn_idx)
> @@ -863,6 +865,69 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re
> return 0;
> }
>
> +static void __mark_reg_unknown(const struct bpf_verifier_env *env,
> + struct bpf_reg_state *reg);
> +
> +static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env,
> + struct bpf_func_state *state, int spi)
> +{
> + int i;
> +
> + /* We always ensure that STACK_DYNPTR is never set partially,
> + * hence just checking for slot_type[0] is enough. This is
> + * different for STACK_SPILL, where it may be only set for
> + * 1 byte, so code has to use is_spilled_reg.
> + */
> + if (state->stack[spi].slot_type[0] != STACK_DYNPTR)
> + return 0;
> +
> + /* Reposition spi to first slot */
> + if (!state->stack[spi].spilled_ptr.dynptr.first_slot)
> + spi = spi + 1;
> +
> + if (dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type)) {
> + verbose(env, "cannot overwrite referenced dynptr\n");
> + return -EINVAL;
> + }
> +
> + mark_stack_slot_scratched(env, spi);
> + mark_stack_slot_scratched(env, spi - 1);
> +
> + /* Writing partially to one dynptr stack slot destroys both. */
> + for (i = 0; i < BPF_REG_SIZE; i++) {
> + state->stack[spi].slot_type[i] = STACK_INVALID;
> + state->stack[spi - 1].slot_type[i] = STACK_INVALID;
> + }
> +
> + /* Invalidate any slices associated with this dynptr */
> + if (dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type)) {
We'll never get here because referenced dynptrs can't be overwritten
(we check this above and return -EINVAL if dynptr_type_refcounted() is
true).
I think we should invalidate any slices associated with non-referenced
dynptrs as well.
> + int ref_obj_id = state->stack[spi].spilled_ptr.ref_obj_id;
> + struct bpf_func_state *fstate;
> + struct bpf_reg_state *reg;
> +
> + bpf_for_each_reg_in_vstate(env->cur_state, fstate, reg, ({
> + if (reg->ref_obj_id == ref_obj_id) {
> + if (!env->allow_ptr_leaks)
> + __mark_reg_not_init(env, reg);
> + else
> + __mark_reg_unknown(env, reg);
> + }
> + }));
> + }
> +
> + /* Do not release reference state, we are destroying dynptr on stack,
> + * not using some helper to release it. Just reset register.
> + */
> + __mark_reg_not_init(env, &state->stack[spi].spilled_ptr);
> + __mark_reg_not_init(env, &state->stack[spi - 1].spilled_ptr);
> +
> + /* Same reason as unmark_stack_slots_dynptr above */
> + state->stack[spi].spilled_ptr.live |= REG_LIVE_WRITTEN;
> + state->stack[spi - 1].spilled_ptr.live |= REG_LIVE_WRITTEN;
> +
> + return 0;
> +}
> +
> static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> {
> struct bpf_func_state *state = func(env, reg);
> @@ -3391,6 +3456,10 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
> env->insn_aux_data[insn_idx].sanitize_stack_spill = true;
> }
>
> + err = destroy_if_dynptr_stack_slot(env, state, spi);
> + if (err)
> + return err;
> +
> mark_stack_slot_scratched(env, spi);
> if (reg && !(off % BPF_REG_SIZE) && register_is_bounded(reg) &&
> !register_is_null(reg) && env->bpf_capable) {
> @@ -3504,6 +3573,14 @@ static int check_stack_write_var_off(struct bpf_verifier_env *env,
> if (err)
> return err;
>
> + for (i = min_off; i < max_off; i++) {
> + int spi;
> +
> + spi = __get_spi(i);
> + err = destroy_if_dynptr_stack_slot(env, state, spi);
> + if (err)
> + return err;
> + }
>
> /* Variable offset writes destroy any spilled pointers in range. */
> for (i = min_off; i < max_off; i++) {
> @@ -5531,6 +5608,31 @@ static int check_stack_range_initialized(
> }
>
> if (meta && meta->raw_mode) {
> + /* Ensure we won't be overwriting dynptrs when simulating byte
> + * by byte access in check_helper_call using meta.access_size.
> + * This would be a problem if we have a helper in the future
> + * which takes:
> + *
> + * helper(uninit_mem, len, dynptr)
> + *
> + * Now, uninint_mem may overlap with dynptr pointer. Hence, it
> + * may end up writing to dynptr itself when touching memory from
> + * arg 1. This can be relaxed on a case by case basis for known
> + * safe cases, but reject due to the possibilitiy of aliasing by
> + * default.
> + */
> + for (i = min_off; i < max_off + access_size; i++) {
> + int stack_off = -i - 1;
> +
> + spi = __get_spi(i);
> + /* raw_mode may write past allocated_stack */
> + if (state->allocated_stack <= stack_off)
> + continue;
> + if (state->stack[spi].slot_type[stack_off % BPF_REG_SIZE] == STACK_DYNPTR) {
> + verbose(env, "potential write to dynptr at off=%d disallowed\n", i);
> + return -EACCES;
> + }
> + }
> meta->access_size = access_size;
> meta->regno = regno;
> return 0;
> diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
> index 02d57b95cf6e..9dc3f23a8270 100644
> --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
> +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
> @@ -420,7 +420,7 @@ int invalid_write1(void *ctx)
> * offset
> */
> SEC("?raw_tp")
> -__failure __msg("Expected an initialized dynptr as arg #3")
> +__failure __msg("cannot overwrite referenced dynptr")
> int invalid_write2(void *ctx)
> {
> struct bpf_dynptr ptr;
> @@ -444,7 +444,7 @@ int invalid_write2(void *ctx)
> * non-const offset
> */
> SEC("?raw_tp")
> -__failure __msg("Expected an initialized dynptr as arg #1")
> +__failure __msg("cannot overwrite referenced dynptr")
> int invalid_write3(void *ctx)
> {
> struct bpf_dynptr ptr;
> @@ -476,7 +476,7 @@ static int invalid_write4_callback(__u32 index, void *data)
> * be invalidated as a dynptr
> */
> SEC("?raw_tp")
> -__failure __msg("arg 1 is an unacquired reference")
> +__failure __msg("cannot overwrite referenced dynptr")
> int invalid_write4(void *ctx)
> {
> struct bpf_dynptr ptr;
> --
> 2.39.1
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 04/11] bpf: Allow reinitializing unreferenced dynptr stack slots
2023-01-19 2:14 ` [PATCH bpf-next v2 04/11] bpf: Allow reinitializing unreferenced dynptr stack slots Kumar Kartikeya Dwivedi
@ 2023-01-19 22:13 ` Joanne Koong
0 siblings, 0 replies; 22+ messages in thread
From: Joanne Koong @ 2023-01-19 22:13 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, David Vernet, Eduard Zingerman
On Wed, Jan 18, 2023 at 6:14 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> Consider a program like below:
>
> void prog(void)
> {
> {
> struct bpf_dynptr ptr;
> bpf_dynptr_from_mem(...);
> }
> ...
> {
> struct bpf_dynptr ptr;
> bpf_dynptr_from_mem(...);
> }
> }
>
> Here, the C compiler based on lifetime rules in the C standard would be
> well within in its rights to share stack storage for dynptr 'ptr' as
> their lifetimes do not overlap in the two distinct scopes. Currently,
> such an example would be rejected by the verifier, but this is too
> strict. Instead, we should allow reinitializing over dynptr stack slots
> and forget information about the old dynptr object.
>
> The destroy_if_dynptr_stack_slot function already makes necessary checks
> to avoid overwriting referenced dynptr slots. This is done to present a
> better error message instead of forgetting dynptr information on stack
> and preserving reference state, leading to an inevitable but
> undecipherable error at the end about an unreleased reference which has
> to be associated back to its allocating call instruction to make any
> sense to the user.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Acked-by: Joanne Koong <joannelkoong@gmail.com>
> ---
> kernel/bpf/verifier.c | 34 ++++++++++++++++++++++++++--------
> 1 file changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 09c09d9bfd89..4feaddd5d6dc 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -777,7 +777,7 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
> {
> struct bpf_func_state *state = func(env, reg);
> enum bpf_dynptr_type type;
> - int spi, i, id;
> + int spi, i, id, err;
>
> spi = dynptr_get_spi(env, reg);
> if (spi < 0)
> @@ -786,6 +786,22 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
> if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS))
> return -EINVAL;
>
> + /* We cannot assume both spi and spi - 1 belong to the same dynptr,
> + * hence we need to call destroy_if_dynptr_stack_slot twice for both,
> + * to ensure that for the following example:
> + * [d1][d1][d2][d2]
> + * spi 3 2 1 0
> + * So marking spi = 2 should lead to destruction of both d1 and d2. In
> + * case they do belong to same dynptr, second call won't see slot_type
> + * as STACK_DYNPTR and will simply skip destruction.
> + */
> + err = destroy_if_dynptr_stack_slot(env, state, spi);
> + if (err)
> + return err;
> + err = destroy_if_dynptr_stack_slot(env, state, spi - 1);
> + if (err)
> + return err;
> +
> for (i = 0; i < BPF_REG_SIZE; i++) {
> state->stack[spi].slot_type[i] = STACK_DYNPTR;
> state->stack[spi - 1].slot_type[i] = STACK_DYNPTR;
> @@ -931,7 +947,7 @@ static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env,
> static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> {
> struct bpf_func_state *state = func(env, reg);
> - int spi, i;
> + int spi;
>
> if (reg->type == CONST_PTR_TO_DYNPTR)
> return false;
> @@ -944,12 +960,14 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_
> if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS))
> return true;
>
> - for (i = 0; i < BPF_REG_SIZE; i++) {
> - if (state->stack[spi].slot_type[i] == STACK_DYNPTR ||
> - state->stack[spi - 1].slot_type[i] == STACK_DYNPTR)
> - return false;
> - }
> -
> + /* We allow overwriting existing unreferenced STACK_DYNPTR slots, see
> + * mark_stack_slots_dynptr which calls destroy_if_dynptr_stack_slot to
> + * ensure dynptr objects at the slots we are touching are completely
> + * destructed before we reinitialize them for a new one. For referenced
> + * ones, destroy_if_dynptr_stack_slot returns an error early instead of
> + * delaying it until the end where the user will get "Unreleased
> + * reference" error.
> + */
> return true;
> }
>
> --
> 2.39.1
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 02/11] bpf: Fix missing var_off check for ARG_PTR_TO_DYNPTR
2023-01-19 22:00 ` Joanne Koong
@ 2023-01-19 22:14 ` Kumar Kartikeya Dwivedi
2023-01-19 22:25 ` Joanne Koong
0 siblings, 1 reply; 22+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2023-01-19 22:14 UTC (permalink / raw)
To: Joanne Koong
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, David Vernet, Eduard Zingerman
On Fri, 20 Jan 2023 at 03:30, Joanne Koong <joannelkoong@gmail.com> wrote:
>
> On Wed, Jan 18, 2023 at 6:14 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > Currently, the dynptr function is not checking the variable offset part
> > of PTR_TO_STACK that it needs to check. The fixed offset is considered
> > when computing the stack pointer index, but if the variable offset was
> > not a constant (such that it could not be accumulated in reg->off), we
> > will end up a discrepency where runtime pointer does not point to the
> > actual stack slot we mark as STACK_DYNPTR.
> >
> > It is impossible to precisely track dynptr state when variable offset is
> > not constant, hence, just like bpf_timer, kptr, bpf_spin_lock, etc.
> > simply reject the case where reg->var_off is not constant. Then,
> > consider both reg->off and reg->var_off.value when computing the stack
> > pointer index.
> >
> > A new helper dynptr_get_spi is introduced to hide over these details
> > since the dynptr needs to be located in multiple places outside the
> > process_dynptr_func checks, hence once we know it's a PTR_TO_STACK, we
> > need to enforce these checks in all places.
> >
> > Note that it is disallowed for unprivileged users to have a non-constant
> > var_off, so this problem should only be possible to trigger from
> > programs having CAP_PERFMON. However, its effects can vary.
> >
> > Without the fix, it is possible to replace the contents of the dynptr
> > arbitrarily by making verifier mark different stack slots than actual
> > location and then doing writes to the actual stack address of dynptr at
> > runtime.
> >
> > Fixes: 97e03f521050 ("bpf: Add verifier support for dynptrs")
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> > kernel/bpf/verifier.c | 83 +++++++++++++++----
> > .../bpf/prog_tests/kfunc_dynptr_param.c | 2 +-
> > .../testing/selftests/bpf/progs/dynptr_fail.c | 4 +-
> > 3 files changed, 68 insertions(+), 21 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 89de5bc46f27..eeb6f1b2bd60 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -638,11 +638,34 @@ static void print_liveness(struct bpf_verifier_env *env,
> > verbose(env, "D");
> > }
> >
> > -static int get_spi(s32 off)
> > +static int __get_spi(s32 off)
> > {
> > return (-off - 1) / BPF_REG_SIZE;
> > }
> >
> > +static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> > +{
> > + int off, spi;
> > +
> > + if (!tnum_is_const(reg->var_off)) {
> > + verbose(env, "dynptr has to be at the constant offset\n");
>
> nit: "at a constant offset" instead of "at the constant offset"?
>
Will fix.
> > + return -EINVAL;
> > + }
> > +
> > + off = reg->off + reg->var_off.value;
> > + if (off % BPF_REG_SIZE) {
> > + verbose(env, "cannot pass in dynptr at an offset=%d\n", off);
> > + return -EINVAL;
> > + }
> > +
> > + spi = __get_spi(off);
> > + if (spi < 1) {
> > + verbose(env, "cannot pass in dynptr at an offset=%d\n", off);
> > + return -EINVAL;
> > + }
>
> I still think this if (spi < 1) check should have the same logic
> is_spi_bounds_valid() does (eg checking against total allocated slots
> as well). I think we can combine is_spi_bounds_valid() with this
> function and then remove every place we call is_spi_bounds_valid().
> WDYT?
>
I believe I addressed this in patch 5, but kept the name of the
combined check as dynptr_get_spi instead (it looks better to me in the
context of the code compared to is_spi_bounds_valid). Please take a
look.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 03/11] bpf: Fix partial dynptr stack slot reads/writes
2023-01-19 22:06 ` Joanne Koong
@ 2023-01-19 22:15 ` Kumar Kartikeya Dwivedi
0 siblings, 0 replies; 22+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2023-01-19 22:15 UTC (permalink / raw)
To: Joanne Koong
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, David Vernet, Eduard Zingerman
On Fri, 20 Jan 2023 at 03:36, Joanne Koong <joannelkoong@gmail.com> wrote:
>
> On Wed, Jan 18, 2023 at 6:14 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > Currently, while reads are disallowed for dynptr stack slots, writes are
> > not. Reads don't work from both direct access and helpers, while writes
> > do work in both cases, but have the effect of overwriting the slot_type.
> >
> > While this is fine, handling for a few edge cases is missing. Firstly,
> > a user can overwrite the stack slots of dynptr partially.
> >
> > Consider the following layout:
> > spi: [d][d][?]
> > 2 1 0
> >
> > First slot is at spi 2, second at spi 1.
> > Now, do a write of 1 to 8 bytes for spi 1.
> >
> > This will essentially either write STACK_MISC for all slot_types or
> > STACK_MISC and STACK_ZERO (in case of size < BPF_REG_SIZE partial write
> > of zeroes). The end result is that slot is scrubbed.
> >
> > Now, the layout is:
> > spi: [d][m][?]
> > 2 1 0
> >
> > Suppose if user initializes spi = 1 as dynptr.
> > We get:
> > spi: [d][d][d]
> > 2 1 0
> >
> > But this time, both spi 2 and spi 1 have first_slot = true.
> >
> > Now, when passing spi 2 to dynptr helper, it will consider it as
> > initialized as it does not check whether second slot has first_slot ==
> > false. And spi 1 should already work as normal.
> >
> > This effectively replaced size + offset of first dynptr, hence allowing
> > invalid OOB reads and writes.
> >
> > Make a few changes to protect against this:
> > When writing to PTR_TO_STACK using BPF insns, when we touch spi of a
> > STACK_DYNPTR type, mark both first and second slot (regardless of which
> > slot we touch) as STACK_INVALID. Reads are already prevented.
> >
> > Second, prevent writing to stack memory from helpers if the range may
> > contain any STACK_DYNPTR slots. Reads are already prevented.
> >
> > For helpers, we cannot allow it to destroy dynptrs from the writes as
> > depending on arguments, helper may take uninit_mem and dynptr both at
> > the same time. This would mean that helper may write to uninit_mem
> > before it reads the dynptr, which would be bad.
> >
> > PTR_TO_MEM: [?????dd]
> >
> > Depending on the code inside the helper, it may end up overwriting the
> > dynptr contents first and then read those as the dynptr argument.
> >
> > Verifier would only simulate destruction when it does byte by byte
> > access simulation in check_helper_call for meta.access_size, and
> > fail to catch this case, as it happens after argument checks.
> >
> > The same would need to be done for any other non-trivial objects created
> > on the stack in the future, such as bpf_list_head on stack, or
> > bpf_rb_root on stack.
> >
> > A common misunderstanding in the current code is that MEM_UNINIT means
> > writes, but note that writes may also be performed even without
> > MEM_UNINIT in case of helpers, in that case the code after handling meta
> > && meta->raw_mode will complain when it sees STACK_DYNPTR. So that
> > invalid read case also covers writes to potential STACK_DYNPTR slots.
> > The only loophole was in case of meta->raw_mode which simulated writes
> > through instructions which could overwrite them.
> >
> > A future series sequenced after this will focus on the clean up of
> > helper access checks and bugs around that.
> >
> > Fixes: 97e03f521050 ("bpf: Add verifier support for dynptrs")
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> > kernel/bpf/verifier.c | 102 ++++++++++++++++++
> > .../testing/selftests/bpf/progs/dynptr_fail.c | 6 +-
> > 2 files changed, 105 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index eeb6f1b2bd60..09c09d9bfd89 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -769,6 +769,8 @@ static void mark_dynptr_cb_reg(struct bpf_reg_state *reg,
> > __mark_dynptr_reg(reg, type, true);
> > }
> >
> > +static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env,
> > + struct bpf_func_state *state, int spi);
> >
> > static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> > enum bpf_arg_type arg_type, int insn_idx)
> > @@ -863,6 +865,69 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re
> > return 0;
> > }
> >
> > +static void __mark_reg_unknown(const struct bpf_verifier_env *env,
> > + struct bpf_reg_state *reg);
> > +
> > +static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env,
> > + struct bpf_func_state *state, int spi)
> > +{
> > + int i;
> > +
> > + /* We always ensure that STACK_DYNPTR is never set partially,
> > + * hence just checking for slot_type[0] is enough. This is
> > + * different for STACK_SPILL, where it may be only set for
> > + * 1 byte, so code has to use is_spilled_reg.
> > + */
> > + if (state->stack[spi].slot_type[0] != STACK_DYNPTR)
> > + return 0;
> > +
> > + /* Reposition spi to first slot */
> > + if (!state->stack[spi].spilled_ptr.dynptr.first_slot)
> > + spi = spi + 1;
> > +
> > + if (dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type)) {
> > + verbose(env, "cannot overwrite referenced dynptr\n");
> > + return -EINVAL;
> > + }
> > +
> > + mark_stack_slot_scratched(env, spi);
> > + mark_stack_slot_scratched(env, spi - 1);
> > +
> > + /* Writing partially to one dynptr stack slot destroys both. */
> > + for (i = 0; i < BPF_REG_SIZE; i++) {
> > + state->stack[spi].slot_type[i] = STACK_INVALID;
> > + state->stack[spi - 1].slot_type[i] = STACK_INVALID;
> > + }
> > +
> > + /* Invalidate any slices associated with this dynptr */
> > + if (dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type)) {
>
> We'll never get here because referenced dynptrs can't be overwritten
> (we check this above and return -EINVAL if dynptr_type_refcounted() is
> true).
> I think we should invalidate any slices associated with non-referenced
> dynptrs as well.
Ah, right. Will fix, and I'll add a test for this to ensure slices are
invalidated in v3.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 02/11] bpf: Fix missing var_off check for ARG_PTR_TO_DYNPTR
2023-01-19 22:14 ` Kumar Kartikeya Dwivedi
@ 2023-01-19 22:25 ` Joanne Koong
0 siblings, 0 replies; 22+ messages in thread
From: Joanne Koong @ 2023-01-19 22:25 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, David Vernet, Eduard Zingerman
On Thu, Jan 19, 2023 at 2:14 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Fri, 20 Jan 2023 at 03:30, Joanne Koong <joannelkoong@gmail.com> wrote:
> >
> > On Wed, Jan 18, 2023 at 6:14 PM Kumar Kartikeya Dwivedi
> > <memxor@gmail.com> wrote:
> > >
> > > Currently, the dynptr function is not checking the variable offset part
> > > of PTR_TO_STACK that it needs to check. The fixed offset is considered
> > > when computing the stack pointer index, but if the variable offset was
> > > not a constant (such that it could not be accumulated in reg->off), we
> > > will end up a discrepency where runtime pointer does not point to the
> > > actual stack slot we mark as STACK_DYNPTR.
> > >
> > > It is impossible to precisely track dynptr state when variable offset is
> > > not constant, hence, just like bpf_timer, kptr, bpf_spin_lock, etc.
> > > simply reject the case where reg->var_off is not constant. Then,
> > > consider both reg->off and reg->var_off.value when computing the stack
> > > pointer index.
> > >
> > > A new helper dynptr_get_spi is introduced to hide over these details
> > > since the dynptr needs to be located in multiple places outside the
> > > process_dynptr_func checks, hence once we know it's a PTR_TO_STACK, we
> > > need to enforce these checks in all places.
> > >
> > > Note that it is disallowed for unprivileged users to have a non-constant
> > > var_off, so this problem should only be possible to trigger from
> > > programs having CAP_PERFMON. However, its effects can vary.
> > >
> > > Without the fix, it is possible to replace the contents of the dynptr
> > > arbitrarily by making verifier mark different stack slots than actual
> > > location and then doing writes to the actual stack address of dynptr at
> > > runtime.
> > >
> > > Fixes: 97e03f521050 ("bpf: Add verifier support for dynptrs")
> > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Acked-by: Joanne Koong <joannelkoong@gmail.com>
> > > ---
> > > kernel/bpf/verifier.c | 83 +++++++++++++++----
> > > .../bpf/prog_tests/kfunc_dynptr_param.c | 2 +-
> > > .../testing/selftests/bpf/progs/dynptr_fail.c | 4 +-
> > > 3 files changed, 68 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 89de5bc46f27..eeb6f1b2bd60 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -638,11 +638,34 @@ static void print_liveness(struct bpf_verifier_env *env,
> > > verbose(env, "D");
> > > }
> > >
> > > -static int get_spi(s32 off)
> > > +static int __get_spi(s32 off)
> > > {
> > > return (-off - 1) / BPF_REG_SIZE;
> > > }
> > >
> > > +static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> > > +{
> > > + int off, spi;
> > > +
> > > + if (!tnum_is_const(reg->var_off)) {
> > > + verbose(env, "dynptr has to be at the constant offset\n");
> >
> > nit: "at a constant offset" instead of "at the constant offset"?
> >
>
> Will fix.
>
> > > + return -EINVAL;
> > > + }
> > > +
> > > + off = reg->off + reg->var_off.value;
> > > + if (off % BPF_REG_SIZE) {
> > > + verbose(env, "cannot pass in dynptr at an offset=%d\n", off);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + spi = __get_spi(off);
> > > + if (spi < 1) {
> > > + verbose(env, "cannot pass in dynptr at an offset=%d\n", off);
> > > + return -EINVAL;
> > > + }
> >
> > I still think this if (spi < 1) check should have the same logic
> > is_spi_bounds_valid() does (eg checking against total allocated slots
> > as well). I think we can combine is_spi_bounds_valid() with this
> > function and then remove every place we call is_spi_bounds_valid().
> > WDYT?
> >
>
> I believe I addressed this in patch 5, but kept the name of the
> combined check as dynptr_get_spi instead (it looks better to me in the
> context of the code compared to is_spi_bounds_valid). Please take a
> look.
Ok, I see, these changes are in a separate patch.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 05/11] bpf: Combine dynptr_get_spi and is_spi_bounds_valid
2023-01-19 2:14 ` [PATCH bpf-next v2 05/11] bpf: Combine dynptr_get_spi and is_spi_bounds_valid Kumar Kartikeya Dwivedi
@ 2023-01-19 22:30 ` Joanne Koong
0 siblings, 0 replies; 22+ messages in thread
From: Joanne Koong @ 2023-01-19 22:30 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, David Vernet, Eduard Zingerman
On Wed, Jan 18, 2023 at 6:15 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> Currently, a check on spi resides in dynptr_get_spi, while others
> checking its validity for being within the allocated stack slots happens
> in is_spi_bounds_valid. Almost always barring a couple of cases (where
> being beyond allocated stack slots is not an error as stack slots need
> to be populated), both are used together to make checks. Hence, subsume
> the is_spi_bounds_valid check in dynptr_get_spi, and return -ERANGE to
> specially distinguish the case where spi is valid but not within
> allocated slots in the stack state.
>
> The is_spi_bounds_valid function is still kept around as it is a generic
> helper that will be useful for other objects on stack similar to dynptr
> in the future.
>
> Suggested-by: Joanne Koong <joannelkoong@gmail.com>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Acked-by: Joanne Koong <joannelkoong@gmail.com>
> ---
> kernel/bpf/verifier.c | 75 +++++++++++++++++++------------------------
> 1 file changed, 33 insertions(+), 42 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 4feaddd5d6dc..18b54b219fac 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -643,6 +643,28 @@ static int __get_spi(s32 off)
> return (-off - 1) / BPF_REG_SIZE;
> }
>
> +static struct bpf_func_state *func(struct bpf_verifier_env *env,
> + const struct bpf_reg_state *reg)
> +{
> + struct bpf_verifier_state *cur = env->cur_state;
> +
> + return cur->frame[reg->frameno];
> +}
> +
> +static bool is_spi_bounds_valid(struct bpf_func_state *state, int spi, int nr_slots)
> +{
> + int allocated_slots = state->allocated_stack / BPF_REG_SIZE;
> +
> + /* We need to check that slots between [spi - nr_slots + 1, spi] are
> + * within [0, allocated_stack).
> + *
> + * Please note that the spi grows downwards. For example, a dynptr
> + * takes the size of two stack slots; the first slot will be at
> + * spi and the second slot will be at spi - 1.
> + */
> + return spi - nr_slots + 1 >= 0 && spi < allocated_slots;
> +}
> +
> static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> {
> int off, spi;
> @@ -663,29 +685,10 @@ static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *re
> verbose(env, "cannot pass in dynptr at an offset=%d\n", off);
> return -EINVAL;
> }
> - return spi;
> -}
> -
> -static bool is_spi_bounds_valid(struct bpf_func_state *state, int spi, int nr_slots)
> -{
> - int allocated_slots = state->allocated_stack / BPF_REG_SIZE;
>
> - /* We need to check that slots between [spi - nr_slots + 1, spi] are
> - * within [0, allocated_stack).
> - *
> - * Please note that the spi grows downwards. For example, a dynptr
> - * takes the size of two stack slots; the first slot will be at
> - * spi and the second slot will be at spi - 1.
> - */
> - return spi - nr_slots + 1 >= 0 && spi < allocated_slots;
> -}
> -
> -static struct bpf_func_state *func(struct bpf_verifier_env *env,
> - const struct bpf_reg_state *reg)
> -{
> - struct bpf_verifier_state *cur = env->cur_state;
> -
> - return cur->frame[reg->frameno];
> + if (!is_spi_bounds_valid(func(env, reg), spi, BPF_DYNPTR_NR_SLOTS))
> + return -ERANGE;
> + return spi;
> }
>
> static const char *kernel_type_name(const struct btf* btf, u32 id)
> @@ -783,9 +786,6 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
> if (spi < 0)
> return spi;
>
> - if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS))
> - return -EINVAL;
> -
> /* We cannot assume both spi and spi - 1 belong to the same dynptr,
> * hence we need to call destroy_if_dynptr_stack_slot twice for both,
> * to ensure that for the following example:
> @@ -839,9 +839,6 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re
> if (spi < 0)
> return spi;
>
> - if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS))
> - return -EINVAL;
> -
> for (i = 0; i < BPF_REG_SIZE; i++) {
> state->stack[spi].slot_type[i] = STACK_INVALID;
> state->stack[spi - 1].slot_type[i] = STACK_INVALID;
> @@ -946,20 +943,18 @@ static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env,
>
> static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> {
> - struct bpf_func_state *state = func(env, reg);
> int spi;
>
> if (reg->type == CONST_PTR_TO_DYNPTR)
> return false;
>
> spi = dynptr_get_spi(env, reg);
> + /* For -ERANGE (i.e. spi not falling into allocated stack slots), we
> + * will do check_mem_access to check and update stack bounds later, so
> + * return true for that case.
> + */
> if (spi < 0)
> - return false;
> -
> - /* We will do check_mem_access to check and update stack bounds later */
> - if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS))
> - return true;
> -
> + return spi == -ERANGE;
> /* We allow overwriting existing unreferenced STACK_DYNPTR slots, see
> * mark_stack_slots_dynptr which calls destroy_if_dynptr_stack_slot to
> * ensure dynptr objects at the slots we are touching are completely
> @@ -983,8 +978,7 @@ static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_re
> spi = dynptr_get_spi(env, reg);
> if (spi < 0)
> return false;
> - if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS) ||
> - !state->stack[spi].spilled_ptr.dynptr.first_slot)
> + if (!state->stack[spi].spilled_ptr.dynptr.first_slot)
> return false;
>
> for (i = 0; i < BPF_REG_SIZE; i++) {
> @@ -6153,7 +6147,7 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno,
> */
> if (reg->type == PTR_TO_STACK) {
> err = dynptr_get_spi(env, reg);
> - if (err < 0)
> + if (err < 0 && err != -ERANGE)
> return err;
> }
>
> @@ -6646,10 +6640,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> */
> if (reg->type == PTR_TO_STACK) {
> spi = dynptr_get_spi(env, reg);
> - if (spi < 0)
> - return spi;
> - if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS) ||
> - !state->stack[spi].spilled_ptr.ref_obj_id) {
> + if (spi < 0 || !state->stack[spi].spilled_ptr.ref_obj_id) {
> verbose(env, "arg %d is an unacquired reference\n", regno);
> return -EINVAL;
> }
> --
> 2.39.1
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 06/11] bpf: Avoid recomputing spi in process_dynptr_func
2023-01-19 2:14 ` [PATCH bpf-next v2 06/11] bpf: Avoid recomputing spi in process_dynptr_func Kumar Kartikeya Dwivedi
@ 2023-01-19 22:35 ` Joanne Koong
0 siblings, 0 replies; 22+ messages in thread
From: Joanne Koong @ 2023-01-19 22:35 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, David Vernet, Eduard Zingerman
On Wed, Jan 18, 2023 at 6:15 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> Currently, process_dynptr_func first calls dynptr_get_spi and then
> is_dynptr_reg_valid_init and is_dynptr_reg_valid_uninit have to call it
> again to obtain the spi value. Instead of doing this twice, reuse the
> already obtained value (which is by default 0, and is only set for
> PTR_TO_STACK, and only used in that case in aforementioned functions).
> The input value for these two functions will either be -ERANGE or >= 1,
> and can either be permitted or rejected based on the respective check.
>
> Suggested-by: Joanne Koong <joannelkoong@gmail.com>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Acked-by: Joanne Koong <joannelkoong@gmail.com>
> ---
> kernel/bpf/verifier.c | 24 +++++++++++-------------
> 1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 18b54b219fac..7b8de84568a3 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -941,14 +941,12 @@ static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env,
> return 0;
> }
>
> -static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> +static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> + int spi)
> {
> - int spi;
> -
> if (reg->type == CONST_PTR_TO_DYNPTR)
> return false;
>
> - spi = dynptr_get_spi(env, reg);
> /* For -ERANGE (i.e. spi not falling into allocated stack slots), we
> * will do check_mem_access to check and update stack bounds later, so
> * return true for that case.
> @@ -966,16 +964,16 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_
> return true;
> }
>
> -static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> +static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> + int spi)
> {
> struct bpf_func_state *state = func(env, reg);
> - int spi, i;
> + int i;
>
> /* This already represents first slot of initialized bpf_dynptr */
> if (reg->type == CONST_PTR_TO_DYNPTR)
> return true;
>
> - spi = dynptr_get_spi(env, reg);
> if (spi < 0)
> return false;
> if (!state->stack[spi].spilled_ptr.dynptr.first_slot)
> @@ -6132,7 +6130,7 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno,
> enum bpf_arg_type arg_type, struct bpf_call_arg_meta *meta)
> {
> struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno];
> - int err;
> + int err, spi = 0;
>
> /* MEM_UNINIT and MEM_RDONLY are exclusive, when applied to an
> * ARG_PTR_TO_DYNPTR (or ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_*):
> @@ -6146,9 +6144,9 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno,
> * and its alignment for PTR_TO_STACK.
> */
> if (reg->type == PTR_TO_STACK) {
> - err = dynptr_get_spi(env, reg);
> - if (err < 0 && err != -ERANGE)
> - return err;
> + spi = dynptr_get_spi(env, reg);
> + if (spi < 0 && spi != -ERANGE)
> + return spi;
> }
>
> /* MEM_UNINIT - Points to memory that is an appropriate candidate for
> @@ -6167,7 +6165,7 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno,
> * to.
> */
> if (arg_type & MEM_UNINIT) {
> - if (!is_dynptr_reg_valid_uninit(env, reg)) {
> + if (!is_dynptr_reg_valid_uninit(env, reg, spi)) {
> verbose(env, "Dynptr has to be an uninitialized dynptr\n");
> return -EINVAL;
> }
> @@ -6188,7 +6186,7 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno,
> return -EINVAL;
> }
>
> - if (!is_dynptr_reg_valid_init(env, reg)) {
> + if (!is_dynptr_reg_valid_init(env, reg, spi)) {
> verbose(env,
> "Expected an initialized dynptr as arg #%d\n",
> regno);
> --
> 2.39.1
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 09/11] selftests/bpf: Add dynptr var_off tests
2023-01-19 2:14 ` [PATCH bpf-next v2 09/11] selftests/bpf: Add dynptr var_off tests Kumar Kartikeya Dwivedi
@ 2023-01-19 22:49 ` Joanne Koong
2023-01-20 1:59 ` Kumar Kartikeya Dwivedi
0 siblings, 1 reply; 22+ messages in thread
From: Joanne Koong @ 2023-01-19 22:49 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, David Vernet, Eduard Zingerman
On Wed, Jan 18, 2023 at 6:15 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> Ensure that variable offset is handled correctly, and verifier takes
> both fixed and variable part into account. Also ensures that only
> constant var_off is allowed.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
> .../testing/selftests/bpf/progs/dynptr_fail.c | 40 +++++++++++++++++++
> 1 file changed, 40 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
> index 023b3c36bc78..063d351f327a 100644
> --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
> +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
> @@ -794,3 +794,43 @@ int dynptr_pruning_type_confusion(struct __sk_buff *ctx)
> );
> return 0;
> }
> +
> +SEC("?tc")
> +__failure __msg("dynptr has to be at the constant offset") __log_level(2)
> +int dynptr_var_off_overwrite(struct __sk_buff *ctx)
> +{
> + asm volatile (
> + "r9 = 16;"
> + "*(u32 *)(r10 - 4) = r9;"
> + "r8 = *(u32 *)(r10 - 4);"
> + "if r8 >= 0 goto vjmp1;"
> + "r0 = 1;"
> + "exit;"
> + "vjmp1:"
> + "if r8 <= 16 goto vjmp2;"
> + "r0 = 1;"
> + "exit;"
> + "vjmp2:"
> + "r8 &= 16;"
> + "r1 = %[ringbuf] ll;"
> + "r2 = 8;"
> + "r3 = 0;"
> + "r4 = r10;"
> + "r4 += -32;"
> + "r4 += r8;"
> + "call %[bpf_ringbuf_reserve_dynptr];"
> + "r9 = 0xeB9F;"
> + "*(u64 *)(r10 - 16) = r9;"
> + "r1 = r10;"
> + "r1 += -32;"
> + "r1 += r8;"
> + "r2 = 0;"
> + "call %[bpf_ringbuf_discard_dynptr];"
> + :
> + : __imm(bpf_ringbuf_reserve_dynptr),
> + __imm(bpf_ringbuf_discard_dynptr),
> + __imm_addr(ringbuf)
> + : __clobber_all
> + );
> + return 0;
> +}
Thanks for adding these series of tests. From a readibility
perspective, are we able to simulate these tests in C instead of
assembly?
> --
> 2.39.1
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 09/11] selftests/bpf: Add dynptr var_off tests
2023-01-19 22:49 ` Joanne Koong
@ 2023-01-20 1:59 ` Kumar Kartikeya Dwivedi
0 siblings, 0 replies; 22+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2023-01-20 1:59 UTC (permalink / raw)
To: Joanne Koong
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, David Vernet, Eduard Zingerman
On Fri, Jan 20, 2023 at 04:19:31AM IST, Joanne Koong wrote:
> On Wed, Jan 18, 2023 at 6:15 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > Ensure that variable offset is handled correctly, and verifier takes
> > both fixed and variable part into account. Also ensures that only
> > constant var_off is allowed.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> > .../testing/selftests/bpf/progs/dynptr_fail.c | 40 +++++++++++++++++++
> > 1 file changed, 40 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
> > index 023b3c36bc78..063d351f327a 100644
> > --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
> > +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
> > @@ -794,3 +794,43 @@ int dynptr_pruning_type_confusion(struct __sk_buff *ctx)
> > );
> > return 0;
> > }
> > +
> > +SEC("?tc")
> > +__failure __msg("dynptr has to be at the constant offset") __log_level(2)
> > +int dynptr_var_off_overwrite(struct __sk_buff *ctx)
> > +{
> > + asm volatile (
> > + "r9 = 16;"
> > + "*(u32 *)(r10 - 4) = r9;"
> > + "r8 = *(u32 *)(r10 - 4);"
> > + "if r8 >= 0 goto vjmp1;"
> > + "r0 = 1;"
> > + "exit;"
> > + "vjmp1:"
> > + "if r8 <= 16 goto vjmp2;"
> > + "r0 = 1;"
> > + "exit;"
> > + "vjmp2:"
> > + "r8 &= 16;"
> > + "r1 = %[ringbuf] ll;"
> > + "r2 = 8;"
> > + "r3 = 0;"
> > + "r4 = r10;"
> > + "r4 += -32;"
> > + "r4 += r8;"
> > + "call %[bpf_ringbuf_reserve_dynptr];"
> > + "r9 = 0xeB9F;"
> > + "*(u64 *)(r10 - 16) = r9;"
> > + "r1 = r10;"
> > + "r1 += -32;"
> > + "r1 += r8;"
> > + "r2 = 0;"
> > + "call %[bpf_ringbuf_discard_dynptr];"
> > + :
> > + : __imm(bpf_ringbuf_reserve_dynptr),
> > + __imm(bpf_ringbuf_discard_dynptr),
> > + __imm_addr(ringbuf)
> > + : __clobber_all
> > + );
> > + return 0;
> > +}
>
> Thanks for adding these series of tests. From a readibility
> perspective, are we able to simulate these tests in C instead of
> assembly?
It should be possible, but I went with assembly for two reasons:
- In one of the tests extra instructions are emitted at a specific place in the
program to trigger add_new_state heuristic of the verifier. I was having
trouble making this work when initially trying to trigger the bug in C.
- Preventing compiler changes from changing the desired BPF assembly that should
be produced over time. I'm mostly worried about these changes happening
silently and the test becoming useless over time without being able to detect
so. Right now you can take these ASM tests and run them on an unpatched kernel
and it will successfully load the program (on running crash it).
In the last patch I still added C tests as those shouldn't need assembly.
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2023-01-20 2:00 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-19 2:14 [PATCH bpf-next v2 00/11] Dynptr fixes Kumar Kartikeya Dwivedi
2023-01-19 2:14 ` [PATCH bpf-next v2 01/11] bpf: Fix state pruning for STACK_DYNPTR stack slots Kumar Kartikeya Dwivedi
2023-01-19 2:14 ` [PATCH bpf-next v2 02/11] bpf: Fix missing var_off check for ARG_PTR_TO_DYNPTR Kumar Kartikeya Dwivedi
2023-01-19 22:00 ` Joanne Koong
2023-01-19 22:14 ` Kumar Kartikeya Dwivedi
2023-01-19 22:25 ` Joanne Koong
2023-01-19 2:14 ` [PATCH bpf-next v2 03/11] bpf: Fix partial dynptr stack slot reads/writes Kumar Kartikeya Dwivedi
2023-01-19 22:06 ` Joanne Koong
2023-01-19 22:15 ` Kumar Kartikeya Dwivedi
2023-01-19 2:14 ` [PATCH bpf-next v2 04/11] bpf: Allow reinitializing unreferenced dynptr stack slots Kumar Kartikeya Dwivedi
2023-01-19 22:13 ` Joanne Koong
2023-01-19 2:14 ` [PATCH bpf-next v2 05/11] bpf: Combine dynptr_get_spi and is_spi_bounds_valid Kumar Kartikeya Dwivedi
2023-01-19 22:30 ` Joanne Koong
2023-01-19 2:14 ` [PATCH bpf-next v2 06/11] bpf: Avoid recomputing spi in process_dynptr_func Kumar Kartikeya Dwivedi
2023-01-19 22:35 ` Joanne Koong
2023-01-19 2:14 ` [PATCH bpf-next v2 07/11] selftests/bpf: convenience macro for use with 'asm volatile' blocks Kumar Kartikeya Dwivedi
2023-01-19 2:14 ` [PATCH bpf-next v2 08/11] selftests/bpf: Add dynptr pruning tests Kumar Kartikeya Dwivedi
2023-01-19 2:14 ` [PATCH bpf-next v2 09/11] selftests/bpf: Add dynptr var_off tests Kumar Kartikeya Dwivedi
2023-01-19 22:49 ` Joanne Koong
2023-01-20 1:59 ` Kumar Kartikeya Dwivedi
2023-01-19 2:14 ` [PATCH bpf-next v2 10/11] selftests/bpf: Add dynptr partial slot overwrite tests Kumar Kartikeya Dwivedi
2023-01-19 2:14 ` [PATCH bpf-next v2 11/11] selftests/bpf: Add dynptr helper tests Kumar Kartikeya Dwivedi
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).