linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH] attr: block mode changes of symlinks
@ 2023-10-18 18:34 Jesse Hathaway
  2023-10-18 18:40 ` Greg KH
  0 siblings, 1 reply; 13+ messages in thread
From: Jesse Hathaway @ 2023-10-18 18:34 UTC (permalink / raw)
  To: Greg KH
  Cc: Linus Torvalds, Christoph Hellwig, Florian Weimer, Aleksa Sarai,
	linux-fsdevel, Al Viro, stable

> 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.

Unfortunately, this has not held up in LTSes without causing
regressions, specifically in crun:

Crun issue and patch
 1. https://github.com/containers/crun/issues/1308
 2. https://github.com/containers/crun/pull/1309

Debian bug report
 1. https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1053821

I think it should be reverted in LTSes and possibly in upstream.

Yours kindly, Jesse Hathaway

P.S. apologies for not having the correct threading headers. I am not on
the list.

^ permalink raw reply	[flat|nested] 13+ messages in thread
* [PATCH] attr: block mode changes of symlinks
@ 2023-07-12  9:56 Christian Brauner
  2023-07-12 16:21 ` Greg KH
  2023-07-12 16:24 ` Linus Torvalds
  0 siblings, 2 replies; 13+ messages in thread
From: Christian Brauner @ 2023-07-12  9:56 UTC (permalink / raw)
  To: Linus Torvalds, Christoph Hellwig, Florian Weimer
  Cc: Aleksa Sarai, linux-fsdevel, Al Viro, stable, Christian Brauner

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
Suggested-by: Christoph Hellwig <hch@lst.de>
Suggested-by: Florian Weimer <fweimer@redhat.com>
Reviewed-by: Aleksa Sarai <cyphar@cyphar.com>
Signed-off-by: Christian Brauner <brauner@kernel.org>
---

---
 fs/attr.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/attr.c b/fs/attr.c
index d60dc1edb526..529153ecde25 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -394,6 +394,9 @@ int notify_change(struct mnt_idmap *idmap, struct dentry *dentry,
 		return error;
 
 	if ((ia_valid & ATTR_MODE)) {
+		if (S_ISLNK(inode->i_mode))
+			return -EOPNOTSUPP;
+
 		umode_t amode = attr->ia_mode;
 		/* Flag setting protected by i_mutex */
 		if (is_sxid(amode))

---
base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
change-id: 20230712-vfs-chmod-symlinks-a3ad1923a992


^ permalink raw reply related	[flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-10-20 14:25 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-18 18:34 [PATCH] attr: block mode changes of symlinks Jesse Hathaway
2023-10-18 18:40 ` Greg KH
2023-10-18 18:49   ` Jesse Hathaway
2023-10-18 19:09     ` Greg KH
2023-10-20  8:34     ` Linux regression tracking (Thorsten Leemhuis)
2023-10-20 11:01       ` Christian Brauner
2023-10-20 13:26         ` Giuseppe Scrivano
2023-10-20 14:25         ` Greg KH
  -- strict thread matches above, loose matches on Subject: below --
2023-07-12  9:56 Christian Brauner
2023-07-12 16:21 ` Greg KH
2023-07-12 17:58   ` Christian Brauner
2023-07-12 16:24 ` Linus Torvalds
2023-07-12 17:56   ` Christian Brauner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).