linux-kernel - Re: [RFC] a corner case of open(2)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160427053458.GA30149@ZenIV.linux.org.uk>
Date:	Wed, 27 Apr 2016 06:34:58 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	Miklos Szeredi <mszeredi@...hat.com>
Subject: Re: [RFC] a corner case of open(2)

Fun bugs caught while trying to massage atomic_open()...  Patch below is
in vfs.git#for-linus (along with two more fixes); I would like to get
an ACK from Miklos on that one - it's his code and this thing had been
present in there since the original merge.  I might be misreading what
it tries to do, but
        open("/mnt/no-such-file", O_CREAT | O_TRUNC);
        perror("open"); errno = 0;
        stat("/mnt/no-such-file", &st);
        perror("stat"); errno = 0;
        open("/mnt/no-such-file", O_CREAT | O_TRUNC);
        perror("open");
should *not* end up with
	open: Read-only file system
	stat: No such file or directory
	open: No such file or directory
no matter what.  And it's very easy to arrange just that - mount nfs4
read-only on /mnt and run the snippet above.  First open() will fail with
EROFS (as it should), but as soon as the thing is in dcache we start getting
ENOENT.  Obviously bogus.

commit 1aa57f2aaa108ead7d81481af68085b0a77708f1
Author: Al Viro <viro@...iv.linux.org.uk>
Date:   Wed Apr 27 01:11:55 2016 -0400

    atomic_open(): fix the handling of create_error
    
    * if we have a hashed negative dentry and either CREAT|EXCL on
    r/o filesystem, or CREAT|TRUNC on r/o filesystem, or CREAT|EXCL
    with failing may_o_create(), we should fail with EROFS or the
    error may_o_create() has returned, but not ENOENT.  Which is what
    the current code ends up returning.
    
    * if we have CREAT|TRUNC hitting a regular file on a read-only
    filesystem, we can't fail with EROFS here.  At the very least,
    not until we'd done follow_managed() - we might have a writable
    file (or a device, for that matter) bound on top of that one.
    Moreover, the code downstream will see that O_TRUNC and attempt
    to grab the write access (*after* following possible mount), so
    if we really should fail with EROFS, it will happen.  No need
    to do that inside atomic_open().
    
    The real logics is much simpler than what the current code is
    trying to do - if we decided to go for simple lookup, ended
    up with a negative dentry *and* had create_error set, fail with
    create_error.  No matter whether we'd got that negative dentry
    from lookup_real() or had found it in dcache.
    
    Cc: stable@...r.kernel.org # v3.6+
    Signed-off-by: Al Viro <viro@...iv.linux.org.uk>

diff --git a/fs/namei.c b/fs/namei.c
index 1d9ca2d..b458992 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2942,22 +2942,10 @@ no_open:
 		dentry = lookup_real(dir, dentry, nd->flags);
 		if (IS_ERR(dentry))
 			return PTR_ERR(dentry);
-
-		if (create_error) {
-			int open_flag = op->open_flag;
-
-			error = create_error;
-			if ((open_flag & O_EXCL)) {
-				if (!dentry->d_inode)
-					goto out;
-			} else if (!dentry->d_inode) {
-				goto out;
-			} else if ((open_flag & O_TRUNC) &&
-				   d_is_reg(dentry)) {
-				goto out;
-			}
-			/* will fail later, go on to get the right error */
-		}
+	}
+	if (create_error && !dentry->d_inode) {
+		error = create_error;
+		goto out;
 	}
 looked_up:
 	path->dentry = dentry;