LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [RFC] correct flags to f_mode conversion in __dentry_open
@ 2008-03-12 18:25 Eric Paris
  2008-03-12 18:34 ` Al Viro
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Eric Paris @ 2008-03-12 18:25 UTC (permalink / raw)
  To: linux-kernel; +Cc: alan, aviro, drepper, hch, sds, jmorris

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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC] correct flags to f_mode conversion in __dentry_open
  2008-03-12 18:25 [RFC] correct flags to f_mode conversion in __dentry_open Eric Paris
@ 2008-03-12 18:34 ` Al Viro
  2008-03-12 18:41   ` Eric Paris
  2008-03-12 18:47 ` Andreas Schwab
  2008-03-15 21:59 ` Alan Cox
  2 siblings, 1 reply; 9+ messages in thread
From: Al Viro @ 2008-03-12 18:34 UTC (permalink / raw)
  To: Eric Paris; +Cc: linux-kernel, alan, aviro, drepper, hch, sds, jmorris

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC] correct flags to f_mode conversion in __dentry_open
  2008-03-12 18:34 ` Al Viro
@ 2008-03-12 18:41   ` Eric Paris
  0 siblings, 0 replies; 9+ messages in thread
From: Eric Paris @ 2008-03-12 18:41 UTC (permalink / raw)
  To: Al Viro; +Cc: linux-kernel, alan, aviro, drepper, hch, sds, jmorris


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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC] correct flags to f_mode conversion in __dentry_open
  2008-03-12 18:25 [RFC] correct flags to f_mode conversion in __dentry_open Eric Paris
  2008-03-12 18:34 ` Al Viro
@ 2008-03-12 18:47 ` Andreas Schwab
  2008-03-15 21:59 ` Alan Cox
  2 siblings, 0 replies; 9+ messages in thread
From: Andreas Schwab @ 2008-03-12 18:47 UTC (permalink / raw)
  To: Eric Paris; +Cc: linux-kernel

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."

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC] correct flags to f_mode conversion in __dentry_open
  2008-03-12 18:25 [RFC] correct flags to f_mode conversion in __dentry_open Eric Paris
  2008-03-12 18:34 ` Al Viro
  2008-03-12 18:47 ` Andreas Schwab
@ 2008-03-15 21:59 ` Alan Cox
  2008-03-15 22:00   ` Alexander Viro
  2008-03-17 10:45   ` Christoph Hellwig
  2 siblings, 2 replies; 9+ messages in thread
From: Alan Cox @ 2008-03-15 21:59 UTC (permalink / raw)
  To: Eric Paris; +Cc: linux-kernel, alan, aviro, drepper, hch, sds, jmorris

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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC] correct flags to f_mode conversion in __dentry_open
  2008-03-15 21:59 ` Alan Cox
@ 2008-03-15 22:00   ` Alexander Viro
  2008-03-17 10:45   ` Christoph Hellwig
  1 sibling, 0 replies; 9+ messages in thread
From: Alexander Viro @ 2008-03-15 22:00 UTC (permalink / raw)
  To: Alan Cox; +Cc: Eric Paris, linux-kernel, aviro, drepper, hch, sds, jmorris

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC] correct flags to f_mode conversion in __dentry_open
  2008-03-15 21:59 ` Alan Cox
  2008-03-15 22:00   ` Alexander Viro
@ 2008-03-17 10:45   ` Christoph Hellwig
  2008-05-21 17:54     ` Michael Kerrisk
  1 sibling, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2008-03-17 10:45 UTC (permalink / raw)
  To: Alan Cox
  Cc: Eric Paris, linux-kernel, aviro, drepper, hch, sds, jmorris,
	mtk.manpages

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC] correct flags to f_mode conversion in __dentry_open
  2008-03-17 10:45   ` Christoph Hellwig
@ 2008-05-21 17:54     ` Michael Kerrisk
  2008-05-22  2:10       ` James Morris
  0 siblings, 1 reply; 9+ messages in thread
From: Michael Kerrisk @ 2008-05-21 17:54 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Alan Cox, Eric Paris, linux-kernel, aviro, drepper, sds, jmorris

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC] correct flags to f_mode conversion in __dentry_open
  2008-05-21 17:54     ` Michael Kerrisk
@ 2008-05-22  2:10       ` James Morris
  0 siblings, 0 replies; 9+ messages in thread
From: James Morris @ 2008-05-22  2:10 UTC (permalink / raw)
  To: Michael Kerrisk
  Cc: Christoph Hellwig, Alan Cox, Eric Paris, linux-kernel, aviro,
	drepper, sds

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>

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2008-05-22  2:11 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-03-12 18:25 [RFC] correct flags to f_mode conversion in __dentry_open Eric Paris
2008-03-12 18:34 ` Al Viro
2008-03-12 18:41   ` Eric Paris
2008-03-12 18:47 ` Andreas Schwab
2008-03-15 21:59 ` Alan Cox
2008-03-15 22:00   ` Alexander Viro
2008-03-17 10:45   ` Christoph Hellwig
2008-05-21 17:54     ` Michael Kerrisk
2008-05-22  2:10       ` James Morris

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).