Re: [PATCH] fsnotify: add generic perm check for unlink/rmdir

From: Amir Goldstein <amir73il@gmail.com>
To: Jan Kara <jack@suse.cz>
Cc: Guowei Du <duguoweisz@gmail.com>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Al Viro <viro@zeniv.linux.org.uk>,
	James Morris <jmorris@namei.org>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
	kafai@fb.com, songliubraving@fb.com, yhs@fb.com,
	john.fastabend@gmail.com, kpsingh@kernel.org,
	LSM List <linux-security-module@vger.kernel.org>,
	Netdev <netdev@vger.kernel.org>,
	bpf@vger.kernel.org, Paul Moore <paul@paul-moore.com>,
	Stephen Smalley <stephen.smalley.work@gmail.com>,
	Eric Paris <eparis@parisplace.org>,
	Kees Cook <keescook@chromium.org>,
	anton@enomsg.org, ccross@android.com, tony.luck@intel.com,
	selinux@vger.kernel.org, duguowei <duguowei@xiaomi.com>
Subject: Re: [PATCH] fsnotify: add generic perm check for unlink/rmdir
Date: Wed, 4 May 2022 17:12:16 +0300	[thread overview]
Message-ID: <CAOQ4uxguXW05_YSpgT=kGgxztQYqhJ3x4MFsz9ZTO0crc9=4tA@mail.gmail.com> (raw)
In-Reply-To: <20220503194943.6bcmsxjvinfjrqxa@quack3.lan>

On Tue, May 3, 2022 at 10:49 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 04-05-22 02:37:50, Guowei Du wrote:
> > From: duguowei <duguowei@xiaomi.com>
> >
> > For now, there have been open/access/open_exec perms for file operation,
> > so we add new perms check with unlink/rmdir syscall. if one app deletes
> > any file/dir within pubic area, fsnotify can sends fsnotify_event to
> > listener to deny that, even if the app have right dac/mac permissions.
> >
> > Signed-off-by: duguowei <duguowei@xiaomi.com>
>
> Before we go into technical details of implementation can you tell me more
> details about the usecase? Why do you need to check specifically for unlink
> / delete?
>
> Also on the design side of things: Do you realize these permission events
> will not be usable together with other permission events like
> FAN_OPEN_PERM? Because these require notification group returning file
> descriptors while your events will return file handles... I guess we should
> somehow fix that.
>

IMO, regardless of file descriptions vs. file handles, blocking events have
no business with async events in the same group at all.
What is the use case for that?
Sure, we have the legacy permission event, but if we do add new blocking
events to UAPI, IMO they should be added to a group that was initialized with a
different class to indicate "blocking events only".

And if we do that, we will not need to pollute the event mask namespace
for every permission event.
When users request to get FAN_UNLINK/FAN_RMDIR events in a
FAN_CLASS_PERMISSION group, internally, that only captures
events reported from fsnotify_perm()/fsnotify_path_perm().

FYI, I do intend to try and upload "pre-modify events" [1].
I had no intention to expose those in fanotify and my implementation
does not have the granularity of UNLINK/RMDIR, but we do need
to think about not duplicating too much code with those overlapping
features.

Thanks,
Amir.

[1] https://github.com/amir73il/linux/commits/fsnotify_pre_modify