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

From: Yonghong Song
Date: Tue Nov 02 2021 - 19:16:15 EST




On 11/2/21 4:03 PM, Florent Revest wrote:
On Tue, Nov 2, 2021 at 5:06 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:

On Mon, Nov 1, 2021 at 8:20 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:

On Mon, Nov 1, 2021 at 8:16 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> 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!