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

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

[HyeongJun An]

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: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: HyeongJun An <sammiee5311@gmail.com>
---
 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 */
+	shend = size_add(size_mul(elf_shdr_get_size, (size_t)shstrndx + 1), shoff);
+	if (shend > fw->size) {
 		dev_err(dev, "Image is too small\n");
 		return -EINVAL;
 	}
-- 
2.43.0

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

[Mathieu Poirier]

Good day,

On Thu, 23 Jul 2026 at 20:53, HyeongJun An <sammiee5311@gmail.com> 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: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: HyeongJun An <sammiee5311@gmail.com>
> ---
>  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
>

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

[HyeongJun An]

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.
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!

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

[Mathieu Poirier]

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!