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

Good day,

On Thu, 23 Jul 2026 at 20:53, HyeongJun An <[email protected]> wrote:
>
> The rproc_elf_sanity_check() only checks the image is big enough to hold
> one section header. But find_table() walks e_shnum of them, and first
> dereferences the header at e_shstrndx to locate the section name table.
> Both fields are u16 and both come from the image, so an e_shstrndx of
> 65535 reads about 4 MB past the buffer.
>
> The commit 9f9967fed9d0 ("soc: qcom: mdt_loader: Ensure we don't read
> past the ELF header") added the same check to the MDT loader, and says
> the header "is sanitized beforehand" under remoteproc. That is what this
> patch makes true.
>
> Check e_shoff against the image size first, so the two bounds can use
> size_add() without an e_shoff above SIZE_MAX wrapping on 32-bit. The
> bounds are separate because e_shnum may be zero while find_table() still
> reads the e_shstrndx header.
>
> Fixes: 400e64df6b23 ("remoteproc: add framework for controlling remote processors")
> Cc: [email protected]
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: HyeongJun An <[email protected]>
> ---
>  drivers/remoteproc/remoteproc_elf_loader.c | 23 ++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
> index 94177e416047..da3cddbe7d4c 100644
> --- a/drivers/remoteproc/remoteproc_elf_loader.c
> +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> @@ -46,8 +46,9 @@ int rproc_elf_sanity_check(struct rproc *rproc, const struct firmware *fw)
>         struct elf32_hdr *ehdr;
>         u32 elf_shdr_get_size;
>         u64 phoff, shoff;
> +       size_t shend;
>         char class;
> -       u16 phnum;
> +       u16 phnum, shnum, shstrndx;
>
>         if (!fw) {
>                 dev_err(dev, "failed to load %s\n", name);
> @@ -90,9 +91,27 @@ int rproc_elf_sanity_check(struct rproc *rproc, const struct firmware *fw)
>         phoff = elf_hdr_get_e_phoff(class, fw->data);
>         shoff = elf_hdr_get_e_shoff(class, fw->data);
>         phnum =  elf_hdr_get_e_phnum(class, fw->data);
> +       shnum = elf_hdr_get_e_shnum(class, fw->data);
> +       shstrndx = elf_hdr_get_e_shstrndx(class, fw->data);
>         elf_shdr_get_size = elf_size_of_shdr(class);
>
> -       if (fw->size < shoff + elf_shdr_get_size) {
> +       /* keeps shoff in size_t range for the two bounds below */
> +       if (shoff > fw->size) {
> +               dev_err(dev, "Section header table is out of bounds\n");
> +               return -EINVAL;
> +       }
> +
> +       if (shnum) {
> +               shend = size_add(size_mul(elf_shdr_get_size, shnum), shoff);
> +               if (shend > fw->size) {
> +                       dev_err(dev, "Section headers are out of bounds\n");
> +                       return -EINVAL;
> +               }
> +       }
> +
> +       /* 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.

> +       shend = size_add(size_mul(elf_shdr_get_size, (size_t)shstrndx + 1), shoff);

Why the shstrndx + 1?

Also, there is no point in doing this check if @shnum is 0.  Please
move this block in the "if (shnum)".

Thanks,
Mathieu

> +       if (shend > fw->size) {
>                 dev_err(dev, "Image is too small\n");
>                 return -EINVAL;
>         }
> --
> 2.43.0
>