Re: [PATCH 2/5] elf-parse: check that the section header table is inside the file

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] elf-parse: missing SHN_UNDEF and SHN_XINDEX bounds checks in elf_parse()
--

commit 8e951169d34059326033b08713c8c35ae69b423f
Author: Leizhen Zhang <lzsx618@gmail.com>

elf-parse: check that the section header table is inside the file

This commit passes the mapped file size to elf_parse() to validate that the
ELF header fits in the mapping, the section header table lies within the
mapping bounds, and the section name string table index is in range.

> diff --git a/scripts/elf-parse.c b/scripts/elf-parse.c
> index 9e7d1a4f263fe..0e16a9cf67f45 100644
> --- a/scripts/elf-parse.c
> +++ b/scripts/elf-parse.c

[ ... ]

> @@ -168,6 +176,34 @@ static int elf_parse(const char *fname, void *addr, uint32_t types)
>  			ehdr->e32.e_ident[EI_CLASS], fname);
>  		return -1;
>  	}
> +
> +	/*
> +	 * Everything past here walks the section headers with offsets and
> +	 * indices taken from the file, so the table the ELF header describes
> +	 * has to be inside the mapping first. e_shnum == 0 means the real
> +	 * count lives in section 0, which must then be present to be read.
> +	 */
> +	shoff = ehdr_shoff(ehdr);
> +	shentsize = ehdr_shentsize(ehdr);
> +	shnum = ehdr_shnum(ehdr);
> +	if (shnum == SHN_UNDEF)
> +		shnum = 1;
> +	if (shoff > size || shentsize == 0 ||
> +	    (size - shoff) / shentsize < shnum) {
> +		fprintf(stderr, "ELF section headers out of range: %s\n",
> +			fname);
> +		return -1;
> +	}
> +
> +	/* SHN_XINDEX defers the index to section 0; elf_open_ro() checks it. */
> +	shstrndx = ehdr_shstrndx(ehdr);
> +	if (shstrndx != SHN_XINDEX && ehdr_shnum(ehdr) != SHN_UNDEF &&
> +	    shstrndx >= ehdr_shnum(ehdr)) {
> +		fprintf(stderr, "ELF section name index out of range: %s\n",
> +			fname);
> +		return -1;
> +	}
> +
>  	return 0;
>  }

[Severity: Medium]
Can sorttable and tracepoint-update still read past the end of the mapping
when e_shnum is SHN_UNDEF or e_shstrndx is SHN_XINDEX?

The comment and commit message note that resolving the real section count for
SHN_UNDEF and the section name index for SHN_XINDEX stays with elf_open_ro()
(which is updated in the follow-up commit "elf-parse: check the section
indices that elf_open_ro() takes from the file").

However, sorttable and tracepoint-update do not call elf_open_ro(). Instead,
they call elf_map() and resolve section 0's sh_link and sh_size directly
without bounds checks:

scripts/sorttable.c:do_sort() and
scripts/tracepoint-update.c:process_tracepoints():
	shstrndx = ehdr_shstrndx(ehdr);
	if (shstrndx == SHN_XINDEX)
		shstrndx = shdr_link(shdr_start);
	string_sec = get_index(shdr_start, shentsize, shstrndx);
	secstrings = (const char *)ehdr + shdr_offset(string_sec);

	shnum = ehdr_shnum(ehdr);
	if (shnum == SHN_UNDEF)
		shnum = shdr_size(shdr_start);

If e_shnum == SHN_UNDEF and e_shstrndx is an out-of-range index (not equal to
SHN_XINDEX), elf_parse() skips the shstrndx check because ehdr_shnum(ehdr) ==
SHN_UNDEF, and both tools immediately index shdr_start out of bounds via
get_index(shdr_start, shentsize, shstrndx).

Similarly, if e_shnum == SHN_UNDEF and section 0's sh_size exceeds the number
of section headers that fit in the mapping, or if e_shstrndx == SHN_XINDEX
and section 0's sh_link >= shnum, elf_parse() succeeds and both tools read
past the end of the mapping.

Since elf_parse() already verifies that section 0 lies within the mapping when
shnum > 0, should elf_parse() resolve shdr_size() and shdr_link() from
section 0 and validate the real shnum and shstrndx right here so that all
callers of elf_map() are protected as well?

[ ... ]

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