Re: [PATCH] attr: block mode changes of symlinks

[Christian Brauner <brauner@kernel.org>]

On Wed, Jul 12, 2023 at 06:21:51PM +0200, Greg KH wrote:
> On Wed, Jul 12, 2023 at 11:56:35AM +0200, Christian Brauner wrote:
> > Changing the mode of symlinks is meaningless as the vfs doesn't take the
> > mode of a symlink into account during path lookup permission checking.
> > 
> > However, the vfs doesn't block mode changes on symlinks. This however,
> > has lead to an untenable mess roughly classifiable into the following
> > two categories:
> > 
> > (1) Filesystems that don't implement a i_op->setattr() for symlinks.
> > 
> >     Such filesystems may or may not know that without i_op->setattr()
> >     defined, notify_change() falls back to simple_setattr() causing the
> >     inode's mode in the inode cache to be changed.
> > 
> >     That's a generic issue as this will affect all non-size changing
> >     inode attributes including ownership changes.
> > 
> >     Example: afs
> > 
> > (2) Filesystems that fail with EOPNOTSUPP but change the mode of the
> >     symlink nonetheless.
> > 
> >     Some filesystems will happily update the mode of a symlink but still
> >     return EOPNOTSUPP. This is the biggest source of confusion for
> >     userspace.
> > 
> >     The EOPNOTSUPP in this case comes from POSIX ACLs. Specifically it
> >     comes from filesystems that call posix_acl_chmod(), e.g., btrfs via
> > 
> >         if (!err && attr->ia_valid & ATTR_MODE)
> >                 err = posix_acl_chmod(idmap, dentry, inode->i_mode);
> > 
> >     Filesystems including btrfs don't implement i_op->set_acl() so
> >     posix_acl_chmod() will report EOPNOTSUPP.
> > 
> >     When posix_acl_chmod() is called, most filesystems will have
> >     finished updating the inode.
> > 
> >     Perversely, this has the consequences that this behavior may depend
> >     on two kconfig options and mount options:
> > 
> >     * CONFIG_POSIX_ACL={y,n}
> >     * CONFIG_${FSTYPE}_POSIX_ACL={y,n}
> >     * Opt_acl, Opt_noacl
> > 
> >     Example: btrfs, ext4, xfs
> > 
> > The only way to change the mode on a symlink currently involves abusing
> > an O_PATH file descriptor in the following manner:
> > 
> >         fd = openat(-1, "/path/to/link", O_CLOEXEC | O_PATH | O_NOFOLLOW);
> > 
> >         char path[PATH_MAX];
> >         snprintf(path, sizeof(path), "/proc/self/fd/%d", fd);
> >         chmod(path, 0000);
> > 
> > But for most major filesystems with POSIX ACL support such as btrfs,
> > ext4, ceph, tmpfs, xfs and others this will fail with EOPNOTSUPP with
> > the mode still updated due to the aforementioned posix_acl_chmod()
> > nonsense.
> > 
> > So, given that for all major filesystems this would fail with EOPNOTSUPP
> > and that both glibc (cf. [1]) and musl (cf. [2]) outright block mode
> > changes on symlinks we should just try and block mode changes on
> > symlinks directly in the vfs and have a clean break with this nonsense.
> > 
> > If this causes any regressions, we do the next best thing and fix up all
> > filesystems that do return EOPNOTSUPP with the mode updated to not call
> > posix_acl_chmod() on symlinks.
> > 
> > But as usual, let's try the clean cut solution first. It's a simple
> > patch that can be easily reverted. Not marking this for backport as I'll
> > do that manually if we're reasonably sure that this works and there are
> > no strong objections.
> > 
> > We could block this in chmod_common() but it's more appropriate to do it
> > notify_change() as it will also mean that we catch filesystems that
> > change symlink permissions explicitly or accidently.
> > 
> > Similar proposals were floated in the past as in [3] and [4] and again
> > recently in [5]. There's also a couple of bugs about this inconsistency
> > as in [6] and [7].
> > 
> > Link: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/fchmodat.c;h=99527a3727e44cb8661ee1f743068f108ec93979;hb=HEAD [1]
> > Link: https://git.musl-libc.org/cgit/musl/tree/src/stat/fchmodat.c [2]
> > Link: https://lore.kernel.org/all/20200911065733.GA31579@infradead.org [3]
> > Link: https://sourceware.org/legacy-ml/libc-alpha/2020-02/msg00518.html [4]
> > Link: https://lore.kernel.org/lkml/87lefmbppo.fsf@oldenburg.str.redhat.com [5]
> > Link: https://sourceware.org/legacy-ml/libc-alpha/2020-02/msg00467.html [6]
> > Link: https://sourceware.org/bugzilla/show_bug.cgi?id=14578#c17 [7]
> > Cc: stable@vger.kernel.org # no backport before v6.6-rc2 is tagged
> 
> How far back should this go?

If this holds up without regressions than all LTSes. That's what Amir
and Leah did for some other work. I can add that to the comment for
clarity.