Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Wed, 11 Oct 2023 12:05:41 +0200
From:   Jan Kara <jack@suse.cz>
To:     Max Kellermann <max.kellermann@ionos.com>
Cc:     Jan Kara <jack@suse.cz>, Xiubo Li <xiubli@redhat.com>,
        Ilya Dryomov <idryomov@gmail.com>,
        Jeff Layton <jlayton@kernel.org>, Jan Kara <jack@suse.com>,
        Dave Kleikamp <shaggy@kernel.org>, ceph-devel@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-ext4@vger.kernel.org,
        jfs-discussion@lists.sourceforge.net,
        Christian Brauner <brauner@kernel.org>, fdevel@quack3,
        Yang Xu <xuyang2018.jy@fujitsu.com>
Subject: Re: [PATCH v2] fs/{posix_acl,ext2,jfs,ceph}: apply umask if ACL
 support is disabled
Message-ID: <20231011100541.sfn3prgtmp7hk2oj@quack3>
References: <69dda7be-d7c8-401f-89f3-7a5ca5550e2f@oracle.com>
 <20231009144340.418904-1-max.kellermann@ionos.com>
 <20231010131125.3uyfkqbcetfcqsve@quack3>
 <CAKPOu+-nC2bQTZYL0XTzJL6Tx4Pi1gLfNWCjU2Qz1f_5CbJc1w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAKPOu+-nC2bQTZYL0XTzJL6Tx4Pi1gLfNWCjU2Qz1f_5CbJc1w@mail.gmail.com>
Precedence: bulk

On Tue 10-10-23 15:17:17, Max Kellermann wrote:
> On Tue, Oct 10, 2023 at 3:11 PM Jan Kara <jack@suse.cz> wrote:
> > Thanks for the updated changelog! But as I'm looking into VFS code isn't
> > this already handled by mode_strip_umask() / vfs_prepare_mode() in
> > fs/namei.c? Because posix_acl_create() doesn't do anything to 'mode' for
> > !IS_POSIXACL() filesystems either. So at least ext2 (where I've checked
> > the mount option handling) does seem to have umask properly applied in all
> > the cases. But I might be missing something...
> 
> I'm not sure either. I was hoping the VFS experts could tell something
> about how this API is supposed to be used and whose responsibility it
> is to apply the umask. There used to be some confusion in the code, to
> the point it was missing completely for O_TMPFILE. I'm still somewhat
> confused. Maybe this is a chance to clear this confusion up and then
> document it?

So I've checked some more and the kernel doc comments before
mode_strip_umask() and vfs_prepare_mode() make it pretty obvious - all
paths creating new inodes must be calling vfs_prepare_mode(). As a result
mode_strip_umask() which handles umask stripping for filesystems not
supporting posix ACLs. For filesystems that do support ACLs,
posix_acl_create() must be call and that handles umask stripping. So your
fix should not be needed. CCed some relevant people for confirmation.

> I wish there was one central place to apply the umask, and not spread
> it around two (or more?) different code locations, depending on
> whether there's an ACL. For my taste, that sort of policy is too error
> prone for something as sensitive as umasks. After we already had the
> O_TMPFILE vulnerability (which was only fixed last year, three
> years(!) after I reported it).

I agree having umask stripping in two places is not great but it's
difficult to avoid with how posix ACLs are implemented and intertwined in
various filesystem implementations. At least the current design made it
quite a bit harder to forget to strip the umask.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR