| Message ID | 20181213115717.p7ncktng2ihxw5md@merlin (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: Evaluate O_WRONLY | O_RDWR to O_RDWR | expand |
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.
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.
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.
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...
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.
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
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(-)