Re: [PATCH v4 1/1] soc: qcom: mdt_loader: Enhance split binary detection

Gokul Krishna Krishnakumar
2023-04-06T17:52:41Z

On 4/4/2023 12:05 PM, Bjorn Andersson wrote:
> On Tue, Apr 04, 2023 at 10:54:17AM -0700, Gokul krishna Krishnakumar wrote:
>> It may be that the offset of the first program header lies inside the mdt's
>> filesize, in this case the loader would incorrectly assume that the bins
>> were not split. The loading would then continue on to fail for split bins.
>
> What does "continue on to fail for split bins" actually mean? In what
> way does it fail?
> Authentication fails in the firmware for the split bins because the
current conditional check do not go through the entire mdt file for a
given firmware.

Thanks,
Gokul
>> This change updates the logic used by the mdt loader to understand whether
>> the firmware images are split or not. It figures this out by checking if
>> each programs header's segment lies within the file or not.
>>
>> Signed-off-by: Melody Olvera <quic_molvera@quicinc.com>
>> Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
>
> The first S-o-b should be that of the author (unless Co-developed-by)
> and in [1] you're the author and Melody provide her S-o-b to certify
> the origin of her contribution.
>
>> ---
>> V4 addresses the comments from V3.
>
> That's nice, but not very helpful. Please spell out what you changed.
>

>>
>> V3 is separated out from [1] and includes
>> changes addressing comments from that patch set.
>>
>> [1] https://lore.kernel.org/all/20230306231202.12223-5-quic_molvera@quicinc.com/
>> ---
>> drivers/soc/qcom/mdt_loader.c | 27 +++++++++++++++++++++++++--
>> 1 file changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
>> index 33dd8c315eb7..9270992728d4 100644
>> --- a/drivers/soc/qcom/mdt_loader.c
>> +++ b/drivers/soc/qcom/mdt_loader.c
>> @@ -258,6 +258,26 @@ int qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
>> }
>> EXPORT_SYMBOL_GPL(qcom_mdt_pas_init);
>>
>> +static bool qcom_mdt_bins_are_split(const struct firmware *fw, const char* fw_name)
>> +{
>> + const struct elf32_phdr *phdrs;
>> + const struct elf32_hdr *ehdr;
>> + uint64_t seg_start, seg_end;
>> + int i;
>> +
>> + ehdr = (struct elf32_hdr *)fw->data;
>> + phdrs = (struct elf32_phdr *)(ehdr + 1);
>> +
>> + for (i = 0; i < ehdr->e_phnum; i++) {
>> + seg_start = phdrs[i].p_offset;
>> + seg_end = phdrs[i].p_offset + phdrs[i].p_filesz;
>> + if (seg_start > fw->size || seg_end > fw->size)
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
>> const char *fw_name, int pas_id, void *mem_region,
>> phys_addr_t mem_phys, size_t mem_size,
>> @@ -270,6 +290,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
>> phys_addr_t min_addr = PHYS_ADDR_MAX;
>> ssize_t offset;
>> bool relocate = false;
>> + bool is_split;
>> void *ptr;
>> int ret = 0;
>> int i;
>> @@ -277,6 +298,8 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
>> if (!fw || !mem_region || !mem_phys || !mem_size)
>> return -EINVAL;
>>
>> +
>
> Double empty lines here?
>
>> + is_split = qcom_mdt_bins_are_split(fw, fw_name);
>> ehdr = (struct elf32_hdr *)fw->data;
>> phdrs = (struct elf32_phdr *)(ehdr + 1);
>>
>> @@ -330,8 +353,8 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
>>
>> ptr = mem_region + offset;
>>
>> - if (phdr->p_filesz && phdr->p_offset < fw->size &&
>> - phdr->p_offset + phdr->p_filesz <= fw->size) {
>> +
>> + if (phdr->p_filesz && !is_split) {
>
> This looks much better now, thanks.
>
> Regards,
> Bjorn
>
>> /* Firmware is large enough to be non-split */
>> if (phdr->p_offset + phdr->p_filesz > fw->size) {
>> dev_err(dev, "file %s segment %d would be truncated\n",
>> --
>> 2.39.2
>>