From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5C96FC433DB for ; Wed, 24 Feb 2021 10:53:09 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id F1F9664EC9 for ; Wed, 24 Feb 2021 10:53:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234429AbhBXKxG (ORCPT ); Wed, 24 Feb 2021 05:53:06 -0500 Received: from mx2.suse.de ([195.135.220.15]:45044 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233794AbhBXKws (ORCPT ); Wed, 24 Feb 2021 05:52:48 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id C5CEEAF19; Wed, 24 Feb 2021 10:52:05 +0000 (UTC) Received: by quack2.suse.cz (Postfix, from userid 1000) id BF5321E14EF; Wed, 24 Feb 2021 11:52:04 +0100 (CET) Date: Wed, 24 Feb 2021 11:52:04 +0100 From: Jan Kara To: Amir Goldstein Cc: Jan Kara , Matthew Bobrowski , linux-fsdevel , Linux API , Kees Cook Subject: Re: [RFC][PATCH 2/2] fanotify: support limited functionality for unprivileged users Message-ID: <20210224105204.GC20583@quack2.suse.cz> References: <20210124184204.899729-1-amir73il@gmail.com> <20210124184204.899729-3-amir73il@gmail.com> <20210216170154.GG21108@quack2.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org On Tue 23-02-21 19:16:40, Amir Goldstein wrote: > On Fri, Feb 19, 2021 at 6:16 PM Amir Goldstein wrote: > > > > On Tue, Feb 16, 2021 at 8:12 PM Amir Goldstein wrote: > > > > > > On Tue, Feb 16, 2021 at 7:01 PM Jan Kara wrote: > > > > > > > > On Sun 24-01-21 20:42:04, Amir Goldstein wrote: > > > > > Add limited support for unprivileged fanotify event listener. > > > > > An unprivileged event listener does not get an open file descriptor in > > > > > the event nor the process pid of another process. An unprivileged event > > > > > listener cannot request permission events, cannot set mount/filesystem > > > > > marks and cannot request unlimited queue/marks. > > > > > > > > > > This enables the limited functionality similar to inotify when watching a > > > > > set of files and directories for OPEN/ACCESS/MODIFY/CLOSE events, without > > > > > requiring SYS_CAP_ADMIN privileges. > > > > > > > > > > The FAN_REPORT_DFID_NAME init flag, provide a method for an unprivileged > > > > > event listener watching a set of directories (with FAN_EVENT_ON_CHILD) > > > > > to monitor all changes inside those directories. > > > > > > > > > > This typically requires that the listener keeps a map of watched directory > > > > > fid to dirfd (O_PATH), where fid is obtained with name_to_handle_at() > > > > > before starting to watch for changes. > > > > > > > > > > When getting an event, the reported fid of the parent should be resolved > > > > > to dirfd and fstatsat(2) with dirfd and name should be used to query the > > > > > state of the filesystem entry. > > > > > > > > > > Note that even though events do not report the event creator pid, > > > > > fanotify does not merge similar events on the same object that were > > > > > generated by different processes. This is aligned with exiting behavior > > > > > when generating processes are outside of the listener pidns (which > > > > > results in reporting 0 pid to listener). > > > > > > > > > > Signed-off-by: Amir Goldstein > > > > > > > > The patch looks mostly good to me. Just two questions: > > > > > > > > a) Remind me please, why did we decide pid isn't safe to report to > > > > unpriviledged listeners? > > > > > > Just because the information that process X modified file Y is not an > > > information that user can generally obtain without extra capabilities(?) > > > I can add a flag FAN_REPORT_OWN_PID to make that behavior > > > explicit and then we can relax reporting pids later. > > > > > > > FYI a patch for flag FAN_REPORT_SELF_PID is pushed to branch > > fanotify_unpriv. > > > > The UAPI feels a bit awkward with this flag, but that is the easiest way > > to start without worrying about disclosing pids. > > > > I guess we can require that unprivileged listener has pid 1 in its own > > pid ns. The outcome is similar to FAN_REPORT_SELF_PID, except > > it can also get pids of its children which is probably fine. > > > > Jan, > > WRT your comment in github: > "So maybe we can just require that this flag is already set by userspace > instead of silently setting it? Like: > > if (!(flags & FAN_REPORT_SELF_PID)) return -EPERM; > > I'd say that variant is more futureproof and the difference for user > is minimal." > > I started with this approach and then I wrote the tests and imagined > the man page > requiring this flag would be a bit awkward, so I changed it to auto-enable. > > I am not strongly against the more implicit flag requirement, but in > favor of the > auto-enable approach I would like to argue that with current fanotify you CAN > get zero pid in event, so think about it this way: > If a listener is started in (or moved into) its own pid ns, it will > get zero pid in all > events (other than those generated by itself and its own children). > > With the proposed change, the same applies also if the listener is started > without CAP_SYS_ADMIN. > > As a matter of fact, we do not need the flag at all, we can determine whether > or not to report pid according to capabilities of the event reader at > event read time. > And we can check for one of: > - CAP_SYS_ADMIN > - CAP_SYS_PACCT > - CAP_SYS_PTRACE > > Do you prefer this flag-less approach? Well, I don't have strong opinion what we should do internally either. The flag seems OK to me. The biggest question is whether we should expose the FAN_REPORT_SELF_PID flag to userspace or not. If we would not require explicit flag for unpriv users, I see little reason to expose that flag at all. Honza -- Jan Kara SUSE Labs, CR