fix: zero-fill NOBITS sections in ELF loader#1379
fix: zero-fill NOBITS sections in ELF loader#1379ludfjig wants to merge 1 commit intohyperlight-dev:mainfrom
Conversation
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>
| // 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; |
There was a problem hiding this comment.
Nit: I am not sure if it can happen, but is this guaranteed to not fail? Maybe a checked sub is better here
There was a problem hiding this comment.
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.
| let sh_start = (addr - base_va) as usize; | ||
| let sh_end = sh_start + size as usize; |
There was a problem hiding this comment.
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.
| 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; | |
| }; |
| 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() |
There was a problem hiding this comment.
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.
| // 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); |
There was a problem hiding this comment.
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.
|
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. |
|
Won't merge rn. Focusing on #1385 |
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