Re: [PATCH] remoteproc: elf_loader: bound the section header table

[Mathieu Poirier <mathieu.poirier@linaro.org>]

On Wed, Aug 19, 2026 at 09:15:27PM +0900, HyeongJun An wrote:
> Thanks for taking a look!
> 
> On Wed, Aug 19, 2026 at 1:42 AM Mathieu Poirier
> <mathieu.poirier@linaro.org> wrote:
> > On Thu, 23 Jul 2026 at 20:53, HyeongJun An <sammiee5311@gmail.com> wrote:
> > > +       /* find_table() reads the header at shstrndx even with no sections */
> >
> > Right, but if there is no sections, @shnum in find_tables is 0 and not
> > arm is done.
> 
> The loop is skipped, but there is one load before it:
> 
>   :266  name_table_shdr = shdr + (shstrndx * elf_shdr_get_size);
>   :268  name_table = elf_data + elf_shdr_get_sh_offset(class, name_table_shdr);
>   :270  for (i = 0; i < shnum; i++, shdr += elf_shdr_get_size) {
> 
> :268 reads sh_offset out of the header at index shstrndx, before shnum is
> tested at :270.  Both callers, :336 and :380, reach find_table() without
> checking shnum.  The value is unused when shnum is 0, so it is a 4 or 8
> byte read past the buffer and nothing worse.
> 
> > > +       shend = size_add(size_mul(elf_shdr_get_size, (size_t)shstrndx + 1), shoff);
> >
> > Why the shstrndx + 1?
> 
> To hold the header at index shstrndx the table needs shstrndx + 1 entries.

You are correct.

I would assume that if e_shnum is 0, then e_shstrndx would also be 0.  If it
isn't the case then your patch is valid.  I will queue it when rc1 comes out.

Thanks,
Mathieu

> It is keyed on shstrndx and not shnum because nothing here requires
> e_shstrndx < e_shnum.
> 
> > Also, there is no point in doing this check if @shnum is 0.  Please
> > move this block in the "if (shnum)".
> 
> The check it replaces, :95, sits outside any shnum test today, so moving
> it in loses what master already has.
> 
> If you want it inside, the way there is to drop the read:
> 
> if (!shnum)
> return NULL;
> 
> at the top of find_table().  The loop already falls through to return NULL
> at :313, so it changes nothing today, and the bound can then go inside
> "if (shnum)".
> 
> I am happy to do either.  Let me know which you would prefer for v2.
> 
> Thanks a lot!