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 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.
> > 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).
> Looks good to me. I will be happy to review patches if you pick up the
> work.
>
Thanks to the both of you for looking into this!
Regards,
Bjorn
