Skip to content

Commit cdcde47

Browse files
geoffw0geoffw0
authored
Merge pull request #22438 from Tito0015/feature/cpp-mmio-unsanitized-memcpy
cpp: Add 'cpp/mmio-unsanitized-memcpy' query
2 parents 48e43e1 + e41e090 commit cdcde47

8 files changed

Lines changed: 225 additions & 0 deletions

File tree

‎cpp/ql/integration-tests/query-suite/not_included_in_qls.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,7 @@ ql/cpp/ql/src/experimental/Security/CWE/CWE-078/WordexpTainted.ql
242242
ql/cpp/ql/src/experimental/Security/CWE/CWE-1041/FindWrapperFunctions.ql
243243
ql/cpp/ql/src/experimental/Security/CWE/CWE-1126/DeclarationOfVariableWithUnnecessarilyWideScope.ql
244244
ql/cpp/ql/src/experimental/Security/CWE/CWE-120/MemoryUnsafeFunctionScan.ql
245+
ql/cpp/ql/src/experimental/Security/CWE/CWE-120/MmioUnsanitizedMemcpy.ql
245246
ql/cpp/ql/src/experimental/Security/CWE/CWE-1240/CustomCryptographicPrimitive.ql
246247
ql/cpp/ql/src/experimental/Security/CWE/CWE-125/DangerousWorksWithMultibyteOrWideCharacters.ql
247248
ql/cpp/ql/src/experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.ql

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
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+
allowlisted MMIO register macros such as <code>READ_REG</code> or <code>GET_MMIO</code>.
9+
When those lengths are not validated against the destination buffer size, an attacker who
10+
can influence hardware registers or DMA metadata can trigger buffer overflows.
11+
</p>
12+
</overview>
13+
<recommendation>
14+
<p>
15+
Always validate MMIO/DMA-derived lengths before passing them to <code>memcpy</code>,
16+
<code>memmove</code>, or <code>strncpy</code>. Compare against a compile-time maximum
17+
and reject or clamp out-of-range values before copying.
18+
</p>
19+
</recommendation>
20+
<example>
21+
<p>Bad: length from an MMIO register used directly as the copy size.</p>
22+
<sample src="MmioUnsanitizedMemcpyBad.c" />
23+
<p>Good: defensive bounds check before the copy.</p>
24+
<sample src="MmioUnsanitizedMemcpyGood.c" />
25+
</example>
26+
<references>
27+
</references>
28+
</qhelp>

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

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

‎cpp/ql/src/experimental/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/experimental/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/test/experimental/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/MmioUnsanitizedMemcpy.expected‎

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

‎cpp/ql/test/experimental/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: experimental/Security/CWE/CWE-120/MmioUnsanitizedMemcpy.ql
2+
postprocess: utils/test/InlineExpectationsTestQuery.ql

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

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
/* Test case for MmioUnsanitizedMemcpy.ql
2+
* Allowlisted MMIO/DMA register macros 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 GET_MMIO(addr) (*(volatile uint32_t *)(addr))
13+
#define REG_READ(addr) (*(volatile uint32_t *)(addr))
14+
#define DMA_READ(addr) (*(volatile uint32_t *)(addr))
15+
#define MAX_DMA_LEN 64
16+
17+
struct VolatileField {
18+
volatile uint32_t len;
19+
};
20+
21+
volatile uint32_t mmio_len_reg;
22+
struct VolatileField vf;
23+
24+
static void bad_read_reg(char *dst, char *src) {
25+
uint32_t len = READ_REG(0x40001000); // $ Source
26+
memcpy(dst, src, len); // $ Alert
27+
}
28+
29+
static void bad_get_mmio(char *dst, char *src) {
30+
uint32_t len = GET_MMIO(0x50000000); // $ Source
31+
memmove(dst, src, len); // $ Alert
32+
}
33+
34+
static void bad_reg_read(char *dst, char *src) {
35+
uint32_t len = REG_READ(0x51000000); // $ Source
36+
strncpy(dst, src, len); // $ Alert
37+
}
38+
39+
static void bad_dma_read(char *dst, char *src) {
40+
uint32_t len = DMA_READ(0x60000000); // $ Source
41+
memcpy(dst, src, len); // $ Alert
42+
}
43+
44+
static void good_bounded(char *dst, char *src) {
45+
uint32_t len = READ_REG(0x40001000);
46+
if (len <= MAX_DMA_LEN)
47+
memcpy(dst, src, len); // GOOD
48+
}
49+
50+
static void good_early_return(char *dst, char *src) {
51+
uint32_t len = DMA_READ(0x60000000);
52+
if (len > MAX_DMA_LEN)
53+
return;
54+
memcpy(dst, src, len); // GOOD
55+
}
56+
57+
static void good_constant_size(char *dst, char *src) {
58+
uint32_t len = READ_REG(0x40001000);
59+
memcpy(dst, src, 32); // GOOD
60+
}
61+
62+
static void negative_volatile_global(char *dst, char *src) {
63+
uint32_t len = mmio_len_reg;
64+
memcpy(dst, src, len); // GOOD
65+
}
66+
67+
static void negative_volatile_field(char *dst, char *src) {
68+
uint32_t len = vf.len;
69+
memcpy(dst, src, len); // GOOD
70+
}
71+
72+
static void negative_volatile_deref(char *dst, char *src) {
73+
volatile uint32_t *reg = (volatile uint32_t *)0x40001000;
74+
uint32_t len = *reg;
75+
memcpy(dst, src, len); // GOOD
76+
}
77+
78+
static uint32_t GET_MMIO_fn(unsigned long addr);
79+
80+
static void negative_get_mmio_function(char *dst, char *src) {
81+
uint32_t len = GET_MMIO_fn(0x50000000);
82+
memcpy(dst, src, len); // GOOD
83+
}

0 commit comments

Comments
 (0)