netdev - Re: [PATCH bpf-next] selftests/bpf: Check trampoline execution in d

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 14 Sep 2020 19:30:33 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Jiri Olsa <jolsa@...hat.com>
Cc:     Jiri Olsa <jolsa@...nel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andriin@...com>,
        Network Development <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>
Subject: Re: [PATCH bpf-next] selftests/bpf: Check trampoline execution in
 d_path test

On Fri, Sep 11, 2020 at 6:16 AM Jiri Olsa <jolsa@...hat.com> wrote:
>
> On Thu, Sep 10, 2020 at 05:46:21PM -0700, Alexei Starovoitov wrote:
> > On Thu, Sep 10, 2020 at 5:22 AM Jiri Olsa <jolsa@...nel.org> wrote:
> > >
> > > Some kernels builds might inline vfs_getattr call within
> > > fstat syscall code path, so fentry/vfs_getattr trampoline
> > > is not called.
> > >
> > > I'm not sure how to handle this in some generic way other
> > > than use some other function, but that might get inlined at
> > > some point as well.
> >
> > It's great that we had the test and it failed.
> > Doing the test skipping will only hide the problem.
> > Please don't do it here and in the future.
> > Instead let's figure out the real solution.
> > Assuming that vfs_getattr was added to btf_allowlist_d_path
> > for a reason we have to make this introspection place
> > reliable regardless of compiler inlining decisions.
> > We can mark it as 'noinline', but that's undesirable.
> > I suggest we remove it from the allowlist and replace it with
> > security_inode_getattr.
> > I think that is a better long term fix.
>
> in my case vfs_getattr got inlined in vfs_statx_fd and both
> of them are defined in fs/stat.c
>
> so the idea is that inlining will not happen if the function
> is defined in another object? or less likely..?

when it's in a different .o file. yes.
Very few folks build LTO kernels, so I propose to cross that bridge when
we get there.
Eventually we can replace security_inode_getattr
with bpf_lsm_inode_getattr or simply add noinline to security_inode_getattr.

> we should be safe when it's called from module

what do you mean?

> > While at it I would apply the same critical thinking to other
> > functions in the allowlist. They might suffer the same issue.
> > So s/vfs_truncate/security_path_truncate/ and so on?
> > Things won't work when CONFIG_SECURITY is off, but that is a rare kernel config?
> > Or add both security_* and vfs_* variants and switch tests to use security_* ?
> > but it feels fragile to allow inline-able funcs in allowlist.
>
> hm, what's the difference between vfs_getattr and security_inode_getattr
> in this regard? I'd expect compiler could inline it same way as for vfs_getattr

not really because they're in different files and LTO is not on.
Even with LTO the chances of inlining are small. The compiler will
consider profitability of it. Since there is a loop inside, it's unlikely.