[RFC v2 0/4] vfs: detect symlink corruption with attributes

[RFC v2 0/4] vfs: detect symlink corruption with attributes

[Luis R. Rodriguez]

Filesystems which detect symlinks with append/immutable should inform
users their filesystem is corrupted and their respective filesystem
checker tool should fix this.

In lieu of this though users may be stuck with pesky files or
directories which they cannot remove. We cannot expect all filesystems
to be updated to address this, so since the VFS does not let filesystems
set these attributes -- let the VFS enable users to remove symlink with
these attributes, but also provide a fallback warning, in case the
users's own filesystem does not catch it.

Sending again as RFC as this just goes compile tested so far, and it
is still unclear if this is the direction we want to go with this.

v2:

As per Darrick, even though the VFS should probably allow not so popular
filesystems to delete corrupt symlinks with append/immutable -- popular
filesystems should check for this themselves and inform the users of
corruption. These filesystems should have their respective filesystem
checker tools updated to correct this as well.

v1:

Sent out a single patch just to ignore the append/immutable attributes
set on symlinks.

Luis R. Rodriguez (4):
  vfs: skip extra attributes check on removal for symlinks
  xfs: add verifier check for symlink with append/immutable flags
  ext4: add verifier check for symlink with append/immutable flags
  btrfs: verify symlinks with append/immutable flags

 fs/btrfs/inode.c                   |  9 +++++++++
 fs/ext4/inode.c                    |  7 +++++++
 fs/namei.c                         | 24 ++++++++++++++++++++++--
 fs/xfs/libxfs/xfs_symlink_remote.c |  5 +++++
 4 files changed, 43 insertions(+), 2 deletions(-)

-- 
2.17.0

[RFC v2 1/4] vfs: skip extra attributes check on removal for symlinks

[Luis R. Rodriguez]

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

[RFC v2 2/4] xfs: add verifier check for symlink with append/immutable flags

[Luis R. Rodriguez]

The Linux VFS does not allow a way to set append/immuttable attributes
to symlinks, this is just not possible. If this is detected we can
correct this with xfs_repair, so inform the user.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 fs/xfs/libxfs/xfs_symlink_remote.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_symlink_remote.c b/fs/xfs/libxfs/xfs_symlink_remote.c
index 5ef5f354587e..42dd81ede3d6 100644
--- a/fs/xfs/libxfs/xfs_symlink_remote.c
+++ b/fs/xfs/libxfs/xfs_symlink_remote.c
@@ -242,5 +242,10 @@ xfs_symlink_shortform_verify(
 	/* We /did/ null-terminate the buffer, right? */
 	if (*endp != 0)
 		return __this_address;
+
+	/* Immutable and append flags are not allowed on symlinks */
+	if (ip->i_d.di_flags & (XFS_DIFLAG_APPEND | XFS_DIFLAG_IMMUTABLE))
+		return __this_address;
+
 	return NULL;
 }
-- 
2.17.0

Re: [RFC v2 2/4] xfs: add verifier check for symlink with append/immutable flags

[Darrick J. Wong]

On Thu, May 10, 2018 at 04:13:57PM -0700, Luis R. Rodriguez wrote:
> The Linux VFS does not allow a way to set append/immuttable attributes
> to symlinks, this is just not possible. If this is detected we can
> correct this with xfs_repair, so inform the user.
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_symlink_remote.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_symlink_remote.c b/fs/xfs/libxfs/xfs_symlink_remote.c
> index 5ef5f354587e..42dd81ede3d6 100644
> --- a/fs/xfs/libxfs/xfs_symlink_remote.c
> +++ b/fs/xfs/libxfs/xfs_symlink_remote.c
> @@ -242,5 +242,10 @@ xfs_symlink_shortform_verify(
>  	/* We /did/ null-terminate the buffer, right? */
>  	if (*endp != 0)
>  		return __this_address;
> +
> +	/* Immutable and append flags are not allowed on symlinks */
> +	if (ip->i_d.di_flags & (XFS_DIFLAG_APPEND | XFS_DIFLAG_IMMUTABLE))
> +		return __this_address;

This belongs in xfs_dinode_verify so that it checks all symlinks, not
just the one shortform ones.

--D

> +
>  	return NULL;
>  }
> -- 
> 2.17.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC v2 3/4] ext4: add verifier check for symlink with append/immutable flags

[Luis R. Rodriguez]

The Linux VFS does not allow a way to set append/immuttable
attributes to symlinks, this is just not possible. If this is
detected inform the user as the filesystem must be corrupted.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 fs/ext4/inode.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 37a2f7a2b66a..6acf0dd6b6e6 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4947,6 +4947,13 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
 		inode->i_op = &ext4_dir_inode_operations;
 		inode->i_fop = &ext4_dir_operations;
 	} else if (S_ISLNK(inode->i_mode)) {
+		/* VFS does not allow setting these so must be corruption */
+		if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) {
+			EXT4_ERROR_INODE(inode,
+			  "immutable or append flags not allowed on symlinks");
+			ret = -EFSCORRUPTED;
+			goto bad_inode;
+		}
 		if (ext4_encrypted_inode(inode)) {
 			inode->i_op = &ext4_encrypted_symlink_inode_operations;
 			ext4_set_aops(inode);
-- 
2.17.0

Re: [RFC v2 3/4] ext4: add verifier check for symlink with append/immutable flags

[Jan Kara]

On Thu 10-05-18 16:13:58, Luis R. Rodriguez wrote:
> The Linux VFS does not allow a way to set append/immuttable
> attributes to symlinks, this is just not possible. If this is
> detected inform the user as the filesystem must be corrupted.
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/inode.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 37a2f7a2b66a..6acf0dd6b6e6 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4947,6 +4947,13 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
>  		inode->i_op = &ext4_dir_inode_operations;
>  		inode->i_fop = &ext4_dir_operations;
>  	} else if (S_ISLNK(inode->i_mode)) {
> +		/* VFS does not allow setting these so must be corruption */
> +		if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) {
> +			EXT4_ERROR_INODE(inode,
> +			  "immutable or append flags not allowed on symlinks");
> +			ret = -EFSCORRUPTED;
> +			goto bad_inode;
> +		}
>  		if (ext4_encrypted_inode(inode)) {
>  			inode->i_op = &ext4_encrypted_symlink_inode_operations;
>  			ext4_set_aops(inode);
> -- 
> 2.17.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [RFC v2 3/4] ext4: add verifier check for symlink with append/immutable flags

[Theodore Y. Ts'o]

On Fri, May 11, 2018 at 11:12:18PM +0200, Jan Kara wrote:
> On Thu 10-05-18 16:13:58, Luis R. Rodriguez wrote:
> > The Linux VFS does not allow a way to set append/immuttable
> > attributes to symlinks, this is just not possible. If this is
> > detected inform the user as the filesystem must be corrupted.
> > 
> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> 
> Looks good to me. You can add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>

Applied into the ext4 tree after verifying that e2fsck already handles
this case:

% e2fsck -fy /tmp/foo.img
e2fsck 1.44.1 (24-Mar-2018)
Pass 1: Checking inodes, blocks, and sizes
Special (device/socket/fifo/symlink) file (inode 13) has immutable
or append-only flag set.  Clear? yes

(The btrfs and xfs maintainers might want to make a similar check
before accepting their respective patches.)

						- Ted

[RFC v2 4/4] btrfs: verify symlinks with append/immutable flags

[Luis R. Rodriguez]

The Linux VFS does not allow a way to set append/immuttable
attributes to symlinks, this is just not possible. If this is
detected inform the user as the filesystem must be corrupted.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 fs/btrfs/inode.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index c4bdb597b323..d9c786be408c 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3933,6 +3933,15 @@ static int btrfs_read_locked_inode(struct inode *inode)
 		inode->i_op = &btrfs_dir_inode_operations;
 		break;
 	case S_IFLNK:
+		/* VFS does not allow setting these so must be corruption */
+		if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) {
+			ret = -EUCLEAN;
+			btrfs_crit(fs_info,
+				  "corrupt symlink with append/immutable ino=%llu root=%llu\n",
+				  btrfs_ino(BTRFS_I(inode)),
+				  root->root_key.objectid);
+			goto make_bad;
+		}
 		inode->i_op = &btrfs_symlink_inode_operations;
 		inode_nohighmem(inode);
 		inode->i_mapping->a_ops = &btrfs_symlink_aops;
-- 
2.17.0

Re: [RFC v2 4/4] btrfs: verify symlinks with append/immutable flags

[David Sterba]

On Thu, May 10, 2018 at 04:13:59PM -0700, Luis R. Rodriguez wrote:
> The Linux VFS does not allow a way to set append/immuttable
                                                   ^^^^^^^^^^

Typo, in all 3 patches.

> attributes to symlinks, this is just not possible. If this is
> detected inform the user as the filesystem must be corrupted.
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  fs/btrfs/inode.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index c4bdb597b323..d9c786be408c 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -3933,6 +3933,15 @@ static int btrfs_read_locked_inode(struct inode *inode)
>  		inode->i_op = &btrfs_dir_inode_operations;
>  		break;
>  	case S_IFLNK:
> +		/* VFS does not allow setting these so must be corruption */
> +		if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) {
> +			ret = -EUCLEAN;
> +			btrfs_crit(fs_info,
> +				  "corrupt symlink with append/immutable ino=%llu root=%llu\n",

no "\n" and please un-indent the string so it fits 80 columns.

> +				  btrfs_ino(BTRFS_I(inode)),
> +				  root->root_key.objectid);
> +			goto make_bad;

I found some error handling issues, before the switch, there's
btrfs_free_path and there's one more at the make_bad label.

To fix that, please set path = NULL after the first btrfs_free_path, it
can handle a NULL when it's called again.

Next thing I'm not sure about are the ACLs that get initialized in some
cases. There's cache_no_acl() that only resets the inode::i_acl and
inode::i_default_acl, so I think this should be called too. Thanks.