Re: [RFC] - vfs: Null pointer dereference issue with symlink create and read of symlink

[Gao Xiang <hsiangkao@aol.com>]

On Tue, Sep 03, 2019 at 05:28:26PM +0530, Ritesh Harjani wrote:
> Hi Viro/All,
> 
> Could you please review below issue and it's proposed solutions.
> If you could let me know which of the two you think will be a better
> approach to solve this or in case if you have any other better approach, I
> can prepare and submit a official patch with that.
> 
> 
> 
> 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
> 
> 
> 
> Test case:-
> shell-1 - "while [ 1 ]; do cat /gpfs/g1/testdir/file3; sleep 1; done"
> shell-2 - "while [ 1 ]; do ln -s /gpfs/g1/testdir/file1
> /gpfs/g1/testdir/file3; sleep 1; rm /gpfs/g1/testdir/file3 sleep 1; done
> 
> 
> 
> Problem description:-
> In some filesystems like GPFS below described scenario may happen on some
> platforms (Reported-By:- wugyuan)
> 
> Here, two threads are being run in 2 different shells. Thread-1(cat) does
> cat of the symlink and Thread-2(ln) is creating the symlink.
> 
> Now on any platform with GPFS like filesystem, if CPU does out-of-order
> execution (or any kind of re-ordering due compiler optimization?) in
> function __d_set_and_inode_type(), then we see a NULL pointer dereference
> due to inode->i_uid.
> 
> This happens because in lookup_fast in nonRCU path or say REF-walk (i.e. in
> else condition), we check d_is_negative() without any lock protection.
> And since in __d_set_and_inode_type() re-ordering may happen in setting of
> dentry->type & dentry->inode => this means that there is this tiny window
> where things are going wrong.
> 
> 
> (GPFS like):- Any FS with -inode_operations ->permission callback returning
> -ECHILD in case of (mask & MAY_NOT_BLOCK) may cause this problem to happen.
> (few e.g. found were - ocfs2, ceph, coda, afs)
> 
> int xxx_permission(struct inode *inode, int mask)
> {
>          if (mask & MAY_NOT_BLOCK)
>                  return -ECHILD;
> 	<...>
> }
> 
> Wugyuan(cc), could reproduce this problem with GPFS filesystem.
> Since, I didn't have the GPFS setup, so I tried replicating on a native FS
> by forcing out-of-order execution in function __d_set_inode_and_type() and
> making sure we return -ECHILD in MAY_NOT_BLOCK case in ->permission
> operation for all inodes.
> 
> With above changes in kernel, I could as well hit this issue on a native FS
> too.
> 
> (basically what we observed is link_path_walk will do nonRCU(REF-walk)
> lookup due to may_lookup -> inode_permission return -ECHILD and then
> unlazy_walk drops the LOOKUP_RCU flag (nd->flag). After that below race is
> possible).
> 
> 
> 
> 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 type 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
> 					
> 	

It seems much similar to
https://lore.kernel.org/r/20190419084810.63732-1-houtao1@huawei.com/

Thanks,
Gao Xiang

> 
> 
> 
> Approach-1:- using wmb()
> 
> diff --git a/fs/dcache.c b/fs/dcache.c
> index e88cf0554e65..966172df77ee 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -316,6 +316,7 @@ static inline void __d_set_inode_and_type(struct dentry
> *dentry,
>         unsigned flags;
> 
>         dentry->d_inode = inode;
> +       smp_wmb();
>         flags = READ_ONCE(dentry->d_flags);
>         flags &= ~(DCACHE_ENTRY_TYPE | DCACHE_FALLTHRU);
>         flags |= type_flags;
> 
> 
> 
> Approach-2:- using spin_lock(&dentry->d_lock);
> 
> Do you think lock should be a better approach, given that we are already
> in REF-walk mode. As per the Documentation, we should be able to take
> spin_lock(&dentry->d_lock) in Ref-walk mode whenever required?
> 
> 
> With smp_wmb(), if added, should add a small latency in both
> RCU/REF-walk. But should be able to cover all the cases in general related
> to dentry->type & dentry->inode ordering. Though, we don't have any other
> race conditions reported or tested, other than the one, mentioned in this
> email.
> 
> Confused :(
> 
> 
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 209c51a5226c..a3145594da1c 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1557,6 +1557,7 @@ static int lookup_fast(struct nameidata *nd,
>         struct dentry *dentry, *parent = nd->path.dentry;
>         int status = 1;
>         int err;
> +       bool negative;
> 
>         /*
>          * Rename seqlock is not required here because in the off chance
> @@ -1565,7 +1566,6 @@ static int lookup_fast(struct nameidata *nd,
>          */
>         if (nd->flags & LOOKUP_RCU) {
>                 unsigned seq;
> -               bool negative;
>                 dentry = __d_lookup_rcu(parent, &nd->last, &seq);
>                 if (unlikely(!dentry)) {
>                         if (unlazy_walk(nd))
> @@ -1623,7 +1623,11 @@ static int lookup_fast(struct nameidata *nd,
>                 dput(dentry);
>                 return status;
>         }
> -       if (unlikely(d_is_negative(dentry))) {
> +
> +       spin_lock(&dentry->d_lock);
> +       negative = d_is_negative(dentry);
> +       spin_unlock(&dentry->d_lock);
> +       if (unlikely(negative)) {
>                 dput(dentry);
>                 return -ENOENT;
>         }
> 
> 
> Regards
> Ritesh
>