From: Shawn Guo <shawn.guo@linaro.org>
To: Marijn Suijten <marijn.suijten@somainline.org>
Cc: Shawn Guo <shawnguo@kernel.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
linux-arm-msm@vger.kernel.org,
Konrad Dybcio <konrad.dybcio@somainline.org>,
AngeloGioacchino Del Regno
<angelogioacchino.delregno@somainline.org>,
Martin Botka <martin.botka@somainline.org>,
Pavel Dubrova <pashadubrova@gmail.com>,
Siddharth Gupta <sidgup@codeaurora.org>
Subject: Re: [PATCH] soc: qcom: mdt_loader: Drop PT_LOAD check on hash segment
Date: Sat, 28 Aug 2021 14:03:50 +0800 [thread overview]
Message-ID: <20210828060349.GB4274@dragon> (raw)
In-Reply-To: <16c45f98-60ed-61d0-9e6a-d0c2aa2a20d1@somainline.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
next prev parent reply other threads:[~2021-08-28 6:03 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-24 9:41 [PATCH] soc: qcom: mdt_loader: Drop PT_LOAD check on hash segment Shawn Guo
2021-08-24 15:18 ` Marijn Suijten
2021-08-24 15:34 ` Marijn Suijten
2021-08-26 14:18 ` Shawn Guo
2021-08-26 20:52 ` Marijn Suijten
2021-08-27 6:24 ` Shawn Guo
2021-08-27 8:29 ` Marijn Suijten
2021-08-27 9:57 ` Shawn Guo
2021-08-27 10:46 ` Marijn Suijten
2021-08-27 14:12 ` Shawn Guo
2021-08-27 15:13 ` Marijn Suijten
2021-08-28 6:03 ` Shawn Guo [this message]
2021-08-28 8:58 ` Marijn Suijten
2021-08-27 16:07 ` Bjorn Andersson
2021-08-27 17:40 ` Marijn Suijten
2021-08-27 21:25 ` Bjorn Andersson
2021-08-27 23:42 ` Marijn Suijten
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210828060349.GB4274@dragon \
--to=shawn.guo@linaro.org \
--cc=angelogioacchino.delregno@somainline.org \
--cc=bjorn.andersson@linaro.org \
--cc=konrad.dybcio@somainline.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=marijn.suijten@somainline.org \
--cc=martin.botka@somainline.org \
--cc=pashadubrova@gmail.com \
--cc=shawnguo@kernel.org \
--cc=sidgup@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.