linux-kernel - [PATCH RESEND 1/1] vfs: Really check for inode ptr in lookup

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-next>] [day] [month] [year] [list]

Message-Id: <20190927044243.18856-1-riteshh@linux.ibm.com>
Date:   Fri, 27 Sep 2019 10:12:43 +0530
From:   Ritesh Harjani <riteshh@...ux.ibm.com>
To:     viro@...iv.linux.org.uk
Cc:     linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        wugyuan@...ibm.com, jlayton@...nel.org, hsiangkao@....com,
        riteshh@...ux.ibm.com
Subject: [PATCH RESEND 1/1] vfs: Really check for inode ptr in lookup_fast

d_is_negative can race with d_instantiate_new()
-> __d_set_inode_and_type().
For e.g. in use cases where Thread-1 is creating
symlink (doing d_instantiate_new()) & Thread-2 is doing
cat of that symlink while doing lookup_fast (via REF-walk-
one such case is, when ->permission returns -ECHILD).

During this race if __d_set_and_inode_type() does out-of-order
execution and set the dentry->d_flags before setting
dentry->inode, then it can result into following kernel panic.

This change fixes the issue by directly checking for inode.

E.g. kernel panic, since inode was NULL.
trailing_symlink() -> may_follow_link() -> inode->i_uid.
Issue signature:-
  [NIP  : trailing_symlink+80]
  [LR   : trailing_symlink+1092]
  #4 [c00000198069bb70] trailing_symlink at c0000000004bae60  (unreliable)
  #5 [c00000198069bc00] path_openat at c0000000004bdd14
  #6 [c00000198069bc90] do_filp_open at c0000000004c0274
  #7 [c00000198069bdb0] do_sys_open at c00000000049b248
  #8 [c00000198069be30] system_call at c00000000000b388

Sequence of events:-
Thread-2(Comm: ln) 	       Thread-1(Comm: cat)

	                dentry = __d_lookup() //nonRCU

__d_set_and_inode_type() (Out-of-order execution)
    flags = READ_ONCE(dentry->d_flags);
    flags &= ~(DCACHE_ENTRY_TYPE | DCACHE_FALLTHRU);
    flags |= type_flags;
    WRITE_ONCE(dentry->d_flags, flags);

	                if (unlikely(d_is_negative()) // fails
	                       {}
	                // since d_flags is already updated in
	                // Thread-2 in parallel but inode
	                // not yet set.
	                // d_is_negative returns false

	                *inode = d_backing_inode(path->dentry);
	                // means inode is still NULL

    dentry->d_inode = inode;

	                trailing_symlink()
	                    may_follow_link()
	                        inode = nd->link_inode;
	                        // nd->link_inode = NULL
	                        //Then it crashes while
	                        //doing inode->i_uid

Reported-by: Guang Yuan Wu <wugyuan@...ibm.com>
Tested-by: Guang Yuan Wu <wugyuan@...ibm.com>
Acked-by: Jeff Layton <jlayton@...nel.org>
Signed-off-by: Ritesh Harjani <riteshh@...ux.ibm.com>
---
 fs/namei.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/fs/namei.c b/fs/namei.c
index 671c3c1a3425..7c5337cddebd 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1617,7 +1617,21 @@ static int lookup_fast(struct nameidata *nd,
 		dput(dentry);
 		return status;
 	}
-	if (unlikely(d_is_negative(dentry))) {
+
+	/*
+	 * Caution: d_is_negative() can race with
+	 * __d_set_inode_and_type().
+	 * For e.g. in use cases where Thread-1 is creating
+	 * symlink (doing d_instantiate_new()) & Thread-2 is doing
+	 * cat of that symlink and falling here (via Ref-walk) while
+	 * doing lookup_fast (one such case is when ->permission
+	 * returns -ECHILD).
+	 * Now if __d_set_inode_and_type() does out-of-order execution
+	 * i.e. it first sets the dentry->d_flags & then dentry->inode
+	 * then it can result into inode being NULL, causing panic later.
+	 * Hence directly check if inode is NULL here.
+	 */
+	if (unlikely(d_really_is_negative(dentry))) {
 		dput(dentry);
 		return -ENOENT;
 	}
-- 
2.21.0