Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add logic for reading KASLR offset #951

Merged
merged 1 commit into from
Jan 13, 2025
Merged

Conversation

d-e-s-o
Copy link
Collaborator

@d-e-s-o d-e-s-o commented Dec 30, 2024

In order to (eventually...) support normalization of kernel addresses, we need to take into account whether the running kernel has address space layout randomization enabled. If that is the case, we will need to incorporate the randomization offset in the normalization process. This change introduces the necessary logic for reading said offset, so that it can be used down the line.
This change builds on all the infrastructure we added for making the ELF parser optionally work with using regular I/O APIs instead of relying on memory mapping.

Refs: #950

In order to (eventually...) support normalization of kernel addresses,
we need to take into account whether the running kernel has address
space layout randomization enabled. If that is the case, we will need to
incorporate the randomization offset in the normalization process. This
change introduces the necessary logic for reading said offset, so that
it can be used down the line.
This change builds on all the infrastructure we added for making the ELF
parser optionally work with using regular I/O APIs instead of relying on
memory mapping.

Refs: libbpf#950

Signed-off-by: Daniel Müller <[email protected]>
@d-e-s-o d-e-s-o requested a review from anakryiko December 30, 2024 19:27
Copy link

codecov bot commented Dec 30, 2024

Codecov Report

Attention: Patch coverage is 72.64151% with 29 lines in your changes missing coverage. Please review.

Project coverage is 94.28%. Comparing base (ebaf85f) to head (c337f08).

Files with missing lines Patch % Lines
src/normalize/kernel.rs 56.71% 29 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #951      +/-   ##
==========================================
- Coverage   94.50%   94.28%   -0.22%     
==========================================
  Files          57       58       +1     
  Lines       10603    10708     +105     
==========================================
+ Hits        10020    10096      +76     
- Misses        583      612      +29     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@d-e-s-o d-e-s-o force-pushed the topic/kaslr branch 3 times, most recently from 866e604 to cf78df4 Compare December 31, 2024 00:30
@danielocfb danielocfb force-pushed the topic/kaslr branch 2 times, most recently from 12d9352 to a87768d Compare January 2, 2025 20:40

/// Try to determine the KASLR state of the system.
fn determine_kaslr_state() -> Result<KaslrState> {
// https://www.kernel.org/doc/html/latest/admin-guide/sysctl/kernel.html#randomize-va-space
Copy link
Member

Choose a reason for hiding this comment

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

are you sure this has anything to do with KASLR? My interpretation is that this is purely user address space randomization, which you shouldn't care about here. I think I have it on my devserver set to 1, but I don't have KASLR enabled for the kernel.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm, you may be right. Let me remove this bit.


// Iterate through all available notes. See `elf(5)` for
// details.
while offset + (size_of::<ElfN_Nhdr>() as u64) <= phdr.file_size() {
Copy link
Member

Choose a reason for hiding this comment

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

nit: blazesym parses Elf notes at least in two places now, right? Why not add a proper iterator support and clean up this code (and the one that does build ID, right?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I thought about it, but couldn't find a good abstraction and the implementations have to exhibit certain differences for technical reasons. It's just a few lines of code, so I think it's fine.

Copy link
Member

Choose a reason for hiding this comment

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

No big deal and can be done as a follow up. But I do find all those offset checks, file size, etc quite mundane and error prone, so I still feel like iterator would be an improvement. It can return Nhdr + name/descr slices, which would save a bunch of error check and otherwise distracting plumbing. But up to you.

@d-e-s-o d-e-s-o force-pushed the topic/kaslr branch 2 times, most recently from c337f08 to 213b70d Compare January 13, 2025 22:13
Copy link
Member

@anakryiko anakryiko left a comment

Choose a reason for hiding this comment

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

LGTM, we'll find more bugs when this is actually put to practice ;)


// Iterate through all available notes. See `elf(5)` for
// details.
while offset + (size_of::<ElfN_Nhdr>() as u64) <= phdr.file_size() {
Copy link
Member

Choose a reason for hiding this comment

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

No big deal and can be done as a follow up. But I do find all those offset checks, file size, etc quite mundane and error prone, so I still feel like iterator would be an improvement. It can return Nhdr + name/descr slices, which would save a bunch of error check and otherwise distracting plumbing. But up to you.

@d-e-s-o d-e-s-o merged commit f9c81b4 into libbpf:main Jan 13, 2025
41 checks passed
@d-e-s-o d-e-s-o deleted the topic/kaslr branch January 13, 2025 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants