Message-ID: <968F7C58-691D-4636-AA91-D0EA999EE3FD@fb.com>
Date: Fri, 15 Nov 2024 21:05:31 +0000
From: Song Liu <songliubraving@...a.com>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
CC: Song Liu <songliubraving@...a.com>, Song Liu <song@...nel.org>,
        bpf
	<bpf@...r.kernel.org>,
        Linux-Fsdevel <linux-fsdevel@...r.kernel.org>,
        LKML
	<linux-kernel@...r.kernel.org>,
        LSM List
	<linux-security-module@...r.kernel.org>,
        Kernel Team <kernel-team@...a.com>,
        Andrii Nakryiko <andrii@...nel.org>, Eddy Z <eddyz87@...il.com>,
        Alexei
 Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin
 KaFai Lau <martin.lau@...ux.dev>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
        KP Singh
	<kpsingh@...nel.org>,
        Matt Bobrowski <mattbobrowski@...gle.com>,
        Amir
 Goldstein <amir73il@...il.com>,
        "repnop@...gle.com" <repnop@...gle.com>,
        Jeff
 Layton <jlayton@...nel.org>, Josef Bacik <josef@...icpanda.com>,
        Mickaël Salaün <mic@...ikod.net>,
        "gnoack@...gle.com"
	<gnoack@...gle.com>
Subject: Re: [RFC/PATCH v2 bpf-next fanotify 7/7] selftests/bpf: Add test for
 BPF based fanotify fastpath handler



> On Nov 15, 2024, at 11:41 AM, Alexei Starovoitov <alexei.starovoitov@...il.com> wrote:
> 
> On Thu, Nov 14, 2024 at 11:01 PM Song Liu <songliubraving@...a.com> wrote:
>> 
>>> 
>>> I think bpf-lsm hook fires before fanotify, so bpf-lsm prog
>>> implementing some security policy has to decide right
>>> at the moment what to do with, say, security_file_open().
>>> fanotify with or without bpf fastpath is too late.
>> 
>> Actually, fanotify in permission mode can stop a file open.
> 
> The proposed patch 1 did:
> 
> +/* Return value of fp_handler */
> +enum fanotify_fastpath_return {
> + /* The event should be sent to user space */
> + FAN_FP_RET_SEND_TO_USERSPACE = 0,
> + /* The event should NOT be sent to user space */
> + FAN_FP_RET_SKIP_EVENT = 1,
> +};
> 
> It looked like a read-only notification to user space
> where bpf prog is merely a filter.

Yep. As Amir also pointed out, this part needs more work and
clarifications. 

> 
>> In current upstream code, fsnotify hook fsnotify_open_perm
>> is actually part of security_file_open(). It will be moved
>> to do_dentry_open(), right after security_file_open(). This
>> move is done by 1cda52f1b461 in linux-next.
> 
> Separating fsnotify from LSM makes sense.
> 
>> In practice, we are not likely to use BPF LSM and fanotify
>> on the same hook at the same time. Instead, we can use
>> BPF LSM hooks to gather information and use fanotify to
>> make allow/deny decision, or vice versa.
> 
> Pick one.
> If the proposal is changing to let fsnotify-bpf prog to deny
> file_open then it's a completely different discussion.
> 
> In such a case make it clear upfront that fsnotify will
> rely on CONFIG_FANOTIFY_ACCESS_PERMISSIONS and

I am not sure whether we should limit fsnotify-bpf to 
only work with CONFIG_FANOTIFY_ACCESS_PERMISSIONS. I still 
think it can be useful just as a filter. But I guess we can
start with this dependency. 

> bpf-lsm part of file access will not be used,
> since interaction of two callbacks at file_open makes little sense.

Agreed that having two hooks on file_open doesn't make sense. 
I was actually thinking about combining fsnotify-bpf with 
other LSM hooks. But I agree we should start as simple as
possible. 

> 
>>> In general fanotify is not for security. It's notifying
>>> user space of events that already happened, so I don't see
>>> how these two can be combined.
>> 
>> fanotify is actually used by AntiVirus softwares. For
>> example, CalmAV (https://urldefense.com/v3/__https://www.clamav.net/__;!!Bt8RZUm9aw!4p7JB_E8RuNLldz_TYR07jnW5kHbr4H2FMm9vLbShKOBMznXwES7Dlt5_R6B_-HMzgV3Qk_9WKlhmjHSpYHRhTb2WM3hIg$ ) uses fanotify
>> for its Linux version (it also supports Window and MacOS).
> 
> It's relying on user space to send back FANOTIFY_PERM_EVENTS ?

Yes, it goes all the way to another process, and comes back. 

> 
> fsnotify_open_perm->fsnotify->send_to_group->fanotify_handle_event.
> 
> is a pretty long path to call bpf prog and
> preparing a giant 'struct fanotify_fastpath_event'
> is not going to fast either.
> 
> If we want to accelerate that with bpf it needs to be done
> sooner with negligible overhead.

Agreed. This is actually something I have been thinking 
since the beginning of this work: Shall it be fanotify-bpf 
or fsnotify-bpf. Given we have more materials, this is a 
good time to have broader discussions on this. 

@all, please chime in whether we should redo this as
fsnotify-bpf. AFAICT:

Pros of fanotify-bpf: 
- There is existing user space that we can leverage/reuse.

Pros of fsnotify-bpf: 
- Faster fast path. 

Another major pros/cons did I miss?

> 
>> I guess I didn't state the motivation clearly. So let me
>> try it now.
>> 
>> Tracing is a critical part of a security solution. With
>> LSM, blocking an operation is straightforward. However,
>> knowing which operation should be blocked is not always
>> easy. Also, security hooks (LSM or fanotify) sit in the
>> critical path of user requests. It is very important to
>> optimize the latency of a security hook. Ideally, the
>> tracing logic should gather all the information ahead
>> of time, and make the actual hook fast.
>> 
>> For example, if security_file_open() only needs to read
>> a flag from inode local storage, the overhead is minimal
>> and predictable. If security_file_open() has to walk the
>> dentry tree, or call d_path(), the overhead will be
>> much higher. fanotify_file_perm() provides another
>> level of optimization over security_file_open(). If a
>> file is not being monitored, fanotify will not generate
>> the event.
> 
> I agree with motivation, but don't see this in the patches.

Agreed. I should definitely do better job in this. 

> The overhead to call into bpf prog is big.
> Even if prog does nothing it's still going to be slower.
> 
>> Security solutions hold higher bars for the tracing logic:
>> 
>> - It needs to be accurate, as false positives and false
>>  negatives can be very annoying and/or harmful.
>> - It needs to be efficient, as security daemons run 24/7.
>> 
>> Given these requirements of security solutions, I believe
>> it is important to optimize tracing logic as much as
>> possible. And BPF based fanotify fastpath handler can
>> bring non-trivials benefit to BPF based security solutions.
> 
> Doing everything in the kernel is certainly faster than
> going back and forth to user space,
> but bpf-lsm should be able to do the same already.
> 
> Without patch 1 and only patches 4,5 that add few kfuncs,
> bpf-lsm prog will be able to remember subtree dentry and
> do the same is_subdir() to deny.
> The patch 7 stays pretty much as-is. All in bpf-lsm.
> Close to zero overhead without long chain of fsnotify callbacks.

One of the advantages of fanotify-bpf or fsnotify-bpf is 
that it only calls the BPF program for being monitored
files. OTOH, BPF LSM program is always global. 

> 
>> fanotify also has a feature that LSM doesn't provide.
>> When a file is accessed, user space daemon can get a
>> fd on this file from fanotify. OTOH, LSM can only send
>> an ino or a path to user space, which is not always
>> reliable.
> 
> That sounds useful, but we're mixing too many things.
> If user space cares about fd it will be using the existing
> mechanism with all accompanied overhead. fsnotify-bpf can
> barely accelerate anything, since user space makes
> ultimate decisions.
> If user space is not in the driving seat then existing bpf-lsm
> plus few kfuncs to remember dentry and call is_subdir()
> will do the job and no need for patch 1.

In many cases, we only need the user space to look into 
the file when necessary. For example, when a binary is 
first written, the user space daemon will scan through
it (for virus, etc.) and mark it as safe/dangerous in 
some BPF maps. Later, when the file is opened for 
execution, f[s|a]notify-bpf can make the allow/deny
decision based on the information in BPF maps. 

Thanks again for your review and inputs!

Song

Powered by blists - more mailing lists