Skip to content

Commit 62d2f25

Browse files
kdudkakdudka
committed
parser-cov: tweak key event detection for LOCK_EVASION
Instead of `use_same_locks_for_read_and_modify` we need to pick `thread1_overwrites_value_in_field` in the example used as a test.
1 parent 0a73880 commit 62d2f25

5 files changed

+112
-0
lines changed

‎src/lib/parser-cov.cc

+1
Original file line numberDiff line numberDiff line change
@@ -296,6 +296,7 @@ KeyEventDigger::KeyEventDigger():
296296
d->denyList.insert("remediation");
297297
d->denyList.insert("rounding_remediation");
298298
d->denyList.insert("scope_hint");
299+
d->denyList.insert("use_same_locks_for_read_and_modify");
299300

300301
// trace events
301302
d->traceEvts.insert("break");

‎tests/csgrep/0121-cov-parser-lock-evasion-args.txt

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
--mode=json

‎tests/csgrep/0121-cov-parser-lock-evasion-stdin.txt

+13
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
Error: LOCK_EVASION (CWE-543):
2+
p11-kit-0.23.22/p11-kit/rpc-client.c:808: thread1_checks_field: Thread1 uses the value read from field "initialized_forkid" in the condition "module->initialized_forkid == p11_forkid". It sees that the condition is true. Control is switched to Thread2.
3+
p11-kit-0.23.22/p11-kit/rpc-client.c:808: thread2_checks_field: Thread2 uses the value read from field "initialized_forkid" in the condition "module->initialized_forkid == p11_forkid". It sees that the condition is true.
4+
p11-kit-0.23.22/p11-kit/rpc-client.c:811: thread2_acquires_lock: Thread2 acquires lock "rpc_client.mutex".
5+
p11-kit-0.23.22/p11-kit/rpc-client.c:826: thread2_modifies_field: Thread2 sets "initialized_forkid" to a new value. Note that this write can be reordered at runtime to occur before instructions that do not access this field within this locked region. After Thread2 leaves the critical section, control is switched back to Thread1.
6+
p11-kit-0.23.22/p11-kit/rpc-client.c:811: thread1_acquires_lock: Thread1 acquires lock "rpc_client.mutex".
7+
p11-kit-0.23.22/p11-kit/rpc-client.c:826: thread1_overwrites_value_in_field: Thread1 sets "initialized_forkid" to a new value. Now the two threads have an inconsistent view of "initialized_forkid" and updates to fields correlated with "initialized_forkid" may be lost.
8+
p11-kit-0.23.22/p11-kit/rpc-client.c:808: use_same_locks_for_read_and_modify: Guard the modification of "initialized_forkid" and the read used to decide whether to modify "initialized_forkid" with the same set of locks.
9+
# 824| }
10+
# 825|
11+
# 826|-> module->initialized_forkid = 0;
12+
# 827|
13+
# 828| p11_mutex_unlock (&module->mutex);

‎tests/csgrep/0121-cov-parser-lock-evasion-stdout.txt

+96
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
{
2+
"defects": [
3+
{
4+
"checker": "LOCK_EVASION",
5+
"cwe": 543,
6+
"tool": "coverity",
7+
"key_event_idx": 5,
8+
"events": [
9+
{
10+
"file_name": "p11-kit-0.23.22/p11-kit/rpc-client.c",
11+
"line": 808,
12+
"event": "thread1_checks_field",
13+
"message": "Thread1 uses the value read from field \"initialized_forkid\" in the condition \"module->initialized_forkid == p11_forkid\". It sees that the condition is true. Control is switched to Thread2.",
14+
"verbosity_level": 1
15+
},
16+
{
17+
"file_name": "p11-kit-0.23.22/p11-kit/rpc-client.c",
18+
"line": 808,
19+
"event": "thread2_checks_field",
20+
"message": "Thread2 uses the value read from field \"initialized_forkid\" in the condition \"module->initialized_forkid == p11_forkid\". It sees that the condition is true.",
21+
"verbosity_level": 1
22+
},
23+
{
24+
"file_name": "p11-kit-0.23.22/p11-kit/rpc-client.c",
25+
"line": 811,
26+
"event": "thread2_acquires_lock",
27+
"message": "Thread2 acquires lock \"rpc_client.mutex\".",
28+
"verbosity_level": 1
29+
},
30+
{
31+
"file_name": "p11-kit-0.23.22/p11-kit/rpc-client.c",
32+
"line": 826,
33+
"event": "thread2_modifies_field",
34+
"message": "Thread2 sets \"initialized_forkid\" to a new value. Note that this write can be reordered at runtime to occur before instructions that do not access this field within this locked region. After Thread2 leaves the critical section, control is switched back to Thread1.",
35+
"verbosity_level": 1
36+
},
37+
{
38+
"file_name": "p11-kit-0.23.22/p11-kit/rpc-client.c",
39+
"line": 811,
40+
"event": "thread1_acquires_lock",
41+
"message": "Thread1 acquires lock \"rpc_client.mutex\".",
42+
"verbosity_level": 1
43+
},
44+
{
45+
"file_name": "p11-kit-0.23.22/p11-kit/rpc-client.c",
46+
"line": 826,
47+
"event": "thread1_overwrites_value_in_field",
48+
"message": "Thread1 sets \"initialized_forkid\" to a new value. Now the two threads have an inconsistent view of \"initialized_forkid\" and updates to fields correlated with \"initialized_forkid\" may be lost.",
49+
"verbosity_level": 0
50+
},
51+
{
52+
"file_name": "p11-kit-0.23.22/p11-kit/rpc-client.c",
53+
"line": 808,
54+
"event": "use_same_locks_for_read_and_modify",
55+
"message": "Guard the modification of \"initialized_forkid\" and the read used to decide whether to modify \"initialized_forkid\" with the same set of locks.",
56+
"verbosity_level": 1
57+
},
58+
{
59+
"file_name": "",
60+
"line": 0,
61+
"event": "#",
62+
"message": " 824| \t}",
63+
"verbosity_level": 1
64+
},
65+
{
66+
"file_name": "",
67+
"line": 0,
68+
"event": "#",
69+
"message": " 825| ",
70+
"verbosity_level": 1
71+
},
72+
{
73+
"file_name": "",
74+
"line": 0,
75+
"event": "#",
76+
"message": " 826|-> \tmodule->initialized_forkid = 0;",
77+
"verbosity_level": 1
78+
},
79+
{
80+
"file_name": "",
81+
"line": 0,
82+
"event": "#",
83+
"message": " 827| ",
84+
"verbosity_level": 1
85+
},
86+
{
87+
"file_name": "",
88+
"line": 0,
89+
"event": "#",
90+
"message": " 828| \tp11_mutex_unlock (&module->mutex);",
91+
"verbosity_level": 1
92+
}
93+
]
94+
}
95+
]
96+
}

‎tests/csgrep/CMakeLists.txt

+1
Original file line numberDiff line numberDiff line change
@@ -164,3 +164,4 @@ test_csgrep("0117-csgrep-set-imp-level" )
164164
test_csgrep("0118-gcc-parser-ubsan-dedup" )
165165
test_csgrep("0119-cov-parser-sigma" )
166166
test_csgrep("0120-sarif-parser-semgrep" )
167+
test_csgrep("0121-cov-parser-lock-evasion" )

0 commit comments

Comments
 (0)