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
