On Thu, Jun 05, 2025 at 06:57:41PM +0300, Dmitry Baryshkov wrote:
> On Thu, Jun 05, 2025 at 08:43:00AM -0500, Bjorn Andersson wrote:
> > When the MDT loader is used in remoteproc, the ELF header is sanitized
> > beforehand, but that's not necessary the case for other clients.
> >
> > Validate the size of the firmware buffer to ensure that we don't read
> > past the end as we iterate over the header.
> >
> > Fixes: 2aad40d911ee ("remoteproc: Move qcom_mdt_loader into
> > drivers/soc/qcom")
> > Cc: <sta...@vger.kernel.org>
> > Reported-by: Doug Anderson <diand...@chromium.org>
> > Signed-off-by: Bjorn Andersson <bjorn.anders...@oss.qualcomm.com>
> > ---
> > drivers/soc/qcom/mdt_loader.c | 37 +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 37 insertions(+)
> >
> > diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> > index
> > b2c0fb55d4ae678ee333f0d6b8b586de319f53b1..1da22b23d19d28678ec78cccdf8c328b50d3ffda
> > 100644
> > --- a/drivers/soc/qcom/mdt_loader.c
> > +++ b/drivers/soc/qcom/mdt_loader.c
> > @@ -18,6 +18,31 @@
> > #include <linux/slab.h>
> > #include <linux/soc/qcom/mdt_loader.h>
> >
> > +static bool mdt_header_valid(const struct firmware *fw)
> > +{
> > + const struct elf32_hdr *ehdr;
> > + size_t phend;
> > + size_t shend;
> > +
> > + if (fw->size < sizeof(*ehdr))
> > + return false;
> > +
> > + ehdr = (struct elf32_hdr *)fw->data;
> > +
> > + if (memcmp(ehdr->e_ident, ELFMAG, SELFMAG))
> > + return false;
> > +
> > + phend = size_add(size_mul(sizeof(struct elf32_phdr), ehdr->e_phnum),
> > ehdr->e_phoff);
>
> Nit, this should be a max(sizeof() and ehdr->e_phentsize.
>
Hmm, I forgot about e_phentsize.
But the fact is that the check matches what we do below and validates
that we won't reach outside the provided buffer.
If e_phentsize != sizeof(struct elf32_phdr) we're not going to be able
to parse the header.
Not sure if it's worth it, but that would make sense to change
separately. In which case ehdr->e_phentsize * ehdr->e_phnum would be the
correct thing to check (no max()). Or perhaps just a check to ensure
that e_phentsize == sizeof(struct elf32_phdr)?
Regards,
Bjorn
