Re: [PATCH v1] fs: Fix inconsistent f_mode

From: Mickaël Salaün
Date: Tue Mar 01 2022 - 05:15:24 EST


On 01/03/2022 10:22, Christian Brauner wrote:
On Mon, Feb 28, 2022 at 10:59:35PM +0100, Mickaël Salaün wrote:
From: Mickaël Salaün <mic@xxxxxxxxxxxxxxxxxxx>

While transitionning to ACC_MODE() with commit 5300990c0370 ("Sanitize
f_flags helpers") and then fixing it with commit 6d125529c6cb ("Fix
ACC_MODE() for real"), we lost an open flags consistency check. Opening
a file with O_WRONLY | O_RDWR leads to an f_flags containing MAY_READ |
MAY_WRITE (thanks to the ACC_MODE() helper) and an empty f_mode.
Indeed, the OPEN_FMODE() helper transforms 3 (an incorrect value) to 0.

Fortunately, vfs_read() and vfs_write() both check for FMODE_READ, or
respectively FMODE_WRITE, and return an EBADF error if it is absent.
Before commit 5300990c0370 ("Sanitize f_flags helpers"), opening a file
with O_WRONLY | O_RDWR returned an EINVAL error. Let's restore this safe
behavior.

That specific part seems a bit risky at first glance. Given that the
patch referenced is from 2009 this means we've been allowing O_WRONLY |
O_RDWR to succeed for almost 13 years now.

Yeah, it's an old bug, but we should keep in mind that a file descriptor created with such flags cannot be used to read nor write. However, unfortunately, it can be used for things like ioctl, fstat, chdir… I don't know if there is any user of this trick.

Either way, there is an inconsistency between those using ACC_MODE() and those using OPEN_FMODE(). If we decide to take a side for the behavior of one or the other, without denying to create such FD, it could also break security policies. We have to choose what to potentially break…




To make it consistent with ACC_MODE(), this patch also changes
OPEN_FMODE() to return FMODE_READ | FMODE_WRITE for O_WRONLY | O_RDWR.
This may help protect from potential spurious issues.

This issue could result in inconsistencies with AppArmor, Landlock and
SELinux, but the VFS checks would still forbid read and write accesses.
Tomoyo uses the ACC_MODE() transformation which is correct, and Smack
doesn't check the file mode. Filesystems using OPEN_FMODE() should also
be protected by the VFS checks.

Fixes: 5300990c0370 ("Sanitize f_flags helpers")
Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
Cc: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
Cc: Darrick J. Wong <djwong@xxxxxxxxxx>
Cc: Eric Paris <eparis@xxxxxxxxxxxxxx>
Cc: John Johansen <john.johansen@xxxxxxxxxxxxx>
Cc: Kentaro Takeda <takedakn@xxxxxxxxxxxxx>
Cc: Miklos Szeredi <miklos@xxxxxxxxxx>
Cc: Paul Moore <paul@xxxxxxxxxxxxxx>
Cc: Stephen Smalley <stephen.smalley.work@xxxxxxxxx>
Cc: Steve French <sfrench@xxxxxxxxx>
Cc: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
Signed-off-by: Mickaël Salaün <mic@xxxxxxxxxxxxxxxxxxx>
Link: https://lore.kernel.org/r/20220228215935.748017-1-mic@xxxxxxxxxxx
---
fs/file_table.c | 3 +++
include/linux/fs.h | 5 +++--
2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index 7d2e692b66a9..b936f69525d0 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -135,6 +135,9 @@ static struct file *__alloc_file(int flags, const struct cred *cred)
struct file *f;
int error;
+ if ((flags & O_ACCMODE) == O_ACCMODE)
+ return ERR_PTR(-EINVAL);
+
f = kmem_cache_zalloc(filp_cachep, GFP_KERNEL);
if (unlikely(!f))
return ERR_PTR(-ENOMEM);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e2d892b201b0..83bc5aaf1c41 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3527,8 +3527,9 @@ int __init list_bdev_fs_names(char *buf, size_t size);
#define __FMODE_NONOTIFY ((__force int) FMODE_NONOTIFY)
#define ACC_MODE(x) ("\004\002\006\006"[(x)&O_ACCMODE])
-#define OPEN_FMODE(flag) ((__force fmode_t)(((flag + 1) & O_ACCMODE) | \
- (flag & __FMODE_NONOTIFY)))
+#define OPEN_FMODE(flag) ((__force fmode_t)( \
+ (((flag + 1) & O_ACCMODE) ?: O_ACCMODE) | \
+ (flag & __FMODE_NONOTIFY)))
static inline bool is_sxid(umode_t mode)
{

base-commit: 7e57714cd0ad2d5bb90e50b5096a0e671dec1ef3
--
2.35.1