[PATCH] fs: Evaluate O_WRONLY | O_RDWR to O_RDWR

[PATCH] fs: Evaluate O_WRONLY | O_RDWR to O_RDWR

[Goldwyn Rodrigues]

A user can open(O_WRONLY | O_RDWR) and the options are valid.
However, OPEN_FMODE() evaluates both FMODE_READ and FMODE_WRITE,
as negative. We also need to protect the lower layers from this
anomaly.

Solve it by dropping O_WRONLY, so O_RDWR takes precedence.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
Reported-by: syzbot+ae82084b07d0297e566b@syzkaller.appspotmail.com
Fixes: a408e4a86b36 ("ima: open a new file instance if no read permissions")
---
 fs/open.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/open.c b/fs/open.c
index 0285ce7dbd51..3d0a21600fa3 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -924,7 +924,12 @@ EXPORT_SYMBOL(open_with_fake_path);
 static inline int build_open_flags(int flags, umode_t mode, struct open_flags *op)
 {
 	int lookup_flags = 0;
-	int acc_mode = ACC_MODE(flags);
+	int acc_mode;
+
+	if ((flags & (O_RDWR | O_WRONLY)) == (O_RDWR | O_WRONLY))
+		flags &= ~O_WRONLY;
+
+	acc_mode = ACC_MODE(flags);
 
 	/*
 	 * Clear out all open flags we don't know about so that we don't report
-- 
2.16.4

Re: [PATCH] fs: Evaluate O_WRONLY | O_RDWR to O_RDWR

[Al Viro]

On Thu, Dec 13, 2018 at 05:57:17AM -0600, Goldwyn Rodrigues wrote:
> A user can open(O_WRONLY | O_RDWR) and the options are valid.
> However, OPEN_FMODE() evaluates both FMODE_READ and FMODE_WRITE,
> as negative. We also need to protect the lower layers from this
> anomaly.
> 
> Solve it by dropping O_WRONLY, so O_RDWR takes precedence.

Congratulations, you've broken fdutils...  Passing 3 in lower bits
of open() flags is *not* the same as O_RDWR; behaiviour is
different and deliberately chosen by existing userland code.

IOW, NAK.

Re: [PATCH] fs: Evaluate O_WRONLY | O_RDWR to O_RDWR

[Amir Goldstein]

On Thu, Dec 13, 2018 at 2:10 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Thu, Dec 13, 2018 at 05:57:17AM -0600, Goldwyn Rodrigues wrote:
> > A user can open(O_WRONLY | O_RDWR) and the options are valid.
> > However, OPEN_FMODE() evaluates both FMODE_READ and FMODE_WRITE,
> > as negative. We also need to protect the lower layers from this
> > anomaly.
> >
> > Solve it by dropping O_WRONLY, so O_RDWR takes precedence.
>
> Congratulations, you've broken fdutils...  Passing 3 in lower bits
> of open() flags is *not* the same as O_RDWR; behaiviour is
> different and deliberately chosen by existing userland code.
>
> IOW, NAK.

Yap, sorry, I missed FMODE_WRITE_IOCTL when I made this suggestion.
open mode 3 seems to be reserved to some special case of ioctl to floppy
block device.

I guess it would be simpler for you to accommodate for the special case
in ima_calc_file_hash() by masking out O_ACCMODE from flags before
adding O_RDONLY.

Thanks,
Amir.

Re: [PATCH] fs: Evaluate O_WRONLY | O_RDWR to O_RDWR

[Dmitry Vyukov]

On Thu, Dec 13, 2018 at 1:10 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Thu, Dec 13, 2018 at 05:57:17AM -0600, Goldwyn Rodrigues wrote:
> > A user can open(O_WRONLY | O_RDWR) and the options are valid.
> > However, OPEN_FMODE() evaluates both FMODE_READ and FMODE_WRITE,
> > as negative. We also need to protect the lower layers from this
> > anomaly.
> >
> > Solve it by dropping O_WRONLY, so O_RDWR takes precedence.
>
> Congratulations, you've broken fdutils...  Passing 3 in lower bits
> of open() flags is *not* the same as O_RDWR; behaiviour is
> different and deliberately chosen by existing userland code.
>
> IOW, NAK.

I am eager to hear what it means then.

Re: [PATCH] fs: Evaluate O_WRONLY | O_RDWR to O_RDWR

[Al Viro]

On Thu, Dec 13, 2018 at 01:16:45PM +0100, Dmitry Vyukov wrote:
> On Thu, Dec 13, 2018 at 1:10 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Thu, Dec 13, 2018 at 05:57:17AM -0600, Goldwyn Rodrigues wrote:
> > > A user can open(O_WRONLY | O_RDWR) and the options are valid.
> > > However, OPEN_FMODE() evaluates both FMODE_READ and FMODE_WRITE,
> > > as negative. We also need to protect the lower layers from this
> > > anomaly.
> > >
> > > Solve it by dropping O_WRONLY, so O_RDWR takes precedence.
> >
> > Congratulations, you've broken fdutils...  Passing 3 in lower bits
> > of open() flags is *not* the same as O_RDWR; behaiviour is
> > different and deliberately chosen by existing userland code.
> >
> > IOW, NAK.
> 
> I am eager to hear what it means then.

Open for ioctls, basically.  No read/write allowed, no checks for
media writability, etc. done at open() time, both read and write
*permissions* required from device node.

Think of the things like formatting a floppy, for example...

Re: [PATCH] fs: Evaluate O_WRONLY | O_RDWR to O_RDWR

[Goldwyn Rodrigues]

On 12:34 13/12, Al Viro wrote:
> On Thu, Dec 13, 2018 at 01:16:45PM +0100, Dmitry Vyukov wrote:
> > On Thu, Dec 13, 2018 at 1:10 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> > >
> > > On Thu, Dec 13, 2018 at 05:57:17AM -0600, Goldwyn Rodrigues wrote:
> > > > A user can open(O_WRONLY | O_RDWR) and the options are valid.
> > > > However, OPEN_FMODE() evaluates both FMODE_READ and FMODE_WRITE,
> > > > as negative. We also need to protect the lower layers from this
> > > > anomaly.
> > > >
> > > > Solve it by dropping O_WRONLY, so O_RDWR takes precedence.
> > >
> > > Congratulations, you've broken fdutils...  Passing 3 in lower bits
> > > of open() flags is *not* the same as O_RDWR; behaiviour is
> > > different and deliberately chosen by existing userland code.
> > >
> > > IOW, NAK.
> > 
> > I am eager to hear what it means then.
> 
> Open for ioctls, basically.  No read/write allowed, no checks for
> media writability, etc. done at open() time, both read and write
> *permissions* required from device node.
> 
> Think of the things like formatting a floppy, for example...

Sorry, I was not aware of the ioctl usage. Thanks.

-- 
Goldwyn