Re: [PATCH v5 02/11] perf tools: Check ELF section type with name

[Namhyung Kim <namhyung@kernel.org>]

On Tue, Oct 13, 2026 at 05:31:14PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] perf symbol-elf: uninitialized memory read on gelf_getshdr failure
> --
> 
> commit f2750bf0f2a1c49778b5a9d0173f02f48eb9e193
> Author: Namhyung Kim <namhyung@kernel.org>
> 
> perf tools: Check ELF section type with name
> 
> This commit updates filename__has_section() to verify that a section
> has the SHT_PROGBITS type instead of just checking its name. This prevents
> empty sections with SHT_NOBITS from being incorrectly identified as present.
> 
> > diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> > --- a/tools/perf/util/symbol-elf.c
> > +++ b/tools/perf/util/symbol-elf.c
> > @@ -228,7 +228,8 @@ bool filename__has_section(const char *filename, const char *sec)
> >  	if (gelf_getehdr(elf, &ehdr) == NULL)
> >  		goto elf_out;
> >  
> > -	found = !!elf_section_by_name(elf, &ehdr, &shdr, sec, NULL);
> > +	found = !!elf_section_by_name(elf, &ehdr, &shdr, sec, NULL) &&
> > +		shdr.sh_type == SHT_PROGBITS;
> 
> [Severity: Medium]
> Can this lead to an uninitialized memory read of shdr.sh_type if the ELF
> file is malformed?
> 
> Looking at elf_section_by_name(), it does not check the return value of
> gelf_getshdr():
> 
> tools/perf/util/symbol-elf.c:elf_section_by_name() {
>     ...
>     gelf_getshdr(sec, shp);
>     str = elf_strptr(elf, ep->e_shstrndx, shp->sh_name);
>     ...
> }
> 
> If gelf_getshdr() fails, it returns NULL and leaves the shp struct unmodified.
> Because shp points to the stack-allocated shdr in filename__has_section(),
> a failure in gelf_getshdr() leaves shdr potentially uninitialized.
> 
> Since this patch adds a direct read of shdr.sh_type after elf_section_by_name()
> returns, does this new check risk reading uninitialized stack memory?

Well.. it's already checked shp->sh_name is valid so I think it's ok.

But probably safer to check the return value.  It's also a separate
change though.

Thanks,
Namhyung