[PATCH 0/2] remoteproc: elf: ignore PT_LOAD type segment with memsz as 0

[PATCH 0/2] remoteproc: elf: ignore PT_LOAD type segment with memsz as 0

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

i.MX DSP firmware has segments with PT_LOAD and memsz/filesz as zero.
It is valid case the memsz set to zero according to elf spec:
https://refspecs.linuxbase.org/elf/elf.pdf page 40

So we could let remoteproc elf loader handle this case, then no
duplicate code in imx dsp rproc driver

Tested i.MX8MP DSP and M7 remoteproc

Peng Fan (2):
  remoteproc: elf_loader: skip segment with memsz as zero
  remoteproc: imx_dsp_rproc: use common rproc_elf_load_segments

 drivers/remoteproc/imx_dsp_rproc.c         | 95 +---------------------
 drivers/remoteproc/remoteproc_elf_loader.c |  9 +-
 2 files changed, 9 insertions(+), 95 deletions(-)

-- 
2.25.1

[PATCH 0/2] remoteproc: elf: ignore PT_LOAD type segment with memsz as 0

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

i.MX DSP firmware has segments with PT_LOAD and memsz/filesz as zero.
It is valid case the memsz set to zero according to elf spec:
https://refspecs.linuxbase.org/elf/elf.pdf page 40

So we could let remoteproc elf loader handle this case, then no
duplicate code in imx dsp rproc driver

Tested i.MX8MP DSP and M7 remoteproc

Peng Fan (2):
  remoteproc: elf_loader: skip segment with memsz as zero
  remoteproc: imx_dsp_rproc: use common rproc_elf_load_segments

 drivers/remoteproc/imx_dsp_rproc.c         | 95 +---------------------
 drivers/remoteproc/remoteproc_elf_loader.c |  9 +-
 2 files changed, 9 insertions(+), 95 deletions(-)

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] remoteproc: elf_loader: skip segment with memsz as zero

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Per elf specification,
p_filesz: This member gives the number of bytes in the file image of
the segment; it may be zero.
p_memsz: This member gives the number of bytes in the memory image
of the segment; it may be zero.

There is a case that i.MX DSP firmware has segment with PT_LOAD and
p_memsz/p_filesz set to zero. Such segment needs to be ignored,
otherwize rproc_da_to_va would report error.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_elf_loader.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
index d635d19a5aa8..cb77f9e4dc70 100644
--- a/drivers/remoteproc/remoteproc_elf_loader.c
+++ b/drivers/remoteproc/remoteproc_elf_loader.c
@@ -181,7 +181,14 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
 		bool is_iomem = false;
 		void *ptr;
 
-		if (type != PT_LOAD)
+		/*
+		 *  There is a case that with PT_LOAD type, the
+		 *  filesz = memsz = 0. If memsz = 0, rproc_da_to_va
+		 *  should return NULL ptr, then error is returned.
+		 *  So this case should be skipped from the loop.
+		 *  Add !memsz checking here.
+		 */
+		if (type != PT_LOAD || !memsz)
 			continue;
 
 		dev_dbg(dev, "phdr: type %d da 0x%llx memsz 0x%llx filesz 0x%llx\n",
-- 
2.25.1

[PATCH 1/2] remoteproc: elf_loader: skip segment with memsz as zero

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Per elf specification,
p_filesz: This member gives the number of bytes in the file image of
the segment; it may be zero.
p_memsz: This member gives the number of bytes in the memory image
of the segment; it may be zero.

There is a case that i.MX DSP firmware has segment with PT_LOAD and
p_memsz/p_filesz set to zero. Such segment needs to be ignored,
otherwize rproc_da_to_va would report error.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_elf_loader.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
index d635d19a5aa8..cb77f9e4dc70 100644
--- a/drivers/remoteproc/remoteproc_elf_loader.c
+++ b/drivers/remoteproc/remoteproc_elf_loader.c
@@ -181,7 +181,14 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
 		bool is_iomem = false;
 		void *ptr;
 
-		if (type != PT_LOAD)
+		/*
+		 *  There is a case that with PT_LOAD type, the
+		 *  filesz = memsz = 0. If memsz = 0, rproc_da_to_va
+		 *  should return NULL ptr, then error is returned.
+		 *  So this case should be skipped from the loop.
+		 *  Add !memsz checking here.
+		 */
+		if (type != PT_LOAD || !memsz)
 			continue;
 
 		dev_dbg(dev, "phdr: type %d da 0x%llx memsz 0x%llx filesz 0x%llx\n",
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] remoteproc: imx_dsp_rproc: use common rproc_elf_load_segments

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

remoteproc elf loader supports the specific case that segments
have PT_LOAD and memsz/filesz set to zero, so no duplicate
code.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_dsp_rproc.c | 95 +-----------------------------
 1 file changed, 1 insertion(+), 94 deletions(-)

diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
index 2abee78df96e..eee3c44c2146 100644
--- a/drivers/remoteproc/imx_dsp_rproc.c
+++ b/drivers/remoteproc/imx_dsp_rproc.c
@@ -649,99 +649,6 @@ static int imx_dsp_rproc_add_carveout(struct imx_dsp_rproc *priv)
 	return 0;
 }
 
-/**
- * imx_dsp_rproc_elf_load_segments() - load firmware segments to memory
- * @rproc: remote processor which will be booted using these fw segments
- * @fw: the ELF firmware image
- *
- * This function specially checks if memsz is zero or not, otherwise it
- * is mostly same as rproc_elf_load_segments().
- */
-static int imx_dsp_rproc_elf_load_segments(struct rproc *rproc,
-					   const struct firmware *fw)
-{
-	struct device *dev = &rproc->dev;
-	u8 class = fw_elf_get_class(fw);
-	u32 elf_phdr_get_size = elf_size_of_phdr(class);
-	const u8 *elf_data = fw->data;
-	const void *ehdr, *phdr;
-	int i, ret = 0;
-	u16 phnum;
-
-	ehdr = elf_data;
-	phnum = elf_hdr_get_e_phnum(class, ehdr);
-	phdr = elf_data + elf_hdr_get_e_phoff(class, ehdr);
-
-	/* go through the available ELF segments */
-	for (i = 0; i < phnum; i++, phdr += elf_phdr_get_size) {
-		u64 da = elf_phdr_get_p_paddr(class, phdr);
-		u64 memsz = elf_phdr_get_p_memsz(class, phdr);
-		u64 filesz = elf_phdr_get_p_filesz(class, phdr);
-		u64 offset = elf_phdr_get_p_offset(class, phdr);
-		u32 type = elf_phdr_get_p_type(class, phdr);
-		void *ptr;
-
-		/*
-		 *  There is a case that with PT_LOAD type, the
-		 *  filesz = memsz = 0. If memsz = 0, rproc_da_to_va
-		 *  should return NULL ptr, then error is returned.
-		 *  So this case should be skipped from the loop.
-		 *  Add !memsz checking here.
-		 */
-		if (type != PT_LOAD || !memsz)
-			continue;
-
-		dev_dbg(dev, "phdr: type %d da 0x%llx memsz 0x%llx filesz 0x%llx\n",
-			type, da, memsz, filesz);
-
-		if (filesz > memsz) {
-			dev_err(dev, "bad phdr filesz 0x%llx memsz 0x%llx\n",
-				filesz, memsz);
-			ret = -EINVAL;
-			break;
-		}
-
-		if (offset + filesz > fw->size) {
-			dev_err(dev, "truncated fw: need 0x%llx avail 0x%zx\n",
-				offset + filesz, fw->size);
-			ret = -EINVAL;
-			break;
-		}
-
-		if (!rproc_u64_fit_in_size_t(memsz)) {
-			dev_err(dev, "size (%llx) does not fit in size_t type\n",
-				memsz);
-			ret = -EOVERFLOW;
-			break;
-		}
-
-		/* grab the kernel address for this device address */
-		ptr = rproc_da_to_va(rproc, da, memsz, NULL);
-		if (!ptr) {
-			dev_err(dev, "bad phdr da 0x%llx mem 0x%llx\n", da,
-				memsz);
-			ret = -EINVAL;
-			break;
-		}
-
-		/* put the segment where the remote processor expects it */
-		if (filesz)
-			memcpy(ptr, elf_data + offset, filesz);
-
-		/*
-		 * Zero out remaining memory for this segment.
-		 *
-		 * This isn't strictly required since dma_alloc_coherent already
-		 * did this for us. albeit harmless, we may consider removing
-		 * this.
-		 */
-		if (memsz > filesz)
-			memset(ptr + filesz, 0, memsz - filesz);
-	}
-
-	return ret;
-}
-
 /* Prepare function for rproc_ops */
 static int imx_dsp_rproc_prepare(struct rproc *rproc)
 {
@@ -808,7 +715,7 @@ static const struct rproc_ops imx_dsp_rproc_ops = {
 	.start		= imx_dsp_rproc_start,
 	.stop		= imx_dsp_rproc_stop,
 	.kick		= imx_dsp_rproc_kick,
-	.load		= imx_dsp_rproc_elf_load_segments,
+	.load		= rproc_elf_load_segments,
 	.parse_fw	= rproc_elf_load_rsc_table,
 	.sanity_check	= rproc_elf_sanity_check,
 	.get_boot_addr	= rproc_elf_get_boot_addr,
-- 
2.25.1

[PATCH 2/2] remoteproc: imx_dsp_rproc: use common rproc_elf_load_segments

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

remoteproc elf loader supports the specific case that segments
have PT_LOAD and memsz/filesz set to zero, so no duplicate
code.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_dsp_rproc.c | 95 +-----------------------------
 1 file changed, 1 insertion(+), 94 deletions(-)

diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
index 2abee78df96e..eee3c44c2146 100644
--- a/drivers/remoteproc/imx_dsp_rproc.c
+++ b/drivers/remoteproc/imx_dsp_rproc.c
@@ -649,99 +649,6 @@ static int imx_dsp_rproc_add_carveout(struct imx_dsp_rproc *priv)
 	return 0;
 }
 
-/**
- * imx_dsp_rproc_elf_load_segments() - load firmware segments to memory
- * @rproc: remote processor which will be booted using these fw segments
- * @fw: the ELF firmware image
- *
- * This function specially checks if memsz is zero or not, otherwise it
- * is mostly same as rproc_elf_load_segments().
- */
-static int imx_dsp_rproc_elf_load_segments(struct rproc *rproc,
-					   const struct firmware *fw)
-{
-	struct device *dev = &rproc->dev;
-	u8 class = fw_elf_get_class(fw);
-	u32 elf_phdr_get_size = elf_size_of_phdr(class);
-	const u8 *elf_data = fw->data;
-	const void *ehdr, *phdr;
-	int i, ret = 0;
-	u16 phnum;
-
-	ehdr = elf_data;
-	phnum = elf_hdr_get_e_phnum(class, ehdr);
-	phdr = elf_data + elf_hdr_get_e_phoff(class, ehdr);
-
-	/* go through the available ELF segments */
-	for (i = 0; i < phnum; i++, phdr += elf_phdr_get_size) {
-		u64 da = elf_phdr_get_p_paddr(class, phdr);
-		u64 memsz = elf_phdr_get_p_memsz(class, phdr);
-		u64 filesz = elf_phdr_get_p_filesz(class, phdr);
-		u64 offset = elf_phdr_get_p_offset(class, phdr);
-		u32 type = elf_phdr_get_p_type(class, phdr);
-		void *ptr;
-
-		/*
-		 *  There is a case that with PT_LOAD type, the
-		 *  filesz = memsz = 0. If memsz = 0, rproc_da_to_va
-		 *  should return NULL ptr, then error is returned.
-		 *  So this case should be skipped from the loop.
-		 *  Add !memsz checking here.
-		 */
-		if (type != PT_LOAD || !memsz)
-			continue;
-
-		dev_dbg(dev, "phdr: type %d da 0x%llx memsz 0x%llx filesz 0x%llx\n",
-			type, da, memsz, filesz);
-
-		if (filesz > memsz) {
-			dev_err(dev, "bad phdr filesz 0x%llx memsz 0x%llx\n",
-				filesz, memsz);
-			ret = -EINVAL;
-			break;
-		}
-
-		if (offset + filesz > fw->size) {
-			dev_err(dev, "truncated fw: need 0x%llx avail 0x%zx\n",
-				offset + filesz, fw->size);
-			ret = -EINVAL;
-			break;
-		}
-
-		if (!rproc_u64_fit_in_size_t(memsz)) {
-			dev_err(dev, "size (%llx) does not fit in size_t type\n",
-				memsz);
-			ret = -EOVERFLOW;
-			break;
-		}
-
-		/* grab the kernel address for this device address */
-		ptr = rproc_da_to_va(rproc, da, memsz, NULL);
-		if (!ptr) {
-			dev_err(dev, "bad phdr da 0x%llx mem 0x%llx\n", da,
-				memsz);
-			ret = -EINVAL;
-			break;
-		}
-
-		/* put the segment where the remote processor expects it */
-		if (filesz)
-			memcpy(ptr, elf_data + offset, filesz);
-
-		/*
-		 * Zero out remaining memory for this segment.
-		 *
-		 * This isn't strictly required since dma_alloc_coherent already
-		 * did this for us. albeit harmless, we may consider removing
-		 * this.
-		 */
-		if (memsz > filesz)
-			memset(ptr + filesz, 0, memsz - filesz);
-	}
-
-	return ret;
-}
-
 /* Prepare function for rproc_ops */
 static int imx_dsp_rproc_prepare(struct rproc *rproc)
 {
@@ -808,7 +715,7 @@ static const struct rproc_ops imx_dsp_rproc_ops = {
 	.start		= imx_dsp_rproc_start,
 	.stop		= imx_dsp_rproc_stop,
 	.kick		= imx_dsp_rproc_kick,
-	.load		= imx_dsp_rproc_elf_load_segments,
+	.load		= rproc_elf_load_segments,
 	.parse_fw	= rproc_elf_load_rsc_table,
 	.sanity_check	= rproc_elf_sanity_check,
 	.get_boot_addr	= rproc_elf_get_boot_addr,
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] remoteproc: imx_dsp_rproc: use common rproc_elf_load_segments

[Daniel Baluta]

On Thu, Mar 24, 2022 at 1:34 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>
> From: Peng Fan <peng.fan@nxp.com>
>
> remoteproc elf loader supports the specific case that segments
> have PT_LOAD and memsz/filesz set to zero, so no duplicate
> code.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>

I think this change OK, but we have a case with the DSP were
reads/writes should be done in multiples of 32/64.

We need a way to provide our own "memcpy" function to be used by
rproc_elf_load_segments.

> ---
>  drivers/remoteproc/imx_dsp_rproc.c | 95 +-----------------------------
>  1 file changed, 1 insertion(+), 94 deletions(-)
>
> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> index 2abee78df96e..eee3c44c2146 100644
> --- a/drivers/remoteproc/imx_dsp_rproc.c
> +++ b/drivers/remoteproc/imx_dsp_rproc.c
> @@ -649,99 +649,6 @@ static int imx_dsp_rproc_add_carveout(struct imx_dsp_rproc *priv)
>         return 0;
>  }
>
> -/**
> - * imx_dsp_rproc_elf_load_segments() - load firmware segments to memory
> - * @rproc: remote processor which will be booted using these fw segments
> - * @fw: the ELF firmware image
> - *
> - * This function specially checks if memsz is zero or not, otherwise it
> - * is mostly same as rproc_elf_load_segments().
> - */
> -static int imx_dsp_rproc_elf_load_segments(struct rproc *rproc,
> -                                          const struct firmware *fw)
> -{
> -       struct device *dev = &rproc->dev;
> -       u8 class = fw_elf_get_class(fw);
> -       u32 elf_phdr_get_size = elf_size_of_phdr(class);
> -       const u8 *elf_data = fw->data;
> -       const void *ehdr, *phdr;
> -       int i, ret = 0;
> -       u16 phnum;
> -
> -       ehdr = elf_data;
> -       phnum = elf_hdr_get_e_phnum(class, ehdr);
> -       phdr = elf_data + elf_hdr_get_e_phoff(class, ehdr);
> -
> -       /* go through the available ELF segments */
> -       for (i = 0; i < phnum; i++, phdr += elf_phdr_get_size) {
> -               u64 da = elf_phdr_get_p_paddr(class, phdr);
> -               u64 memsz = elf_phdr_get_p_memsz(class, phdr);
> -               u64 filesz = elf_phdr_get_p_filesz(class, phdr);
> -               u64 offset = elf_phdr_get_p_offset(class, phdr);
> -               u32 type = elf_phdr_get_p_type(class, phdr);
> -               void *ptr;
> -
> -               /*
> -                *  There is a case that with PT_LOAD type, the
> -                *  filesz = memsz = 0. If memsz = 0, rproc_da_to_va
> -                *  should return NULL ptr, then error is returned.
> -                *  So this case should be skipped from the loop.
> -                *  Add !memsz checking here.
> -                */
> -               if (type != PT_LOAD || !memsz)
> -                       continue;
> -
> -               dev_dbg(dev, "phdr: type %d da 0x%llx memsz 0x%llx filesz 0x%llx\n",
> -                       type, da, memsz, filesz);
> -
> -               if (filesz > memsz) {
> -                       dev_err(dev, "bad phdr filesz 0x%llx memsz 0x%llx\n",
> -                               filesz, memsz);
> -                       ret = -EINVAL;
> -                       break;
> -               }
> -
> -               if (offset + filesz > fw->size) {
> -                       dev_err(dev, "truncated fw: need 0x%llx avail 0x%zx\n",
> -                               offset + filesz, fw->size);
> -                       ret = -EINVAL;
> -                       break;
> -               }
> -
> -               if (!rproc_u64_fit_in_size_t(memsz)) {
> -                       dev_err(dev, "size (%llx) does not fit in size_t type\n",
> -                               memsz);
> -                       ret = -EOVERFLOW;
> -                       break;
> -               }
> -
> -               /* grab the kernel address for this device address */
> -               ptr = rproc_da_to_va(rproc, da, memsz, NULL);
> -               if (!ptr) {
> -                       dev_err(dev, "bad phdr da 0x%llx mem 0x%llx\n", da,
> -                               memsz);
> -                       ret = -EINVAL;
> -                       break;
> -               }
> -
> -               /* put the segment where the remote processor expects it */
> -               if (filesz)
> -                       memcpy(ptr, elf_data + offset, filesz);
> -
> -               /*
> -                * Zero out remaining memory for this segment.
> -                *
> -                * This isn't strictly required since dma_alloc_coherent already
> -                * did this for us. albeit harmless, we may consider removing
> -                * this.
> -                */
> -               if (memsz > filesz)
> -                       memset(ptr + filesz, 0, memsz - filesz);
> -       }
> -
> -       return ret;
> -}
> -
>  /* Prepare function for rproc_ops */
>  static int imx_dsp_rproc_prepare(struct rproc *rproc)
>  {
> @@ -808,7 +715,7 @@ static const struct rproc_ops imx_dsp_rproc_ops = {
>         .start          = imx_dsp_rproc_start,
>         .stop           = imx_dsp_rproc_stop,
>         .kick           = imx_dsp_rproc_kick,
> -       .load           = imx_dsp_rproc_elf_load_segments,
> +       .load           = rproc_elf_load_segments,
>         .parse_fw       = rproc_elf_load_rsc_table,
>         .sanity_check   = rproc_elf_sanity_check,
>         .get_boot_addr  = rproc_elf_get_boot_addr,
> --
> 2.25.1
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] remoteproc: imx_dsp_rproc: use common rproc_elf_load_segments

[Daniel Baluta]

On Thu, Mar 24, 2022 at 1:34 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>
> From: Peng Fan <peng.fan@nxp.com>
>
> remoteproc elf loader supports the specific case that segments
> have PT_LOAD and memsz/filesz set to zero, so no duplicate
> code.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>

I think this change OK, but we have a case with the DSP were
reads/writes should be done in multiples of 32/64.

We need a way to provide our own "memcpy" function to be used by
rproc_elf_load_segments.

> ---
>  drivers/remoteproc/imx_dsp_rproc.c | 95 +-----------------------------
>  1 file changed, 1 insertion(+), 94 deletions(-)
>
> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> index 2abee78df96e..eee3c44c2146 100644
> --- a/drivers/remoteproc/imx_dsp_rproc.c
> +++ b/drivers/remoteproc/imx_dsp_rproc.c
> @@ -649,99 +649,6 @@ static int imx_dsp_rproc_add_carveout(struct imx_dsp_rproc *priv)
>         return 0;
>  }
>
> -/**
> - * imx_dsp_rproc_elf_load_segments() - load firmware segments to memory
> - * @rproc: remote processor which will be booted using these fw segments
> - * @fw: the ELF firmware image
> - *
> - * This function specially checks if memsz is zero or not, otherwise it
> - * is mostly same as rproc_elf_load_segments().
> - */
> -static int imx_dsp_rproc_elf_load_segments(struct rproc *rproc,
> -                                          const struct firmware *fw)
> -{
> -       struct device *dev = &rproc->dev;
> -       u8 class = fw_elf_get_class(fw);
> -       u32 elf_phdr_get_size = elf_size_of_phdr(class);
> -       const u8 *elf_data = fw->data;
> -       const void *ehdr, *phdr;
> -       int i, ret = 0;
> -       u16 phnum;
> -
> -       ehdr = elf_data;
> -       phnum = elf_hdr_get_e_phnum(class, ehdr);
> -       phdr = elf_data + elf_hdr_get_e_phoff(class, ehdr);
> -
> -       /* go through the available ELF segments */
> -       for (i = 0; i < phnum; i++, phdr += elf_phdr_get_size) {
> -               u64 da = elf_phdr_get_p_paddr(class, phdr);
> -               u64 memsz = elf_phdr_get_p_memsz(class, phdr);
> -               u64 filesz = elf_phdr_get_p_filesz(class, phdr);
> -               u64 offset = elf_phdr_get_p_offset(class, phdr);
> -               u32 type = elf_phdr_get_p_type(class, phdr);
> -               void *ptr;
> -
> -               /*
> -                *  There is a case that with PT_LOAD type, the
> -                *  filesz = memsz = 0. If memsz = 0, rproc_da_to_va
> -                *  should return NULL ptr, then error is returned.
> -                *  So this case should be skipped from the loop.
> -                *  Add !memsz checking here.
> -                */
> -               if (type != PT_LOAD || !memsz)
> -                       continue;
> -
> -               dev_dbg(dev, "phdr: type %d da 0x%llx memsz 0x%llx filesz 0x%llx\n",
> -                       type, da, memsz, filesz);
> -
> -               if (filesz > memsz) {
> -                       dev_err(dev, "bad phdr filesz 0x%llx memsz 0x%llx\n",
> -                               filesz, memsz);
> -                       ret = -EINVAL;
> -                       break;
> -               }
> -
> -               if (offset + filesz > fw->size) {
> -                       dev_err(dev, "truncated fw: need 0x%llx avail 0x%zx\n",
> -                               offset + filesz, fw->size);
> -                       ret = -EINVAL;
> -                       break;
> -               }
> -
> -               if (!rproc_u64_fit_in_size_t(memsz)) {
> -                       dev_err(dev, "size (%llx) does not fit in size_t type\n",
> -                               memsz);
> -                       ret = -EOVERFLOW;
> -                       break;
> -               }
> -
> -               /* grab the kernel address for this device address */
> -               ptr = rproc_da_to_va(rproc, da, memsz, NULL);
> -               if (!ptr) {
> -                       dev_err(dev, "bad phdr da 0x%llx mem 0x%llx\n", da,
> -                               memsz);
> -                       ret = -EINVAL;
> -                       break;
> -               }
> -
> -               /* put the segment where the remote processor expects it */
> -               if (filesz)
> -                       memcpy(ptr, elf_data + offset, filesz);
> -
> -               /*
> -                * Zero out remaining memory for this segment.
> -                *
> -                * This isn't strictly required since dma_alloc_coherent already
> -                * did this for us. albeit harmless, we may consider removing
> -                * this.
> -                */
> -               if (memsz > filesz)
> -                       memset(ptr + filesz, 0, memsz - filesz);
> -       }
> -
> -       return ret;
> -}
> -
>  /* Prepare function for rproc_ops */
>  static int imx_dsp_rproc_prepare(struct rproc *rproc)
>  {
> @@ -808,7 +715,7 @@ static const struct rproc_ops imx_dsp_rproc_ops = {
>         .start          = imx_dsp_rproc_start,
>         .stop           = imx_dsp_rproc_stop,
>         .kick           = imx_dsp_rproc_kick,
> -       .load           = imx_dsp_rproc_elf_load_segments,
> +       .load           = rproc_elf_load_segments,
>         .parse_fw       = rproc_elf_load_rsc_table,
>         .sanity_check   = rproc_elf_sanity_check,
>         .get_boot_addr  = rproc_elf_get_boot_addr,
> --
> 2.25.1
>

RE: [PATCH 2/2] remoteproc: imx_dsp_rproc: use common rproc_elf_load_segments

[Peng Fan]

> Subject: Re: [PATCH 2/2] remoteproc: imx_dsp_rproc: use common
> rproc_elf_load_segments
> 
> On Thu, Mar 24, 2022 at 1:34 AM Peng Fan (OSS) <peng.fan@oss.nxp.com>
> wrote:
> >
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > remoteproc elf loader supports the specific case that segments have
> > PT_LOAD and memsz/filesz set to zero, so no duplicate code.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> 
> I think this change OK, but we have a case with the DSP were reads/writes
> should be done in multiples of 32/64.
> 
> We need a way to provide our own "memcpy" function to be used by
> rproc_elf_load_segments.

I think when generating elf file, the sections needs to be 32/64bits aligned.

Regards,
Peng.

> 
> > ---
> >  drivers/remoteproc/imx_dsp_rproc.c | 95
> > +-----------------------------
> >  1 file changed, 1 insertion(+), 94 deletions(-)
> >
> > diff --git a/drivers/remoteproc/imx_dsp_rproc.c
> > b/drivers/remoteproc/imx_dsp_rproc.c
> > index 2abee78df96e..eee3c44c2146 100644
> > --- a/drivers/remoteproc/imx_dsp_rproc.c
> > +++ b/drivers/remoteproc/imx_dsp_rproc.c
> > @@ -649,99 +649,6 @@ static int imx_dsp_rproc_add_carveout(struct
> imx_dsp_rproc *priv)
> >         return 0;
> >  }
> >
> > -/**
> > - * imx_dsp_rproc_elf_load_segments() - load firmware segments to
> > memory
> > - * @rproc: remote processor which will be booted using these fw
> > segments
> > - * @fw: the ELF firmware image
> > - *
> > - * This function specially checks if memsz is zero or not, otherwise
> > it
> > - * is mostly same as rproc_elf_load_segments().
> > - */
> > -static int imx_dsp_rproc_elf_load_segments(struct rproc *rproc,
> > -                                          const struct firmware
> *fw)
> > -{
> > -       struct device *dev = &rproc->dev;
> > -       u8 class = fw_elf_get_class(fw);
> > -       u32 elf_phdr_get_size = elf_size_of_phdr(class);
> > -       const u8 *elf_data = fw->data;
> > -       const void *ehdr, *phdr;
> > -       int i, ret = 0;
> > -       u16 phnum;
> > -
> > -       ehdr = elf_data;
> > -       phnum = elf_hdr_get_e_phnum(class, ehdr);
> > -       phdr = elf_data + elf_hdr_get_e_phoff(class, ehdr);
> > -
> > -       /* go through the available ELF segments */
> > -       for (i = 0; i < phnum; i++, phdr += elf_phdr_get_size) {
> > -               u64 da = elf_phdr_get_p_paddr(class, phdr);
> > -               u64 memsz = elf_phdr_get_p_memsz(class, phdr);
> > -               u64 filesz = elf_phdr_get_p_filesz(class, phdr);
> > -               u64 offset = elf_phdr_get_p_offset(class, phdr);
> > -               u32 type = elf_phdr_get_p_type(class, phdr);
> > -               void *ptr;
> > -
> > -               /*
> > -                *  There is a case that with PT_LOAD type, the
> > -                *  filesz = memsz = 0. If memsz = 0, rproc_da_to_va
> > -                *  should return NULL ptr, then error is returned.
> > -                *  So this case should be skipped from the loop.
> > -                *  Add !memsz checking here.
> > -                */
> > -               if (type != PT_LOAD || !memsz)
> > -                       continue;
> > -
> > -               dev_dbg(dev, "phdr: type %d da 0x%llx memsz 0x%llx
> filesz 0x%llx\n",
> > -                       type, da, memsz, filesz);
> > -
> > -               if (filesz > memsz) {
> > -                       dev_err(dev, "bad phdr filesz 0x%llx memsz
> 0x%llx\n",
> > -                               filesz, memsz);
> > -                       ret = -EINVAL;
> > -                       break;
> > -               }
> > -
> > -               if (offset + filesz > fw->size) {
> > -                       dev_err(dev, "truncated fw: need 0x%llx avail
> 0x%zx\n",
> > -                               offset + filesz, fw->size);
> > -                       ret = -EINVAL;
> > -                       break;
> > -               }
> > -
> > -               if (!rproc_u64_fit_in_size_t(memsz)) {
> > -                       dev_err(dev, "size (%llx) does not fit in size_t
> type\n",
> > -                               memsz);
> > -                       ret = -EOVERFLOW;
> > -                       break;
> > -               }
> > -
> > -               /* grab the kernel address for this device address */
> > -               ptr = rproc_da_to_va(rproc, da, memsz, NULL);
> > -               if (!ptr) {
> > -                       dev_err(dev, "bad phdr da 0x%llx mem
> 0x%llx\n", da,
> > -                               memsz);
> > -                       ret = -EINVAL;
> > -                       break;
> > -               }
> > -
> > -               /* put the segment where the remote processor expects
> it */
> > -               if (filesz)
> > -                       memcpy(ptr, elf_data + offset, filesz);
> > -
> > -               /*
> > -                * Zero out remaining memory for this segment.
> > -                *
> > -                * This isn't strictly required since dma_alloc_coherent
> already
> > -                * did this for us. albeit harmless, we may consider
> removing
> > -                * this.
> > -                */
> > -               if (memsz > filesz)
> > -                       memset(ptr + filesz, 0, memsz - filesz);
> > -       }
> > -
> > -       return ret;
> > -}
> > -
> >  /* Prepare function for rproc_ops */
> >  static int imx_dsp_rproc_prepare(struct rproc *rproc)  { @@ -808,7
> > +715,7 @@ static const struct rproc_ops imx_dsp_rproc_ops = {
> >         .start          = imx_dsp_rproc_start,
> >         .stop           = imx_dsp_rproc_stop,
> >         .kick           = imx_dsp_rproc_kick,
> > -       .load           = imx_dsp_rproc_elf_load_segments,
> > +       .load           = rproc_elf_load_segments,
> >         .parse_fw       = rproc_elf_load_rsc_table,
> >         .sanity_check   = rproc_elf_sanity_check,
> >         .get_boot_addr  = rproc_elf_get_boot_addr,
> > --
> > 2.25.1
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 2/2] remoteproc: imx_dsp_rproc: use common rproc_elf_load_segments

[Peng Fan]

> Subject: Re: [PATCH 2/2] remoteproc: imx_dsp_rproc: use common
> rproc_elf_load_segments
> 
> On Thu, Mar 24, 2022 at 1:34 AM Peng Fan (OSS) <peng.fan@oss.nxp.com>
> wrote:
> >
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > remoteproc elf loader supports the specific case that segments have
> > PT_LOAD and memsz/filesz set to zero, so no duplicate code.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> 
> I think this change OK, but we have a case with the DSP were reads/writes
> should be done in multiples of 32/64.
> 
> We need a way to provide our own "memcpy" function to be used by
> rproc_elf_load_segments.

I think when generating elf file, the sections needs to be 32/64bits aligned.

Regards,
Peng.

> 
> > ---
> >  drivers/remoteproc/imx_dsp_rproc.c | 95
> > +-----------------------------
> >  1 file changed, 1 insertion(+), 94 deletions(-)
> >
> > diff --git a/drivers/remoteproc/imx_dsp_rproc.c
> > b/drivers/remoteproc/imx_dsp_rproc.c
> > index 2abee78df96e..eee3c44c2146 100644
> > --- a/drivers/remoteproc/imx_dsp_rproc.c
> > +++ b/drivers/remoteproc/imx_dsp_rproc.c
> > @@ -649,99 +649,6 @@ static int imx_dsp_rproc_add_carveout(struct
> imx_dsp_rproc *priv)
> >         return 0;
> >  }
> >
> > -/**
> > - * imx_dsp_rproc_elf_load_segments() - load firmware segments to
> > memory
> > - * @rproc: remote processor which will be booted using these fw
> > segments
> > - * @fw: the ELF firmware image
> > - *
> > - * This function specially checks if memsz is zero or not, otherwise
> > it
> > - * is mostly same as rproc_elf_load_segments().
> > - */
> > -static int imx_dsp_rproc_elf_load_segments(struct rproc *rproc,
> > -                                          const struct firmware
> *fw)
> > -{
> > -       struct device *dev = &rproc->dev;
> > -       u8 class = fw_elf_get_class(fw);
> > -       u32 elf_phdr_get_size = elf_size_of_phdr(class);
> > -       const u8 *elf_data = fw->data;
> > -       const void *ehdr, *phdr;
> > -       int i, ret = 0;
> > -       u16 phnum;
> > -
> > -       ehdr = elf_data;
> > -       phnum = elf_hdr_get_e_phnum(class, ehdr);
> > -       phdr = elf_data + elf_hdr_get_e_phoff(class, ehdr);
> > -
> > -       /* go through the available ELF segments */
> > -       for (i = 0; i < phnum; i++, phdr += elf_phdr_get_size) {
> > -               u64 da = elf_phdr_get_p_paddr(class, phdr);
> > -               u64 memsz = elf_phdr_get_p_memsz(class, phdr);
> > -               u64 filesz = elf_phdr_get_p_filesz(class, phdr);
> > -               u64 offset = elf_phdr_get_p_offset(class, phdr);
> > -               u32 type = elf_phdr_get_p_type(class, phdr);
> > -               void *ptr;
> > -
> > -               /*
> > -                *  There is a case that with PT_LOAD type, the
> > -                *  filesz = memsz = 0. If memsz = 0, rproc_da_to_va
> > -                *  should return NULL ptr, then error is returned.
> > -                *  So this case should be skipped from the loop.
> > -                *  Add !memsz checking here.
> > -                */
> > -               if (type != PT_LOAD || !memsz)
> > -                       continue;
> > -
> > -               dev_dbg(dev, "phdr: type %d da 0x%llx memsz 0x%llx
> filesz 0x%llx\n",
> > -                       type, da, memsz, filesz);
> > -
> > -               if (filesz > memsz) {
> > -                       dev_err(dev, "bad phdr filesz 0x%llx memsz
> 0x%llx\n",
> > -                               filesz, memsz);
> > -                       ret = -EINVAL;
> > -                       break;
> > -               }
> > -
> > -               if (offset + filesz > fw->size) {
> > -                       dev_err(dev, "truncated fw: need 0x%llx avail
> 0x%zx\n",
> > -                               offset + filesz, fw->size);
> > -                       ret = -EINVAL;
> > -                       break;
> > -               }
> > -
> > -               if (!rproc_u64_fit_in_size_t(memsz)) {
> > -                       dev_err(dev, "size (%llx) does not fit in size_t
> type\n",
> > -                               memsz);
> > -                       ret = -EOVERFLOW;
> > -                       break;
> > -               }
> > -
> > -               /* grab the kernel address for this device address */
> > -               ptr = rproc_da_to_va(rproc, da, memsz, NULL);
> > -               if (!ptr) {
> > -                       dev_err(dev, "bad phdr da 0x%llx mem
> 0x%llx\n", da,
> > -                               memsz);
> > -                       ret = -EINVAL;
> > -                       break;
> > -               }
> > -
> > -               /* put the segment where the remote processor expects
> it */
> > -               if (filesz)
> > -                       memcpy(ptr, elf_data + offset, filesz);
> > -
> > -               /*
> > -                * Zero out remaining memory for this segment.
> > -                *
> > -                * This isn't strictly required since dma_alloc_coherent
> already
> > -                * did this for us. albeit harmless, we may consider
> removing
> > -                * this.
> > -                */
> > -               if (memsz > filesz)
> > -                       memset(ptr + filesz, 0, memsz - filesz);
> > -       }
> > -
> > -       return ret;
> > -}
> > -
> >  /* Prepare function for rproc_ops */
> >  static int imx_dsp_rproc_prepare(struct rproc *rproc)  { @@ -808,7
> > +715,7 @@ static const struct rproc_ops imx_dsp_rproc_ops = {
> >         .start          = imx_dsp_rproc_start,
> >         .stop           = imx_dsp_rproc_stop,
> >         .kick           = imx_dsp_rproc_kick,
> > -       .load           = imx_dsp_rproc_elf_load_segments,
> > +       .load           = rproc_elf_load_segments,
> >         .parse_fw       = rproc_elf_load_rsc_table,
> >         .sanity_check   = rproc_elf_sanity_check,
> >         .get_boot_addr  = rproc_elf_get_boot_addr,
> > --
> > 2.25.1
> >

Re: [PATCH 2/2] remoteproc: imx_dsp_rproc: use common rproc_elf_load_segments

[Daniel Baluta]

On Wed, Apr 6, 2022 at 1:58 PM Peng Fan <peng.fan@nxp.com> wrote:
>
> > Subject: Re: [PATCH 2/2] remoteproc: imx_dsp_rproc: use common
> > rproc_elf_load_segments
> >
> > On Thu, Mar 24, 2022 at 1:34 AM Peng Fan (OSS) <peng.fan@oss.nxp.com>
> > wrote:
> > >
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > remoteproc elf loader supports the specific case that segments have
> > > PT_LOAD and memsz/filesz set to zero, so no duplicate code.
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> >
> > I think this change OK, but we have a case with the DSP were reads/writes
> > should be done in multiples of 32/64.
> >
> > We need a way to provide our own "memcpy" function to be used by
> > rproc_elf_load_segments.
>
> I think when generating elf file, the sections needs to be 32/64bits aligned.

Sure, that could be a fix. But some malicious user can crash the kernel
by crafting an elf with unaligned sections.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] remoteproc: imx_dsp_rproc: use common rproc_elf_load_segments

[Daniel Baluta]

On Wed, Apr 6, 2022 at 1:58 PM Peng Fan <peng.fan@nxp.com> wrote:
>
> > Subject: Re: [PATCH 2/2] remoteproc: imx_dsp_rproc: use common
> > rproc_elf_load_segments
> >
> > On Thu, Mar 24, 2022 at 1:34 AM Peng Fan (OSS) <peng.fan@oss.nxp.com>
> > wrote:
> > >
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > remoteproc elf loader supports the specific case that segments have
> > > PT_LOAD and memsz/filesz set to zero, so no duplicate code.
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> >
> > I think this change OK, but we have a case with the DSP were reads/writes
> > should be done in multiples of 32/64.
> >
> > We need a way to provide our own "memcpy" function to be used by
> > rproc_elf_load_segments.
>
> I think when generating elf file, the sections needs to be 32/64bits aligned.

Sure, that could be a fix. But some malicious user can crash the kernel
by crafting an elf with unaligned sections.

Re: [PATCH 2/2] remoteproc: imx_dsp_rproc: use common rproc_elf_load_segments

[Daniel Baluta]

On Wed, Apr 6, 2022 at 2:25 PM Daniel Baluta <daniel.baluta@gmail.com> wrote:
>
> On Wed, Apr 6, 2022 at 1:58 PM Peng Fan <peng.fan@nxp.com> wrote:
> >
> > > Subject: Re: [PATCH 2/2] remoteproc: imx_dsp_rproc: use common
> > > rproc_elf_load_segments
> > >
> > > On Thu, Mar 24, 2022 at 1:34 AM Peng Fan (OSS) <peng.fan@oss.nxp.com>
> > > wrote:
> > > >
> > > > From: Peng Fan <peng.fan@nxp.com>
> > > >
> > > > remoteproc elf loader supports the specific case that segments have
> > > > PT_LOAD and memsz/filesz set to zero, so no duplicate code.
> > > >
> > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>

Acked-by: Daniel Baluta <daniel.baluta@nxp.com>

Peng,

I'm fine going on with this now. Next we need to replace the boolean is_iomem
with a flags parameter to hold ATT_IOMEM, ATT_IOMEM32, etc.

> > >
> > > I think this change OK, but we have a case with the DSP were reads/writes
> > > should be done in multiples of 32/64.
> > >
> > > We need a way to provide our own "memcpy" function to be used by
> > > rproc_elf_load_segments.
> >
> > I think when generating elf file, the sections needs to be 32/64bits aligned.
>
> Sure, that could be a fix. But some malicious user can crash the kernel
> by crafting an elf with unaligned sections.

Re: [PATCH 2/2] remoteproc: imx_dsp_rproc: use common rproc_elf_load_segments

[Daniel Baluta]

On Wed, Apr 6, 2022 at 2:25 PM Daniel Baluta <daniel.baluta@gmail.com> wrote:
>
> On Wed, Apr 6, 2022 at 1:58 PM Peng Fan <peng.fan@nxp.com> wrote:
> >
> > > Subject: Re: [PATCH 2/2] remoteproc: imx_dsp_rproc: use common
> > > rproc_elf_load_segments
> > >
> > > On Thu, Mar 24, 2022 at 1:34 AM Peng Fan (OSS) <peng.fan@oss.nxp.com>
> > > wrote:
> > > >
> > > > From: Peng Fan <peng.fan@nxp.com>
> > > >
> > > > remoteproc elf loader supports the specific case that segments have
> > > > PT_LOAD and memsz/filesz set to zero, so no duplicate code.
> > > >
> > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>

Acked-by: Daniel Baluta <daniel.baluta@nxp.com>

Peng,

I'm fine going on with this now. Next we need to replace the boolean is_iomem
with a flags parameter to hold ATT_IOMEM, ATT_IOMEM32, etc.

> > >
> > > I think this change OK, but we have a case with the DSP were reads/writes
> > > should be done in multiples of 32/64.
> > >
> > > We need a way to provide our own "memcpy" function to be used by
> > > rproc_elf_load_segments.
> >
> > I think when generating elf file, the sections needs to be 32/64bits aligned.
>
> Sure, that could be a fix. But some malicious user can crash the kernel
> by crafting an elf with unaligned sections.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] remoteproc: elf_loader: skip segment with memsz as zero

[Mathieu Poirier]

Hi Peng,

On Wed, Mar 23, 2022 at 02:49:43PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Per elf specification,
> p_filesz: This member gives the number of bytes in the file image of
> the segment; it may be zero.
> p_memsz: This member gives the number of bytes in the memory image
> of the segment; it may be zero.
> 
> There is a case that i.MX DSP firmware has segment with PT_LOAD and
> p_memsz/p_filesz set to zero. Such segment needs to be ignored,
> otherwize rproc_da_to_va would report error.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/remoteproc_elf_loader.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
> index d635d19a5aa8..cb77f9e4dc70 100644
> --- a/drivers/remoteproc/remoteproc_elf_loader.c
> +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> @@ -181,7 +181,14 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
>  		bool is_iomem = false;
>  		void *ptr;
>  
> -		if (type != PT_LOAD)
> +		/*
> +		 *  There is a case that with PT_LOAD type, the
> +		 *  filesz = memsz = 0. If memsz = 0, rproc_da_to_va
> +		 *  should return NULL ptr, then error is returned.
> +		 *  So this case should be skipped from the loop.
> +		 *  Add !memsz checking here.

There are several architecture where XYZ_da_to_va() does not return a NULL
pointer when @len is 0, making this comment inaccurate.  Please remove that
part.

> +		 */
> +		if (type != PT_LOAD || !memsz)
>  			continue;

I have reflected long and hard on this one...

If @memsz is 0 then @filesz _has_ to be 0, otherwise rproc_elf_load_segments()
returns -EINVAL.  If @filesz is also 0 then nothing gets copied (for
architectures where XYZ_da_to_va() doesn't return a NULL pointer when @len is
0), which is exactly the same as what the above change does. 

As such I am inclined to view this set favourably.  That being said we will
have to proceed cautiously.  If something breaks we will have to revert it.

Please send another revision as quickly as possible so that it can stay in
linux-next long enough to, hopefully, catch any problems.

With the above and for this set:

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
 
>  
>  		dev_dbg(dev, "phdr: type %d da 0x%llx memsz 0x%llx filesz 0x%llx\n",
> -- 
> 2.25.1
> 

Re: [PATCH 1/2] remoteproc: elf_loader: skip segment with memsz as zero

[Mathieu Poirier]

Hi Peng,

On Wed, Mar 23, 2022 at 02:49:43PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Per elf specification,
> p_filesz: This member gives the number of bytes in the file image of
> the segment; it may be zero.
> p_memsz: This member gives the number of bytes in the memory image
> of the segment; it may be zero.
> 
> There is a case that i.MX DSP firmware has segment with PT_LOAD and
> p_memsz/p_filesz set to zero. Such segment needs to be ignored,
> otherwize rproc_da_to_va would report error.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/remoteproc_elf_loader.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
> index d635d19a5aa8..cb77f9e4dc70 100644
> --- a/drivers/remoteproc/remoteproc_elf_loader.c
> +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> @@ -181,7 +181,14 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
>  		bool is_iomem = false;
>  		void *ptr;
>  
> -		if (type != PT_LOAD)
> +		/*
> +		 *  There is a case that with PT_LOAD type, the
> +		 *  filesz = memsz = 0. If memsz = 0, rproc_da_to_va
> +		 *  should return NULL ptr, then error is returned.
> +		 *  So this case should be skipped from the loop.
> +		 *  Add !memsz checking here.

There are several architecture where XYZ_da_to_va() does not return a NULL
pointer when @len is 0, making this comment inaccurate.  Please remove that
part.

> +		 */
> +		if (type != PT_LOAD || !memsz)
>  			continue;

I have reflected long and hard on this one...

If @memsz is 0 then @filesz _has_ to be 0, otherwise rproc_elf_load_segments()
returns -EINVAL.  If @filesz is also 0 then nothing gets copied (for
architectures where XYZ_da_to_va() doesn't return a NULL pointer when @len is
0), which is exactly the same as what the above change does. 

As such I am inclined to view this set favourably.  That being said we will
have to proceed cautiously.  If something breaks we will have to revert it.

Please send another revision as quickly as possible so that it can stay in
linux-next long enough to, hopefully, catch any problems.

With the above and for this set:

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
 
>  
>  		dev_dbg(dev, "phdr: type %d da 0x%llx memsz 0x%llx filesz 0x%llx\n",
> -- 
> 2.25.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel