[bpf-next,v3,1/2] bpf: Make the pointer returned by iter next method valid

Commit Message

Juntong Deng Aug. 29, 2024, 10:44 a.m. UTC

Currently we cannot pass the pointer returned by iter next method as
argument to KF_TRUSTED_ARGS or KF_RCU kfuncs, because the pointer
returned by iter next method is not "valid".

This patch sets the pointer returned by iter next method to be valid.

This is based on the fact that if the iterator is implemented correctly,
then the pointer returned from the iter next method should be valid.

This does not make NULL pointer valid. If the iter next method has
KF_RET_NULL flag, then the verifier will ask the ebpf program to
check NULL pointer.

KF_RCU_PROTECTED iterator is a special case, the pointer returned by
iter next method should only be valid within RCU critical section,
so it should be with MEM_RCU, not PTR_TRUSTED.

Another special case is bpf_iter_num_next, which returns a pointer with
base type PTR_TO_MEM. PTR_TO_MEM should not be combined with type flag
PTR_TRUSTED (PTR_TO_MEM already means the pointer is valid).

The pointer returned by iter next method of other types of iterators
is with PTR_TRUSTED.

In addition, this patch adds get_iter_from_state to help us get the
current iterator from the current state.

Signed-off-by: Juntong Deng <[e-mail]>
---
v2 -> v3: Move modifications to check_kfunc_call. Handle PTR_TO_MEM case
and add corresponding test case. Add get_iter_from_state.

v1 -> v2: Handle KF_RCU_PROTECTED case and add corresponding test cases

 kernel/bpf/verifier.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

Comments

Alexei Starovoitov Aug. 29, 2024, 6:37 p.m. UTC | #1

On Thu, Aug 29, 2024 at 3:45 AM Juntong Deng <[e-mail]> wrote:
>
>                 if (prev_st)
> @@ -12860,6 +12867,16 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>                         /* For mark_ptr_or_null_reg, see 93c230e3f5bd6 */
>                         regs[BPF_REG_0].id = ++env->id_gen;
>                 }
> +
> +               if (is_iter_next_kfunc(&meta) && base_type(regs[BPF_REG_0].type) != PTR_TO_MEM) {

The != PTR_TO_MEM part is a bit ugly.

Why not do it in {} scope right above?
Just move it up by a few lines?
Right after regs[BPF_REG_0].type = PTR_TO_BTF_ID;

Juntong Deng Aug. 29, 2024, 8:18 p.m. UTC | #2

On 8/29/24 19:37, Alexei Starovoitov wrote:
> On Thu, Aug 29, 2024 at 3:45 AM Juntong Deng <[e-mail]> wrote:
>>
>>                  if (prev_st)
>> @@ -12860,6 +12867,16 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>                          /* For mark_ptr_or_null_reg, see 93c230e3f5bd6 */
>>                          regs[BPF_REG_0].id = ++env->id_gen;
>>                  }
>> +
>> +               if (is_iter_next_kfunc(&meta) && base_type(regs[BPF_REG_0].type) != PTR_TO_MEM) {
> 
> The != PTR_TO_MEM part is a bit ugly.
> 
> Why not do it in {} scope right above?
> Just move it up by a few lines?
> Right after regs[BPF_REG_0].type = PTR_TO_BTF_ID;

You are right, I eliminated the != PTR_TO_MEM part and sent the
version 4 patch set:

https://lore.kernel.org/bpf/[e-mail]/T/#t

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index f32e3b9bb4e5..bc146671742c 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -8148,6 +8148,15 @@  static int widen_imprecise_scalars(struct bpf_verifier_env *env,
 	return 0;
 }
 
+static struct bpf_reg_state *get_iter_from_state(struct bpf_verifier_state *cur_st,
+						 struct bpf_kfunc_call_arg_meta *meta)
+{
+	int iter_frameno = meta->iter.frameno;
+	int iter_spi = meta->iter.spi;
+
+	return &cur_st->frame[iter_frameno]->stack[iter_spi].spilled_ptr;
+}
+
 /* process_iter_next_call() is called when verifier gets to iterator's next
  * "method" (e.g., bpf_iter_num_next() for numbers iterator) call. We'll refer
  * to it as just "iter_next()" in comments below.
@@ -8232,12 +8241,10 @@  static int process_iter_next_call(struct bpf_verifier_env *env, int insn_idx,
 	struct bpf_verifier_state *cur_st = env->cur_state, *queued_st, *prev_st;
 	struct bpf_func_state *cur_fr = cur_st->frame[cur_st->curframe], *queued_fr;
 	struct bpf_reg_state *cur_iter, *queued_iter;
-	int iter_frameno = meta->iter.frameno;
-	int iter_spi = meta->iter.spi;
 
 	BTF_TYPE_EMIT(struct bpf_iter);
 
-	cur_iter = &env->cur_state->frame[iter_frameno]->stack[iter_spi].spilled_ptr;
+	cur_iter = get_iter_from_state(cur_st, meta);
 
 	if (cur_iter->iter.state != BPF_ITER_STATE_ACTIVE &&
 	    cur_iter->iter.state != BPF_ITER_STATE_DRAINED) {
@@ -8265,7 +8272,7 @@  static int process_iter_next_call(struct bpf_verifier_env *env, int insn_idx,
 		if (!queued_st)
 			return -ENOMEM;
 
-		queued_iter = &queued_st->frame[iter_frameno]->stack[iter_spi].spilled_ptr;
+		queued_iter = get_iter_from_state(queued_st, meta);
 		queued_iter->iter.state = BPF_ITER_STATE_ACTIVE;
 		queued_iter->iter.depth++;
 		if (prev_st)
@@ -12860,6 +12867,16 @@  static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 			/* For mark_ptr_or_null_reg, see 93c230e3f5bd6 */
 			regs[BPF_REG_0].id = ++env->id_gen;
 		}
+
+		if (is_iter_next_kfunc(&meta) && base_type(regs[BPF_REG_0].type) != PTR_TO_MEM) {
+			struct bpf_reg_state *cur_iter = get_iter_from_state(env->cur_state, &meta);
+
+			if (cur_iter->type & MEM_RCU) /* KF_RCU_PROTECTED */
+				regs[BPF_REG_0].type |= MEM_RCU;
+			else
+				regs[BPF_REG_0].type |= PTR_TRUSTED;
+		}
+
 		mark_btf_func_reg_size(env, BPF_REG_0, sizeof(void *));
 		if (is_kfunc_acquire(&meta)) {
 			int id = acquire_reference_state(env, insn_idx);