Hi Shawn,
On 8/26/21 4:18 PM, Shawn Guo wrote:
Hi Marijn,
Thanks much for the information!
On Tue, Aug 24, 2021 at 05:18:05PM +0200, Marijn Suijten wrote:
Hi Shawn,
On 8/24/21 11:41 AM, Shawn Guo wrote:
From: Shawn Guo <shawn.guo@xxxxxxxxxx>
It's been observed on Sony Xperia M4 Aqua phone, that wcnss firmware has
the type of the second segment holding hashes just be PT_LOAD. So drop
the check on phdrs[1].p_type to get it go on that phone.
Signed-off-by: Shawn Guo <shawn.guo@xxxxxxxxxx>
---
drivers/soc/qcom/mdt_loader.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index eba7f76f9d61..6034cd8992b0 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -98,7 +98,7 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
if (ehdr->e_phnum < 2)
return ERR_PTR(-EINVAL);
- if (phdrs[0].p_type == PT_LOAD || phdrs[1].p_type == PT_LOAD)
+ if (phdrs[0].p_type == PT_LOAD)
return ERR_PTR(-EINVAL);
if ((phdrs[1].p_flags & QCOM_MDT_TYPE_MASK) != QCOM_MDT_TYPE_HASH)
Konrad (on the CC-list) originally came up with a similar patch to make his
Sony phone boot (Xperia XZ, MSM8996). We however concluded that this
solution is wrong, for the simple fact that the code below expects a PT_NULL
header with data in the right place. Skipping this check most likely means
that the code below will read out of bounds since the mdt file isn't large
enough; this data is supposed to be read from an external file as indicated
by the meaning of PT_LOAD. We built a solution for this, and fortunately
CAF independently submitted a similar solution to the lists a while ago
which is more thorough:
https://lore.kernel.org/linux-arm-msm/1609968211-7579-1-git-send-email-sidgup@xxxxxxxxxxxxxx/
While a thorough solution is good, I do not think my patch makes
anything worse or breaks anything, while fixing my issue on Sony Xperia
M4 Aqua. All the current assumptions hold true for my WCNSS firmware,
only except that phdrs[1] gets a PT_LOAD type.
It's not up to me to choose between a thorough or quick solution, 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.
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?
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`? 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)?
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)
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.
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.
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.
Shawn
[1] http://bits-please.blogspot.com/2016/04/exploring-qualcomms-secure-execution.html
- Marijn
