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=-17.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham 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 81FB8C04FF3 for ; Mon, 24 May 2021 10:00:55 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 4F534610C8 for ; Mon, 24 May 2021 10:00:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232476AbhEXKCU (ORCPT ); Mon, 24 May 2021 06:02:20 -0400 Received: from mx2.suse.de ([195.135.220.15]:41218 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232517AbhEXKCT (ORCPT ); Mon, 24 May 2021 06:02:19 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1621850405; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=qSPLeftBkaRi2IbEx9BDvKcI70DSUwvnUcKk00xfE2I=; b=szyq3OjWKtKfzthHNCHPRB2aQqPw222IA1FLBAH1oPHwANdtnpoiBhMMPn4e+MxFQYGskN bJg03JvwfYVn2DJIHLxOKuDxY8DZoPQIQdKAwX54EqESXbLFl3ADcNN0wNZKhXfAAbEQdF t/5bESTflEEe3tEiLIcfoJGOZfx07vw= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1621850405; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=qSPLeftBkaRi2IbEx9BDvKcI70DSUwvnUcKk00xfE2I=; b=RVVSTIjqW4GrtmtdSqdBls/3B8SQJfWxFpHDD0NjWw6DKvYgMyTK65UCYnVvWlxEZ779E6 9vun1opmG3SxXuAg== Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 1CB66ABB1; Mon, 24 May 2021 10:00:05 +0000 (UTC) Received: by quack2.suse.cz (Postfix, from userid 1000) id 75D5E1F2CA2; Mon, 24 May 2021 12:00:04 +0200 (CEST) Date: Mon, 24 May 2021 12:00:04 +0200 From: Jan Kara To: Matthew Bobrowski Cc: Amir Goldstein , Jan Kara , Matthew Bobrowski , linux-fsdevel@vger.kernel.org Subject: Re: [PATCH] fanotify: fix permission model of unprivileged group Message-ID: <20210524100004.GI32705@quack2.suse.cz> References: <20210522091916.196741-1-amir73il@gmail.com> 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 Mon 24-05-21 18:41:36, Matthew Bobrowski wrote: > On Sat, May 22, 2021 at 12:19:16PM +0300, Amir Goldstein wrote: > > Reporting event->pid should depend on the privileges of the user that > > initialized the group, not the privileges of the user reading the > > events. > > > > Use an internal group flag FANOTIFY_UNPRIV to record the fact the the > > group was initialized by an unprivileged user. > > > > To be on the safe side, the premissions to setup filesystem and mount > > marks now require that both the user that initialized the group and > > the user setting up the mark have CAP_SYS_ADMIN. > > > > Fixes: 7cea2a3c505e ("fanotify: support limited functionality for unprivileged users") > > Signed-off-by: Amir Goldstein > > Thanks for sending through this patch Amir! > > In general, the patch looks good to me, however there's just a few > nits below. > > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > > index 71fefb30e015..7df6cba4a06d 100644 > > --- a/fs/notify/fanotify/fanotify_user.c > > +++ b/fs/notify/fanotify/fanotify_user.c > > @@ -424,11 +424,18 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group, > > * events generated by the listener process itself, without disclosing > > * the pids of other processes. > > */ > > - if (!capable(CAP_SYS_ADMIN) && > > + if (FAN_GROUP_FLAG(group, FANOTIFY_UNPRIV) && > > task_tgid(current) != event->pid) > > metadata.pid = 0; > > > > - if (path && path->mnt && path->dentry) { > > + /* > > + * For now, we require fid mode for unprivileged listener, which does > > + * record path events, but keep this check for safety in case we want > > + * to allow unprivileged listener to get events with no fd and no fid > > + * in the future. > > + */ > > I think it's best if we keep clear of using first person in our > comments throughout our code base. Maybe we could change this to: > > * For now, fid mode is required for an unprivileged listener, which > does record path events. However, this check must be kept... Actually, I have no problem with the first person in comments. It is a standard "anonymous" language and IMO easy to understand as well. Also frequently used in the kernel AFAICT. What problem do you see with the first person? I'm well aware that unlike us you are a native speaker ;) > > @@ -1305,11 +1320,13 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask, > > group = f.file->private_data; > > > > /* > > - * An unprivileged user is not allowed to watch a mount point nor > > - * a filesystem. > > + * An unprivileged user is not allowed to setup mount point nor > ^ > s > > + * filesystem marks. It is not allowed to setup those marks for > > + * a group that was initialized by an unprivileged user. > > I think the second sentence would better read as: > > * This also includes setting up such marks by a group that was > intialized by an unprivileged user. Yes, this is probably better. > > show_fdinfo(m, f, fanotify_fdinfo); > > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h > > index bad41bcb25df..f277d1c4e6b8 100644 > > --- a/include/linux/fanotify.h > > +++ b/include/linux/fanotify.h > > @@ -51,6 +51,10 @@ extern struct ctl_table fanotify_table[]; /* for sysctl */ > > #define FANOTIFY_INIT_FLAGS (FANOTIFY_ADMIN_INIT_FLAGS | \ > > FANOTIFY_USER_INIT_FLAGS) > > > > +/* Internal flags */ > > +#define FANOTIFY_UNPRIV 0x80000000 > > +#define FANOTIFY_INTERNAL_FLAGS (FANOTIFY_UNPRIV) > > Should we be more distinct here i.e. FANOTIFY_INTERNAL_INIT_FLAGS? > Just thinking about a possible case where there's some other internal > fanotify flags that are used for something else? Well, do we need to distinguish different uses of internal flags? I don't think so... Honza -- Jan Kara SUSE Labs, CR