Re: [PATCH bpf-next v9 7/8] bpf: lsm: Add selftests for BPF_PROG_TYPE_LSM

[Jann Horn <jannh@google.com>]

On Thu, Apr 2, 2020 at 6:30 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Thu, Apr 02, 2020 at 06:03:57AM +0200, KP Singh wrote:
> > On 01-Apr 17:09, Alexei Starovoitov wrote:
> > > On Sat, Mar 28, 2020 at 5:44 PM KP Singh <kpsingh@chromium.org> wrote:
> > > > +int BPF_PROG(test_int_hook, struct vm_area_struct *vma,
> > > > +            unsigned long reqprot, unsigned long prot, int ret)
> > > > +{
> > > > +       if (ret != 0)
> > > > +               return ret;
> > > > +
> > > > +       __u32 pid = bpf_get_current_pid_tgid() >> 32;
> > > > +       int is_heap = 0;
> > > > +
> > > > +       is_heap = (vma->vm_start >= vma->vm_mm->start_brk &&
> > > > +                  vma->vm_end <= vma->vm_mm->brk);
> > >
> > > This test fails for me.
> >
> > Trying this from bpf/master:
> >
> >   b9258a2cece4 ("slcan: Don't transmit uninitialized stack data in padding")
> >
> > also from bpf-next/master:
> >
> >  1a323ea5356e ("x86: get rid of 'errret' argument to __get_user_xyz() macross")
> >
> > and I am unable to reproduce the failure (the output when using bpf/master):
> ..
> >
> > Also, I am wondering if this happens just in the BPF program or also
> > in the kernel as the other variable I can think of is the compiled
> > bpf program itself which might be reading a different value thinking
> > it's vm->vma_start, possible something to do with BTF / CO RE due to a
> > compiler bug:
>
> I don't think it's anything to do with clang/btf or core.
> I think that condition is simply incorrect.
> I've added:
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index 311c0dadf71c..16ae0ada34ba 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -577,6 +577,7 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
>                         goto out;
>                 }
>
> +               printk("start %llx %llx\n", vma->vm_start, vma->vm_mm->start_brk);
>                 error = security_file_mprotect(vma, reqprot, prot);
>
> and see exactly the same values as bpf side (at least it was nice to see
> that all CO-RE logic is working as expected :))
>
> [   24.787442] start 523000 39b9000
>
> I think it has something to do with the way test_progs is linked.
> But the problem is in condition itself.
> I suspect you copy-pasted it from selinux_file_mprotect() ?
> I think it's incorrect there as well.
> Did we just discover a way to side step selinux protection?
> Try objdump -h test_progs|grep bss
> the number I see in vma->vm_start is the beginning of .bss rounded to page boundary.
> I wonder where your 55dc6e8df000 is coming from.

I suspect that you're using different versions of libc, or something's
different in the memory layout, or something like that. The brk region
is used for memory allocations using brk(), but memory allocations
using mmap() land outside it. At least some versions of libc try to
allocate memory for malloc() with brk(), then fall back to mmap() if
that fails because there's something else behind the current end of
the brk region; but I think there might also be versions of libc that
directly use mmap() and don't even try to use brk().

So yeah, security checks based on the brk region aren't exactly
useful; but e.g. in SELinux, both cases have appropriate checks. The
brk region gets SECCLASS_PROCESS:PROCESS__EXECHEAP, anonymous mmap
allocations get SECCLASS_PROCESS:PROCESS__EXECMEM in
file_map_prot_check() instead. (This makes *some* amount of sense -
although not a lot - because for the brk region you know that it comes
from something like malloc(), while an anonymous mmap() allocation
might reasonably be used for JIT executable memory.)

In other words, you may want to pick something different as test case,
since the behavior here depends on libc.