Re: NFS: nfs_find_open_context() may only select open files

[Trond Myklebust]

On Tue, 2021-06-29 at 18:05 +0100, Colin Ian King wrote:
> Hi,
> 
> Static analysis on linux-next with Coverity has found a potential
> null
> pointer dereference in the following commit:
> 
> commit 92735943dc6cf52aeaf2ce9aee397dee55e3ef05
> Author: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> Date:   Tue May 11 23:41:10 2021 -0400
> 
>     NFS: nfs_find_open_context() may only select open files
> 
> The analysis is as follows:
> 
> 1113 struct nfs_open_context *nfs_find_open_context(struct inode
> *inode,
> const struct cred *cred, fmode_t mode)
> 1114 {
> 1115        struct nfs_inode *nfsi = NFS_I(inode);
> 
>     1. assign_zero: Assigning: ctx = NULL.
> 
> 1116        struct nfs_open_context *pos, *ctx = NULL;
> 1117
> 1118        rcu_read_lock();
> 
>     2. Condition 1 /* !0 */, taking true branch.
>     3. Condition !rcu_read_lock_any_held(), taking true branch.
>     4. Condition debug_lockdep_rcu_enabled(), taking true branch.
>     5. Condition !__warned, taking true branch.
>     6. Condition 0 /* !((((sizeof (nfsi->open_files.next) == sizeof
> (char) || sizeof (nfsi->open_files.next) == sizeof (short)) || sizeof
> (nfsi->open_files.next) == sizeof (int)) || sizeof
> (nfsi->open_files.next) == sizeof (long)) || sizeof
> (nfsi->open_files.next) == sizeof (long long)) */, taking false
> branch.
>     7. Condition 0 /* !!(!__builtin_types_compatible_p() &&
> !__builtin_types_compatible_p()) */, taking false branch.
>     8. Condition &pos->list != &nfsi->open_files, taking true branch.
>     13. Condition 0 /* !((((sizeof (pos->list.next) == sizeof (char)
> ||
> sizeof (pos->list.next) == sizeof (short)) || sizeof (pos->list.next)
> ==
> sizeof (int)) || sizeof (pos->list.next) == sizeof (long)) || sizeof
> (pos->list.next) == sizeof (long long)) */, taking false branch.
>     14. Condition 0 /* !!(!__builtin_types_compatible_p() &&
> !__builtin_types_compatible_p()) */, taking false branch.
>     15. Condition &pos->list != &nfsi->open_files, taking true
> branch.
>     20. Condition 0 /* !((((sizeof (pos->list.next) == sizeof (char)
> ||
> sizeof (pos->list.next) == sizeof (short)) || sizeof (pos->list.next)
> ==
> sizeof (int)) || sizeof (pos->list.next) == sizeof (long)) || sizeof
> (pos->list.next) == sizeof (long long)) */, taking false branch.
>     21. Condition 0 /* !!(!__builtin_types_compatible_p() &&
> !__builtin_types_compatible_p()) */, taking false branch.
>     22. Condition &pos->list != &nfsi->open_files, taking true
> branch.
> 1119        list_for_each_entry_rcu(pos, &nfsi->open_files, list) {
>     9. Condition cred != NULL, taking true branch.
>     10. Condition cred_fscmp(pos->cred, cred) != 0, taking false
> branch.
>     16. Condition cred != NULL, taking true branch.
>     17. Condition cred_fscmp(pos->cred, cred) != 0, taking false
> branch.
>     23. Condition cred != NULL, taking true branch.
>     24. Condition cred_fscmp(pos->cred, cred) != 0, taking false
> branch.
> 
> 1120                if (cred != NULL && cred_fscmp(pos->cred, cred)
> != 0)
> 1121                        continue;
> 
>     11. Condition (pos->mode & (3U /* (fmode_t)1 | (fmode_t)2 */)) !=
> mode, taking true branch.
>     18. Condition (pos->mode & (3U /* (fmode_t)1 | (fmode_t)2 */)) !=
> mode, taking true branch.
>     25. Condition (pos->mode & (3U /* (fmode_t)1 | (fmode_t)2 */)) !=
> mode, taking false branch.
> 1122                if ((pos->mode & (FMODE_READ|FMODE_WRITE)) !=
> mode)
>     12. Continuing loop.
>     19. Continuing loop.
> 1123                        continue;
> 
>     Explicit null dereferenced (FORWARD_NULL)
>     26. var_deref_model: Passing null pointer &ctx->flags to
> test_bit,
> which dereferences it.
> 
> 1124                if (!test_bit(NFS_CONTEXT_FILE_OPEN, &ctx-
> >flags))

That should be &pos->flags....

> 1125                        continue;
> 1126                ctx = get_nfs_open_context(pos);
> 1127                if (ctx)
> 1128                        break;
> 1129        }
> 1130        rcu_read_unlock();
> 1131        return ctx;
> 1132 }
> 
> Coverity is indicating that the test_bit call on &ctx->flags can
> cause a
> null pointer dereference when ctx is NULL.  I'm not entirely
> convinced
> if this is a false positive, so I though I had better report this
> issue.
> 
> Colin
> 

Thanks for reporting this! This would be a brown paper bag moment.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx