You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(hrr): ignore the capture output variable in privileged processes - #12252
HRR capture is enabled by the HIP_HRR_CAPTURE_OUTPUT environment variable, and the runtime has so far acted on it in every process. A set-user-ID, set-group-ID or file-capability program runs with more privileges than the user who starts it, while its environment still comes from that user, so it should not create files or record its own memory because an environment variable asks it to. This change makes capture ignore the variable in such processes, the same rule secure_getenv() applies.
Technical Details
On Linux the kernel sets AT_SECURE in the auxiliary vector when an exec changes the effective user or group ID or raises capabilities, and when an LSM asks for it (an SELinux domain transition, for example). The new hrr_cap::metadata::secure_exec() reads it with getauxval(AT_SECURE) and is always false on other platforms, so Windows builds are unaffected.
It lives in hip_capture_metadata.cpp rather than hip_capture.cpp because glibc's <sys/auxv.h> includes <elf.h>, whose Elf64_Ehdr and Elf64_Shdr typedefs collide with the ones CLR's ELF headers put in the global namespace; the metadata file includes no CLR headers. No build files change.
hip_capture_enabled() now requires that capture was requested, meaning the flag holds a character that is not whitespace (hrr_capture_requested(); CLR stores an exported empty variable as a single space), and that the process is not in secure-execution mode, read once. In such a process capture stays off entirely: no dispatch shims, no writer, nothing on disk.
When capture was requested but is ignored, hip_capture_init() prints one line on stderr saying that the program was started in secure-execution mode, so a user who expected an archive can see why there is none. It writes to stderr directly because the CLR log is silent at the default AMD_LOG_LEVEL.
The rule is documented in hip_capture.h, in projects/hrr/DESIGN.md and in the capture environment table of projects/hrr/README.md. The HIP how-to hip_record_replay.rst gets the same note in a separate pull request, because it needs a HIP docs review.
Rebased on develop after #12148: the change sits inside the HIP_HRR_CAPTURE_ENABLED block of hip_capture_init(), and nothing is added to its #else branch.
Issue Tracking
JIRA ID: AIRUNTIME-2828
Test Plan
hrr_secure_exec_test.cc in hrr-integration-tests covers both rules. Unit_HRR_BlankCaptureOutputCapturesNothing runs the workload with an empty and a whitespace value. Unit_HRR_SecureExecIgnoresCaptureOutput runs a set-group-ID copy of the test binary for one of the user's supplementary groups, without root. It skips unless that copy loads the same libamdhip64 without LD_LIBRARY_PATH, and on the hrr-ci bare runners it does not.
Regenerate the HRR API files with gen_hrr_api_args.py --check-hrr-coverage and compare them with the committed ones.
Build the CLR capture objects with -Wall -Wextra, with and without -DHIP_HRR_CAPTURE_ENABLED.
With the define, build for gfx950 and run hrr-unit-tests and hrr-integration-tests on an MI350X.
By hand on the same machine: a small HIP program installed as a plain binary, set-user-ID root, set-group-ID root and with a file capability, each started by an unprivileged user with HIP_HRR_CAPTURE_OUTPUT set, once with the develop capture runtime and once with this one.
Test Result
Check
Result
Generated HRR API files
identical to the committed ones
CLR capture objects, default build
compile, no warnings in the HRR files
CLR capture objects, -DHIP_HRR_CAPTURE_ENABLED
compile, no warnings in the HRR files
hrr-unit-tests, gfx950 build with capture compiled in
pass (23 test cases)
hrr-integration-tests, MI350X
pass (54 test cases)
New cases on hrr-ci, cross-checked on users/danielbolsson/hrr-development
pass; with the change reverted both fail. The set-group-ID case runs on gfx94x once the capture build is in the test RUNPATH
Manual check. Every program is started by uid 65534 unless the row says otherwise. The develop runtime is 89103b1c577; since then develop has only moved the capture code inside the HIP_HRR_CAPTURE_ENABLED block.
Program
develop
this PR
plain, started by root
archive written
archive written
plain
archive written
archive written
set-group-ID root
archive written
notice, nothing written
file capability
archive written
notice, nothing written
set-user-ID root
archive partly written, then crashes
notice, nothing written, then crashes
set-user-ID root, variable unset
crashes
crashes
set-group-ID root, variable set to ""
nothing written
nothing written, no notice
The set-user-ID program crashes with SIGSEGV with or without the variable, on develop as well, so the crash does not come from this change. The last row is the case the review fix changed: the previous revision printed the notice there although capture had not been requested.
The reason will be displayed to describe this comment to others. Learn more.
No blocking issues. The gate is sound: every HRR side effect (shim install, writer open, atexit/crash-handler registration, metadata collection, HIP_HRR_DEBUG_ARGS traces and the AMD_LOG_LEVEL raise) sits behind hip_capture_enabled() in hip_capture_init(), and hip_capture_init() runs once under std::call_once in hip::init, so the stderr line prints at most once per process. I found no path that writes or records memory in an AT_SECURE process after this change.
Docs that the new rule makes incomplete (please update in this PR, outside the diff):
projects/clr/hipamd/src/hrr/hip_capture.h:36: "Check if capture is enabled (HIP_HRR_CAPTURE_OUTPUT env var set and non-empty)". The rule now also requires a non-secure exec.
projects/hrr/DESIGN.md:671: "Capture is enabled when HIP_HRR_CAPTURE_OUTPUT is set to a non-empty directory".
projects/hrr/README.md:88: the HIP_HRR_CAPTURE_OUTPUT row in the capture environment table.
projects/hip/docs/how-to/hip_record_replay.rst:72 (user-facing): one sentence saying the variable is ignored in set-user-ID, set-group-ID and file-capability programs would fit next to the existing data-sensitivity warning.
Non-blocking:
The closest in-tree precedent, ROCr's HSA_TOOLS_LIB check (projects/rocr-runtime/runtime/hsa-runtime/core/runtime/runtime.cpp:3141-3151), also checks geteuid() != getuid() || getegid() != getgid(), and libhsakmt (hsakmtmodel.c:57-74) fails closed when getauxval is not available. AT_SECURE alone covers the cases in the PR description, so this is only for consistency (see inline).
No automated test. That seems fine because it needs a set-user-ID or setcap binary, but please say so in the Test Plan. A root-gated test skipped by default, built on SpawnProc with stderr capture, would be possible later.
Docs are updated in 1c7fd040b9: the hip_capture.h comment, the Enable Flag and Init sections of DESIGN.md, the README row and a sentence after the warning in the HIP how-to. The Test Plan now says there's no automated test and why, and the SpawnProc test goes in a follow-up ticket. It turns out it doesn't need root: a set-group-ID copy for one of the user's own groups is enough. On libhsakmt, secure_exec() doesn't build on Linux without getauxval, so there's no fallback that could let capture through.
The reason will be displayed to describe this comment to others. Learn more.
Security-gap complement on AIRUNTIME-2828. Two cases are not covered by this change yet.
An exported empty HIP_HRR_CAPTURE_OUTPUT still enables capture. #12257 records that CLR's flag parser turns an empty value into a single space, and this PR treats only "" as off. An empty value, including that single space, needs to leave capture off.
The setgid test the description leaves as a follow-up needs to land with this change: a setgid copy for one of the test user's supplementary groups, on a filesystem that is not mounted nosuid, captures nothing and prints the secure-execution notice.
Done in 5c274aa: a value that is empty or only whitespace leaves capture off, since CLR stores an exported empty variable as a single space. Unit_HRR_BlankCaptureOutputCapturesNothing runs the workload with "" and with " \t " and expects nothing written.
Done in 8b48244: Unit_HRR_SecureExecIgnoresCaptureOutput runs a set-group-ID copy of the test binary for one of the user's supplementary groups, so it needs no root. It expects no archive and the notice. A copy without the bit must capture and print no notice, and the set-group-ID copy prints no notice when the variable is unset.
Secure-execution mode ignores LD_LIBRARY_PATH, so the case skips unless the copy loads this build's libamdhip64 without it. On hrr-ci as it is, it skips everywhere. The test binary's RUNPATH points at the SDK lib, so on gfx94x and gfx1151 the copy loads _rocm_sdk_devel's libamdhip64, which has no gate. On gfx103x and gfx120x the copy doesn't start (libhiprtc.so.7 not found). It also skips on a nosuid filesystem, under no_new_privs and when AT_SECURE isn't set. Every skip comes before the first assertion, so a pass means the gate ran.
In my cross-check on your branch I added -DCMAKE_BUILD_RPATH=${GITHUB_WORKSPACE}/build/clr/hipamd/lib to the hrr configure. With it the case passes on gfx94x. With the gate reverted it fails on gfx94x and on the gfx120x pod runners, and the blank case fails on gfx94x, gfx103x, gfx110x and gfx120x. Happy to send that line for hrr-ci.yml if you want it.
The set-group-ID case now defines AT_SECURE to its Linux ABI value when
<sys/auxv.h> lacks it, as hip_capture_metadata.cpp does. It also checks
that the secure process leaves no capture directory at all, not only no
events.bin under it.
Co-Authored-By: Claude <noreply@anthropic.com>
When cap is empty this leaves the parent’s HIP_HRR_CAPTURE_OUTPUT in the child rather than unsetting it. If the integration test itself is launched with capture enabled, the supposedly unset run still requests capture, prints the secure-execution notice, and fails the assertion at line 219; add an unset/remove-environment operation to SpawnProc (or otherwise build a clean environment) for this case.
The secure-execution note in hip_record_replay.rst needs a HIP docs
code-owner review. It moves to its own pull request so this one can
merge on the HRR review alone.
Co-Authored-By: Claude <noreply@anthropic.com>
… programs
The HRR how-to gains one paragraph: on Linux, a program the kernel
starts in secure-execution mode ignores HIP_HRR_CAPTURE_OUTPUT, records
nothing and says why on standard error. The behaviour comes with
#12252.
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
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.
danielbolsson
left a comment
github-actions
Bot
added
documentation
Motivation
HRR capture is enabled by the
HIP_HRR_CAPTURE_OUTPUTenvironment variable, and the runtime has so far acted on it in every process. A set-user-ID, set-group-ID or file-capability program runs with more privileges than the user who starts it, while its environment still comes from that user, so it should not create files or record its own memory because an environment variable asks it to. This change makes capture ignore the variable in such processes, the same rulesecure_getenv()applies.Technical Details
AT_SECUREin the auxiliary vector when an exec changes the effective user or group ID or raises capabilities, and when an LSM asks for it (an SELinux domain transition, for example). The newhrr_cap::metadata::secure_exec()reads it withgetauxval(AT_SECURE)and is always false on other platforms, so Windows builds are unaffected.hip_capture_metadata.cpprather thanhip_capture.cppbecause glibc's<sys/auxv.h>includes<elf.h>, whoseElf64_EhdrandElf64_Shdrtypedefs collide with the ones CLR's ELF headers put in the global namespace; the metadata file includes no CLR headers. No build files change.hip_capture_enabled()now requires that capture was requested, meaning the flag holds a character that is not whitespace (hrr_capture_requested(); CLR stores an exported empty variable as a single space), and that the process is not in secure-execution mode, read once. In such a process capture stays off entirely: no dispatch shims, no writer, nothing on disk.hip_capture_init()prints one line on stderr saying that the program was started in secure-execution mode, so a user who expected an archive can see why there is none. It writes to stderr directly because the CLR log is silent at the defaultAMD_LOG_LEVEL.hip_capture.h, inprojects/hrr/DESIGN.mdand in the capture environment table ofprojects/hrr/README.md. The HIP how-tohip_record_replay.rstgets the same note in a separate pull request, because it needs a HIP docs review.HIP_HRR_CAPTURE_ENABLEDblock ofhip_capture_init(), and nothing is added to its#elsebranch.Issue Tracking
JIRA ID: AIRUNTIME-2828
Test Plan
hrr_secure_exec_test.ccinhrr-integration-testscovers both rules.Unit_HRR_BlankCaptureOutputCapturesNothingruns the workload with an empty and a whitespace value.Unit_HRR_SecureExecIgnoresCaptureOutputruns a set-group-ID copy of the test binary for one of the user's supplementary groups, without root. It skips unless that copy loads the same libamdhip64 withoutLD_LIBRARY_PATH, and on the hrr-ci bare runners it does not.gen_hrr_api_args.py --check-hrr-coverageand compare them with the committed ones.-Wall -Wextra, with and without-DHIP_HRR_CAPTURE_ENABLED.hrr-unit-testsandhrr-integration-testson an MI350X.HIP_HRR_CAPTURE_OUTPUTset, once with the develop capture runtime and once with this one.Test Result
-DHIP_HRR_CAPTURE_ENABLEDhrr-unit-tests, gfx950 build with capture compiled inhrr-integration-tests, MI350Xusers/danielbolsson/hrr-developmentManual check. Every program is started by uid 65534 unless the row says otherwise. The develop runtime is 89103b1c577; since then develop has only moved the capture code inside the
HIP_HRR_CAPTURE_ENABLEDblock.""The set-user-ID program crashes with SIGSEGV with or without the variable, on develop as well, so the crash does not come from this change. The last row is the case the review fix changed: the previous revision printed the notice there although capture had not been requested.
Submission Checklist