Skip to content

cpp: Add 'cpp/mmio-unsanitized-memcpy' query - #22438

Open
Tito0015 wants to merge 6 commits into
github:mainfrom
Tito0015:feature/cpp-mmio-unsanitized-memcpy
Open

cpp: Add 'cpp/mmio-unsanitized-memcpy' query#22438
Tito0015 wants to merge 6 commits into
github:mainfrom
Tito0015:feature/cpp-mmio-unsanitized-memcpy

Conversation

@Tito0015

Copy link
Copy Markdown

Summary

Adds a new security query cpp/mmio-unsanitized-memcpy targeting unsanitized memory copy operations (memcpy, memmove, strncpy) where size parameters derive directly from hardware registers (MMIO/DMA) without relational bounds checks.

Motivation & Domain Context

Standard buffer overflow queries (UnboundedWrite.ql, OverrunWrite.ql) model user-space strings and generic memory ops, but do not model volatile register macro reads (READ_REG, GET_MMIO) commonly found in microcontroller drivers, RTOS kernels, and embedded hardware stacks. This query fills a gap for embedded C/C++ static analysis.

Query Design & Architecture

  • Taint Engine: Modern DataFlow::ConfigSig with TaintTracking::Global.
  • Sources: Volatile variables, volatile struct fields, volatile pointer dereferences, and MMIO macros (READ_REG, GET_MMIO, REG_READ, DMA_READ).
  • Sinks: Parameter index 2 (size/count) of memcpy, memmove, strncpy, wmemcpy, wmemmove.
  • Barriers: Public IRGuards via DataFlow::BarrierGuard<lessThanOrEqual/3> to recognize if (len <= MAX) conditions and prevent false positives. lessThanOrEqual uses the public Operand + getConvertedResultExpression() pattern.
  • Public API Compliance: Uses only public APIs (cpp, TaintTracking, IRGuards). Zero internal. / DataFlowImplCommon dependencies.

Verification & Test Results

  • Test Command:codeql test run cpp/ql/test/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/
  • Result:All 1 tests passed (3 positive alerts, 3 false-positive barrier test cases clean).
  • Documentation:codeql generate query-help passed DTD verification and rendered clean markdown.
  • Suite Integration: Explicit include of cpp/mmio-unsanitized-memcpy in cpp-security-extended.qls.

Checklist

  • Query metadata follows upstream style guidelines (@kind path-problem, @precision medium, @security-severity 8.6).
  • .qhelp file provided with valid DTD structure and Bad/Good examples.
  • Test directory contains .qlref, test.c, and verified .expected output.
  • Query added to appropriate .qls suite.
  • No internal. module imports used.

@Tito0015

Tito0015 commented Sep 2, 2026

Copy link
Copy Markdown
Author

Hi @github/codeql-cpp just checking in on this query submission! The query adds MMIO/DMA-to-memcpy bounds modeling for embedded C/C++ drivers, complete with unit tests and QL documentation. Let me know whenever the team has a moment to review or trigger CI.

@jketemajketema left a comment

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.

Hi, I'll try to find a reviewer for you. However, this query has very low quality (see below), not what we would consider "medium". Hence, at the very least it should be moved into the directory for experimental queries.

I ran the query on about 1000 databases, and most of the results seem unrelated to memory mapped I/O and look more cases where volatile is used for other (incorrect) reasons.

Comment threadcpp/ql/src/codeql-suites/cpp-security-extended.qls Outdated
Comment threadcpp/ql/src/Security/CWE/CWE-120/MmioUnsanitizedMemcpy.ql Outdated
@Tito0015
Tito0015 requested a review from a team as a code ownerSeptember 2, 2026 20:00
Tito0015 added a commit to Tito0015/codeql that referenced this pull request Sep 2, 2026
Relocate the query under experimental/, narrow sources to allowlisted MMIO register macros only, drop the security-extended suite include, and update tests and change notes for maintainer feedback on PR github#22438.
Co-authored-by: Cursor <cursoragent@cursor.com>
@Tito0015

Copy link
Copy Markdown
Author

Hi @jketema — thank you again for the feedback and for running this across the DB corpus! You were spot on regarding the generic volatile False Positive trap.

I have updated the PR with the following changes:

  • Relocated: Moved the query out of cpp-security-extended.qls and relocated it to cpp/ql/src/experimental/Security/CWE/CWE-120/MmioUnsanitizedMemcpy.ql with @precision low and query ID cpp/experimental/mmio-unsanitized-memcpy.
  • AST Deduplication: Removed redundant FunctionCall checks and strictly anchored sources to MacroInvocation expansions (READ_REG, GET_MMIO, REG_READ, DMA_READ).
  • Eliminated FP Trap: Completely removed generic volatile variable/field/pointer checks to eliminate noise on user-space applications.
  • Local SARIF Verification: Built a local verification harness and tested the query against a built CodeQL database of curl. The False Positive rate on standard C desktop code is now zero (0 alerts), while catching target MMIO/DMA register flows into memcpy without bounds checks in unit tests.

Let me know if this updated AST modeling looks ready for the experimental queue!

Comment thread.gitignore Outdated
Comment on lines +1 to +4
---
category: minorAnalysis
---
* Added a new experimental query, `cpp/experimental/mmio-unsanitized-memcpy`, to detect memory copy operations whose size argument is derived from allowlisted MMIO/DMA register-read macros without sufficient bounds validation.

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.

We don't publish change notes for experimental queries.

Comment threadcpp/ql/src/experimental/Security/CWE/CWE-120/MmioUnsanitizedMemcpy.ql Outdated
Comment threadcpp/ql/src/experimental/Security/CWE/CWE-120/MmioUnsanitizedMemcpy.ql Outdated
@Tito0015
Tito0015force-pushed the feature/cpp-mmio-unsanitized-memcpy branch from 971e182 to 5c92f5eCompareSeptember 3, 2026 00:51
@github-actions

Copy link
Copy Markdown
Contributor

QHelp previews:

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

MMIO/DMA unsanitized memory copy

Firmware and embedded drivers often copy data into buffers using lengths read from allowlisted MMIO register macros such as READ_REG or GET_MMIO. 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.

Recommendation

Always validate MMIO/DMA-derived lengths before passing them to memcpy, memmove, or strncpy. Compare against a compile-time maximum and reject or clamp out-of-range values before copying.

Example

Bad: length from an MMIO register used directly as the copy size.

#defineREAD_REG(addr) (*(volatile unsigned int *)(addr))
#defineMAX_DMA_LEN 64
void*memcpy(void*dest, constvoid*src, unsigned longn);
voidbad_mmio_memcpy(char*dst, char*src) {
unsigned intlen=READ_REG(0x40001000);
memcpy(dst, src, len);
}

Good: defensive bounds check before the copy.

#defineREAD_REG(addr) (*(volatile unsigned int *)(addr))
#defineMAX_DMA_LEN 64
void*memcpy(void*dest, constvoid*src, unsigned longn);
voidgood_mmio_memcpy(char*dst, char*src) {
unsigned intlen=READ_REG(0x40001000);
if (len <= MAX_DMA_LEN)
memcpy(dst, src, len);
}

References

  • CWE-120: Buffer Copy without Checking Size of Input
  • CWE-787: Out-of-bounds Write
  • Common Weakness Enumeration: CWE-120.
  • Common Weakness Enumeration: CWE-787.

@jketema

Copy link
Copy Markdown
Contributor

ql/cpp/ql/integration-tests/query-suite/test.py::test_not_included_queries fails because the new query has not been added to the test results.

Comment threadcpp/ql/src/experimental/Security/CWE/CWE-120/MmioUnsanitizedMemcpy.ql Outdated
@Tito0015
Tito0015force-pushed the feature/cpp-mmio-unsanitized-memcpy branch from 5c92f5e to 9ad7072CompareSeptember 4, 2026 01:01
Tito0015 added a commit to Tito0015/codeql that referenced this pull request Sep 4, 2026
Relocate the query under experimental/, narrow sources to allowlisted MMIO register macros only, drop the security-extended suite include, and update tests and change notes for maintainer feedback on PR github#22438.
Co-authored-by: Cursor <cursoragent@cursor.com>
@Tito0015

Copy link
Copy Markdown
Author

Hi @jketema — updated the query to use sink.getNode() as the alert location, updated the test .expected files, and added the new query path to not_included_in_qls.expected. Rebased on main. Thanks!

@jketema

jketema commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

I'm now seeing the following failure:

 File "ql/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/test.c" contains a non-ASCII character at the location marked with `|` in:
len = READ_REG(0x40001000);
memcpy(dst, src, 32); // GOOD |
ASCII check failed!

Tito0015and others added 6 commits September 4, 2026 18:12
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Relocate the query under experimental/, narrow sources to allowlisted MMIO register macros only, drop the security-extended suite include, and update tests and change notes for maintainer feedback on PR github#22438.
Co-authored-by: Cursor <cursoragent@cursor.com>
@Tito0015
Tito0015force-pushed the feature/cpp-mmio-unsanitized-memcpy branch from 9ad7072 to b858cfdCompareSeptember 4, 2026 22:12
@Tito0015

Copy link
Copy Markdown
Author

Thanks for catching that! Fixed the em dash comment in test.c to use ASCII-only text and rebased against upstream/main. Ready for a fresh CI pass.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@Tito0015@jketema