Skip to content

fix: zero-fill NOBITS sections in ELF loader#1379

Closed
ludfjig wants to merge 1 commit intohyperlight-dev:mainfrom
ludfjig:elf-nobits-fix
Closed

fix: zero-fill NOBITS sections in ELF loader#1379
ludfjig wants to merge 1 commit intohyperlight-dev:mainfrom
ludfjig:elf-nobits-fix

Conversation

@ludfjig
Copy link
Copy Markdown
Contributor

@ludfjig ludfjig commented Apr 15, 2026

Some linkers emit PT_LOAD segments where filesz == memsz but contain .bss sections whose VMA range overlaps with file bytes from unrelated sections. The loader copies the full segment verbatim, leaving .bss with stale data instead of zeros.

Collect NOBITS section ranges (excluding .tbss) during ELF parsing and zero-fill them after loading PT_LOAD segments.

This is needed for unikraft

Some linkers emit PT_LOAD segments where filesz == memsz but contain
.bss sections whose VMA range overlaps with file bytes from unrelated
sections. The loader copies the full segment verbatim, leaving .bss
with stale data instead of zeros.

Collect NOBITS section ranges (excluding .tbss) during ELF parsing and
zero-fill them after loading PT_LOAD segments.

Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>

Co-authored-by: danbugs <danilochiarlone@gmail.com>
@ludfjig ludfjig added the kind/bugfix For PRs that fix bugs label Apr 15, 2026
@ludfjig ludfjig marked this pull request as ready for review April 15, 2026 03:07
This was referenced Apr 15, 2026
// Zero-fill NOBITS sections (e.g. .bss) that were not already
// covered by the filesz < memsz zeroing above.
for &(addr, size) in &self.nobits_ranges {
let sh_start = (addr - base_va) as usize;
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.

Nit: I am not sure if it can happen, but is this guaranteed to not fail? Maybe a checked sub is better here

@danbugs danbugs requested a review from Copilot April 15, 2026 22:49
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the Hyperlight host ELF loader to correctly zero-initialize NOBITS sections (e.g. .bss) even in cases where PT_LOAD segments have p_filesz == p_memsz, addressing linkers that place .bss VMAs overlapping unrelated file-backed bytes (notably needed for Unikraft).

Changes:

  • Collects NOBITS section (VA, size) ranges during ELF parsing, excluding TLS NOBITS (.tbss).
  • After loading PT_LOAD segments, explicitly zero-fills the collected NOBITS ranges.

Comment on lines +226 to +227
let sh_start = (addr - base_va) as usize;
let sh_end = sh_start + size as usize;
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

sh_start is computed with (addr - base_va) as usize and sh_end with sh_start + size as usize. If addr < base_va (or if the addition overflows usize), this can wrap and potentially make sh_end <= target.len() true, leading to a panic on the subsequent slice. Use checked arithmetic (checked_sub, checked_add, and usize::try_from) and skip/return an error when the range is not representable or falls before base_va.

Suggested change
let sh_start = (addr - base_va) as usize;
let sh_end = sh_start + size as usize;
let Some(sh_offset) = addr.checked_sub(base_va) else {
tracing::warn!(
"NOBITS section at VA {:#x} (size {:#x}) falls before base VA {:#x}, skipping zero-fill",
addr,
size,
base_va
);
continue;
};
let Ok(sh_start) = usize::try_from(sh_offset) else {
tracing::warn!(
"NOBITS section at VA {:#x} (size {:#x}) offset from base VA {:#x} is not representable, skipping zero-fill",
addr,
size,
base_va
);
continue;
};
let Ok(sh_size) = usize::try_from(size) else {
tracing::warn!(
"NOBITS section at VA {:#x} has size {:#x} that is not representable, skipping zero-fill",
addr,
size
);
continue;
};
let Some(sh_end) = sh_start.checked_add(sh_size) else {
tracing::warn!(
"NOBITS section at VA {:#x} (size {:#x}) overflows target range calculation, skipping zero-fill",
addr,
size
);
continue;
};

Copilot uses AI. Check for mistakes.
Comment on lines +133 to +141
elf.section_headers
.iter()
.filter(|sh| {
sh.sh_type == section_header::SHT_NOBITS
&& sh.sh_size > 0
&& (sh.sh_flags & u64::from(section_header::SHF_TLS)) == 0
})
.map(|sh| (sh.sh_addr, sh.sh_size))
.collect()
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

When collecting NOBITS ranges, consider filtering to only sections that are actually mapped into memory (e.g. SHF_ALLOC) in addition to excluding TLS. As-is, non-ALLOC NOBITS sections (often with sh_addr == 0) can cause spurious warnings during load, and in some layouts could lead to zero-filling unintended parts of the image if base_va is also 0.

Copilot uses AI. Check for mistakes.
Comment on lines +223 to +229
// Zero-fill NOBITS sections (e.g. .bss) that were not already
// covered by the filesz < memsz zeroing above.
for &(addr, size) in &self.nobits_ranges {
let sh_start = (addr - base_va) as usize;
let sh_end = sh_start + size as usize;
if sh_end <= target.len() {
target[sh_start..sh_end].fill(0);
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

This change introduces new loader behavior (zero-filling NOBITS sections even when p_filesz == p_memsz). There doesn’t appear to be a test asserting this correctness fix; adding a unit/integration test that reproduces the Unikraft-style layout (NOBITS VMA overlapping file bytes) would help prevent regressions.

Copilot generated this review using guidance from repository custom instructions.
@danbugs
Copy link
Copy Markdown
Contributor

danbugs commented Apr 16, 2026

I think this might still be worth to do from a correctiveness standpoint. But it seems that, at some point, this stopped being a requirement for Unikraft–see hyperlight-unikraft running off unmodified latest HL.

@danbugs
Copy link
Copy Markdown
Contributor

danbugs commented Apr 16, 2026

Won't merge rn. Focusing on #1385

@danbugs danbugs closed this Apr 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bugfix For PRs that fix bugs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants