On Fri, Sep 17, 2021 at 05:44:44PM +0800, Shengjiu Wang wrote:

On Fri, Sep 17, 2021 at 1:20 PM Shengjiu Wang [off-list ref] wrote:

quoted

On Fri, Sep 17, 2021 at 1:00 AM Mathieu Poirier
[off-list ref] wrote:

quoted

[...]

quoted

quoted

quoted

+
+/**
+ * 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;
+             bool is_iomem;
+
+             if (type != PT_LOAD || !memsz)

You did a really good job with adding comments but this part is undocumented...
If I read this correctly you need to check for !memsz because some part of
the program segment may have a header but its memsz is zero, in which case it can
be safely skipped.  So why is that segment in the image to start with, and why
is it marked PT_LOAD if it is not needed?  This is very puzzling...

Actually I have added comments in the header of this function.

Indeed there is a mention of memsz in the function's header but it doesn't
mention _why_ this is needed, and that is what I'm looking for.

quoted

memsz= 0 with PT_LOAD issue, I have asked the toolchain's vendor,
they said that this case is allowed by elf spec...

And in the "pru_rproc.c" and "mtk_scp.c", seems they met same problem
they also check the filesz in their internal xxx_elf_load_segments() function.

In both cases they are skipping PT_LOAD sections where "filesz" is '0', which
makes sense because we don't know how many bytes to copy.  But here you are
skipping over a PT_LOAD section with a potentially valid filesz, and that is the
part I don't understand.

Ok, I can use filesz instead. For my case, filesz = memsz = 0,
it is the same result I want.

If that is the case then rproc_elf_load_segments() should work, i.e it won't
copy anything.  If rproc_elf_load_segments() doesn't work for you then there are
corner cases you haven't told me about.

quoted

The reason why I use "memsz '' is because there is  "if (filesz > memsz) "
check after this,  if memsz is zero, then "filesz" should be zero too, other
values are not allowed.

But I still think checking "!memsz" is better than filesz,  because
memsz > filesz is allowed (filesz = 0),  the code below can be executed.
filesz > memsz is not allowed.

What do you think?

I don't see a need to add a custom implementation for things that _may_ happen.
If using the default rproc_elf_load_segments() works than go with that.  We can deal
with problems if/when there is a need for it.

Best regards
Wang shengjiu

quoted

quoted

quoted

quoted

quoted

+                     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, &is_iomem);

                rproc_da_to_va(rproc, da, memsz, NULL);

yes, will update it.

quoted

More comments to follow later today or tomorrow.

Thanks.

Best regards
Wang Shengjiu

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