Repository navigation
fix(hrr): bound the symbol offset in graph memcpy to/from-symbol replay - #14350
Merged
Merged
Conversation
The six symbol-copy graph handlers added the archive's uint64_t offset to the live symbol address and passed the sum to the 1D memcpy node calls without reading the registered symbol size, so nothing bounded offset + count. An offset past the symbol could write or read outside it, or fault at graph launch. Resolve the symbol through one helper that refuses the record with hipErrorInvalidValue, before any HIP call, when offset + count exceeds the registered size. The comparison never forms the sum, so it cannot wrap. A symbol registered with size 0 (the archive size was substituted) refuses every copy. Add handler tests with stand-ins for the three 1D HIP calls: in-range copies reach HIP at symbol + offset; out-of-range, wrapping and size-0 records return hipErrorInvalidValue without reaching it. AIRUNTIME-3093
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
andraghetti
reviewed
Oct 29, 2026
andraghetti
left a comment
andraghetti
left a comment
Contributor
There was a problem hiding this comment.
Gated on MI350 (gbt350): head 8ad105f green; with hip_playback.cpp reverted, OutOfRange, WrappingSum and ZeroSize fail.
…und (AIRUNTIME-3093) playback___hipRegisterVar never registers a size-0 symbol: hipModuleGetGlobal fails for a size-0 variable, so the symbol is not recorded at all. The 'size == 0 ||' term in resolve_symbol_range could only fire on a registry state a real archive cannot produce. Remove it and the two unit cases that did not earn their place: the size-0 case, and the unregistered-symbol case, which passes against develop's own hipErrorInvalidSymbol.
# Conflicts: # projects/hrr/tests/unit/CMakeLists.txt
andraghetti
approved these changes
Nov 2, 2026
Combine the symbol-copy bounds check (resolve_symbol_range, AIRUNTIME-3093) with the host-source refusal (to_symbol_source) in every symbol-copy handler; keep both new unit test files.
andraghetti
reviewed
Nov 2, 2026
Contributor
TheRock Submodule Bump ActivityNewest first
|
danielbolsson
added a commit
that referenced
this pull request
Nov 6, 2026
develop's #14350 added three hrr-handler-tests cases (52 compiled vs 49 listed), so the compiled-case-list check failed. The case set was checked against the TEST_CASE names in the handler sources, not a built binary. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
JIRA ID : AIRUNTIME-3093
Motivation
An HRR archive is untrusted input. The six graph symbol-copy replay handlers add the record's
uint64_t offsetto the live address of the symbol and pass the sum to CLR's 1D memcpy node calls. None of them reads the symbol's registered size, so nothing boundsoffset + count. When the address does not resolve to a memory object, CLR skips its own direction and size checks, and a host-to-host or default-kind node can reach an unchecked hostmemcpyat an address the symbol does not own.On gbt350 (MI350X) with a 64-byte
__device__global, a standalone program making the samehipGraphAddMemcpyNode1D(symbol + offset, ...)call showed: offsets 0 to 64 validated, offsets 65 to about 4095 succeeded silently as an out-of-bounds write, and offset 4096 faulted at graph launch. The thresholds depend on the process layout.Technical Details
projects/hrr/playback/hip_playback.cpp: newresolve_symbol_range(). It looks the symbol up with its registered size and refuses the record withhipErrorInvalidValue, before any HIP call, whenoffset > sizeorcount > size - offset. The sum is never formed, so it cannot wrap.translate_symbolcalls inhipGraphAddMemcpyNodeToSymbolandhipGraphAddMemcpyNodeFromSymbol, and the file-localsymbol_or_complainused by the fourhipGraph{,Exec}MemcpyNodeSetParams{To,From}Symbolhandlers, so all six go through one check.projects/hrr/tests/unit/hrr_symbol_copy_bounds_test.cc(new, added toHRR_HANDLER_TEST_SOURCES): three handler-level cases over all six handlers, with a fake symbol registry and stand-ins forhipGraphAddMemcpyNode1D,hipGraphMemcpyNodeSetParams1DandhipGraphExecMemcpyNodeSetParams1Dthat count calls. No GPU is needed.Not changed: CLR's arena fallback reports a
SIZE_MAXsize, which defeats its own bound check for other producers of such nodes. That is a separate change.Issue Tracking
JIRA ID : AIRUNTIME-3093
Test Plan
hrr-handler-tests "Unit_HRR_SymbolCopyBounds_*": an in-range copy (including one ending exactly at the symbol's end and a zero-length one atoffset == size) reaches HIP once, aimed atsymbol + offset; out-of-range and wrapping (UINT64_MAX - 3plus 8) records returnhipErrorInvalidValuewith zero HIP calls.hip_playback.cppreverted todevelop.hrr-unit-tests,hrr-handler-testsandhrr-integration-testson gbt350 (MI350X,HIP_VISIBLE_DEVICES=4,5) with a capture-enabled CLR built from the same tree.hrr-playback: a program captures all six spellings against a 64-byte__device__global;offsetin each record is then patched inevents.binto 64, 4096 and0xFFFFFFFFFFFFFFFC, and each archive is replayed with this branch and withdevelop.Test Result
hrr-handler-tests: 35 cases, 710 assertions pass (three new).hrr-unit-tests: 82 cases, 1524 assertions pass.hrr-integration-tests: 150 pass, 1 fails,Unit_HRR_SecureExecIgnoresCaptureOutput(AIRUNTIME-3074, known and unrelated).hip_playback.cppreverted (andraghetti's MI350 run); the in-range control passes.... does not fit symbol ... (64 bytes registered); refusing the call. Withdevelop, 16 of the 18 replay with exit 0 and no error; the other two fail inside CLR athipGraphLaunch(offset 4096, Exec spellings).hrr_test_config.py checkforintegration.yamlis OK (292 cases). The same check forunit.yamlalready fails ondevelop(42 cases not in the config, none of them from this change); the new cases are inhrr-handler-tests, which no config lists.Submission Checklist