Re: [PATCH v1] fs: Fix inconsistent f_mode

[Mickaël Salaün]

On 12/03/2022 16:17, Paul Moore wrote:
> On Fri, Mar 11, 2022 at 8:35 PM Tetsuo Handa
> <penguin-kernel@xxxxxxxxxxxxxxxxxxx> 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/20090212032821.GD28946@xxxxxxxxxxxxxxxxxx
> > 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.