Re: [PATCH bpf-next 1/2] bpf: Add kfuncs for read-only string operations

[Viktor Malik <vmalik@redhat.com>]

On 10/1/24 19:40, Andrii Nakryiko wrote:
> On Tue, Oct 1, 2024 at 10:34 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>>
>> On Tue, Oct 1, 2024 at 10:04 AM Andrii Nakryiko
>> <andrii.nakryiko@gmail.com> wrote:
>>>
>>> On Tue, Oct 1, 2024 at 7:48 AM Alexei Starovoitov
>>> <alexei.starovoitov@gmail.com> wrote:
>>>>
>>>> On Tue, Oct 1, 2024 at 4:26 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>>>>>
>>>>> On Mon, 2024-09-30 at 15:00 -0700, Andrii Nakryiko wrote:
>>>>>
>>>>> [...]
>>>>>
>>>>>> Right now, the only way to pass dynamically sized anything is through
>>>>>> dynptr, AFAIU.
>>>>>
>>>>> But we do have 'is_kfunc_arg_mem_size()' that checks for __sz suffix,
>>>>> e.g. used for bpf_copy_from_user_str():
>>>>>
>>>>> /**
>>>>>  * bpf_copy_from_user_str() - Copy a string from an unsafe user address
>>>>>  * @dst:             Destination address, in kernel space.  This buffer must be
>>>>>  *                   at least @dst__sz bytes long.
>>>>>  * @dst__sz:         Maximum number of bytes to copy, includes the trailing NUL.
>>>>>  * ...
>>>>>  */
>>>>> __bpf_kfunc int bpf_copy_from_user_str(void *dst, u32 dst__sz, const void __user *unsafe_ptr__ign, u64 flags)
>>>>>
>>>>> However, this suffix won't work for strnstr because of the arguments order.
>>>>
>>>> Stating the obvious... we don't need to keep the order exactly the same.
>>>>
>>>> Regarding all of these kfuncs... as Andrii pointed out 'const char *s'
>>>> means that the verifier will check that 's' points to a valid byte.
>>>> I think we can do a hybrid static + dynamic safety scheme here.
>>>> All of the kfunc signatures can stay the same, but we'd have to
>>>> open code all string helpers with __get_kernel_nofault() instead of
>>>> direct memory access.
>>>> Since the first byte is guaranteed to be valid by the verifier
>>>> we only need to make sure that the s+N bytes won't cause page faults
>>>
>>> You mean to just check that s[N-1] can be read? Given a large enough
>>> N, couldn't it be that some page between s[0] and s[N-1] still can be
>>> unmapped, defeating this check?
>>
>> Just checking s[0] and s[N-1] is not enough, obviously, and especially,
>> since the logic won't know where nul byte is, so N is unknown.
>> I meant to that all of str* kfuncs will be reading all bytes
>> via __get_kernel_nofault() until they find \0.
> 
> Ah, ok, I see what you mean now.
> 
>> It can be optimized to 8 byte access.
>> The open coding (aka copy-paste) is unfortunate, of course.
> 
> Yep, this sucks.

Yeah, that's quite annoying. I really wanted to avoid doing that. Also,
we won't be able to use arch-optimized versions of the functions.

Just to make sure I understand things correctly - can we do what Eduard
suggested and add explicit sizes for all arguments using the __sz
suffix? So something like:

    const char *bpf_strnstr(const char *s1, u32 s1__sz, const char *s2, u32 s2__sz);

Or that would still not be sufficient b/c the strings may still be
unsafe and we need an additional safety check (using
__get_kernel_nofault suggested by Alexei)?