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

[Greg KH <gregkh@linuxfoundation.org>]

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?

thanks,

greg k-h