On Thu, Jun 05, 2025 at 02:29:53PM -0500, Bjorn Andersson wrote:
> 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)?
I think it's the best option.
--
With best wishes
Dmitry
