Re: [PATCH bpf-next] bpf: Allow bpf_d_path in perf_event_mmap

[Yonghong Song <yhs@fb.com>]

On 11/2/21 4:03 PM, Florent Revest wrote:
> On Tue, Nov 2, 2021 at 5:06 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>
>> On Mon, Nov 1, 2021 at 8:20 PM Alexei Starovoitov
>> <alexei.starovoitov@gmail.com> wrote:
>>>
>>> On Mon, Nov 1, 2021 at 8:16 PM Andrii Nakryiko
>>> <andrii.nakryiko@gmail.com> wrote:
>>>>>>
>>>>>>      FILE *vm_file = vma->vm_file; /* no checking is needed, vma from
>>>>>> parameter which is not NULL */
>>>>>>      if (vm_file)
>>>>>>        bpf_d_path(&vm_file->f_path, path, sizeof(path));
>>>>>
>>>>> That should work.
>>>>> The verifier can achieve that by marking certain fields as PTR_TO_BTF_ID_OR_NULL
>>>>> instead of PTR_TO_BTF_ID while walking such pointers.
>>>>> And then disallow pointer arithmetic on PTR_TO_BTF_ID_OR_NULL until it
>>>>> goes through 'if (Rx == NULL)' check inside the program and gets converted to
>>>>> PTR_TO_BTF_ID.
>>>>> Initially we can hard code such fields via BTF_ID(struct, file) macro.'
>>>>> So any pointer that results into a 'struct file' pointer will be
>>>>> PTR_TO_BTF_ID_OR_NULL.
> 
> Right, this is what I had in mind originally. But I was afraid this
> could maybe prevent some existing programs from loading on newer
> kernels ? Not sure if that's an issue.

This potentially could cause an regression, especially in mosts cases
the result of direct memory access is not used as the helper argument.

So the best is to add checking around helper itself.

> 
>>> The helper can check that it's [0, few_pages] and declare it's bad.
>>
>> That's basically what happens with direct memory reads, so I guess it
>> would be fine.
>>
>>> I guess we can do that and only do what I proposed for "more than a page"
>>> math on the pointer. Or even disallow "add more than a page offset to
>>> PTR_TO_BTF_ID"
>>> for now, since it will cover 99% of the cases.
> 
> Otherwise this sounds like a straightforward solution, yes :)
> Especially if this is how direct memory accesses already work.

Alternatively, the verifier can add such a check for the related helper
parameter. But looks like that adding the check inside the helper itself
is easier.

> 
> I'd be happy to look into this when I get some slack time. ;)

Thanks!