Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1641053 > unrolled thread

Re: [PATCH 1/1] remoteproc: fix elf_loader da_to_va translation and writing beyond segment

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/1] remoteproc: fix elf_loader da_to_va translation and  writing beyond segment Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-05-14 06:20 +0200
    Re: [PATCH 1/1] remoteproc: fix elf_loader da_to_va translation and  writing beyond segment Henri Roosen <henri.roosen@ginzinger.com> - 2017-05-15 16:40 +0200
      Re: [PATCH 1/1] remoteproc: fix elf_loader da_to_va translation and  writing beyond segment Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-05-18 20:10 +0200

#1641053 — Re: [PATCH 1/1] remoteproc: fix elf_loader da_to_va translation and writing beyond segment

On Thu 11 May 09:12 PDT 2017, Henri Roosen wrote:

> On 05/11/2017 02:05 AM, Bjorn Andersson wrote:
> > On Wed 03 May 05:12 PDT 2017, Henri Roosen wrote:
> > 
> > > Consider a system with 2 memory regions:
> > > 0x1fff8000 - 0x1fffffff: iram
> > 
> > So I presume there's a hole here.
> > 
> > > 0x21000000 - 0x21007fff: dram
> > > 
> > > The .elf file for this system contains the following Program Headers:
> > > Program Headers:
> > >   Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
> > >   LOAD           0x000094 0x1fff8000 0x1fff8000 0x00240 0x00240 R   0x4
> > >   LOAD           0x0002e0 0x1fff8240 0x1fff8240 0x03d1c 0x03d1c RWE 0x10
> > >   LOAD           0x003ffc 0x21000000 0x1fffbf5c 0x001cc 0x048a0 RW  0x4
> > > 
> > 
> > Your ELF header states that there is a segment of 0x48a0 bytes starting
> > at 0x1fffbf5c, but your iram ends after 0x40a3 bytes. I assume your
> > MemSiz comes from some linker script, which would mean that your
> > firmware expects to be able to use all 0x48a0 bytes, which should be
> > invalid.
> 
> I had a closer look at the linker script. The .data section uses the
> "AT"-keyword to place the initialized .data right after the .text section
> (0x1fffbf5c/PhysAddr).
> 
> Some run-time startup-code is responsible for copying the initialized data
> to its runtime address (0x21000000/VirtAddr). The run-time startup-code is
> also responsible for zero-ing the .bss section.
> 
> The size of the initialized data is 0x1cc (FileSiz). The size of the whole
> 3rd segment at run-time is 0x048a0 (MemSiz), starting from 0x21000000
> (VirtAddr), which also includes the .bss .heap and .stack sections.
> 

[1] specifies that p_memsz is the size of the memory segment and
that the difference between p_filesz and p_memsz are defined to hold the
value 0.

So I believe that your segment list states that the physical range
0x1fffbf5c through 0x200007fc is valid and should be populated.

[1] https://refspecs.linuxfoundation.org/elf/elf.pdf

Regards,
Bjorn

[toc] | [next] | [standalone]

#1641757

On 05/14/2017 06:14 AM, Bjorn Andersson wrote:
> On Thu 11 May 09:12 PDT 2017, Henri Roosen wrote:
>
>> On 05/11/2017 02:05 AM, Bjorn Andersson wrote:
>>> On Wed 03 May 05:12 PDT 2017, Henri Roosen wrote:
>>>
>>>> Consider a system with 2 memory regions:
>>>> 0x1fff8000 - 0x1fffffff: iram
>>>
>>> So I presume there's a hole here.
>>>
>>>> 0x21000000 - 0x21007fff: dram
>>>>
>>>> The .elf file for this system contains the following Program Headers:
>>>> Program Headers:
>>>>   Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
>>>>   LOAD           0x000094 0x1fff8000 0x1fff8000 0x00240 0x00240 R   0x4
>>>>   LOAD           0x0002e0 0x1fff8240 0x1fff8240 0x03d1c 0x03d1c RWE 0x10
>>>>   LOAD           0x003ffc 0x21000000 0x1fffbf5c 0x001cc 0x048a0 RW  0x4
>>>>
>>>
>>> Your ELF header states that there is a segment of 0x48a0 bytes starting
>>> at 0x1fffbf5c, but your iram ends after 0x40a3 bytes. I assume your
>>> MemSiz comes from some linker script, which would mean that your
>>> firmware expects to be able to use all 0x48a0 bytes, which should be
>>> invalid.
>>
>> I had a closer look at the linker script. The .data section uses the
>> "AT"-keyword to place the initialized .data right after the .text section
>> (0x1fffbf5c/PhysAddr).
>>
>> Some run-time startup-code is responsible for copying the initialized data
>> to its runtime address (0x21000000/VirtAddr). The run-time startup-code is
>> also responsible for zero-ing the .bss section.
>>
>> The size of the initialized data is 0x1cc (FileSiz). The size of the whole
>> 3rd segment at run-time is 0x048a0 (MemSiz), starting from 0x21000000
>> (VirtAddr), which also includes the .bss .heap and .stack sections.
>>
>
> [1] specifies that p_memsz is the size of the memory segment and
> that the difference between p_filesz and p_memsz are defined to hold the
> value 0.

I think the main problem is that there is no specification how to deal 
with ELF files which are linked with an AT-attribute in a segment. In 
that case VirtAddr and PhysAddr differ. Then it's unclear whether to use 
VirtAddr or PhysAddr for zeroing (or even whether zeroing should be done 
at all).

IMHO, the way I interpret the specification, zeroing (and loading in 
general) should be done using VirtAddr. However, the spec [1] is not 
very clear on this. Zeroing using PhysAddr might not cause problems on 
ELF files without AT-keyword using GNU-linkers which have PhysAddr and 
VirtAddr to be equal, but I'm not sure if all linkers generate PhysAddr 
and VirtAddr equal.

My patch is clearly not the correct fix for "AT-linked-ELF's", so please 
ignore it. The AT-keyword is actually mainly used when linking for 
micro-controllers with FLASH/ROM and I can remove it from my linker-file 
for the remoteproc-firmware generation. Remoteproc might think of 
detecting and reject loading such ELF's.

>
> So I believe that your segment list states that the physical range
> 0x1fffbf5c through 0x200007fc is valid and should be populated.

As explained above, IMHO the segment list states that the range 
0x21000000 through 0x210048a0 is valid. This range would also fit the 
actual memory region.

(And for the AT-keyword to work, 0x1cc bytes should be populated at 
0x1fffbf5c, but I don't know of any specification for this).

Regards,
Henri

>
> [1] https://refspecs.linuxfoundation.org/elf/elf.pdf
>
> Regards,
> Bjorn
>

[toc] | [prev] | [next] | [standalone]

#1644816

On Mon 15 May 07:37 PDT 2017, Henri Roosen wrote:

> On 05/14/2017 06:14 AM, Bjorn Andersson wrote:
> > On Thu 11 May 09:12 PDT 2017, Henri Roosen wrote:
> > 
> > > On 05/11/2017 02:05 AM, Bjorn Andersson wrote:
> > > > On Wed 03 May 05:12 PDT 2017, Henri Roosen wrote:
> > > > 
> > > > > Consider a system with 2 memory regions:
> > > > > 0x1fff8000 - 0x1fffffff: iram
> > > > 
> > > > So I presume there's a hole here.
> > > > 
> > > > > 0x21000000 - 0x21007fff: dram
> > > > > 
> > > > > The .elf file for this system contains the following Program Headers:
> > > > > Program Headers:
> > > > >   Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
> > > > >   LOAD           0x000094 0x1fff8000 0x1fff8000 0x00240 0x00240 R   0x4
> > > > >   LOAD           0x0002e0 0x1fff8240 0x1fff8240 0x03d1c 0x03d1c RWE 0x10
> > > > >   LOAD           0x003ffc 0x21000000 0x1fffbf5c 0x001cc 0x048a0 RW  0x4
> > > > > 
> > > > 
> > > > Your ELF header states that there is a segment of 0x48a0 bytes starting
> > > > at 0x1fffbf5c, but your iram ends after 0x40a3 bytes. I assume your
> > > > MemSiz comes from some linker script, which would mean that your
> > > > firmware expects to be able to use all 0x48a0 bytes, which should be
> > > > invalid.
> > > 
> > > I had a closer look at the linker script. The .data section uses the
> > > "AT"-keyword to place the initialized .data right after the .text section
> > > (0x1fffbf5c/PhysAddr).
> > > 
> > > Some run-time startup-code is responsible for copying the initialized data
> > > to its runtime address (0x21000000/VirtAddr). The run-time startup-code is
> > > also responsible for zero-ing the .bss section.
> > > 
> > > The size of the initialized data is 0x1cc (FileSiz). The size of the whole
> > > 3rd segment at run-time is 0x048a0 (MemSiz), starting from 0x21000000
> > > (VirtAddr), which also includes the .bss .heap and .stack sections.
> > > 
> > 
> > [1] specifies that p_memsz is the size of the memory segment and
> > that the difference between p_filesz and p_memsz are defined to hold the
> > value 0.
> 
> I think the main problem is that there is no specification how to deal with
> ELF files which are linked with an AT-attribute in a segment. In that case
> VirtAddr and PhysAddr differ. Then it's unclear whether to use VirtAddr or
> PhysAddr for zeroing (or even whether zeroing should be done at all).
> 
> IMHO, the way I interpret the specification, zeroing (and loading in
> general) should be done using VirtAddr. However, the spec [1] is not very
> clear on this. Zeroing using PhysAddr might not cause problems on ELF files
> without AT-keyword using GNU-linkers which have PhysAddr and VirtAddr to be
> equal, but I'm not sure if all linkers generate PhysAddr and VirtAddr equal.
> 

I believe that the cases where VirtAddr != PhysAddr you're supposed to
have some sort of 1:1 mapping between them - e.g. using an MMU. I would
like to stick with this belief, but am not sure if this is the defined
behavior... Sorry about that.

> My patch is clearly not the correct fix for "AT-linked-ELF's", so please
> ignore it. The AT-keyword is actually mainly used when linking for
> micro-controllers with FLASH/ROM and I can remove it from my linker-file for
> the remoteproc-firmware generation. Remoteproc might think of detecting and
> reject loading such ELF's.
> 

We load the ELF files in three steps, first we do a sanity check of the
header, then we parse and act on the resource table and finally we
iterate over the segments loading things into ram.

The second step will currently "define" all the memory regions, so we
can't verify their sanity in the first step. So what you're seeing is
the ELF loader failing due to insufficient space.

Regards,
Bjorn

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web