[v1] fs: Fix inconsistent f_mode

Commit Message

Mickaël Salaün Feb. 28, 2022, 9:59 p.m. UTC

From: Mickaël Salaün <[e-mail]>

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.

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 <[e-mail]>
Cc: Casey Schaufler <[e-mail]>
Cc: Darrick J. Wong <[e-mail]>
Cc: Eric Paris <[e-mail]>
Cc: John Johansen <[e-mail]>
Cc: Kentaro Takeda <[e-mail]>
Cc: Miklos Szeredi <[e-mail]>
Cc: Paul Moore <[e-mail]>
Cc: Stephen Smalley <[e-mail]>
Cc: Steve French <[e-mail]>
Cc: Tetsuo Handa <[e-mail]>
Signed-off-by: Mickaël Salaün <[e-mail]>
Link: https://lore.kernel.org/r/[e-mail]
---
 fs/file_table.c    | 3 +++
 include/linux/fs.h | 5 +++--
 2 files changed, 6 insertions(+), 2 deletions(-)


base-commit: 7e57714cd0ad2d5bb90e50b5096a0e671dec1ef3

Comments

Christian Brauner March 1, 2022, 9:22 a.m. UTC | #1

On Mon, Feb 28, 2022 at 10:59:35PM +0100, Mickaël Salaün wrote:
> From: Mickaël Salaün <[e-mail]>
> 
> 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.

> 
> 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 <[e-mail]>
> Cc: Casey Schaufler <[e-mail]>
> Cc: Darrick J. Wong <[e-mail]>
> Cc: Eric Paris <[e-mail]>
> Cc: John Johansen <[e-mail]>
> Cc: Kentaro Takeda <[e-mail]>
> Cc: Miklos Szeredi <[e-mail]>
> Cc: Paul Moore <[e-mail]>
> Cc: Stephen Smalley <[e-mail]>
> Cc: Steve French <[e-mail]>
> Cc: Tetsuo Handa <[e-mail]>
> Signed-off-by: Mickaël Salaün <[e-mail]>
> Link: https://lore.kernel.org/r/[e-mail]
> ---
>  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
>

Mickaël Salaün March 1, 2022, 10:15 a.m. UTC | #2

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 <[e-mail]>
>>
>> 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 <[e-mail]>
>> Cc: Casey Schaufler <[e-mail]>
>> Cc: Darrick J. Wong <[e-mail]>
>> Cc: Eric Paris <[e-mail]>
>> Cc: John Johansen <[e-mail]>
>> Cc: Kentaro Takeda <[e-mail]>
>> Cc: Miklos Szeredi <[e-mail]>
>> Cc: Paul Moore <[e-mail]>
>> Cc: Stephen Smalley <[e-mail]>
>> Cc: Steve French <[e-mail]>
>> Cc: Tetsuo Handa <[e-mail]>
>> Signed-off-by: Mickaël Salaün <[e-mail]>
>> Link: https://lore.kernel.org/r/[e-mail]
>> ---
>>   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
>>

Paul Moore March 9, 2022, 9:31 p.m. UTC | #3

On Tue, Mar 1, 2022 at 5:15 AM Mickaël Salaün <[e-mail]> wrote:
> 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 <[e-mail]>
> >>
> >> 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…

I'm not really liking the idea that the empty/0 f_mode field leads to
SELinux doing an ioctl access check as opposed to the expected
read|write check.  Yes, other parts of the code catch the problem, but
this is bad from a SELinux perspective.  Looking quickly at the other
LSMs, it would appear that other LSMs are affected as well.

If we're not going to fix file::f_mode, the LSMs probably need to
consider using file::f_flags directly in conjunction with a correct
OPEN_FMODE() macro (or better yet a small inline function that isn't
as ugly).

John Johansen March 10, 2022, 12:36 a.m. UTC | #4

On 3/9/22 13:31, Paul Moore wrote:
> On Tue, Mar 1, 2022 at 5:15 AM Mickaël Salaün <[e-mail]> wrote:
>> 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 <[e-mail]>
>>>>
>>>> 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…
> 
> I'm not really liking the idea that the empty/0 f_mode field leads to
> SELinux doing an ioctl access check as opposed to the expected
> read|write check.  Yes, other parts of the code catch the problem, but
> this is bad from a SELinux perspective.  Looking quickly at the other
> LSMs, it would appear that other LSMs are affected as well.
> 
> If we're not going to fix file::f_mode, the LSMs probably need to
> consider using file::f_flags directly in conjunction with a correct
> OPEN_FMODE() macro (or better yet a small inline function that isn't
> as ugly).
> 
yeah, I have to agree

Paul Moore March 11, 2022, 10:15 p.m. UTC | #5

On Wed, Mar 9, 2022 at 7:36 PM John Johansen
<[e-mail]> wrote:
> On 3/9/22 13:31, Paul Moore wrote:
> > On Tue, Mar 1, 2022 at 5:15 AM Mickaël Salaün <[e-mail]> wrote:
> >> 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 <[e-mail]>
> >>>>
> >>>> 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…
> >
> > I'm not really liking the idea that the empty/0 f_mode field leads to
> > SELinux doing an ioctl access check as opposed to the expected
> > read|write check.  Yes, other parts of the code catch the problem, but
> > this is bad from a SELinux perspective.  Looking quickly at the other
> > LSMs, it would appear that other LSMs are affected as well.
> >
> > If we're not going to fix file::f_mode, the LSMs probably need to
> > consider using file::f_flags directly in conjunction with a correct
> > OPEN_FMODE() macro (or better yet a small inline function that isn't
> > as ugly).
> >
> yeah, I have to agree

The silence on this has been deafening :/  No thoughts on fixing, or
not fixing OPEN_FMODE(), Al?

At this point I have to assume OPEN_FMODE() isn't changing so I'm
going to go ahead with moving SELinux over to file::f_flags.  Once
I've got something working I'll CC the LSM list on the patches in case
the other LSMs want to do something similar.  Full disclosure, that
might not happen until early-to-mid next week due to the weekend, new
kernel expected on Sunday, etc.

Tetsuo Handa March 12, 2022, 1:34 a.m. UTC | #6

On 2022/03/12 7:15, Paul Moore wrote:
> The silence on this has been deafening :/  No thoughts on fixing, or
> not fixing OPEN_FMODE(), Al?

On 2022/03/01 19:15, Mickaël Salaün wrote:
> 
> On 01/03/2022 10:22, Christian Brauner wrote:
>> 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.

I got a reply from Al at https://lkml.kernel.org/r/[e-mail]
that sys_open(path, 3) is for ioctls only. And I'm using this trick when opening something
for ioctls only.

Paul Moore March 12, 2022, 3:17 p.m. UTC | #7

On Fri, Mar 11, 2022 at 8:35 PM Tetsuo Handa
<[e-mail]> wrote:
> On 2022/03/12 7:15, Paul Moore wrote:
> > The silence on this has been deafening :/  No thoughts on fixing, or
> > not fixing OPEN_FMODE(), Al?
>
> On 2022/03/01 19:15, Mickaël Salaün wrote:
> >
> > On 01/03/2022 10:22, Christian Brauner wrote:
> >> 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.
>
> I got a reply from Al at https://lkml.kernel.org/r/[e-mail]
> that sys_open(path, 3) is for ioctls only. And I'm using this trick when opening something
> for ioctls only.

Thanks Tetsuo, that's helpful.  After reading your email I went
digging around to see if this was documented anywhere, and buried in
the open(2) manpage, towards the bottom under the "File access mode"
header, is this paragraph:

 "Linux reserves the special, nonstandard access mode 3 (binary 11)
  in flags to mean: check for read and write permission on the file
  and return a file descriptor that can't be used for reading or
  writing.  This nonstandard access mode is used by some Linux
  drivers to return a file descriptor that is to be used only for
  device-specific ioctl(2) operations."

I learned something new today :)  With this in mind it looks like
doing a SELinux file:ioctl check is the correct thing to do.

Thanks again Tetsuo for clearing things up.

Al Viro March 13, 2022, 12:42 a.m. UTC | #8

On Fri, Mar 11, 2022 at 05:15:01PM -0500, Paul Moore wrote:

> The silence on this has been deafening :/  No thoughts on fixing, or
> not fixing OPEN_FMODE(), Al?
> 
> At this point I have to assume OPEN_FMODE() isn't changing so I'm
> going to go ahead with moving SELinux over to file::f_flags.  Once
> I've got something working I'll CC the LSM list on the patches in case
> the other LSMs want to do something similar.  Full disclosure, that
> might not happen until early-to-mid next week due to the weekend, new
> kernel expected on Sunday, etc.

ENOBUG.  The primary user of that is fdutils; they wanted to be able
to issue ioctls on a floppy disk drive, with no disk inserted.  Or with
a disk that has weird formatting (as the matter of fact, some of those
ioctls are precisely "use such-and-such weird format").

A cleaner solution would be to have a separate device node (or sysfs
file, or...) for that kind of OOB stuff.  However, that's not how it
had been done way back when, and we are stuck with the existing ABI.
Namely, "have the lower two bits of flags both set" for "open for
ioctls only; require MAY_READ|MAY_WRITE on device node, allow
neither read() nor write() on that descriptor".

I'm not sure if anyone _uses_ fdutils these days.  OTOH, you never
know what kind of weird setups gets used, and qemu has floppy
emulation, so we can't even go for "no floppy drive is going to
be in working condition nowadays".

So I'm afraid that this ABI is going to stay ;-/  It's a long-standing
wart, from at least '94 if not earlier, it's documented (open(2)) and
it's used by a package that is shipped at least by debian and ubuntu.

And it's certainly *not* the kind of code anyone sane would want to
migrate to a replacement ABI, no matter how nice - look through the
list of utilities in there and imagine what the testing for regressions
would feel like.

Al Viro March 13, 2022, 12:44 a.m. UTC | #9

On Sat, Mar 12, 2022 at 10:34:27AM +0900, Tetsuo Handa wrote:
> On 2022/03/12 7:15, Paul Moore wrote:
> > The silence on this has been deafening :/  No thoughts on fixing, or
> > not fixing OPEN_FMODE(), Al?
> 
> On 2022/03/01 19:15, Mickaël Salaün wrote:
> > 
> > On 01/03/2022 10:22, Christian Brauner wrote:
> >> 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.
> 
> I got a reply from Al at https://lkml.kernel.org/r/[e-mail]
> that sys_open(path, 3) is for ioctls only. And I'm using this trick when opening something
> for ioctls only.

... so it's not just fdutils.  Cast in stone, IOW.

Mickaël Salaün March 14, 2022, 8:22 a.m. UTC | #10

On 12/03/2022 16:17, Paul Moore wrote:
> On Fri, Mar 11, 2022 at 8:35 PM Tetsuo Handa
> <[e-mail]> wrote:
>> On 2022/03/12 7:15, Paul Moore wrote:
>>> The silence on this has been deafening :/  No thoughts on fixing, or
>>> not fixing OPEN_FMODE(), Al?
>>
>> On 2022/03/01 19:15, Mickaël Salaün wrote:
>>>
>>> On 01/03/2022 10:22, Christian Brauner wrote:
>>>> 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.
>>
>> I got a reply from Al at https://lkml.kernel.org/r/[e-mail]
>> that sys_open(path, 3) is for ioctls only. And I'm using this trick when opening something
>> for ioctls only.
> 
> Thanks Tetsuo, that's helpful.  After reading your email I went
> digging around to see if this was documented anywhere, and buried in
> the open(2) manpage, towards the bottom under the "File access mode"
> header, is this paragraph:
> 
>   "Linux reserves the special, nonstandard access mode 3 (binary 11)
>    in flags to mean: check for read and write permission on the file
>    and return a file descriptor that can't be used for reading or
>    writing.  This nonstandard access mode is used by some Linux
>    drivers to return a file descriptor that is to be used only for
>    device-specific ioctl(2) operations."

Interesting, I missed the reference to this special value in the man page.

> 
> I learned something new today :)  With this in mind it looks like
> doing a SELinux file:ioctl check is the correct thing to do.

Indeed, SELinux uses it in an early ioctl check, but it still seems 
inconsistent (without being a bug) with the handling of the other value 
of this flag. This FD can also be used for chdir or other inode-related 
actions, which may not involve ioctl.

However, it seems there is a more visible inconsistency with the way 
Tomoyo checks for read, write (because of the ACC_MODE use) *and* ioctl 
rights for an ioctl action. At least, the semantic is not the same and 
is not reflected in the documentation.

Because AppArmor and Landlock don't support ioctl, this looks fine for them.

Patch

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)
 {