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

From: Yonghong Song
Date: Mon Nov 01 2021 - 13:32:07 EST




On 11/1/21 8:01 AM, Florent Revest wrote:
On Mon, Nov 1, 2021 at 2:17 PM Hengqi Chen <hengqi.chen@xxxxxxxxx> wrote:

Hi,


On 2021/10/30 1:02 AM, Florent Revest wrote:
On Fri, Oct 29, 2021 at 12:47 AM Martin KaFai Lau <kafai@xxxxxx> wrote:

On Thu, Oct 28, 2021 at 06:43:57PM +0200, Florent Revest wrote:
Allow the helper to be called from the perf_event_mmap hook. This is
convenient to lookup vma->vm_file and implement a similar logic as
perf_event_mmap_event in BPF.
From struct vm_area_struct:
struct file * vm_file; /* File we map to (can be NULL). */

Under perf_event_mmap, vm_file won't be NULL or bpf_d_path can handle it?

Thanks Martin, this is a very good point. :) Yes, vm_file can be NULL
in perf_event_mmap.
I wonder what would happen (and what we could do about it? :|).
bpf_d_path is called on &vma->vm_file->f_path So without NULL checks
(of vm_file) in BPF, the helper wouldn't be called with a NULL pointer
but rather with an address that is offsetof(struct file, f_path).


I tested this patch with the following BCC script:

bpf_text = '''
#include <linux/mm_types.h>

KFUNC_PROBE(perf_event_mmap, struct vm_area_struct *vma)
{
char path[256] = {};

bpf_d_path(&vma->vm_file->f_path, path, sizeof(path));
bpf_trace_printk("perf_event_mmap %s", path);
return 0;
}
'''

b = BPF(text=bpf_text)
print("BPF program loaded")
b.trace_print()

This change causes kernel panic. I think it's because of this NULL pointer.

Thank you for the testing and repro Hengqi :)
Indeed, I was able to reproduce this panic. When vma->vm_file is NULL,
&vma->vm_file->f_path ends up being 0x18 so d_path causes a panic.
I suppose that this sort of issue must be relatively common in helpers
that take a PTR_TO_BTF_ID though ? I wonder if there is anything that

Most non-tracing ARG_PTR_TO_BTF_ID argument has strict helper/prog_type
protection and should be okay although I didn't check them 100%.

For some tracing helpers with ARG_PTR_TO_BTF_ID argument, we have
bpf_seq_printf/bpf_seq_write which has strict context as well and should not be NULL.

For helper bpf_task_pt_regs() which can attach to ANY kernel function, we kind of assume "task" is not NULL which should be the case in "almost all* cases from kernel internal data structure.

the verifier could do about this ? For example if vma->vm_file could
be PTR_TO_BTF_ID_OR_NULL and therefore vma->vm_file->f_path somehow
considered invalid ?

Verifier has no way to know whether vma->vm_file is NULL or not during
verification time. So in your case, if we have to be conservative, that
means verifier will reject the program.

One possible way could be add a mode in verifier, we still *go through* the process for direct memory access but we require user explicit checking NULL pointers. This way, user will be forced to write code like

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));