* [PATCH] remoteproc: qcom_q6v5_mss: Validate p_filesz in ELF loader
@ 2021-01-07 23:50 Bjorn Andersson
2021-01-13 21:22 ` Mathieu Poirier
0 siblings, 1 reply; 4+ messages in thread
From: Bjorn Andersson @ 2021-01-07 23:50 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Ohad Ben-Cohen, Mathieu Poirier,
Sibi Sankar
Cc: linux-arm-msm, linux-remoteproc, linux-kernel
Analog to the issue in the common mdt_loader code the MSS ELF loader
does not validate that p_filesz bytes will fit in the memory region and
that the loaded segments are not truncated. Fix this in the same way
as proposed for the mdt_loader.
Fixes: 135b9e8d1cd8 ("remoteproc: qcom_q6v5_mss: Validate modem blob firmware size before load")
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
drivers/remoteproc/qcom_q6v5_mss.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index 66106ba25ba3..2b59e0cbdce1 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -1210,6 +1210,14 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
goto release_firmware;
}
+ if (phdr->p_filesz > phdr->p_memsz) {
+ dev_err(qproc->dev,
+ "refusing to load segment %d with p_filesz > p_memsz\n",
+ i);
+ ret = -EINVAL;
+ break;
+ }
+
ptr = memremap(qproc->mpss_phys + offset, phdr->p_memsz, MEMREMAP_WC);
if (!ptr) {
dev_err(qproc->dev,
@@ -1241,6 +1249,15 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
goto release_firmware;
}
+ if (seg_fw->size != phdr->p_filesz) {
+ dev_err(qproc->dev,
+ "failed to load segment %d from truncated file %s\n",
+ i, fw_name);
+ ret = -EINVAL;
+ memunmap(ptr);
+ break;
+ }
+
release_firmware(seg_fw);
}
--
2.29.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] remoteproc: qcom_q6v5_mss: Validate p_filesz in ELF loader
2021-01-07 23:50 [PATCH] remoteproc: qcom_q6v5_mss: Validate p_filesz in ELF loader Bjorn Andersson
@ 2021-01-13 21:22 ` Mathieu Poirier
2021-01-13 22:03 ` Bjorn Andersson
0 siblings, 1 reply; 4+ messages in thread
From: Mathieu Poirier @ 2021-01-13 21:22 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Andy Gross, Ohad Ben-Cohen, Sibi Sankar, linux-arm-msm,
linux-remoteproc, linux-kernel
Hi Bjorn,
On Thu, Jan 07, 2021 at 03:50:53PM -0800, Bjorn Andersson wrote:
> Analog to the issue in the common mdt_loader code the MSS ELF loader
> does not validate that p_filesz bytes will fit in the memory region and
> that the loaded segments are not truncated. Fix this in the same way
> as proposed for the mdt_loader.
>
> Fixes: 135b9e8d1cd8 ("remoteproc: qcom_q6v5_mss: Validate modem blob firmware size before load")
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> drivers/remoteproc/qcom_q6v5_mss.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index 66106ba25ba3..2b59e0cbdce1 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -1210,6 +1210,14 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
> goto release_firmware;
> }
>
> + if (phdr->p_filesz > phdr->p_memsz) {
> + dev_err(qproc->dev,
> + "refusing to load segment %d with p_filesz > p_memsz\n",
> + i);
> + ret = -EINVAL;
> + break;
Based on the error handling for the above and below conditions, I would have
expected a "goto release_firmware" rather than a "break".
> + }
> +
> ptr = memremap(qproc->mpss_phys + offset, phdr->p_memsz, MEMREMAP_WC);
> if (!ptr) {
> dev_err(qproc->dev,
> @@ -1241,6 +1249,15 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
> goto release_firmware;
> }
>
> + if (seg_fw->size != phdr->p_filesz) {
> + dev_err(qproc->dev,
> + "failed to load segment %d from truncated file %s\n",
> + i, fw_name);
> + ret = -EINVAL;
> + memunmap(ptr);
> + break;
Same here.
> + }
> +
> release_firmware(seg_fw);
> }
Thanks,
Mathieu
>
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] remoteproc: qcom_q6v5_mss: Validate p_filesz in ELF loader
2021-01-13 21:22 ` Mathieu Poirier
@ 2021-01-13 22:03 ` Bjorn Andersson
2021-01-19 15:57 ` Mathieu Poirier
0 siblings, 1 reply; 4+ messages in thread
From: Bjorn Andersson @ 2021-01-13 22:03 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Andy Gross, Ohad Ben-Cohen, Sibi Sankar, linux-arm-msm,
linux-remoteproc, linux-kernel
On Wed 13 Jan 15:22 CST 2021, Mathieu Poirier wrote:
> Hi Bjorn,
>
> On Thu, Jan 07, 2021 at 03:50:53PM -0800, Bjorn Andersson wrote:
> > Analog to the issue in the common mdt_loader code the MSS ELF loader
> > does not validate that p_filesz bytes will fit in the memory region and
> > that the loaded segments are not truncated. Fix this in the same way
> > as proposed for the mdt_loader.
> >
> > Fixes: 135b9e8d1cd8 ("remoteproc: qcom_q6v5_mss: Validate modem blob firmware size before load")
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> > drivers/remoteproc/qcom_q6v5_mss.c | 17 +++++++++++++++++
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> > index 66106ba25ba3..2b59e0cbdce1 100644
> > --- a/drivers/remoteproc/qcom_q6v5_mss.c
> > +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> > @@ -1210,6 +1210,14 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
> > goto release_firmware;
> > }
> >
> > + if (phdr->p_filesz > phdr->p_memsz) {
> > + dev_err(qproc->dev,
> > + "refusing to load segment %d with p_filesz > p_memsz\n",
> > + i);
> > + ret = -EINVAL;
> > + break;
>
> Based on the error handling for the above and below conditions, I would have
> expected a "goto release_firmware" rather than a "break".
>
You're certainly right!
Yet another reason for the duplication between this function, the
mdt_loader and the remoteproc_elf_loader is a bad idea - still not sure
how to refactor any one of them to fit the three.
Thank you,
Bjorn
> > + }
> > +
> > ptr = memremap(qproc->mpss_phys + offset, phdr->p_memsz, MEMREMAP_WC);
> > if (!ptr) {
> > dev_err(qproc->dev,
> > @@ -1241,6 +1249,15 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
> > goto release_firmware;
> > }
> >
> > + if (seg_fw->size != phdr->p_filesz) {
> > + dev_err(qproc->dev,
> > + "failed to load segment %d from truncated file %s\n",
> > + i, fw_name);
> > + ret = -EINVAL;
> > + memunmap(ptr);
> > + break;
>
> Same here.
>
> > + }
> > +
> > release_firmware(seg_fw);
> > }
>
> Thanks,
> Mathieu
>
> >
> > --
> > 2.29.2
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] remoteproc: qcom_q6v5_mss: Validate p_filesz in ELF loader
2021-01-13 22:03 ` Bjorn Andersson
@ 2021-01-19 15:57 ` Mathieu Poirier
0 siblings, 0 replies; 4+ messages in thread
From: Mathieu Poirier @ 2021-01-19 15:57 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Andy Gross, Ohad Ben-Cohen, Sibi Sankar, linux-arm-msm,
linux-remoteproc, linux-kernel
On Wed, Jan 13, 2021 at 04:03:59PM -0600, Bjorn Andersson wrote:
> On Wed 13 Jan 15:22 CST 2021, Mathieu Poirier wrote:
>
> > Hi Bjorn,
> >
> > On Thu, Jan 07, 2021 at 03:50:53PM -0800, Bjorn Andersson wrote:
> > > Analog to the issue in the common mdt_loader code the MSS ELF loader
> > > does not validate that p_filesz bytes will fit in the memory region and
> > > that the loaded segments are not truncated. Fix this in the same way
> > > as proposed for the mdt_loader.
> > >
> > > Fixes: 135b9e8d1cd8 ("remoteproc: qcom_q6v5_mss: Validate modem blob firmware size before load")
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > ---
> > > drivers/remoteproc/qcom_q6v5_mss.c | 17 +++++++++++++++++
> > > 1 file changed, 17 insertions(+)
> > >
> > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> > > index 66106ba25ba3..2b59e0cbdce1 100644
> > > --- a/drivers/remoteproc/qcom_q6v5_mss.c
> > > +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> > > @@ -1210,6 +1210,14 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
> > > goto release_firmware;
> > > }
> > >
> > > + if (phdr->p_filesz > phdr->p_memsz) {
> > > + dev_err(qproc->dev,
> > > + "refusing to load segment %d with p_filesz > p_memsz\n",
> > > + i);
> > > + ret = -EINVAL;
> > > + break;
> >
> > Based on the error handling for the above and below conditions, I would have
> > expected a "goto release_firmware" rather than a "break".
> >
>
> You're certainly right!
>
> Yet another reason for the duplication between this function, the
> mdt_loader and the remoteproc_elf_loader is a bad idea - still not sure
> how to refactor any one of them to fit the three.
I feel the same way about duplication. On the flip side I don't think we have
to take immediate action... The solution will come out at some point when there
is more convergence. At least we know it is an area that needs attention.
>
> Thank you,
> Bjorn
>
> > > + }
> > > +
> > > ptr = memremap(qproc->mpss_phys + offset, phdr->p_memsz, MEMREMAP_WC);
> > > if (!ptr) {
> > > dev_err(qproc->dev,
> > > @@ -1241,6 +1249,15 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
> > > goto release_firmware;
> > > }
> > >
> > > + if (seg_fw->size != phdr->p_filesz) {
> > > + dev_err(qproc->dev,
> > > + "failed to load segment %d from truncated file %s\n",
> > > + i, fw_name);
> > > + ret = -EINVAL;
> > > + memunmap(ptr);
> > > + break;
> >
> > Same here.
> >
> > > + }
> > > +
> > > release_firmware(seg_fw);
> > > }
> >
> > Thanks,
> > Mathieu
> >
> > >
> > > --
> > > 2.29.2
> > >
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-01-19 16:09 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-07 23:50 [PATCH] remoteproc: qcom_q6v5_mss: Validate p_filesz in ELF loader Bjorn Andersson
2021-01-13 21:22 ` Mathieu Poirier
2021-01-13 22:03 ` Bjorn Andersson
2021-01-19 15:57 ` Mathieu Poirier
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.