Re: [PATCH 5/5] elf-parse: bound symbol names by the string table

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] elf-parse: pointer addition before bounds check in elf_open_ro()
- [Medium] elf-parse: unchecked shstrtab and sh_name in elf_section_name()
--

commit 33f3711117c58495bbc5517ccf237e3d77449cb0
Author: Leizhen Zhang <lzsx618@gmail.com>

elf-parse: bound symbol names by the string table

This commit validates that the ELF symbol string table lies within the file
mapping and ends in a NUL byte during elf_open_ro(). It also introduces
elf_symbol_name() to bounds-check symbol st_name offsets against the string
table size in elf_read_symbols().

> diff --git a/scripts/elf-parse.c b/scripts/elf-parse.c
> index 756843c433ee8..8083b9ec3477f 100644
> --- a/scripts/elf-parse.c
> +++ b/scripts/elf-parse.c

[ ... ]

> @@ -355,8 +356,22 @@ int elf_open_ro(char const *fname, uint32_t types, struct elf_file *elf)
>  		elf_close(elf);
>  		return -1;
>  	}
> -	elf->strtab = (const char *)elf->base +
> -		      shdr_offset(elf_section(elf, shdr_link(elf->symtab)));
> +	strtab = elf_section(elf, shdr_link(elf->symtab));
> +	stroff = shdr_offset(strtab);
> +	elf->strtab_size = shdr_size(strtab);
> +	elf->strtab = (const char *)elf->base + stroff;
> +	/*
> +	 * Symbol names are read by offset into this table, so it has to be
> +	 * inside the mapping and has to end in a NUL for a name that starts
> +	 * inside it to end inside it as well.
> +	 */
> +	if (stroff > elf->size || elf->size - stroff < elf->strtab_size ||
> +	    !elf->strtab_size || elf->strtab[elf->strtab_size - 1]) {

[Severity: Medium]
Can computing elf->strtab before checking stroff trigger a pointer overflow?

In elf_open_ro(), (const char *)elf->base + stroff is evaluated with the
unvalidated file-controlled stroff (such as 0xffffffffffffffff) before the
stroff > elf->size check runs. Under UBSan, this triggers a pointer-overflow
failure on malformed ELF inputs before the bounds check can return an error.

Should elf->strtab be assigned after validating that stroff and
elf->strtab_size fit within elf->size, with elf->strtab[elf->strtab_size - 1]
checked after that assignment?

> +		fprintf(stderr, "%s: symbol string table out of range\n",
> +			fname);
> +		elf_close(elf);
> +		return -1;
> +	}
>  	elf->nr_syms = shdr_size(elf->symtab) / shdr_entsize(elf->symtab);
>  
>  	return 0;

[ ... ]

> diff --git a/scripts/kallsyms-sysmap.c b/scripts/kallsyms-sysmap.c
> index a482968eba511..efca13ab9dc3d 100644
> --- a/scripts/kallsyms-sysmap.c
> +++ b/scripts/kallsyms-sysmap.c
> @@ -198,10 +198,10 @@ static struct sysmap_symbol *elf_read_symbols(const struct elf_file *elf,
>  
>  	for (i = 1; i < elf->nr_syms; i++) {
>  		Elf_Sym *sym = elf_symbol(elf, i);
> -		const char *name = elf->strtab + sym_name(sym);
> +		const char *name = elf_symbol_name(elf, sym);
>  		char type = elf_symbol_type(elf, sym);
>  
> -		if (!type || sysmap_omits(name, type))
> +		if (!name || !type || sysmap_omits(name, type))
>  			continue;
>  
>  		syms[n].addr = sym_value(sym);

[Severity: Medium]
Does elf_read_symbols() -> elf_symbol_type() have the same out-of-bounds
read issue via elf->shstrtab and shdr_name(shdr)?

In scripts/elf-parse.c:elf_open_ro(), elf->shstrtab is set without verifying
that the section header string table fits within elf->size or ends in a NUL
byte:

	elf->shstrtab = (const char *)elf->base +
			shdr_offset(elf_section(elf, shstrndx));

Then in scripts/kallsyms-sysmap.c:elf_read_symbols(), elf_symbol_type() is
called on every symbol (even before checking !name). When a symbol references
a section index shndx < elf->shnum whose section has neither SHF_EXECINSTR
nor SHF_ALLOC set, elf_symbol_type() calls:

	} else if (shndx < elf->shnum) {
		shdr = elf_section(elf, shndx);
		c = section_symbol_type(shdr, elf_section_name(elf, shdr));
	}

where scripts/elf-parse.h:elf_section_name() adds the unchecked 32-bit
shdr_name(shdr) directly to elf->shstrtab:

	static inline const char *elf_section_name(const struct elf_file *elf,
						   Elf_Shdr *shdr)
	{
		return elf->shstrtab + shdr_name(shdr);
	}

and section_symbol_type() passes that pointer to string_starts_with() ->
strncmp(). Should elf->shstrtab and elf_section_name() also be bounds-checked
against the section header string table size?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019000218.3977398-1-lzsx618@gmail.com?part=5