Re: [PATCH] soc: qcom: mdt_loader: Drop PT_LOAD check on hash segment

[Shawn Guo <shawn.guo@linaro.org>]

On Fri, Aug 27, 2021 at 05:13:34PM +0200, Marijn Suijten wrote:
> Hi Shawn,
> 
> On 8/27/21 4:12 PM, Shawn Guo wrote:
> > [..]
> > 
> > So you proposed to reject PT_LOAD in the else clause, which right now
> > handles .mbn case
> 
> 
> Yes, I propose to reject PT_LOAD in the else-case, and additionally reject
> cases where p_offset+p_filesz > sw->size since PT_NULL can also cause
> external file loads (meaning split-firmware).  This is what Siddharth's
> patchset - or my respin of it - is going to implement.
> 
> > are you sure hash segment in .mbn is not going to be
> > PT_LOAD?
> 
> 
> PT_LOAD unambiguously indicates a program header that ought to be loaded
> from an external file.  Any mbn file (non-split firmware) without split
> files that set PT_LOAD are misusing this program header type field.  I have
> no way to validate whether such mbns are in circulation.

Following your take on PT_LOAD, I assume that no PT_LOAD segment should
be found in .mbn file, correct?  Here are two .mbn I found in
circulation.  Both have PT_LOAD type for a few segments.

$ readelf -l venus.mbn 

Elf file type is EXEC (Executable file)
Entry point 0xf500000
There are 5 program headers, starting at offset 52

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  NULL           0x000000 0x00000000 0x00000000 0x000d4 0x00000     0
  LOAD           0x001000 0x0fa00000 0x0fa00000 0x00b88 0x02000     0x1000
  LOAD           0x003000 0x00000000 0x0f500000 0xeecd0 0xeecd0 R E 0x100000
  LOAD           0x0f1cd0 0x000ef000 0x0f5ef000 0x015c0 0x405000 RW  0x4000
  LOAD           0x0f3290 0x004ff000 0x0f9ff000 0x00020 0x00020 RW  0x4

$ readelf -l mba.mbn 

Elf file type is EXEC (Executable file)
Entry point 0x4417000
There are 5 program headers, starting at offset 52

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  NULL           0x000000 0x00000000 0x00000000 0x000d4 0x00000     0
  NULL           0x001000 0x8ea4a000 0x8ea4a000 0x019c8 0x02000     0x1000
  LOAD           0x003000 0x04417000 0x8ea00000 0x350d0 0x3bbc8 RWE 0x1000
  LOAD           0x039000 0x04460000 0x8ea49000 0x00380 0x00380 RW  0x1000
  GNU_STACK      0x002000 0x00000000 0x00000000 0x00000 0x00000 RWE 0x4

Or you think these are all misusing of PT_LOAD?  Sorry, I hardly believe
your understanding on PT_LOAD matches the reality.  Instead, I'm inclined
to agree with Bjorn's comment.

Quote:

"I would expect that PT_LOAD denotes that the segment should be loaded
into the final firmware region and that the hash segment would be
PT_NULL regardless of being part of the .mdt, single .mbn or a separate
.bNN segment."

The only part that doesn't hold is "the hash segment would be PT_NULL".
But the point is that PT_LOAD doesn't mean the segment should be loaded
externally (from .bNN file). 

> 
> Of note, I have never referenced the definition of the program header types
> yet.  Please see [1]:
> 
>     PT_LOAD (1)
>         Indicates that this program header describes a segment to be
>         loaded from the file.
> 
> Let me know if you're planning to send a v2 of this patch with
> aforementioned improvements, then I'll adjust my plans to respin Siddharth's
> patchset accordingly.

I will send v2. However there will be no code change but just commit log
update based on all these discussion.  Thanks!

Shawn

> [1]: https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_node/ld_23.html