Re: [PATCH v5 12/24] vdso: Add the vdsocheck tool

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Parsing failure for valid ELF files containing `SHT_NOBITS` sections.
- [Medium] `validate_linked_dso` blindly rejects DSOs with `SHT_REL` / `SHT_RELA` sections, dropping the workaround for `R_*_NONE` relocations.
- [Low] Unchecked slice indexing in `get_data_subslice` causes unhandled panics on malformed or adversarial ELF files.
- [Low] vdsocheck.rs fails to compile due to multiple lifetime violations and a move-while-borrowed error.
--

commit 043a7ddfce689ccb30ea6aee60c35639fb8be5b1
Author: Thomas Weißschuh <thomas.weissschuh@linutronix.de>

vdso: Add the vdsocheck tool

This commit introduces a Rust-based vdsocheck tool to validate that the vDSO
library does not contain absolute dynamic relocations, moving this logic out of
inline shell commands.

> diff --git a/lib/vdso/check/elf.rs b/lib/vdso/check/elf.rs
> new file mode 100644
> index 0000000000000..520285d6fd10a
> --- /dev/null
> +++ b/lib/vdso/check/elf.rs
[ ... ]
> +fn get_data_subslice(data: &[u8], offset: u64, size: u64) -> ParseResult<&[u8]> {
> +    let start: usize = offset.try_into().map_err(|_| ParseError::IndexOutOfRange)?;
> +    let size: usize = size.try_into().map_err(|_| ParseError::IndexOutOfRange)?;
> +    let end = start.checked_add(size).ok_or(ParseError::IndexOutOfRange)?;
> +
> +    Ok(&data[start..end])
> +}

[Severity: Low]
Does this slice indexing panic if the ELF file specifies an out-of-bounds size
where end exceeds data.len()?

Could this use data.get(start..end).ok_or(...) instead to return a graceful
parsing error rather than failing on malformed ELF files?

[ ... ]
> +impl<'f: 'a, 'a> Iterator for SectionHeaderIterator<'f, 'a> {
> +    type Item = ParseResult<SectionHeader<'f>>;
> +
> +    fn next(&mut self) -> Option<Self::Item> {
[ ... ]
> +        match &mut self.0 {
> +            ClassAlternative::Elf32(iter) => iter.next().map(|n| {
> +                Ok(SectionHeader {
> +                    name: n.sh_name.to_cpu(iter.byteorder).into(),
> +                    type_: n.sh_type.to_cpu(iter.byteorder).into(),
> +                    entsize: n.sh_entsize.to_cpu(iter.byteorder).into(),
> +                    data: get_data_subslice(
> +                        file_data,
> +                        n.sh_offset.to_cpu(iter.byteorder).into(),
> +                        n.sh_size.to_cpu(iter.byteorder).into(),
> +                    )?,
> +                })
> +            }),
[ ... ]

[Severity: Medium]
Will this fail to parse valid ELF files containing sections with no file data
like .bss?

Since sh_size for these sections represents memory allocation size rather than
file size, calling get_data_subslice() here attempts to read non-existent
bytes from the file, which could cause a parsing failure.

[ ... ]
> +impl<'a> RelSection<'a> {
> +    pub fn entries(&'a self) -> ParseResult<RelSectionIterator<'a>> {
> +        RelSectionIterator::new(&self.0)
> +    }
> +}

[Severity: Low]
Does this method signature enforce a lifetime bound (&'a self) that cannot be
satisfied when iterating over locally constructed sections? This appears to
cause a compile failure in validate_object_file().

[ ... ]
> diff --git a/lib/vdso/check/vdsocheck.rs b/lib/vdso/check/vdsocheck.rs
> new file mode 100644
> index 0000000000000..f4c0730bc976d
> --- /dev/null
> +++ b/lib/vdso/check/vdsocheck.rs
[ ... ]
> +fn validate_linked_dso<'a>(file: &'a elf::File<'a>) -> ValidationResult<'a> {
> +    for section in file.sections()? {
> +        let section = section?;
> +
> +        /* No relocations are allowed */
> +        match section {
> +            elf::Section::Rel(_) | elf::Section::Rela(_) => {
> +                return Err(ValidationError::UnexpectedSection(section))
> +            }
> +            _ => {}
> +        }
> +    }
> +
> +    Ok(())
> +}

[Severity: Medium]
Does this blindly reject DSOs containing relocation sections, even if they
solely contain ignored relocations?

The inline shell script this replaces in lib/vdso/Makefile.include explicitly
ignored _NONE relocations:

  cmd_vdso_check = if $(READELF) -rW $@ | grep -v _NONE | grep -q " R_\w*_"; \

Does unconditionally rejecting all relocation sections drop this workaround for
toolchains that emit empty or _NONE relocations?

[ ... ]
> +fn validate_object_file<'a>(file: &'a elf::File<'a>) -> ValidationResult<'a> {
[ ... ]
> +        match section {
> +            elf::Section::Rel(ref rel) => {
> +                for entry in rel.entries()? {
> +                    if !allowed_relocs.in_object_file.contains(&entry.type_) {
> +                        return Err(ValidationError::InvalidRelocation(section, entry.type_));
> +                    }
> +                }
> +            }

[Severity: Low]
Will this fail to compile due to a move-while-borrowed error?

The variable section is moved into ValidationError::InvalidRelocation while it
is still borrowed by ref rel.

Combined with the lifetime requirements of rel.entries(), this breaks the
build when the vdso check config is enabled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-vdso-absolute-reloc-v5-0-3a93791af1cf@linutronix.de?part=12