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

Commit Message

Juntong Deng Aug. 15, 2024, 4:10 p.m. UTC

Currently we cannot pass the pointer returned by iter next method as
argument to KF_TRUSTED_ARGS 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.

Signed-off-by: Juntong Deng <[e-mail]>
---
 kernel/bpf/verifier.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Andrii Nakryiko Aug. 15, 2024, 5:15 p.m. UTC | #1

On Thu, Aug 15, 2024 at 9:11 AM Juntong Deng <[e-mail]> wrote:
>
> Currently we cannot pass the pointer returned by iter next method as
> argument to KF_TRUSTED_ARGS 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.
>
> Signed-off-by: Juntong Deng <[e-mail]>
> ---
>  kernel/bpf/verifier.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index ebec74c28ae3..35a7b7c6679c 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -12832,6 +12832,10 @@ 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))
> +                       regs[BPF_REG_0].type |= PTR_TRUSTED;
> +

It seems a bit too generic to always assign PTR_TRUSTED to anything
returned from any iterator. Let's maybe add KF_RET_TRUSTED or
KF_ITER_TRUSTED or something along those lines to mark such iter_next
kfuncs explicitly?

For the numbers iterator, for instance, this PTR_TRUSTED makes no sense.

>                 mark_btf_func_reg_size(env, BPF_REG_0, sizeof(void *));
>                 if (is_kfunc_acquire(&meta)) {
>                         int id = acquire_reference_state(env, insn_idx);
> --
> 2.39.2
>

Juntong Deng Aug. 16, 2024, 1:42 p.m. UTC | #2

On 8/15/24 18:15, Andrii Nakryiko wrote:
> On Thu, Aug 15, 2024 at 9:11 AM Juntong Deng <[e-mail]> wrote:
>>
>> Currently we cannot pass the pointer returned by iter next method as
>> argument to KF_TRUSTED_ARGS 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.
>>
>> Signed-off-by: Juntong Deng <[e-mail]>
>> ---
>>   kernel/bpf/verifier.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index ebec74c28ae3..35a7b7c6679c 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -12832,6 +12832,10 @@ 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))
>> +                       regs[BPF_REG_0].type |= PTR_TRUSTED;
>> +
> 
> It seems a bit too generic to always assign PTR_TRUSTED to anything
> returned from any iterator. Let's maybe add KF_RET_TRUSTED or
> KF_ITER_TRUSTED or something along those lines to mark such iter_next
> kfuncs explicitly?
> 
> For the numbers iterator, for instance, this PTR_TRUSTED makes no sense.
> 

I had the same idea (KF_RET_TRUSTED) before, but Kumar thought it should
be avoided and pointers returned by iter next method should be trusted
by default [0].

The following are previous related discussions:

 >> For iter_next(), I currently have an idea to add new flags to allow
 >> iter_next() to decide whether the return value is trusted or not,
 >> such as KF_RET_TRUSTED.
 >>
 >> What do you think?
 >
 > Why shouldn't the return value always be trusted?
 > We eventually want to switch over to trusted by default everywhere.
 > It would be nice not to go further in the opposite direction (i.e.
 > having to manually annotate trusted) if we can avoid it.

[0]: 
https://lore.kernel.org/bpf/CAP01T75na=[e-mail]/

Maybe we can have more discussion?

(This email has been CC Kumar)

>>                  mark_btf_func_reg_size(env, BPF_REG_0, sizeof(void *));
>>                  if (is_kfunc_acquire(&meta)) {
>>                          int id = acquire_reference_state(env, insn_idx);
>> --
>> 2.39.2
>>

Alexei Starovoitov Aug. 16, 2024, 3:39 p.m. UTC | #3

On Fri, Aug 16, 2024 at 3:43 PM Juntong Deng <[e-mail]> wrote:
>
> On 8/15/24 18:15, Andrii Nakryiko wrote:
> > On Thu, Aug 15, 2024 at 9:11 AM Juntong Deng <[e-mail]> wrote:
> >>
> >> Currently we cannot pass the pointer returned by iter next method as
> >> argument to KF_TRUSTED_ARGS 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.
> >>
> >> Signed-off-by: Juntong Deng <[e-mail]>
> >> ---
> >>   kernel/bpf/verifier.c | 4 ++++
> >>   1 file changed, 4 insertions(+)
> >>
> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >> index ebec74c28ae3..35a7b7c6679c 100644
> >> --- a/kernel/bpf/verifier.c
> >> +++ b/kernel/bpf/verifier.c
> >> @@ -12832,6 +12832,10 @@ 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))
> >> +                       regs[BPF_REG_0].type |= PTR_TRUSTED;
> >> +
> >
> > It seems a bit too generic to always assign PTR_TRUSTED to anything
> > returned from any iterator. Let's maybe add KF_RET_TRUSTED or
> > KF_ITER_TRUSTED or something along those lines to mark such iter_next
> > kfuncs explicitly?
> >
> > For the numbers iterator, for instance, this PTR_TRUSTED makes no sense.
> >
>
> I had the same idea (KF_RET_TRUSTED) before, but Kumar thought it should
> be avoided and pointers returned by iter next method should be trusted
> by default [0].
>
> The following are previous related discussions:
>
>  >> For iter_next(), I currently have an idea to add new flags to allow
>  >> iter_next() to decide whether the return value is trusted or not,
>  >> such as KF_RET_TRUSTED.
>  >>
>  >> What do you think?
>  >
>  > Why shouldn't the return value always be trusted?
>  > We eventually want to switch over to trusted by default everywhere.
>  > It would be nice not to go further in the opposite direction (i.e.
>  > having to manually annotate trusted) if we can avoid it.
>
> [0]:
> https://lore.kernel.org/bpf/CAP01T75na=[e-mail]/
>
> Maybe we can have more discussion?
>
> (This email has been CC Kumar)

+1
pointer from iterator should always be trusted except
the case of KF_RCU_PROTECTED iterators.
Those iters clear iter itself outside of RCU CS,
so a pointer returned from iter_next should probably be
PTR_TO_BTF_ID | MEM_RCU | PTR_MAYBE_NULL.

For all other iters it should be safe to return
PTR_TO_BTF_ID | PTR_TRUSTED | PTR_MAYBE_NULL

> For the numbers iterator, for instance, this PTR_TRUSTED makes no sense

I see no conflict. It's a trusted pointer to u32.

Andrii Nakryiko Aug. 19, 2024, 4:24 p.m. UTC | #4

On Fri, Aug 16, 2024 at 8:39 AM Alexei Starovoitov
<[e-mail]> wrote:
>
> On Fri, Aug 16, 2024 at 3:43 PM Juntong Deng <[e-mail]> wrote:
> >
> > On 8/15/24 18:15, Andrii Nakryiko wrote:
> > > On Thu, Aug 15, 2024 at 9:11 AM Juntong Deng <[e-mail]> wrote:
> > >>
> > >> Currently we cannot pass the pointer returned by iter next method as
> > >> argument to KF_TRUSTED_ARGS 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.
> > >>
> > >> Signed-off-by: Juntong Deng <[e-mail]>
> > >> ---
> > >>   kernel/bpf/verifier.c | 4 ++++
> > >>   1 file changed, 4 insertions(+)
> > >>
> > >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > >> index ebec74c28ae3..35a7b7c6679c 100644
> > >> --- a/kernel/bpf/verifier.c
> > >> +++ b/kernel/bpf/verifier.c
> > >> @@ -12832,6 +12832,10 @@ 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))
> > >> +                       regs[BPF_REG_0].type |= PTR_TRUSTED;
> > >> +
> > >
> > > It seems a bit too generic to always assign PTR_TRUSTED to anything
> > > returned from any iterator. Let's maybe add KF_RET_TRUSTED or
> > > KF_ITER_TRUSTED or something along those lines to mark such iter_next
> > > kfuncs explicitly?
> > >
> > > For the numbers iterator, for instance, this PTR_TRUSTED makes no sense.
> > >
> >
> > I had the same idea (KF_RET_TRUSTED) before, but Kumar thought it should
> > be avoided and pointers returned by iter next method should be trusted
> > by default [0].
> >
> > The following are previous related discussions:
> >
> >  >> For iter_next(), I currently have an idea to add new flags to allow
> >  >> iter_next() to decide whether the return value is trusted or not,
> >  >> such as KF_RET_TRUSTED.
> >  >>
> >  >> What do you think?
> >  >
> >  > Why shouldn't the return value always be trusted?
> >  > We eventually want to switch over to trusted by default everywhere.
> >  > It would be nice not to go further in the opposite direction (i.e.
> >  > having to manually annotate trusted) if we can avoid it.
> >
> > [0]:
> > https://lore.kernel.org/bpf/CAP01T75na=[e-mail]/
> >
> > Maybe we can have more discussion?
> >
> > (This email has been CC Kumar)
>
> +1
> pointer from iterator should always be trusted except
> the case of KF_RCU_PROTECTED iterators.
> Those iters clear iter itself outside of RCU CS,
> so a pointer returned from iter_next should probably be
> PTR_TO_BTF_ID | MEM_RCU | PTR_MAYBE_NULL.
>
> For all other iters it should be safe to return
> PTR_TO_BTF_ID | PTR_TRUSTED | PTR_MAYBE_NULL
>

Ok, but we at some point might need to return a non-RCU/non-trusted
pointer, so I guess we'll have to add yet another flag to opt-out of
"trustedness"?

> > For the numbers iterator, for instance, this PTR_TRUSTED makes no sense
>
> I see no conflict. It's a trusted pointer to u32.

Alexei Starovoitov Aug. 20, 2024, 6:28 a.m. UTC | #5

On Mon, Aug 19, 2024 at 6:24 PM Andrii Nakryiko
<[e-mail]> wrote:
>
> > >
> > > [0]:
> > > https://lore.kernel.org/bpf/CAP01T75na=[e-mail]/
> > >
> > > Maybe we can have more discussion?
> > >
> > > (This email has been CC Kumar)
> >
> > +1
> > pointer from iterator should always be trusted except
> > the case of KF_RCU_PROTECTED iterators.
> > Those iters clear iter itself outside of RCU CS,
> > so a pointer returned from iter_next should probably be
> > PTR_TO_BTF_ID | MEM_RCU | PTR_MAYBE_NULL.
> >
> > For all other iters it should be safe to return
> > PTR_TO_BTF_ID | PTR_TRUSTED | PTR_MAYBE_NULL
> >
>
> Ok, but we at some point might need to return a non-RCU/non-trusted
> pointer, so I guess we'll have to add yet another flag to opt-out of
> "trustedness"?

If such case ever happens then yes, we'd need a new flag,
but I don't see it's happening

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index ebec74c28ae3..35a7b7c6679c 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -12832,6 +12832,10 @@  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))
+			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);