* re: NFS: nfs_find_open_context() may only select open files
@ 2021-06-29 17:05 Colin Ian King
2021-06-29 17:10 ` Trond Myklebust
0 siblings, 1 reply; 2+ messages in thread
From: Colin Ian King @ 2021-06-29 17:05 UTC (permalink / raw)
To: Trond Myklebust; +Cc: Anna Schumaker, linux-nfs, linux-kernel
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@hammerspace.com>
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))
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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: NFS: nfs_find_open_context() may only select open files
2021-06-29 17:05 NFS: nfs_find_open_context() may only select open files Colin Ian King
@ 2021-06-29 17:10 ` Trond Myklebust
0 siblings, 0 replies; 2+ messages in thread
From: Trond Myklebust @ 2021-06-29 17:10 UTC (permalink / raw)
To: colin.king; +Cc: linux-nfs, anna.schumaker, linux-kernel
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@hammerspace.com>
> 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@hammerspace.com
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-06-29 17:10 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-29 17:05 NFS: nfs_find_open_context() may only select open files Colin Ian King
2021-06-29 17:10 ` Trond Myklebust
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®