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 <[email protected]>
> 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
[email protected]