Hi Bjorn,
On 8/27/21 6:07 PM, Bjorn Andersson wrote:
On Fri 27 Aug 04:57 CDT 2021, Shawn Guo wrote:
On Fri, Aug 27, 2021 at 10:29:59AM +0200, Marijn Suijten wrote:
On 8/27/21 8:24 AM, Shawn Guo wrote:
[..]
For this quick solution to be valid I propose to reject PT_LOAD in the else
clause, and otherwise validate that phdrs[1].p_offset + hash_size <=
fw->size.
I'm not sure what you propose here are required for non-split firmware.
Would it help if I sent a patch based on yours, with this extra validation
and comments + commit message explaining the cases?
Alternatively we can try re-spinning the patches from Siddharth while taking
review comments, the possible .mdt == .b00 + . b01 packing, and my
suggestion to handle the headers generically into account.
I would suggest this path. It's been 8 months, so Siddharth probably lost
the interest here. It's reasonable that someone picks up the work.
Prior to 498b98e93900 ("soc: qcom: mdt_loader: Support loading non-split
images") we'd just blindly pass the entire .mdt into the SCM call.
So reading through your discussion and looking at the problem is that
I assumed (based on the firmware files I looked at) that the hash
segment is explicitly mentioned in the ELF header - i.e. segment 0 and 1
are not PT_LOAD and define the part that should be passed to init_image,
and that this may or may not be packed.
And the problem we have here is that we have the packed case, but the
hash segment isn't described explicitly in the ELF header.
The hash segment is still explicitly described by the presence of
QCOM_MDT_TYPE_HASH, and the external file (.b01 when the program header
is in slot 1) contains the hash signature. And as Shawn demonstrated,
concatenating the ELF header in .b00 and this hash in .b01 results in
the .mdt file. This is the case we can (and already) optimize for, by
passing the entire .mdt as-is into SCM if such packing is detected.
The aforementioned patch series in qcom_mdt_bins_are_split (and certain
firmware files from Sony phones) show that it is also possible to read
PT_NULL headers from external files, as long as the header references a
range that is out of bounds of the mdt file.
It's been shown that hashes can be read from .mdt or hash segment, and
independently the hash segment type could be PT_NULL or PT_LOAD. With
Siddharth's patch, instead of using the hash duplication in .mdt, hash
will be read from hash segment. But again, to resolve my problem, the
assertion that hash segment type cannot be PT_LOAD has to be dropped.
And that's the only thing my patch is doing.
Correct. If I were to respin Siddharth's patchset, I'd write
qcom_mdt_read_metadata as follows:
1. Find the header that contains QCOM_MDT_TYPE_HASH (allowing PT_NONE
and PT_LOAD);
2. If `ehdr_size + hash_size == fw->size`, this is split firmware and
the mdt file can be used as-is for metadata;
3. If the type is PT_LOAD, _or_ if the type is PT_NULL but
`p_offset + p_filesz` does not fit inside the .mdt, this is (a
variant of) split-firmware, and the hash needs to be loaded from an
external file and appended to the ELF header.
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.
I have two mdt files here that load the hash externally, one with
PT_LOAD and one with PT_NULL annotation (QCOM_MDT_TYPE_HASH is set).
Both have their p_offset and/or p_offset+p_filesz well out of bounds of
the .mdt file, but the .mdt file is of the packed variant (.b00 + .b01
== .mdt).
This packing seems to be an optimization, making it possible to send the
.mdt contents as-is over SCM without having to load and concatenate the
header from a separate file.
The image produced by __qcom_mdt_load should still be reconstructed
based on the layout specified in the program headers, of course.
4. If none of the above, this is a non-split firmware file. Concatenate
the ELF and hash headers by reading them directly from the firmware.
I'm happy with this plan and I think Sid will be as well.
I hope that we can introduce a change that fixes Shawn's reported
problem first, so that can be backported to stable, and then add Sid's
need for loading the additional .bNN after that (same series is fine).
I'm totally for fixing Shawn's reported issue first, before diving into
extending mdt_loader as per Sid's patches. In hindsight I'm not sure if
I should be the person writing that until coming across a
platform/firmware that needs this setup (hash in a different header,
hash not included in the packed .mdt file).
Note that backporting needs a Fixes: tag, probably on 498b98e93900.
Shawn, do you want to send your reworded patch with the necessary
safeguards as v2, or should I send it?
- Marijn
