linux-kernel - re: NFS: nfs_find_open_context() may only select open files

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]

Date:   Tue, 29 Jun 2021 18:05:09 +0100
From:   Colin Ian King <colin.king@...onical.com>
To:     Trond Myklebust <trond.myklebust@...merspace.com>
Cc:     Anna Schumaker <anna.schumaker@...app.com>,
        "linux-nfs@...r.kernel.org" <linux-nfs@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: re: NFS: nfs_find_open_context() may only select open files

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@...merspace.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