Hi Shawn,
On 8/27/21 11:57 AM, Shawn Guo wrote:
[..]
PT_LOAD specifies that the segment is to be loaded externally. The fact
that our .mdt file is a tight pack of b00 + b01 is mere convenience, but is
it also a given for the future? Can we rely on this assumption to never
change?
My patch is trying to fix an existing issue, not anything for the
future.
We both agree that the PT_LOAD assertion here is too strict, but
removing it altogether makes the function too lenient and allows for
possible bugs. To solve your issue in the simple case I have already
suggested to add an extra bounds check.
[..]
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.
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.
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.
Looks good to me. I will be happy to review patches if you pick up the
work.
I'll try to get this going over the weekend or next week. I don't think
I can salvage anything from Siddharth's patchset and will likely start
from scratch as I'm not confident "just detecting split firmware" is
enough, but favour generic handling of the program headers as described
above instead (for both the hash and firmware loading itself).
Will keep you posted!
- Marijn
