Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>
<overview>
<p>
Firmware and embedded drivers often copy data into buffers using lengths read from
allowlisted MMIO register macros such as <code>READ_REG</code> or <code>GET_MMIO</code>.
When those lengths are not validated against the destination buffer size, an attacker who
can influence hardware registers or DMA metadata can trigger buffer overflows.
</p>
</overview>
<recommendation>
<p>
Always validate MMIO/DMA-derived lengths before passing them to <code>memcpy</code>,
<code>memmove</code>, or <code>strncpy</code>. Compare against a compile-time maximum
and reject or clamp out-of-range values before copying.
</p>
</recommendation>
<example>
<p>Bad: length from an MMIO register used directly as the copy size.</p>
<sample src="MmioUnsanitizedMemcpyBad.c" />
<p>Good: defensive bounds check before the copy.</p>
<sample src="MmioUnsanitizedMemcpyGood.c" />
</example>
<references>
<li>
CWE-120: Buffer Copy without Checking Size of Input
</li>
<li>
CWE-787: Out-of-bounds Write
</li>
</references>
</qhelp>
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/**
* @name MMIO/DMA unsanitized memory copy
* @description Memory copy sizes derived from allowlisted MMIO/DMA register-read
* macros without bounds validation may overflow destination buffers.
* @kind path-problem
* @problem.severity error
* @precision low
* @id cpp/mmio-unsanitized-memcpy
* @tags security
* experimental
* external/cwe/cwe-120
* external/cwe/cwe-787
*/

import cpp
import semmle.code.cpp.dataflow.new.TaintTracking
import semmle.code.cpp.controlflow.IRGuards
import MmioFlow::PathGraph

/** Holds if `source` reads MMIO/DMA state through an allowlisted register macro. */
predicate isMmioSource(DataFlow::Node source) {
exists(MacroInvocation mi |
mi.getMacro().hasName(["READ_REG", "GET_MMIO", "REG_READ", "DMA_READ"]) and
source.asExpr() = mi.getExpr()
)
}

predicate isMemcpySizeSink(DataFlow::Node sink, FunctionCall fc) {
fc.getTarget().hasName(["memcpy", "memmove", "strncpy", "wmemcpy", "wmemmove"]) and
sink.asExpr() = fc.getArgument(2)
}

/** Recognizes relational comparison bounds checks using public IRGuards API. */
predicate lessThanOrEqual(IRGuardCondition g, Expr e, boolean branch) {
exists(Operand left |
g.comparesLt(left, _, _, true, branch) or
g.comparesEq(left, _, _, true, branch)
|
left.getDef().getConvertedResultExpression() = e
)
}

module MmioConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) { isMmioSource(source) }

predicate isSink(DataFlow::Node sink) { isMemcpySizeSink(sink, _) }

predicate isBarrier(DataFlow::Node node) {
node = DataFlow::BarrierGuard<lessThanOrEqual/3>::getABarrierNode() or
node = DataFlow::BarrierGuard<lessThanOrEqual/3>::getAnIndirectBarrierNode()
}

predicate observeDiffInformedIncrementalMode() { any() }
}

module MmioFlow = TaintTracking::Global<MmioConfig>;

from FunctionCall memcpyCall, MmioFlow::PathNode source, MmioFlow::PathNode sink
where
MmioFlow::flowPath(source, sink) and
isMemcpySizeSink(sink.getNode(), memcpyCall)
select memcpyCall, source, sink,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because you're not using the sink as alert location (you're using memcpyCall instead. CI is giving the errors below. To solve this you should either use the sink as alert location, or implement getASelectedSinkLocation (see here)

Cannot find (26,3)-(26,8).expected file.
--- expected
+++ actual
@@ -1,1 +1,3 @@
-
+Filtering alerts to these ranges:
+  test.c:(26,3)-(26,8)
+Wrongly omitted: | test.c:26:3:26:8 | call to memcpy | 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 |
Error: [2/3] [eval 307ms] FAILED(RESULT) /home/runner/work/semmle-code/semmle-code/ql/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/DIFF-INFORMED/MmioUnsanitizedMemcpy/test.c/(26,3)-(26,8)
Cannot find (31,3)-(31,9).expected file.
--- expected
+++ actual
@@ -1,1 +1,3 @@
-
+Filtering alerts to these ranges:
+  test.c:(31,3)-(31,9)
+Wrongly omitted: | test.c:31:3:31:9 | call to memmove | 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 |
Error: [2/3] [eval 292ms] FAILED(RESULT) /home/runner/work/semmle-code/semmle-code/ql/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/DIFF-INFORMED/MmioUnsanitizedMemcpy/test.c/(31,3)-(31,9)
Cannot find (36,3)-(36,9).expected file.
--- expected
+++ actual
@@ -1,1 +1,3 @@
-
+Filtering alerts to these ranges:
+  test.c:(36,3)-(36,9)
+Wrongly omitted: | test.c:36:3:36:9 | call to strncpy | 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 |
Error: [2/3] [eval 254ms] FAILED(RESULT) /home/runner/work/semmle-code/semmle-code/ql/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/DIFF-INFORMED/MmioUnsanitizedMemcpy/test.c/(36,3)-(36,9)
Cannot find (41,3)-(41,8).expected file.
--- expected
+++ actual
@@ -1,1 +1,3 @@
-
+Filtering alerts to these ranges:
+  test.c:(41,3)-(41,8)
+Wrongly omitted: | test.c:41:3:41:8 | call to memcpy | 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 |
Error: [2/3] [eval 234ms] FAILED(RESULT) /home/runner/work/semmle-code/semmle-code/ql/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/DIFF-INFORMED/MmioUnsanitizedMemcpy/test.c/(41,3)-(41,8)

"Memory copy size argument is derived from $@ without sufficient bounds validation.",
source.getNode(), "an MMIO/DMA hardware register read"
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#define READ_REG(addr) (*(volatile unsigned int *)(addr))
#define MAX_DMA_LEN 64

void *memcpy(void *dest, const void *src, unsigned long n);

void bad_mmio_memcpy(char *dst, char *src) {
unsigned int len = READ_REG(0x40001000);
memcpy(dst, src, len);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#define READ_REG(addr) (*(volatile unsigned int *)(addr))
#define MAX_DMA_LEN 64

void *memcpy(void *dest, const void *src, unsigned long n);

void good_mmio_memcpy(char *dst, char *src) {
unsigned int len = READ_REG(0x40001000);
if (len <= MAX_DMA_LEN)
memcpy(dst, src, len);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
#select
| test.c:26:3:26:8 | call to memcpy | 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 |
| test.c:31:3:31:9 | call to memmove | 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 |
| test.c:36:3:36:9 | call to strncpy | 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 |
| test.c:41:3:41:8 | call to memcpy | 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 |
edges
| test.c:25:18:25:37 | * ... | test.c:25:18:25:37 | * ... | provenance | |
| test.c:25:18:25:37 | * ... | test.c:26:20:26:22 | len | provenance | |
| test.c:30:18:30:37 | * ... | test.c:30:18:30:37 | * ... | provenance | |
| test.c:30:18:30:37 | * ... | test.c:31:21:31:23 | len | provenance | |
| test.c:35:18:35:37 | * ... | test.c:35:18:35:37 | * ... | provenance | |
| test.c:35:18:35:37 | * ... | test.c:36:21:36:23 | len | provenance | |
| test.c:40:18:40:37 | * ... | test.c:40:18:40:37 | * ... | provenance | |
| test.c:40:18:40:37 | * ... | test.c:41:20:41:22 | len | provenance | |
nodes
| test.c:25:18:25:37 | * ... | semmle.label | * ... |
| test.c:25:18:25:37 | * ... | semmle.label | * ... |
| test.c:26:20:26:22 | len | semmle.label | len |
| test.c:30:18:30:37 | * ... | semmle.label | * ... |
| test.c:30:18:30:37 | * ... | semmle.label | * ... |
| test.c:31:21:31:23 | len | semmle.label | len |
| test.c:35:18:35:37 | * ... | semmle.label | * ... |
| test.c:35:18:35:37 | * ... | semmle.label | * ... |
| test.c:36:21:36:23 | len | semmle.label | len |
| test.c:40:18:40:37 | * ... | semmle.label | * ... |
| test.c:40:18:40:37 | * ... | semmle.label | * ... |
| test.c:41:20:41:22 | len | semmle.label | len |
subpaths
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
query: experimental/Security/CWE/CWE-120/MmioUnsanitizedMemcpy.ql
postprocess: utils/test/InlineExpectationsTestQuery.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
/* Semmle test case for MmioUnsanitizedMemcpy.ql
* Allowlisted MMIO/DMA register macros flowing into memcpy/memmove/strncpy size parameters.
*/

typedef unsigned int uint32_t;

void *memcpy(void *dest, const void *src, unsigned long n);
void *memmove(void *dest, const void *src, unsigned long n);
char *strncpy(char *dest, const char *src, unsigned long n);

#define READ_REG(addr) (*(volatile uint32_t *)(addr))
#define GET_MMIO(addr) (*(volatile uint32_t *)(addr))
#define REG_READ(addr) (*(volatile uint32_t *)(addr))
#define DMA_READ(addr) (*(volatile uint32_t *)(addr))
#define MAX_DMA_LEN 64

struct VolatileField {
volatile uint32_t len;
};

volatile uint32_t mmio_len_reg;
struct VolatileField vf;

static void bad_read_reg(char *dst, char *src) {
uint32_t len = READ_REG(0x40001000); // $ Source
memcpy(dst, src, len); // $ Alert
}

static void bad_get_mmio(char *dst, char *src) {
uint32_t len = GET_MMIO(0x50000000); // $ Source
memmove(dst, src, len); // $ Alert
}

static void bad_reg_read(char *dst, char *src) {
uint32_t len = REG_READ(0x51000000); // $ Source
strncpy(dst, src, len); // $ Alert
}

static void bad_dma_read(char *dst, char *src) {
uint32_t len = DMA_READ(0x60000000); // $ Source
memcpy(dst, src, len); // $ Alert
}

static void good_bounded(char *dst, char *src) {
uint32_t len = READ_REG(0x40001000);
if (len <= MAX_DMA_LEN)
memcpy(dst, src, len); // GOOD
}

static void good_early_return(char *dst, char *src) {
uint32_t len = DMA_READ(0x60000000);
if (len > MAX_DMA_LEN)
return;
memcpy(dst, src, len); // GOOD
}

static void good_constant_size(char *dst, char *src) {
uint32_t len = READ_REG(0x40001000);
memcpy(dst, src, 32); // GOOD — constant size, not tainted sink
}

static void negative_volatile_global(char *dst, char *src) {
uint32_t len = mmio_len_reg;
memcpy(dst, src, len); // GOOD
}

static void negative_volatile_field(char *dst, char *src) {
uint32_t len = vf.len;
memcpy(dst, src, len); // GOOD
}

static void negative_volatile_deref(char *dst, char *src) {
volatile uint32_t *reg = (volatile uint32_t *)0x40001000;
uint32_t len = *reg;
memcpy(dst, src, len); // GOOD
}

static uint32_t GET_MMIO_fn(unsigned long addr);

static void negative_get_mmio_function(char *dst, char *src) {
uint32_t len = GET_MMIO_fn(0x50000000);
memcpy(dst, src, len); // GOOD
}
Loading