Conversation
Contributor
|
Thanks @camuso . I will merge this before next release. |
There was a problem hiding this comment.
🟡 Changes recommended
Symlinked parent components remain exploitable, file permissions regress, and existing lock tests will fail.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Hardens privileged file descriptors and safe_fopen() against inheritance and symlink attacks.
Changes:
- Adds
O_CLOEXECto MSR and API-lock descriptors. - Replaces
lstat()/fopen()withopen(O_NOFOLLOW)/fdopen().
File summaries
| File | Description |
|---|---|
pqos/common.c |
Atomically rejects final-component symlinks. |
lib/machine.c |
Prevents MSR descriptor inheritance. |
lib/lock.c |
Prevents API-lock descriptor inheritance. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| else | ||
| return NULL; | ||
|
|
||
| fd = open(name, flags | O_NOFOLLOW, perms); |
Comment on lines
+263
to
+264
| m_apilock = | ||
| open(LOCKFILE, O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, LOCKFILE_PERMS); |
| return NULL; | ||
| else | ||
| new_file = 1; | ||
| mode_t perms = S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH; /* 0644 */ |
Comment on lines
+384
to
+388
| fd = open(name, flags | O_NOFOLLOW, perms); | ||
| if (fd == -1) { | ||
| if (errno == ELOOP) | ||
| printf("File %s is a symlink\n", name); | ||
| return NULL; |
Add O_CLOEXEC to long-lived descriptor opens in lib/machine.c (MSR device files) and lib/lock.c (API lock file) as defense-in-depth against leaking privileged file descriptors across exec boundaries. Without O_CLOEXEC, these descriptors survive execve() and can be inherited by child processes that should not have access to MSR registers or the API lock. Signed-off-by: Tony Camuso <tcamuso@redhat.com>
Replace the lstat()+fopen() pattern in safe_fopen() with
open(O_NOFOLLOW)+fdopen(). The original code validates symlinks
only after fopen() has already opened and potentially truncated
the symlink target. With fopen("w+"), a pre-existing symlink
causes the target to be truncated to zero bytes before the
post-open lstat/fstat comparison can detect the mismatch.
Using open() with O_NOFOLLOW makes the symlink rejection atomic:
the kernel returns ELOOP without opening or modifying anything.
Signed-off-by: Tony Camuso <tcamuso@redhat.com>
Add a new cmocka test suite exercising safe_fopen() from pqos/common.c:
- NULL name / NULL mode / invalid mode → returns NULL
- Regular file write+read round-trip
- Symlink rejection: safe_fopen through a symlink returns NULL
and the target file is not truncated (O_NOFOLLOW)
- File permissions: newly created files get 0666 & ~umask,
matching fopen() behaviour
- Append mode: existing content is preserved
The test links only common.o (plus a stub for
pqos_get_num_mem_regions) so it is independent of the mock library
and builds cleanly with cmocka 2.x.
Signed-off-by: Tony Camuso <tcamuso@redhat.com>
Author
|
@rkanagar |
camuso
force-pushed
the
fix-fd-leak-and-symlink-race
branch
from
September 14, 2026 13:43
e558710 to
c2acdc4
Compare
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.
Two security issues exist in intel-cmt-cat that allow local privilege
escalation or data corruption when the tools are exposed to non-root
users through sudo or similar delegation:
libpqos opens MSR device files and the API lock file without
O_CLOEXEC. These long-lived descriptors surviveexecve()andcan be inherited by child processes that should not have access to
MSR registers or the API lock.
safe_fopen()in pqos is vulnerable to symlink-following filetruncation. The current implementation calls
lstat()to checkfor symlinks, then
fopen()to open the file. With mode"w+",fopen()follows a pre-existing symlink and truncates the targetbefore the post-open
lstat/fstatcomparison can detect themismatch. No race condition is required — a pre-existing symlink
is sufficient to truncate an arbitrary file.
The series is structured as follows:
Commit 1/2 adds
O_CLOEXECto long-lived descriptor opens inlib/machine.c(MSR device files) andlib/lock.c(API lock file)as defense-in-depth. These descriptors will be automatically closed
on any
execve()call.Commit 2/2 replaces the
lstat()+fopen()pattern inpqos/common.c:safe_fopen()withopen(O_NOFOLLOW)+fdopen().This eliminates the symlink vulnerability entirely — the kernel
rejects symlinks atomically at open time with
ELOOP, so there isno window for truncation to occur.
Note:
rdtset(removed in v26.06) also leaked inherited MSR filedescriptors to child processes after privilege drop. That issue is
no longer present on master since
rdtsethas been removed.Tested on RHEL-10 and RHEL-9 with intel-cmt-cat 26.03 on Intel Xeon
Gold and Dell PowerEdge R660 platforms. The symlink test confirms
that on unpatched code,
fopen("w+")truncates the symlink target;on patched code,
O_NOFOLLOWrejects it withELOOP.