Date:   Mon, 1 Nov 2021 10:31:38 -0700
From:   Yonghong Song <yhs@...com>
To:     Florent Revest <revest@...omium.org>,
        Hengqi Chen <hengqi.chen@...il.com>
CC:     Martin KaFai Lau <kafai@...com>, <bpf@...r.kernel.org>,
        <ast@...nel.org>, <daniel@...earbox.net>, <andrii@...nel.org>,
        <kpsingh@...nel.org>, <jackmanb@...gle.com>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH bpf-next] bpf: Allow bpf_d_path in perf_event_mmap



On 11/1/21 8:01 AM, Florent Revest wrote:
> On Mon, Nov 1, 2021 at 2:17 PM Hengqi Chen <hengqi.chen@...il.com> 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@...com> 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));

Powered by blists - more mailing lists