Re: rproc_elf_load_segments: rproc_elf_load_segments elf loading problem

[Stefan Agner <stefan@agner.ch>]

On 05.09.2018 18:23, Denis Ryndine wrote:
> Hi
> 
> On Thu, Sep 6, 2018 at 10:58 AM, Stefan Agner <stefan@agner.ch> wrote:
>> 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...
> 
> Why not allow such elfs?   With the 2 fixes, they should be loadable &
> runnable   (but I haven't done tests on  the NXP's sample elfs for M4
> using the rpoc) .

We treat firmware elfs differently depending on whether they use
different/same Phys/Virt addresses...

At least a information message might be appropriate?

How about:

/*
 * elf files linked with the AT attribute might have different phys/virt
address
 * mapping. Typically this is used to store initialization data on ROM,
and the
 * firmware relocates the data (filesz) into its final place (virt
memory address).
 * In this case it is not safe to zero out the physical address space up
to memsz.
 */
bool virt_differ = phdr->p_paddr != phdr->p_vaddr;

if (virt_differ)
	pr_info("Not zeroing out remaining memory due to phys/virt memory
difference\n");

ptr = rproc_da_to_va(rproc, da, virt_differ ? filesz : memsz);
...

/*
 * 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 (!virt_differ && memsz > filesz)
	memset(ptr + filesz, 0, memsz - filesz);

--
Stefan

> 
> What gets broken by allowing it ?
> 
> 
> Denis
>>
>> --
>> Stefan
>>
>>>
>>> But the suggested fixes for both - filesz and memset - were curbed, ref Bjorn.
>>>
>>> Is this unfortunate .. ?
>>>
>>>
>>> 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