Re: [RFC bpf-next 2/5] bpf: Handle BPF_KPTR_USER in verifier.

[Kui-Feng Lee <sinquersw@gmail.com>]

On 8/13/24 12:35, Alexei Starovoitov wrote:
> On Tue, Aug 13, 2024 at 9:52 AM Kui-Feng Lee <sinquersw@gmail.com> wrote:
>>
>>
>>
>> On 8/12/24 09:48, Alexei Starovoitov wrote:
>>> On Wed, Aug 7, 2024 at 4:58 PM Kui-Feng Lee <thinker.li@gmail.com> 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@gmail.com>
>>>> ---
>>>>    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.

Make sense!