On Tue, Aug 13, 2024 at 9:52 AM Kui-Feng Lee <sinquersw@xxxxxxxxx> wrote:
>
>
>
> On 8/12/24 09:48, Alexei Starovoitov wrote:
> > On Wed, Aug 7, 2024 at 4:58 PM Kui-Feng Lee <thinker.li@xxxxxxxxx> wrote:
> >>
> >> Give PTR_MAYBE_NULL | PTR_UNTRUSTED | MEM_ALLOC | NON_OWN_REF to kptr_user
> >> to the memory pointed by it readable and writable.
> >>
> >> Signed-off-by: Kui-Feng Lee <thinker.li@xxxxxxxxx>
> >> ---
> >> kernel/bpf/verifier.c | 11 +++++++++++
> >> 1 file changed, 11 insertions(+)
> >>
> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >> index df3be12096cf..84647e599595 100644
> >> --- a/kernel/bpf/verifier.c
> >> +++ b/kernel/bpf/verifier.c
> >> @@ -5340,6 +5340,10 @@ static int map_kptr_match_type(struct bpf_verifier_env *env,
> >> int perm_flags;
> >> const char *reg_name = "";
> >>
> >> + if (kptr_field->type == BPF_KPTR_USER)
> >> + /* BPF programs should not change any user kptr */
> >> + return -EACCES;
> >> +
> >> if (btf_is_kernel(reg->btf)) {
> >> perm_flags = PTR_MAYBE_NULL | PTR_TRUSTED | MEM_RCU;
> >>
> >> @@ -5483,6 +5487,12 @@ static u32 btf_ld_kptr_type(struct bpf_verifier_env *env, struct btf_field *kptr
> >> ret |= NON_OWN_REF;
> >> } else {
> >> ret |= PTR_UNTRUSTED;
> >> + if (kptr_field->type == BPF_KPTR_USER)
> >> + /* In oder to access directly from bpf
> >> + * programs. NON_OWN_REF make the memory
> >> + * writable. Check check_ptr_to_btf_access().
> >> + */
> >> + ret |= MEM_ALLOC | NON_OWN_REF;
> >
> > UNTRUSTED | MEM_ALLOC | NON_OWN_REF ?!
> >
> > That doesn't fit into any of the existing verifier schemes.
> > I cannot make sense of this part.
> >
> > UNTRUSTED | MEM_ALLOC is read only through exceptions logic.
> > The uptr has to be read/write through normal load/store.
>
> I will remove UNTRUSTED and leave MEM_ALLOC and NON_OWN_REF.
> Does it make sense to you?
I don't think it fits either.
MEM_ALLOC | NON_OWN_REF is specific to bpf_rbtree/linklist nodes.
There are various checks and logic like:
1.
if (!(type_is_ptr_alloc_obj(reg->type) ||
type_is_non_owning_ref(reg->type)) &&
WARN_ON_ONCE(reg->off))
return;
2.
invalidate_non_owning_refs() during unlock
that shouldn't apply in this case.
PTR_TO_MEM with specific mem_size fits better.
Since it's user/kernel shared memory PTR_TO_BTF_ID logic with field walking
won't work anyway, so opaque array of bytes is better. Which is PTR_TO_MEM.
