by Gokul krishna Krishnakumar

[permalink] [raw]

Subject: Re: [PATCH v2 4/7] soc: qcom: mdt_loader: Enhance split binary detection

Thanks Bjorn for the review comments.
On 3/15/2023 7:12 PM, Bjorn Andersson wrote:
> On Mon, Mar 06, 2023 at 03:11:59PM -0800, Melody Olvera wrote:
>> From: Gokul Krishna Krishnakumar <[email protected]>
>>
>> When booting with split binaries, 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 and fail for split bins.
>
> Can you please be more explicit about the scenario you're having
> problems with?
>
In this scenario below, the first pheader end address is < mdt file size
but the next pheader lies outside the mdt file size. The
_qcom_mdt_load() would continue on to load the firmware as a single
image which leads to load error. By checking if all the pheaders lie
inside the file size, we will be able to fix this issue.

fw = cdsp_dtb.mdt phdr->p_filesz = 148 phdr->p_offset 0 fw->size 4044
fw = cdsp_dtb.mdt phdr->p_filesz = 512 phdr->p_offset 148 fw->size 4044
fw = cdsp_dtb.mdt phdr->p_filesz = 3896 phdr->p_offset 4096 fw->size 4044

> Is the problem that the first segment is represented in both the .mdt
> and .b01, but different? Or is it that you find the hash in both .mdt
> abd .b01, but only one of them is valid?
>
>> This change updates the logic used
>> by the mdt loader to understand whether the firmware images are split or not
>> by checking if each programs header's segment lies within the file or not.
>>
>> Signed-off-by: Gokul Krishna Krishnakumar <[email protected]>
>> Signed-off-by: Melody Olvera <[email protected]>
>> ---
>> drivers/soc/qcom/mdt_loader.c | 64 +++++++++++++++++++----------------
>> 1 file changed, 35 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
>> index 33dd8c315eb7..3aadce299c02 100644
>> --- a/drivers/soc/qcom/mdt_loader.c
>> +++ b/drivers/soc/qcom/mdt_loader.c
>> @@ -31,6 +31,26 @@ static bool mdt_phdr_valid(const struct elf32_phdr *phdr)
>> return true;
>> }
>>
>> +static bool qcom_mdt_bins_are_split(const struct firmware *fw)
>> +{
>> + 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 ssize_t mdt_load_split_segment(void *ptr, const struct elf32_phdr *phdrs,
>> unsigned int segment, const char *fw_name,
>> struct device *dev)
>> @@ -167,23 +187,13 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len,
>> /* Copy ELF header */
>> memcpy(data, fw->data, ehdr_size);
>>
>
> The existing code handles 3 cases:
>
>> - if (ehdr_size + hash_size == fw->size) {
>
> 1) File is split, but hash resides with the ELF header in the .mdt
>
>> - /* Firmware is split and hash is packed following the ELF header */
>> - hash_offset = phdrs[0].p_filesz;
>> - memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
>> - } else if (phdrs[hash_segment].p_offset + hash_size <= fw->size) {
>
> 2) The hash segment exists in a segment of its own, but in the loaded
> image.
>
>> - /* Hash is in its own segment, but within the loaded file */
>> +
>> + if (qcom_mdt_bins_are_split(fw)) {
>> + ret = mdt_load_split_segment(data + ehdr_size, phdrs, hash_segment, fw_name, dev);
>> + } else {
>> hash_offset = phdrs[hash_segment].p_offset;
>> memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
>> - } else {
>
> 3) The image is split, and the hash segment resides in it's own file.
>
>
> Afaict the updated logic maintains #2 and #3, but drops #1. Please
> review the git history to see if you can determine which target this
> case exists with - and ask for someone to specifically verify your
> change there.
>
> Perhaps all your change is doing is removing case #1, in which case this
> should be clear in the commit message; and we need to validate that your
> new assumptions holds.
>
>> - /* Hash is in its own segment, beyond the loaded file */
>> - ret = mdt_load_split_segment(data + ehdr_size, phdrs, hash_segment, fw_name, dev);
>
> For some reason you reversed the condition and got this out of the else
> (seems like an unnecessary change)...but in the process you lost the
> error handling below.
>
Yes, Updating the patch to remove this unnecessary change in the
qcom_mdt_read_metadat().
>> - if (ret) {
>> - kfree(data);
>> - return ERR_PTR(ret);
>> - }
>> }
>> -
>> *data_len = ehdr_size + hash_size;
>>
>> return data;
>> @@ -270,6 +280,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 +288,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
>> if (!fw || !mem_region || !mem_phys || !mem_size)
>> return -EINVAL;
>>
>> + is_split = qcom_mdt_bins_are_split(fw);
>> ehdr = (struct elf32_hdr *)fw->data;
>> phdrs = (struct elf32_phdr *)(ehdr + 1);
>>
>> @@ -330,22 +342,16 @@ 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) {
>> - /* 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",
>> - fw_name, i);
>> - ret = -EINVAL;
>> - break;
>> + if (phdr->p_filesz) {
>
> If you just change the condition (phr->p_filesz && !issplit), then your
> patch becomes easier to read.
>
Done.
V3: only make change in the __qcom_mdt_load() and not in the
qcom_mdt_read_metadata().
> Regards,
> Bjorn
>
>> + if (!is_split) {
>> + /* Firmware is large enough to be non-split */
>> + memcpy(ptr, fw->data + phdr->p_offset, phdr->p_filesz);
>> + } else {
>> + /* Firmware not large enough, load split-out segments */
>> + ret = mdt_load_split_segment(ptr, phdrs, i, fw_name, dev);
>> + if (ret)
>> + break;
>> }
>> -
>> - memcpy(ptr, fw->data + phdr->p_offset, phdr->p_filesz);
>> - } else if (phdr->p_filesz) {
>> - /* Firmware not large enough, load split-out segments */
>> - ret = mdt_load_split_segment(ptr, phdrs, i, fw_name, dev);
>> - if (ret)
>> - break;
>> }
>>
>> if (phdr->p_memsz > phdr->p_filesz)
>> --
>> 2.25.1
>>
Thanks,
Gokul