linux-kernel - [RFC PATCH] f2fs: disallow setting unsettable file attributes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <AM6PR10MB2838F71E6117BFFAD3FD5A69FA249@AM6PR10MB2838.EURPRD10.PROD.OUTLOOK.COM>
Date:   Sun, 30 Jan 2022 17:21:39 +0000
From:   Harry Austen <harryausten@...mail.co.uk>
To:     linux-f2fs-devel@...ts.sourceforge.net
Cc:     linux-kernel@...r.kernel.org, jaegeuk@...nel.org, chao@...nel.org,
        ebiggers@...nel.org, Harry Austen <harryausten@...mail.co.uk>
Subject: [RFC PATCH] f2fs: disallow setting unsettable file attributes

After Eric kindly pointed out the reasons why my initial kernel patch
attempt was incorrect
(https://lore.kernel.org/lkml/Ye79OLCFLR3H+GnY@gmail.com/), I had a
rethink as to if the current implementation could be improved in any
way.

I wondered whether something along the lines of the following patch
would be more acceptable? It is intentionally verbose in order to
demonstrate the concept as this is intended purely as an RFC.

What if SETFLAGS returned EOPNOTSUPP if userspace is actually trying to
*set* one of the unsettable flags (i.e. it isn't set already)? I believe
this would therefore not break chattr(1), as flags that are retrieved
from GETFLAGS can still be passed into SETFLAGS without error.

If there is some other ABI compatibility that needs to be maintained
that is broken by this, then please let me know. Also, I have not yet
determined whether there are any concerns with calling f2fs_fileattr_get
from inside f2fs_fileattr_set, e.g. speed/performance? so any thoughts
would be greatly appreciated.

Signed-off-by: Harry Austen <harryausten@...mail.co.uk>
---
 fs/f2fs/file.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 3c98ef6af97d..3f3d67c1dfda 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3079,6 +3079,18 @@ int f2fs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 	return 0;
 }

+static bool f2fs_contains_unsettable_flags_not_already_set(struct dentry *dentry, u32 fsflags)
+{
+	struct fileattr old;
+
+	/* Get current file attribute flags */
+	f2fs_fileattr_get(dentry, &old);
+	/* Mask away flags that are already set */
+	fsflags &= ~old.flags;
+	/* Return true if any of the remaining flags are unsettable */
+	return (fsflags & ~F2FS_SETTABLE_FS_FL);
+}
+
 int f2fs_fileattr_set(struct user_namespace *mnt_userns,
 		      struct dentry *dentry, struct fileattr *fa)
 {
@@ -3093,6 +3105,8 @@ int f2fs_fileattr_set(struct user_namespace *mnt_userns,
 		return -ENOSPC;
 	if (fsflags & ~F2FS_GETTABLE_FS_FL)
 		return -EOPNOTSUPP;
+	if (f2fs_contains_unsettable_flags_not_already_set(dentry, fsflags))
+		return -EOPNOTSUPP;
 	fsflags &= F2FS_SETTABLE_FS_FL;
 	if (!fa->flags_valid)
 		mask &= FS_COMMON_FL;
-- 
2.35.1