| Message ID | 20220323064944.1351923-3-peng.fan@oss.nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | remoteproc: elf: ignore PT_LOAD type segment with memsz as 0 | expand |
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 >
> 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 > >
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.
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.
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,