> On Nov 15, 2024, at 11:41 AM, Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Thu, Nov 14, 2024 at 11:01 PM Song Liu <songliubraving@xxxxxxxx> 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
