| Message ID | 20250605-mdt-loader-validation-and-fixes-v1-0-29e22e7a82f4@oss.qualcomm.com |
|---|---|
| Headers | show |
| Series | (no cover subject) | expand |
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: <stable@vger.kernel.org> > Reported-by: Doug Anderson <dianders@chromium.org> > Signed-off-by: Bjorn Andersson <bjorn.andersson@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. > + if (phend > fw->size) > + return false; > + > + shend = size_add(size_mul(sizeof(struct elf32_shdr), ehdr->e_shnum), ehdr->e_shoff); > + if (shend > fw->size) Same for e_shentsize > + return false; > + > + return true; > +} > + > static bool mdt_phdr_valid(const struct elf32_phdr *phdr) > { > if (phdr->p_type != PT_LOAD) > @@ -82,6 +107,9 @@ ssize_t qcom_mdt_get_size(const struct firmware *fw) > phys_addr_t max_addr = 0; > int i; > > + if (!mdt_header_valid(fw)) > + return -EINVAL; > + > ehdr = (struct elf32_hdr *)fw->data; > phdrs = (struct elf32_phdr *)(ehdr + 1); > > @@ -134,6 +162,9 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len, > ssize_t ret; > void *data; > > + if (!mdt_header_valid(fw)) > + return ERR_PTR(-EINVAL); > + > ehdr = (struct elf32_hdr *)fw->data; > phdrs = (struct elf32_phdr *)(ehdr + 1); > > @@ -214,6 +245,9 @@ int qcom_mdt_pas_init(struct device *dev, const struct firmware *fw, > int ret; > int i; > > + if (!mdt_header_valid(fw)) > + return -EINVAL; > + > ehdr = (struct elf32_hdr *)fw->data; > phdrs = (struct elf32_phdr *)(ehdr + 1); > > @@ -310,6 +344,9 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw, > if (!fw || !mem_region || !mem_phys || !mem_size) > return -EINVAL; > > + if (!mdt_header_valid(fw)) > + return -EINVAL; > + > is_split = qcom_mdt_bins_are_split(fw, fw_name); > ehdr = (struct elf32_hdr *)fw->data; > phdrs = (struct elf32_phdr *)(ehdr + 1); > > -- > 2.49.0 >
On Thu, Jun 05, 2025 at 08:43:02AM -0500, Bjorn Andersson wrote: > Rather than relying/assuming that the tools generating the firmware > places the program headers immediately following the ELF header, use > e_phoff as intended to find the program headers. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com> > --- > drivers/soc/qcom/mdt_loader.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
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: <stable@vger.kernel.org> > > Reported-by: Doug Anderson <dianders@chromium.org> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@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
Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com> --- Bjorn Andersson (3): soc: qcom: mdt_loader: Ensure we don't read past the ELF header soc: qcom: mdt_loader: Rename mdt_phdr_valid() soc: qcom: mdt_loader: Actually use the e_phoff drivers/soc/qcom/mdt_loader.c | 57 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 47 insertions(+), 10 deletions(-) --- base-commit: a0bea9e39035edc56a994630e6048c8a191a99d8 change-id: 20250604-mdt-loader-validation-and-fixes-5c3267f5b19f Best regards,