Re: [PATCH] soc: qcom: mdt_loader: Drop PT_LOAD check on hash segment

[Shawn Guo <shawn.guo@linaro.org>]

On Fri, Aug 27, 2021 at 10:29:59AM +0200, Marijn Suijten wrote:
> Hi Shawn,
> 
> On 8/27/21 8:24 AM, Shawn Guo wrote:
> > > [..]
> > > It's not up to me to choose between a thorough or quick solution,
> > 
> > To be clear, Siddharth's series doesn't resolve my problem, as the
> > assumption that hash segment type cannot be PT_LOAD is still there.
> > I have to add the following change on top to fix my problem.
> > 
> > @@ -126,8 +126,7 @@ void *qcom_mdt_read_metadata(struct device *dev, const struct firmware *fw, cons
> >                  return ERR_PTR(-EINVAL);
> >          for (hash_index = 1; hash_index < ehdr->e_phnum; hash_index++) {
> > -               if (phdrs[hash_index].p_type != PT_LOAD &&
> > -                  (phdrs[hash_index].p_flags & QCOM_MDT_TYPE_MASK) == QCOM_MDT_TYPE_HASH)
> > +               if ((phdrs[hash_index].p_flags & QCOM_MDT_TYPE_MASK) == QCOM_MDT_TYPE_HASH)
> >                          break;
> >          }
> >          if (hash_index >= ehdr->e_phnum)
> 
> 
> Patch 3/3 allows the hash segment to be read from an external file and
> should indeed get rid of this comparison, as external file loading is
> possible with PT_NULL and required with PT_LOAD.  I'd go as far as moving
> the check into the if, next to qcom_mdt_bins_are_split:
> 
>  if (phdrs[hash_index].p_type == PT_LOAD || qcom_mdt_bins_are_split(fw))
> 
> Unfortunately it seems this patchset lost optimization for the packed
> `ehdr_size + hash_size == fw->size` case, where the hash segment is already
> available in the loaded mbt.
> 
> > That said, my patch is merely to break the assumption that hash segment
> > type cannot be PT_LOAD, and it's really orthogonal to Siddharth's
> > series.
> 
> 
> It is fine - correct even - to break that assumption, but it should go with
> extra validation that we are dealing with packed mdt instead.
> 
> > > but this
> > > patch seems to open up the possibility for an out-of-bounds read. The
> > > assertion was placed in this function for a reason after all.
> > 
> > I would much appreciate it if someone helps to elaborate the reason.
> 
> 
> 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.

> 
> > > > There is a blog[1] illustrating the situation quite nicely.  Again, the
> > > > only thing my WCNSS firmware differs from the illustration is that
> > > > hash segment gets a PT_LOAD type.
> > > 
> > > 
> > > Yes, that blog post nicely explains the format but it doesn't cover that the
> > > hash is encoded in the second program header nor that it can be copied to be
> > > packed tightly against the ELF header?  Maybe that only applies to newer
> > > formats?
> > 
> > Hmm, it does cover the things.  It's been illustrated that .mdt is
> > simply a concatenating of .b00 and .b01.  The .b00 includes ELF header
> > and program headers, while .b01 is just hash segment.
> > 
> > $ cat wcnss.b00 wcnss.b01 > wcnss.b00_b01
> > $ cmp wcnss.b00_b01 wcnss.mdt
> > $
> > 
> > That said, .mdt = ELF header + program headers + hash
> 
> 
> Ack, there's one picture halfway demonstrating the `ehdr_size + hash_size ==
> fw->size` case.
> 
> > > > Skipping the check will not cause problem for firmwares we have seen,
> > > > because hash segment is duplicated in .mdt file, and we are using that
> > > > duplication instead of reading from .b01 file.
> > > 
> > > 
> > > Can you share some more details about the firmware file you're using: size
> > > and the program headers as shown by `readelf -l`?
> > 
> > $ readelf -l wcnss.mdt
> > 
> > Elf file type is EXEC (Executable file)
> > Entry point 0x8b6018d4
> > There are 12 program headers, starting at offset 52
> > 
> > Program Headers:
> >    Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
> >    NULL           0x000000 0x00000000 0x00000000 0x001b4 0x00000     0
> >    LOAD           0x001000 0x8bbe0000 0x8bbe0000 0x00c58 0x02000     0x1000
> >    LOAD           0x003000 0x8b600000 0x8b600000 0x03308 0x03438 RWE 0x100000
> >    LOAD           0x00afcc 0x8b604000 0x8b604000 0x00000 0x08000 RW  0x4000
> >    LOAD           0x00afcc 0x8b60c000 0x8b60c000 0x0f000 0x0f000 RW  0x4
> >    LOAD           0x019fcc 0x8b61b000 0x8b61b000 0x00000 0x0e000 RW  0x4
> >    LOAD           0x019fcc 0x8b629000 0x8b629000 0x32eb04 0x4c2b10 RWE 0x80
> >    LOAD           0x348ad0 0x8baebb80 0x8baebb80 0x00000 0x37cf8 RW  0x8
> >    LOAD           0x348ad0 0x8baebb80 0x8baebb80 0x00000 0x21c44 RW  0x4
> >    LOAD           0x348ad0 0x8bb30000 0x8bb30000 0x00034 0x001a8 RW  0x8
> >    LOAD           0x349fcc 0x8bb31000 0x8bb31000 0xa0000 0xa0000 RW  0x1000
> >    LOAD           0x3e9fcc 0x8bbd1000 0x8bbd1000 0x0ac3c 0x0ee00 RWE 0x1000
> > 
> > > If possible, can you also
> > > validate whether QCOM_MDT_TYPE_HASH is set in phdrs[1].p_flags &
> > > QCOM_MDT_TYPE_MASK (using something like GNU poke)?
> > 
> > It is set, otherwise the QCOM_MDT_TYPE_HASH check in
> > qcom_mdt_read_metadata() will just fail.  To be clear, everything works
> > fine for me, as long as I drop the check of (phdrs[1].p_type == PT_LOAD).
> 
> 
> Thanks, this all checks out and is similar to what I'm seeing here.  It all
> comes down to the mdt packing b00 and b01 tightly together already.
> 
> > > All the files I'm able to test adhere to `/* Is the header and hash already
> > > packed */`: `ehdr_size + hash_size == fw->size` (meaning the file solely
> > > consists of a tightly packed ELF header and hash signature)
> > 
> > Yeah, my wcnss firmware is same here.  Actually, all split firmwares I
> > have seen are this case.  But bear it in mind there are non-split
> > firmware like a single .mbn file.  My understanding is that condition
> > (ehdr_size + hash_size == fw->size) is being used to check whether it's
> > a split firmware or non-split one.
> 
> 
> Is it unreasonable to assume that more configurations besides split and
> non-split firmware might occur in the future (or are already out in the
> wild)?  These program headers allow for a variety of configurations which we
> should - in my opinion - parse and handle in a generic manner.  `ehdr_size +
> hash_size == fw->size` is convenient to have, but not something we should
> rely on.
> 
> > > but I won't be
> > > surprised if there are firmware files out in the wild with different headers
> > > or the hash missing, otherwise the else clause wouldn't exist.
> > > This else clause causes a read starting at fw->data + phdrs[1].p_offset of
> > > phdrs[1].p_filesz bytes which is almost certainly out of bounds if the
> > > program header type is PT_LOAD instead of PT_NONE, otherwise it wouldn't
> > > need to be loaded from a .bXX file in the first place.
> > 
> > So the else clause is there to handle non-split firmware, which has
> > everything within fw->size.
> 
> 
> Is it too much to ask for extra validation here, instead of always assuming
> either "split" or "non-split" firmware?  As mentioned before these program
> headers allow for a variety of configurations, which is confirmed by
> Siddharth's first patch looking for QCOM_MDT_TYPE_HASH in all headers
> instead of assuming it to reside in the second.
> 
> > > 
> > > 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.

> 
> > > 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.

Shawn