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
>
