Skip to content

Commit b681bc4

Browse files
Tito0015Tito0015
committed
cpp: Add 'cpp/mmio-unsanitized-memcpy' query
1 parent 2304a1a commit b681bc4

8 files changed

Lines changed: 215 additions & 0 deletions

File tree

‎cpp/ql/src/Security/CWE/CWE-120/MmioUnsanitizedMemcpy.qhelp‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
Firmware and embedded drivers often copy data into buffers using lengths read from
8+
memory-mapped I/O (MMIO) registers or DMA descriptor fields. When those lengths are not
9+
validated against the destination buffer size, an attacker who can influence hardware
10+
registers or DMA metadata can trigger buffer overflows and potentially achieve remote
11+
code execution on microcontrollers, WiFi stacks, and cellular basebands.
12+
</p>
13+
</overview>
14+
<recommendation>
15+
<p>
16+
Always validate MMIO/DMA-derived lengths before passing them to <code>memcpy</code>,
17+
<code>memmove</code>, or <code>strncpy</code>. Compare against a compile-time maximum
18+
and reject or clamp out-of-range values before copying.
19+
</p>
20+
</recommendation>
21+
<example>
22+
<p>Bad: length from an MMIO register used directly as the copy size.</p>
23+
<sample src="MmioUnsanitizedMemcpyBad.c" />
24+
<p>Good: defensive bounds check before the copy.</p>
25+
<sample src="MmioUnsanitizedMemcpyGood.c" />
26+
</example>
27+
<references>
28+
<li>
29+
CWE-120: Buffer Copy without Checking Size of Input
30+
</li>
31+
<li>
32+
CWE-787: Out-of-bounds Write
33+
</li>
34+
</references>
35+
</qhelp>

‎cpp/ql/src/Security/CWE/CWE-120/MmioUnsanitizedMemcpy.ql‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
/**
2+
* @name MMIO/DMA unsanitized memory copy
3+
* @description Memory copy sizes derived from memory-mapped I/O or DMA
4+
* descriptor fields without bounds validation may overflow
5+
* destination buffers.
6+
* @kind path-problem
7+
* @problem.severity error
8+
* @security-severity 8.6
9+
* @precision medium
10+
* @id cpp/mmio-unsanitized-memcpy
11+
* @tags security
12+
* external/cwe/cwe-120
13+
* external/cwe/cwe-787
14+
*/
15+
16+
import cpp
17+
import semmle.code.cpp.dataflow.new.TaintTracking
18+
import semmle.code.cpp.controlflow.IRGuards
19+
import MmioFlow::PathGraph
20+
21+
/** Holds if `e` is an expression that reads MMIO/DMA hardware state. */
22+
predicate isMmioExpr(Expr e) {
23+
exists(VariableAccess va | va = e and va.getTarget().isVolatile())
24+
or
25+
exists(FieldAccess fa | fa = e and fa.getTarget().getType().isVolatile())
26+
or
27+
exists(FunctionCall call |
28+
call = e and
29+
call.getTarget().hasName(["READ_REG", "GET_MMIO", "REG_READ", "DMA_READ"])
30+
)
31+
or
32+
exists(PointerDereferenceExpr deref |
33+
deref = e and
34+
deref.getOperand().getUnspecifiedType() instanceof PointerType and
35+
deref.getOperand().getUnspecifiedType().(PointerType).getBaseType().isVolatile()
36+
)
37+
}
38+
39+
predicate isMmioSource(DataFlow::Node source) {
40+
isMmioExpr(source.asExpr())
41+
or
42+
exists(MacroInvocation mi |
43+
mi.getMacro().hasName(["READ_REG", "GET_MMIO", "REG_READ", "DMA_READ"]) and
44+
source.asExpr() = mi.getExpr()
45+
)
46+
}
47+
48+
predicate isMemcpySizeSink(DataFlow::Node sink, FunctionCall fc) {
49+
fc.getTarget().hasName(["memcpy", "memmove", "strncpy", "wmemcpy", "wmemmove"]) and
50+
sink.asExpr() = fc.getArgument(2)
51+
}
52+
53+
/** Recognizes relational comparison bounds checks using public IRGuards API. */
54+
predicate lessThanOrEqual(IRGuardCondition g, Expr e, boolean branch) {
55+
exists(Operand left |
56+
g.comparesLt(left, _, _, true, branch) or
57+
g.comparesEq(left, _, _, true, branch)
58+
|
59+
left.getDef().getConvertedResultExpression() = e
60+
)
61+
}
62+
63+
module MmioConfig implements DataFlow::ConfigSig {
64+
predicate isSource(DataFlow::Node source) { isMmioSource(source) }
65+
66+
predicate isSink(DataFlow::Node sink) { isMemcpySizeSink(sink, _) }
67+
68+
predicate isBarrier(DataFlow::Node node) {
69+
node = DataFlow::BarrierGuard<lessThanOrEqual/3>::getABarrierNode() or
70+
node = DataFlow::BarrierGuard<lessThanOrEqual/3>::getAnIndirectBarrierNode()
71+
}
72+
73+
predicate observeDiffInformedIncrementalMode() { any() }
74+
}
75+
76+
module MmioFlow = TaintTracking::Global<MmioConfig>;
77+
78+
from FunctionCall memcpyCall, MmioFlow::PathNode source, MmioFlow::PathNode sink
79+
where
80+
MmioFlow::flowPath(source, sink) and
81+
isMemcpySizeSink(sink.getNode(), memcpyCall)
82+
select memcpyCall, source, sink,
83+
"Memory copy size argument is derived from $@ without sufficient bounds validation.",
84+
source.getNode(), "an MMIO/DMA hardware register read"

‎cpp/ql/src/Security/CWE/CWE-120/MmioUnsanitizedMemcpyBad.c‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
#define READ_REG(addr) (*(volatile unsigned int *)(addr))
2+
#define MAX_DMA_LEN 64
3+
4+
void *memcpy(void *dest, const void *src, unsigned long n);
5+
6+
void bad_mmio_memcpy(char *dst, char *src) {
7+
unsigned int len = READ_REG(0x40001000);
8+
memcpy(dst, src, len);
9+
}

‎cpp/ql/src/Security/CWE/CWE-120/MmioUnsanitizedMemcpyGood.c‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
#define READ_REG(addr) (*(volatile unsigned int *)(addr))
2+
#define MAX_DMA_LEN 64
3+
4+
void *memcpy(void *dest, const void *src, unsigned long n);
5+
6+
void good_mmio_memcpy(char *dst, char *src) {
7+
unsigned int len = READ_REG(0x40001000);
8+
if (len <= MAX_DMA_LEN)
9+
memcpy(dst, src, len);
10+
}

‎cpp/ql/src/codeql-suites/cpp-security-extended.qls‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,3 +3,6 @@
33
- apply: security-extended-selectors.yml
44
from: codeql/suite-helpers
55
- apply: codeql-suites/exclude-slow-queries.yml
6+
# CWE-120: MMIO/DMA unsanitized memcpy (also selected by metadata; explicit for review)
7+
- include:
8+
id: cpp/mmio-unsanitized-memcpy

‎cpp/ql/test/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/MmioUnsanitizedMemcpy.expected‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
#select
2+
| test.c:21:3:21:8 | call to memcpy | test.c:20:18:20:37 | * ... | test.c:21:20:21:22 | len | Memory copy size argument is derived from $@ without sufficient bounds validation. | test.c:20:18:20:37 | * ... | an MMIO/DMA hardware register read |
3+
| test.c:26:3:26:9 | call to memmove | test.c:25:18:25:25 | call to GET_MMIO | test.c:26:21:26:23 | len | Memory copy size argument is derived from $@ without sufficient bounds validation. | test.c:25:18:25:25 | call to GET_MMIO | an MMIO/DMA hardware register read |
4+
| test.c:31:3:31:9 | call to strncpy | test.c:30:18:30:29 | mmio_len_reg | test.c:31:21:31:23 | len | Memory copy size argument is derived from $@ without sufficient bounds validation. | test.c:30:18:30:29 | mmio_len_reg | an MMIO/DMA hardware register read |
5+
edges
6+
| test.c:20:18:20:37 | * ... | test.c:20:18:20:37 | * ... | provenance | |
7+
| test.c:20:18:20:37 | * ... | test.c:21:20:21:22 | len | provenance | |
8+
| test.c:25:18:25:25 | call to GET_MMIO | test.c:25:18:25:25 | call to GET_MMIO | provenance | |
9+
| test.c:25:18:25:25 | call to GET_MMIO | test.c:26:21:26:23 | len | provenance | |
10+
| test.c:30:18:30:29 | mmio_len_reg | test.c:30:18:30:29 | mmio_len_reg | provenance | |
11+
| test.c:30:18:30:29 | mmio_len_reg | test.c:31:21:31:23 | len | provenance | |
12+
nodes
13+
| test.c:20:18:20:37 | * ... | semmle.label | * ... |
14+
| test.c:20:18:20:37 | * ... | semmle.label | * ... |
15+
| test.c:21:20:21:22 | len | semmle.label | len |
16+
| test.c:25:18:25:25 | call to GET_MMIO | semmle.label | call to GET_MMIO |
17+
| test.c:25:18:25:25 | call to GET_MMIO | semmle.label | call to GET_MMIO |
18+
| test.c:26:21:26:23 | len | semmle.label | len |
19+
| test.c:30:18:30:29 | mmio_len_reg | semmle.label | mmio_len_reg |
20+
| test.c:30:18:30:29 | mmio_len_reg | semmle.label | mmio_len_reg |
21+
| test.c:31:21:31:23 | len | semmle.label | len |
22+
subpaths

‎cpp/ql/test/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/MmioUnsanitizedMemcpy.qlref‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
query: Security/CWE/CWE-120/MmioUnsanitizedMemcpy.ql
2+
postprocess: utils/test/InlineExpectationsTestQuery.ql

‎cpp/ql/test/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/test.c‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
/* Semmle test case for MmioUnsanitizedMemcpy.ql
2+
* MMIO/DMA register reads flowing into memcpy/memmove/strncpy size parameters.
3+
*/
4+
5+
typedef unsigned int uint32_t;
6+
7+
void *memcpy(void *dest, const void *src, unsigned long n);
8+
void *memmove(void *dest, const void *src, unsigned long n);
9+
char *strncpy(char *dest, const char *src, unsigned long n);
10+
11+
#define READ_REG(addr) (*(volatile uint32_t *)(addr))
12+
#define MAX_DMA_LEN 64
13+
14+
uint32_t GET_MMIO(unsigned long addr);
15+
uint32_t DMA_READ(unsigned long addr);
16+
17+
volatile uint32_t mmio_len_reg;
18+
19+
static void bad_read_reg(char *dst, char *src) {
20+
uint32_t len = READ_REG(0x40001000); // $ Source
21+
memcpy(dst, src, len); // $ Alert
22+
}
23+
24+
static void bad_get_mmio(char *dst, char *src) {
25+
uint32_t len = GET_MMIO(0x50000000); // $ Source
26+
memmove(dst, src, len); // $ Alert
27+
}
28+
29+
static void bad_volatile_global(char *dst, char *src) {
30+
uint32_t len = mmio_len_reg; // $ Source
31+
strncpy(dst, src, len); // $ Alert
32+
}
33+
34+
static void good_bounded(char *dst, char *src) {
35+
uint32_t len = READ_REG(0x40001000);
36+
if (len <= MAX_DMA_LEN)
37+
memcpy(dst, src, len); // GOOD
38+
}
39+
40+
static void good_early_return(char *dst, char *src) {
41+
uint32_t len = DMA_READ(0x60000000);
42+
if (len > MAX_DMA_LEN)
43+
return;
44+
memcpy(dst, src, len); // GOOD
45+
}
46+
47+
static void good_constant_size(char *dst, char *src) {
48+
uint32_t len = READ_REG(0x40001000);
49+
memcpy(dst, src, 32); // GOOD — constant size, not tainted sink
50+
}

0 commit comments

Comments
 (0)