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

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

[Florent Revest]

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.

Signed-off-by: Florent Revest <revest@chromium.org>
---
 kernel/trace/bpf_trace.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index cbcd0d6fca7c..f6e301c775a5 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -922,6 +922,9 @@ BTF_ID(func, vfs_fallocate)
 BTF_ID(func, dentry_open)
 BTF_ID(func, vfs_getattr)
 BTF_ID(func, filp_close)
+#ifdef CONFIG_PERF_EVENTS
+BTF_ID(func, perf_event_mmap)
+#endif
 BTF_SET_END(btf_allowlist_d_path)
 
 static bool bpf_d_path_allowed(const struct bpf_prog *prog)
-- 
2.33.0.1079.g6e70778dc9-goog

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

[Martin KaFai Lau]

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?

> 
> Signed-off-by: Florent Revest <revest@chromium.org>
> ---
>  kernel/trace/bpf_trace.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index cbcd0d6fca7c..f6e301c775a5 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -922,6 +922,9 @@ BTF_ID(func, vfs_fallocate)
>  BTF_ID(func, dentry_open)
>  BTF_ID(func, vfs_getattr)
>  BTF_ID(func, filp_close)
> +#ifdef CONFIG_PERF_EVENTS
> +BTF_ID(func, perf_event_mmap)
> +#endif
>  BTF_SET_END(btf_allowlist_d_path)
>  
>  static bool bpf_d_path_allowed(const struct bpf_prog *prog)
> -- 
> 2.33.0.1079.g6e70778dc9-goog
> 

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

[Hengqi Chen]

On 10/29/21 6:46 AM, Martin KaFai Lau 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?
> 

Hmm, is perf_event_mmap a proper tracing target ?
It does not appear in /sys/kernel/debug/tracing/available_filter_functions.

I tried using kprobe/fentry to attach to it, both failed.

>>
>> Signed-off-by: Florent Revest <revest@chromium.org>
>> ---
>>  kernel/trace/bpf_trace.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>> index cbcd0d6fca7c..f6e301c775a5 100644
>> --- a/kernel/trace/bpf_trace.c
>> +++ b/kernel/trace/bpf_trace.c
>> @@ -922,6 +922,9 @@ BTF_ID(func, vfs_fallocate)
>>  BTF_ID(func, dentry_open)
>>  BTF_ID(func, vfs_getattr)
>>  BTF_ID(func, filp_close)
>> +#ifdef CONFIG_PERF_EVENTS
>> +BTF_ID(func, perf_event_mmap)
>> +#endif
>>  BTF_SET_END(btf_allowlist_d_path)
>>  
>>  static bool bpf_d_path_allowed(const struct bpf_prog *prog)
>> -- 
>> 2.33.0.1079.g6e70778dc9-goog
>>

--Hengqi

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

[Florent Revest]

On Fri, Oct 29, 2021 at 12:47 AM Martin KaFai Lau <kafai@fb.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).

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

[Florent Revest]

On Fri, Oct 29, 2021 at 5:17 PM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>
>
>
> On 10/29/21 6:46 AM, Martin KaFai Lau 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?
> >
>
> Hmm, is perf_event_mmap a proper tracing target ?
> It does not appear in /sys/kernel/debug/tracing/available_filter_functions.
>
> I tried using kprobe/fentry to attach to it, both failed.

Good observation! :) In the bpf-next tree, perf_event_mmap is still
not exposed to tracing because the kernel/events/Makefile specifically
removes CC_FLAGS_FTRACE for core.c.
However, Song sent a patch to expose the functions of
kernel/events/core.c to tracing:
https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/commit/kernel/events/Makefile?id=79df45731da68772d2285265864a52c900b8c65f

That patch is going through Peter Zijlstra's tree so it should
percolate down to Linus tree probably at ~the same time as this patch
(if it gets in :)). I tested this on bpf-next with Song's patch
cherry-picked.

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

[Hengqi Chen]

Hi,


On 2021/10/30 1:02 AM, Florent Revest wrote:
> On Fri, Oct 29, 2021 at 12:47 AM Martin KaFai Lau <kafai@fb.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.

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

[Florent Revest]

On Mon, Nov 1, 2021 at 2:17 PM Hengqi Chen <hengqi.chen@gmail.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@fb.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
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 ?

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

[Yonghong Song]

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

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

[Alexei Starovoitov]

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

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.

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

[Andrii Nakryiko]

On Mon, Nov 1, 2021 at 7:53 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Nov 1, 2021 at 10:32 AM Yonghong Song <yhs@fb.com> wrote:
> >
> >
> >
> > On 11/1/21 8:01 AM, Florent Revest wrote:
> > > On Mon, Nov 1, 2021 at 2:17 PM Hengqi Chen <hengqi.chen@gmail.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@fb.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));
>
> 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.

Can we just require all helpers to check NULL if they accept
PTR_TO_BTF_ID? It's always been a case that PTR_TO_BTF_ID can be null.
We should audit all the helpers with ARG_PTR_TO_BTF_ID and ensure they
do proper validation, of course.

Or am I missing the essence of the issue?

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

[Alexei Starovoitov]

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.
>
> Can we just require all helpers to check NULL if they accept
> PTR_TO_BTF_ID? It's always been a case that PTR_TO_BTF_ID can be null.
> We should audit all the helpers with ARG_PTR_TO_BTF_ID and ensure they
> do proper validation, of course.
>
> Or am I missing the essence of the issue?

It's not a pointer dereference. It's math on the pointer. The
&vm_file->f_path part.
The helper can check that it's [0, few_pages] and declare it's bad.
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.

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

[Andrii Nakryiko]

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.
> >
> > Can we just require all helpers to check NULL if they accept
> > PTR_TO_BTF_ID? It's always been a case that PTR_TO_BTF_ID can be null.
> > We should audit all the helpers with ARG_PTR_TO_BTF_ID and ensure they
> > do proper validation, of course.
> >
> > Or am I missing the essence of the issue?
>
> It's not a pointer dereference. It's math on the pointer. The
> &vm_file->f_path part.

Ah, I see... Makes sense now.

> 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.

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

[Florent Revest]

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.

> > 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.

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

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

[Yonghong Song]

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!