netdev - Re: [PATCHv2 bpf-next] selftests/bpf: Fix stat probe 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]

Message-ID: <CAADnVQ+OmqycbKTewWPA9D5upP9Ri-yvS1=GKRN1nQs6AL_YVw@mail.gmail.com>
Date:   Mon, 21 Sep 2020 16:32:13 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Jiri Olsa <jolsa@...nel.org>
Cc:     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: [PATCHv2 bpf-next] selftests/bpf: Fix stat probe in d_path test

On Fri, Sep 18, 2020 at 4:23 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.
>
> Alexei suggested [1] we should use security_inode_getattr instead,
> because it's less likely to get inlined. Using this idea also for
> vfs_truncate (replaced with security_path_truncate) and vfs_fallocate
> (replaced with security_file_permission).
>
> Keeping dentry_open and filp_close, because they are in their own
> files, so unlikely to be inlined, but in case they are, adding
> security_file_open.
>
> Switching the d_path test stat trampoline to security_inode_getattr.
>
> Adding flags that indicate trampolines were called and failing
> the test if any of them got missed, so it's easier to identify
> the issue next time.
>
> Suggested-by: Alexei Starovoitov <ast@...nel.org>
> [1] https://lore.kernel.org/bpf/CAADnVQJ0FchoPqNWm+dEppyij-MOvvEG_trEfyrHdabtcEuZGg@mail.gmail.com/
> Fixes: e4d1af4b16f8 ("selftests/bpf: Add test for d_path helper")
> Signed-off-by: Jiri Olsa <jolsa@...hat.com>
> ---
> v2 changes:
>   - replaced vfs_* function with security_* in d_path allow list
>     vfs_truncate  -> security_path_truncate
>     vfs_fallocate -> security_file_permission
>     vfs_getattr   -> security_inode_getattr
>   - added security_file_open to d_path allow list
>   - split verbose output for trampoline flags
>
>  kernel/trace/bpf_trace.c                        |  7 ++++---
>  tools/testing/selftests/bpf/prog_tests/d_path.c | 10 ++++++++++
>  tools/testing/selftests/bpf/progs/test_d_path.c |  9 ++++++++-
>  3 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index b2a5380eb187..e24323d72cac 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1118,10 +1118,11 @@ BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
>  }
>
>  BTF_SET_START(btf_allowlist_d_path)
> -BTF_ID(func, vfs_truncate)
> -BTF_ID(func, vfs_fallocate)
> +BTF_ID(func, security_path_truncate)
> +BTF_ID(func, security_file_permission)
> +BTF_ID(func, security_inode_getattr)
> +BTF_ID(func, security_file_open)
>  BTF_ID(func, dentry_open)
> -BTF_ID(func, vfs_getattr)
>  BTF_ID(func, filp_close)
>  BTF_SET_END(btf_allowlist_d_path)

bpf CI system flagged the build error:
FAILED unresolved symbol security_path_truncate
because CONFIG_SECURITY_PATH wasn't set.
Which points to the issue with this patch that the above
security_* funcs have to be guarded with appropriate #ifdef.
I don't have a use case for tracing vfs_truncate, but
security_path_unlink I would want to do in the future.
Unfortunately it's under the same SECURITY_PATH ifdef.
So my earlier desire to make it fool proof is not feasible at this point.
Adding 'was_probed_func_inlined' check to libbpftrace.a would
solve it eventually.
For now I think we have to live with this function probing fragility.
So I've modified the patch to add these few security_* funcs
and kept vfs_* equivalents.
Also reworded commit log and applied to bpf-next. Thanks
https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=a8a717963fe5ecfd274eb93dd1285ee9428ffca7