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

Commit Message

Juntong Deng Aug. 19, 2024, 10:13 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.

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

Signed-off-by: Juntong Deng <[e-mail]>
---
v1 -> v2: Handle KF_RCU_PROTECTED case and add corresponding test cases

 kernel/bpf/verifier.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Alexei Starovoitov Aug. 21, 2024, 8:53 p.m. UTC | #1

On Mon, Aug 19, 2024 at 3:16 AM Juntong Deng <[e-mail]> wrote:
>
> 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.
>
> The pointer returned by iter next method of other types of iterators
> is with PTR_TRUSTED.
>
> Signed-off-by: Juntong Deng <[e-mail]>
> ---
> v1 -> v2: Handle KF_RCU_PROTECTED case and add corresponding test cases
>
>  kernel/bpf/verifier.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index ebec74c28ae3..d083925c2ba8 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -8233,6 +8233,12 @@ static int process_iter_next_call(struct bpf_verifier_env *env, int insn_idx,
>                         verbose(env, "bug: bad parent state for iter next call");
>                         return -EFAULT;
>                 }
> +
> +               if (cur_iter->type & MEM_RCU) /* KF_RCU_PROTECTED */
> +                       cur_fr->regs[BPF_REG_0].type |= MEM_RCU;
> +               else
> +                       cur_fr->regs[BPF_REG_0].type |= PTR_TRUSTED;
> +

That's an odd place to make such an adjustment.
check_kfunc_call() would fit much better.
That's where r0.type is typically set.

Also, the above is buggy for num iter.
check_kfunc_call() would set r0.type = PTR_TO_MEM for that iter,
since it's proto: int *bpf_iter_num_next(struct bpf_iter_num* it)
but above logic would slap PTR_TRUSTED on top.
PTR_TO_MEM | PTR_TRUSTED is invalid combination.
I'm surprised nothing crashed.

pw-bot: cr

Juntong Deng Aug. 29, 2024, 10:52 a.m. UTC | #2

On 8/21/24 21:53, Alexei Starovoitov wrote:
> On Mon, Aug 19, 2024 at 3:16 AM Juntong Deng <[e-mail]> wrote:
>>
>> 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.
>>
>> The pointer returned by iter next method of other types of iterators
>> is with PTR_TRUSTED.
>>
>> Signed-off-by: Juntong Deng <[e-mail]>
>> ---
>> v1 -> v2: Handle KF_RCU_PROTECTED case and add corresponding test cases
>>
>>   kernel/bpf/verifier.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index ebec74c28ae3..d083925c2ba8 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -8233,6 +8233,12 @@ static int process_iter_next_call(struct bpf_verifier_env *env, int insn_idx,
>>                          verbose(env, "bug: bad parent state for iter next call");
>>                          return -EFAULT;
>>                  }
>> +
>> +               if (cur_iter->type & MEM_RCU) /* KF_RCU_PROTECTED */
>> +                       cur_fr->regs[BPF_REG_0].type |= MEM_RCU;
>> +               else
>> +                       cur_fr->regs[BPF_REG_0].type |= PTR_TRUSTED;
>> +
> 
> That's an odd place to make such an adjustment.
> check_kfunc_call() would fit much better.
> That's where r0.type is typically set.
> 
> Also, the above is buggy for num iter.
> check_kfunc_call() would set r0.type = PTR_TO_MEM for that iter,
> since it's proto: int *bpf_iter_num_next(struct bpf_iter_num* it)
> but above logic would slap PTR_TRUSTED on top.
> PTR_TO_MEM | PTR_TRUSTED is invalid combination.
> I'm surprised nothing crashed.
> 
> pw-bot: cr

I fixed the above issues and added corresponding tests.

I sent the version 3 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 ebec74c28ae3..d083925c2ba8 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -8233,6 +8233,12 @@  static int process_iter_next_call(struct bpf_verifier_env *env, int insn_idx,
 			verbose(env, "bug: bad parent state for iter next call");
 			return -EFAULT;
 		}
+
+		if (cur_iter->type & MEM_RCU) /* KF_RCU_PROTECTED */
+			cur_fr->regs[BPF_REG_0].type |= MEM_RCU;
+		else
+			cur_fr->regs[BPF_REG_0].type |= PTR_TRUSTED;
+
 		/* Note cur_st->parent in the call below, it is necessary to skip
 		 * checkpoint created for cur_st by is_state_visited()
 		 * right at this instruction.