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