[RFC] correct flags to f_mode conversion in __dentry_open

[RFC] correct flags to f_mode conversion in __dentry_open

[Eric Paris]

I recently tried to add an SELinux BUG_ON in the case where the kernel
made a permission request for no permissions and was able to stumble
over it with something as simple as

open("/dev/null", 3);

Notice that 3 == (O_RDWR | O_WRONLY)

First question, is 3 ever a valid flag from from userspace to sys_open?
I see in the comments proceeding do_filp_open()

/*
 * Note that while the flag value (low two bits) for sys_open means:
 *      00 - read-only
 *      01 - write-only
 *      10 - read-write
 *      11 - special
 * it is changed into
 *      00 - no permissions needed
 *      01 - read-permission
 *      10 - write-permission
 *      11 - read-write
 * for the internal routines (ie open_namei()/follow_link() etc). 00 is
 * used by symlinks.
 */

Where someone indicated that 11 is 'special.'  Does 'special' really
mean invalid?  And how should 'special' map to FMODE_*?

I also see that do_filp_open() does the mapping like:
        if ((namei_flags+1) & O_ACCMODE)
                namei_flags++;
and on another code path __dentry_open() is doing a similar mapping:
       f->f_mode = ((flags+1) & O_ACCMODE) | FMODE_LSEEK |
                               FMODE_PREAD | FMODE_PWRITE;

The issue at hand is that the pass through do_filp_open() with flags = 3
will result in the lower two bits still being 11 and SELinux will test
for RDWR.  But the pass through __dentry_open will result in an f_mode
of 00 and will result in SELinux hitting my new (not yet in kernel)
BUG_ON() since 11 was mapped to 00.

What is this mapping supposed to be?  What is 'special' supposed to
mean?  I added the following patch which makes the __dentry_open()
conversion more like the do_filp_open() conversion and my machine seems
to be working well and surviving/acting as the way I expected.  What
does 11 really mean and should it really always be mapped to (FMODE_READ
| FMODE_WRITE) or should it continue to get mapped to 'no permission?'

-Eric

---

diff --git a/fs/open.c b/fs/open.c
index 5419853..6e04926 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -736,10 +736,14 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
 {
 	struct inode *inode;
 	int error;
+	mode_t f_mode;
+
+	f_mode = flags & O_ACCMODE;
+	if ((f_mode+1) & O_ACCMODE)
+		f_mode++;
 
 	f->f_flags = flags;
-	f->f_mode = ((flags+1) & O_ACCMODE) | FMODE_LSEEK |
-				FMODE_PREAD | FMODE_PWRITE;
+	f->f_mode = f_mode | FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE;
 	inode = dentry->d_inode;
 	if (f->f_mode & FMODE_WRITE) {
 		error = get_write_access(inode);

Re: [RFC] correct flags to f_mode conversion in __dentry_open

[Al Viro]

On Wed, Mar 12, 2008 at 02:25:27PM -0400, Eric Paris wrote:
> I recently tried to add an SELinux BUG_ON in the case where the kernel
> made a permission request for no permissions and was able to stumble
> over it with something as simple as
> 
> open("/dev/null", 3);
> 
> Notice that 3 == (O_RDWR | O_WRONLY)
> 
> First question, is 3 ever a valid flag from from userspace to sys_open?

Yes.  "Check for both read and write permissions, set neither FMODE_READ
nor FMODE_WRITE".

Don't break drivers, please - some use that for "ioctl-only" opens,
with special semantics for those.

Re: [RFC] correct flags to f_mode conversion in __dentry_open

[Eric Paris]

On Wed, 2008-03-12 at 18:34 +0000, Al Viro wrote:
> On Wed, Mar 12, 2008 at 02:25:27PM -0400, Eric Paris wrote:
> > I recently tried to add an SELinux BUG_ON in the case where the kernel
> > made a permission request for no permissions and was able to stumble
> > over it with something as simple as
> > 
> > open("/dev/null", 3);
> > 
> > Notice that 3 == (O_RDWR | O_WRONLY)
> > 
> > First question, is 3 ever a valid flag from from userspace to sys_open?
> 
> Yes.  "Check for both read and write permissions, set neither FMODE_READ
> nor FMODE_WRITE".
> 
> Don't break drivers, please - some use that for "ioctl-only" opens,
> with special semantics for those.

Ok, I'll just make SELinux happy allow the request if we don't have
FMODE_READ or FMODE_WRITE set.

-Eric

Re: [RFC] correct flags to f_mode conversion in __dentry_open

[Andreas Schwab]

Eric Paris <eparis@redhat.com> writes:

> Notice that 3 == (O_RDWR | O_WRONLY)

Note that O_RDWR and O_WRONLY are not simple flags, but two possible
values of a 2-bit field (O_ACCMODE).  Thus it does not makes sense to
add them together.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [RFC] correct flags to f_mode conversion in __dentry_open

[Alan Cox]

On Wed, Mar 12, 2008 at 02:25:27PM -0400, Eric Paris wrote:
> I recently tried to add an SELinux BUG_ON in the case where the kernel
> made a permission request for no permissions and was able to stumble
> over it with something as simple as
> 
> open("/dev/null", 3);
> 
> Notice that 3 == (O_RDWR | O_WRONLY)
> 
> First question, is 3 ever a valid flag from from userspace to sys_open?

Yes.

> does 11 really mean and should it really always be mapped to (FMODE_READ
> | FMODE_WRITE) or should it continue to get mapped to 'no permission?'

We've always mapped 3 to "no permission" to read or write. It's a linuxism

Re: [RFC] correct flags to f_mode conversion in __dentry_open

[Alexander Viro]

On Sat, Mar 15, 2008 at 05:59:52PM -0400, Alan Cox wrote:
> On Wed, Mar 12, 2008 at 02:25:27PM -0400, Eric Paris wrote:
> > I recently tried to add an SELinux BUG_ON in the case where the kernel
> > made a permission request for no permissions and was able to stumble
> > over it with something as simple as
> > 
> > open("/dev/null", 3);
> > 
> > Notice that 3 == (O_RDWR | O_WRONLY)
> > 
> > First question, is 3 ever a valid flag from from userspace to sys_open?
> 
> Yes.
> 
> > does 11 really mean and should it really always be mapped to (FMODE_READ
> > | FMODE_WRITE) or should it continue to get mapped to 'no permission?'
> 
> We've always mapped 3 to "no permission" to read or write. It's a linuxism

Note that we *do* request MAY_READ|MAY_WRITE; "no permission" part is
about what you can do to resulting descriptor afterwards.

Re: [RFC] correct flags to f_mode conversion in __dentry_open

[Christoph Hellwig]

On Sat, Mar 15, 2008 at 05:59:52PM -0400, Alan Cox wrote:
> > does 11 really mean and should it really always be mapped to (FMODE_READ
> > | FMODE_WRITE) or should it continue to get mapped to 'no permission?'
> 
> We've always mapped 3 to "no permission" to read or write. It's a linuxism

I've tripped over this recently aswell.  It would for sure be useful
to add a sumbolic O_FOO constant for this magic value '3' and document
it in the manpage.

Re: [RFC] correct flags to f_mode conversion in __dentry_open

[Michael Kerrisk]

Christoph Hellwig wrote:
> On Sat, Mar 15, 2008 at 05:59:52PM -0400, Alan Cox wrote:
>>> does 11 really mean and should it really always be mapped to (FMODE_READ
>>> | FMODE_WRITE) or should it continue to get mapped to 'no permission?'
>> We've always mapped 3 to "no permission" to read or write. It's a linuxism
> 
> I've tripped over this recently aswell.  It would for sure be useful
> to add a sumbolic O_FOO constant for this magic value '3' and document
> it in the manpage.

Late follow-up to this thread
(http://thread.gmane.org/gmane.linux.kernel/653123):
I propose to add the following text to the open(2) man page.

       Unlike the other values that can be specified  in  flags,
       the access mode values O_RDONLY, O_WRONLY, and O_RDWR, do
       not specify individual bits.  Rather, they define the low
       order  two bits of flags, and are defined respectively as
       0, 1, and 2.  In other words, the combination O_RDONLY  |
       O_WRONLY  is a logical error, and certainly does not have
       the same meaning as O_RDWR.  Linux reserves the  special,
       non-standard  access mode 3 (binary 11) in flags to mean:
       check for read and  write  permission  on  the  file  and
       return  a  descriptor  that  can't be used for reading or
       writing.  This non-standard access mode is used  by  some
       Linux  drivers  to return a descriptor that is only to be
       used for device-specific ioctl(2) operations.

Seem okay?

Cheers,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html

Re: [RFC] correct flags to f_mode conversion in __dentry_open

[James Morris]

On Wed, 21 May 2008, Michael Kerrisk wrote:

> Christoph Hellwig wrote:
> > On Sat, Mar 15, 2008 at 05:59:52PM -0400, Alan Cox wrote:
> >>> does 11 really mean and should it really always be mapped to (FMODE_READ
> >>> | FMODE_WRITE) or should it continue to get mapped to 'no permission?'
> >> We've always mapped 3 to "no permission" to read or write. It's a linuxism
> > 
> > I've tripped over this recently aswell.  It would for sure be useful
> > to add a sumbolic O_FOO constant for this magic value '3' and document
> > it in the manpage.
> 
> Late follow-up to this thread
> (http://thread.gmane.org/gmane.linux.kernel/653123):
> I propose to add the following text to the open(2) man page.
> 
>        Unlike the other values that can be specified  in  flags,
>        the access mode values O_RDONLY, O_WRONLY, and O_RDWR, do
>        not specify individual bits.  Rather, they define the low
>        order  two bits of flags, and are defined respectively as
>        0, 1, and 2.  In other words, the combination O_RDONLY  |
>        O_WRONLY  is a logical error, and certainly does not have
>        the same meaning as O_RDWR.  Linux reserves the  special,
>        non-standard  access mode 3 (binary 11) in flags to mean:
>        check for read and  write  permission  on  the  file  and
>        return  a  descriptor  that  can't be used for reading or
>        writing.  This non-standard access mode is used  by  some
>        Linux  drivers  to return a descriptor that is only to be
>        used for device-specific ioctl(2) operations.
> 
> Seem okay?

Looks good to me.  We definitely need this documented.



- James
-- 
James Morris
<jmorris@namei.org>