Message-Id: <20180510231359.16899-2-mcgrof@kernel.org>
Date:   Thu, 10 May 2018 16:13:56 -0700
From:   "Luis R. Rodriguez" <mcgrof@...nel.org>
To:     viro@...iv.linux.org.uk, darrick.wong@...cle.com, tytso@....edu,
        adilger.kernel@...ger.ca, clm@...com, jbacik@...com,
        dsterba@...e.com
Cc:     sandeen@...deen.net, dhowells@...hat.com, fliu@...e.com,
        jack@...e.cz, jeffm@...e.com, nborisov@...e.com,
        jake.norris@...e.com, mtk.manpages@...il.com,
        linux-api@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-xfs@...r.kernel.org, linux-ext4@...r.kernel.org,
        linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org,
        "Luis R. Rodriguez" <mcgrof@...nel.org>
Subject: [RFC v2 1/4] vfs: skip extra attributes check on removal for symlinks

Linux filesystems cannot set extra file attributes (stx_attributes as per
statx(2)) on a symbolic link. To set extra file attributes you issue
ioctl(2) with FS_IOC_SETFLAGS, *all* ioctl(2) calls on a symbolic link
yield EBADF.

This is because ioctl(2) tries to obtain struct fd from the symbolic link
file descriptor passed using fdget(), fdget() in turn always returns no
file set when a file descriptor is open with O_PATH. As per symlink(2)
O_PATH and O_NOFOLLOW must *always* be used when you want to get the file
descriptor of a symbolic link, and this holds true for Linux, as such extra
file attributes cannot possibly be set on symbolic links on Linux.

Filesystems repair utilities should be updated to detect this as
corruption and correct this, however, the VFS *does* respect these
extra attributes on symlinks for removal.

Since we cannot set these attributes we should special-case the
immutable/append on delete for symlinks, this would be consistent with
what we *do* allow on Linux for all filesystems. Since this is a clear
sign to the VFS the filesystem must be corrupted filesystems can
implement a verifier to catch this earlier. A generic warning issued for
filesystems which don't implement these verifiers, and the VFS also lets
users delete these pesky symlinks as otherwise users cannot get rid of
them.

The userspace utility chattr(1) cannot set these attributes on symlinks
*and* other special files as well:

	# chattr -a symlink
	chattr: Operation not supported while reading flags on b

The reason for this is different though. Refer to commit 023d111e92195
("chattr.1.in: Document the compression attribute flags E, X, and ...")
merged on e2fsprogs v1.28 since August 2002. This commit prevented
issuing the ioctl() for symlink *and* special files in consideration for
a buggy DRM driver where issuing lsattr on their special files crashed
the system. For details refer to Debian bug 152029 [0].

You can craft your own tool to query the extra file attributes with
the new shiny statx(2) tool, statx(2) will list the attributes if
they were set for instance on a corrupt filesystem. However statx(2)
is only used for *querying* -- not for setting the attributes.

If you implement issuing your own ioctl() for FS_IOC_FSGETXATTR or
FS_IOC_FSSETXATTR on special files (block, char, fifo) it will fail
returning -1 and errno is set to ENOTTY (Inappropriate ioctl for
device). The reason for this is different than for symlinks.
For special files this fails on vfs_ioctl() when the filesystem
f_op callbacks are not set for these special files:

long vfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
{
	int error = -ENOTTY;

        if (!filp->f_op->unlocked_ioctl)
		goto out;

	error = filp->f_op->unlocked_ioctl(filp, cmd, arg);
	if (error == -ENOIOCTLCMD)
		error = -ENOTTY;
out:
	return error;
}

The same applies to PF_LOCAL named sockets. Since this varies by
filesystem for special files, only make a special rule to respect
the immutable and append attribute on symlinks.

[0] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=152029

Signed-off-by: Luis R. Rodriguez <mcgrof@...nel.org>
---
 fs/namei.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index e861b409c241..23ebc14805dc 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2760,6 +2760,26 @@ int __check_sticky(struct inode *dir, struct inode *inode)
 }
 EXPORT_SYMBOL(__check_sticky);
 
+/* Process extra file attributes only when they make sense */
+static bool may_delete_stx_attributes(struct inode *inode)
+{
+	/*
+	 * The VFS does not allow setting append/immutable on symlinks.
+	 *
+	 * Filesystems can implement their own verifier which would avoid this
+	 * generic splat, this generic splat is desirable if the respective
+	 * filesystem repair utility won't implement a fix for this, otherwise
+	 * users end up with a nagging dangling file which is impossible to
+	 * fix in userspace.
+	 */
+	if (S_ISLNK(inode->i_mode)) {
+		WARN_ONCE((IS_APPEND(inode) || IS_IMMUTABLE(inode)),
+		 "Immutable or append flag set on symlink. VFS does not allow this, must be a filesystem corruption. Allowing deletion though");
+	} else if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
+		return  false;
+	return  true;
+}
+
 /*
  *	Check whether we can remove a link victim from directory dir, check
  *  whether the type of victim is right.
@@ -2798,8 +2818,8 @@ static int may_delete(struct inode *dir, struct dentry *victim, bool isdir)
 	if (IS_APPEND(dir))
 		return -EPERM;
 
-	if (check_sticky(dir, inode) || IS_APPEND(inode) ||
-	    IS_IMMUTABLE(inode) || IS_SWAPFILE(inode) || HAS_UNMAPPED_ID(inode))
+	if (check_sticky(dir, inode) || !may_delete_stx_attributes(inode) ||
+	    IS_SWAPFILE(inode) || HAS_UNMAPPED_ID(inode))
 		return -EPERM;
 	if (isdir) {
 		if (!d_is_dir(victim))
-- 
2.17.0

Powered by blists - more mailing lists