linux-kernel - Re: [PATCH bpf-next v4 3/7] bpf: Introduce BPF_MODIFY

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAEjxPJ4+aW5JVC9QjJywjNUS=+cVJeaWwRHLwOssLsZyhX3siw@mail.gmail.com>
Date:   Thu, 5 Mar 2020 08:51:58 -0500
From:   Stephen Smalley <stephen.smalley.work@...il.com>
To:     KP Singh <kpsingh@...omium.org>
Cc:     linux-security-module@...r.kernel.org,
        linux-kernel@...r.kernel.org, bpf@...r.kernel.org,
        Andrii Nakryiko <andriin@...com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Paul Turner <pjt@...gle.com>, Jann Horn <jannh@...gle.com>,
        Florent Revest <revest@...omium.org>,
        Brendan Jackman <jackmanb@...omium.org>, jmorris@...ei.org,
        Paul Moore <paul@...l-moore.com>, casey@...aufler-ca.com
Subject: Re: [PATCH bpf-next v4 3/7] bpf: Introduce BPF_MODIFY_RETURN

On Wed, Mar 4, 2020 at 2:20 PM KP Singh <kpsingh@...omium.org> wrote:
>
> From: KP Singh <kpsingh@...gle.com>
>
> When multiple programs are attached, each program receives the return
> value from the previous program on the stack and the last program
> provides the return value to the attached function.
>
> The fmod_ret bpf programs are run after the fentry programs and before
> the fexit programs. The original function is only called if all the
> fmod_ret programs return 0 to avoid any unintended side-effects. The
> success value, i.e. 0 is not currently configurable but can be made so
> where user-space can specify it at load time.
>
> For example:
>
> int func_to_be_attached(int a, int b)
> {  <--- do_fentry
>
> do_fmod_ret:
>    <update ret by calling fmod_ret>
>    if (ret != 0)
>         goto do_fexit;
>
> original_function:
>
>     <side_effects_happen_here>
>
> }  <--- do_fexit
>
> The fmod_ret program attached to this function can be defined as:
>
> SEC("fmod_ret/func_to_be_attached")
> int BPF_PROG(func_name, int a, int b, int ret)
> {
>         // This will skip the original function logic.
>         return 1;
> }
>
> The first fmod_ret program is passed 0 in its return argument.
>
> Signed-off-by: KP Singh <kpsingh@...gle.com>
> Acked-by: Andrii Nakryiko <andriin@...com>

IIUC you've switched from a model where the BPF program would be
invoked after the original function logic
and the BPF program is skipped if the original function logic returns
non-zero to a model where the BPF program is invoked first and
the original function logic is skipped if the BPF program returns
non-zero.  I'm not keen on that for userspace-loaded code attached
to LSM hooks; it means that userspace BPF programs can run even if
SELinux would have denied access and SELinux hooks get
skipped entirely if the BPF program returns an error.  I think Casey
may have wrongly pointed you in this direction on the grounds
it can already happen with the base DAC checking logic.  But that's
kernel DAC checking logic, not userspace-loaded code.
And the existing checking on attachment is not sufficient for SELinux
since CAP_MAC_ADMIN is not all powerful to SELinux.
Be careful about designing your mechanisms around Smack because Smack
is not the only LSM.