linux-kernel - Re: [PATCH v3 fanotify 2/2] samples/fanotify: Add a sample fanotify fiter

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: <CAADnVQK-6MFdwD_0j-3x2-t8VUjbNJUuGrTXEWJ0ttdpHvtLOA@mail.gmail.com>
Date: Tue, 26 Nov 2024 16:50:23 -0800
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Amir Goldstein <amir73il@...il.com>
Cc: 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>, repnop@...gle.com, 
	Jeff Layton <jlayton@...nel.org>, Josef Bacik <josef@...icpanda.com>, 
	Mickaël Salaün <mic@...ikod.net>, gnoack@...gle.com
Subject: Re: [PATCH v3 fanotify 2/2] samples/fanotify: Add a sample fanotify fiter

On Sat, Nov 23, 2024 at 9:07 PM Amir Goldstein <amir73il@...il.com> wrote:
> > +++ b/samples/fanotify/filter-mod.c
> > @@ -0,0 +1,105 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
> > +
> > +#include <linux/fsnotify.h>
> > +#include <linux/fanotify.h>
> > +#include <linux/module.h>
> > +#include <linux/path.h>
> > +#include <linux/file.h>
> > +#include "filter.h"
> > +
> > +struct fan_filter_sample_data {
> > +       struct path subtree_path;
> > +       enum fan_filter_sample_mode mode;
> > +};
> > +
> > +static int sample_filter(struct fsnotify_group *group,
> > +                        struct fanotify_filter_hook *filter_hook,
> > +                        struct fanotify_filter_event *filter_event)
> > +{
> > +       struct fan_filter_sample_data *data;
> > +       struct dentry *dentry;
> > +
> > +       dentry = fsnotify_data_dentry(filter_event->data, filter_event->data_type);
> > +       if (!dentry)
> > +               return FAN_FILTER_RET_SEND_TO_USERSPACE;
> > +
> > +       data = filter_hook->data;
> > +
> > +       if (is_subdir(dentry, data->subtree_path.dentry)) {
> > +               if (data->mode == FAN_FILTER_SAMPLE_MODE_BLOCK)
> > +                       return -EPERM;
> > +               return FAN_FILTER_RET_SEND_TO_USERSPACE;
> > +       }
> > +       return FAN_FILTER_RET_SKIP_EVENT;
> > +}
> > +
> > +static int sample_filter_init(struct fsnotify_group *group,
> > +                             struct fanotify_filter_hook *filter_hook,
> > +                             void *argp)
> > +{
> > +       struct fan_filter_sample_args *args;
> > +       struct fan_filter_sample_data *data;
> > +       struct file *file;
> > +       int fd;
> > +
> > +       args = (struct fan_filter_sample_args *)argp;
> > +       fd = args->subtree_fd;
> > +
> > +       file = fget(fd);
> > +       if (!file)
> > +               return -EBADF;
> > +       data = kzalloc(sizeof(struct fan_filter_sample_data), GFP_KERNEL);
> > +       if (!data) {
> > +               fput(file);
> > +               return -ENOMEM;
> > +       }
> > +       path_get(&file->f_path);
> > +       data->subtree_path = file->f_path;
> > +       fput(file);
> > +       data->mode = args->mode;
> > +       filter_hook->data = data;
> > +       return 0;
> > +}
> > +
> > +static void sample_filter_free(struct fanotify_filter_hook *filter_hook)
> > +{
> > +       struct fan_filter_sample_data *data = filter_hook->data;
> > +
> > +       path_put(&data->subtree_path);
> > +       kfree(data);
> > +}
> > +
>
> Hi Song,
>
> This example looks fine but it raises a question.
> This filter will keep the mount of subtree_path busy until the group is closed
> or the filter is detached.
> This is probably fine for many services that keep the mount busy anyway.
>
> But what if this wasn't the intention?
> What if an Anti-malware engine that watches all mounts wanted to use that
> for configuring some ignore/block subtree filters?
>
> One way would be to use a is_subtree() variant that looks for a
> subtree root inode
> number and then verifies it with a subtree root fid.
> A production subtree filter will need to use a variant of is_subtree()
> anyway that
> looks for a set of subtree root inodes, because doing a loop of is_subtree() for
> multiple paths is a no go.
>
> Don't need to change anything in the example, unless other people
> think that we do need to set a better example to begin with...

I think we have to treat this patch as a real filter and not as an example
to make sure that the whole approach is workable end to end.
The point about not holding path/dentry is very valid.
The algorithm needs to support that.
It may very well turn out that the logic of handling many filters
without a loop and not grabbing a path refcnt is too complex for bpf.
Then this subtree filtering would have to stay as a kernel module
or extra flag/feature for fanotify.