Re: rproc_elf_load_segments: rproc_elf_load_segments elf loading problem

[Stefan Agner <stefan@agner.ch>]

On 05.09.2018 16:57, Denis Ryndine wrote:
> Hello there,
> 
> The issues (both) looks like were raised over a year ago by
> henri.roosen@ginzinger.com.
> See here:
> https://lists.gt.net/linux/kernel/2684252?search_string=rproc_elf_load_segments;#2684252

Hm, indeed, the very same issue. And it seems that at least Henri came
to a similar conclusion than I did ("Remoteproc might think of detecting
and reject loading such ELF's.")...

The kernel.org link for the discussion:
https://lore.kernel.org/lkml/1493813529-19184-1-git-send-email-henri.roosen@ginzinger.com/T/#u

I think a warning along with not zeroing out would be ideal...

--
Stefan

> 
> But the suggested fixes for both - filesz and memset - were curbed, ref Bjorn.
> 
> Is this unfortunate .. ?
> 
> Denis Ryndine | Lead Software Engineer | Embedded Synergy (Pty) Ltd |
> P.O. Box 55874 | Polokwane | Limpopo | 0700 | South Africa
> T +27110837168
> dry@embedded-synergy.co.za | www.embedded-synergy.co.za
> 
> 
> On Thu, Aug 30, 2018 at 10:45 PM, Stefan Agner <stefan@agner.ch> wrote:
>> Hi there,
>>
>> On 23.08.2018 16:12, Felix Siegel wrote:
>>> Hi Denis,
>>>
>>> On Thu, 23 Aug 2018 11:28:08 +1000
>>> Denis Ryndine <dry@embedded-synergy.co.za> wrote:
>>>
>>>> Hello Suman,
>>>>
>>>> On Thu, Aug 23, 2018 at 6:50 AM, Suman Anna <s-anna@ti.com> wrote:
>>>> >
>>>> > Hi Denis,
>>>> >
>>>> > On 08/20/2018 12:40 AM, Denis Ryndine wrote:
>>>> > > Hello,
>>>> > >
>>>> > > The following may look like an error, could someone review.
>>>> > >
>>>> > > In rproc_elf_load_segments:
>>>> > >
>>>> > > /* grab the kernel address for this device address */
>>>> > > ptr = rproc_da_to_va(rproc, da, memsz);
>>>> > >
>>>> > > The last parameter should be filesz. Otherwise this call may fail, as
>>>> > > the case when da is an address within a segment / memory.
>>>> > > (E.g.  placing a RO data after the code within text segment /memory ).
>>>> >
>>>> > No, it's alright, memsz >= filesz usually. The actual loadable content
>>>>
>>>> No issue here.
>>>>
>>>> > will be filesz, the rest is zero initialized. Both these are from the
>>>> > program headers. Have you seen some issues around this?
>>>>
>>>> If the da points to the beginning of the segment or the device's
>>>> memory then it's all good. But da can point somewhere with-in or at
>>>> the end of the previous memory segment, where there is enough room to
>>>> fit filesz.
>>>> The check above may fail using memsz  (memsz >= filesz) if there isn't
>>>> physical memory left to fit memsz, but for  >= filesz.
>>>>
>>>> Consider an OCRAM linked firmware (for iMX),  with elf :
>>>>
>>>> Program Headers:
>>>> Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align
>>>> LOAD 0x001000 0x00000000 0x00000000 0x00240 0x00240 R 0x1000
>>>> LOAD 0x002000 0x00910000 0x00910000 0x0c2e0 0x0c2e0 RWE 0x1000
>>>> LOAD 0x00f000 0x20220000 0x0091c2e0 0x00210 0x072e0 RW 0x1000
>>>>
>>>> The grab kernel address fails trying grab too much:
>>>> remoteproc remoteproc0: bad phdr da 0x91c2e0 mem 0x72e0
>>>>
>>>> But it shouldn't, as there is enough space in that memory for the
>>>> filesz, which is what to be programmed into it, not the memsz.
>>>>
>>>> For iMX, for example, the device specific  rproc_da_to_va() would have
>>>> resolved the needed kernel address, if filesz would have been passed.
>>>> See imx_rproc.c - imx_rproc_da_to_va() ->  imx_rproc_da_to_sys(),
>>>> which would return the needed address: there is enough in that memory
>>>> block for filesz, but not for memsz.
>>>
>>> I had a similar problem with the iMX7 working with TCM.
>>> Your fix would probably work however this only occurs due to strange
>>> behaviour in the NXP linker and startup files.
>>> The NXP linker file stores the data segment directly behind the code
>>> segment in the code memory region
>>> (causing the difference between VirtAddr and PhysAddr) and the startup
>>> assembly then loads the data segment into the data memory region.
>>
>> I also worked with the i.MX 7 TCM linker file, and I agree, in the
>> context of remoteproc etc the linker file does really
>> unnecessary/strange section placements.
>>
>> I guess this comes from the microcontroller world, there memory is
>> volatile and the firmware initialization code loads the .data section
>> into memory from ROM.
>>
>> That said, the difference in VirtAddr and PhysAddr is caused by the `AT`
>> keyword in the linker file:
>> https://sourceware.org/binutils/docs/ld/Output-Section-LMA.html
>>
>> From what I can tell, because remoteproc uses paddr as base and memsz as
>> length, remoteproc makes the assumption that the virtual and physical
>> addressing is fully aligned... For a lot of linker files this is
>> probably a reasonable assumption since we do not *need* startup code
>> which relocates sections...
>>
>> However, if we make this assumption, maybe we should check if paddr and
>> vaddr are really aligned, e.g. by using:
>>
>> WARN_ON(phdr->p_paddr != phdr->p_vaddr)
>>
>> Or, we could not zero out in case paddr/vaddr are not aligned, just to
>> be on the safe side e.g.:
>>
>> --- a/drivers/remoteproc/remoteproc_elf_loader.c
>> +++ b/drivers/remoteproc/remoteproc_elf_loader.c
>> @@ -200,7 +200,7 @@ int rproc_elf_load_segments(struct rproc *rproc,
>> const struct firmware *fw)
>>                  * did this for us. albeit harmless, we may consider
>> removing
>>                  * this.
>>                  */
>> -               if (memsz > filesz)
>> +               if (phdr->p_paddr == phdr->p_vaddr && memsz > filesz)
>>                         memset(ptr + filesz, 0, memsz - filesz);
>>         }
>>
>>
>>>
>>> This would make sense for a normal microcontroller with persistent
>>> memory to boot from, but atleast on the imx7 the M4 requires the A7 to
>>> start it
>>> and the supported memory regions are all volatile anyway.
>>>
>>> After I changed the linker script and the startup routine it worked
>>> for me. It also avoids needlessly copying data around.
>>
>> True, and that is what we ended up doing to.
>>
>> Still, maybe the kernel could behave a bit smarter.
>>
>> --
>> Stefan
>>
>>>>
>>>>
>>>> > regards
>>>> > Suman
>>>>
>>>> Regards,
>>>>
>>>> Denis
>>>
>>> Regards,
>>>
>>> Felix