On Wed 06 Jan 15:23 CST 2021, Siddharth Gupta 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.
> 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 program header's segment lies within the file or not.
>
> Signed-off-by: Siddharth Gupta <sidgup@xxxxxxxxxxxxxx>
> ---
> drivers/soc/qcom/mdt_loader.c | 60 +++++++++++++++++++++++++++----------------
> 1 file changed, 38 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> index 813216d..c9bbd8c 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;
> +}
> +
> /**
> * qcom_mdt_get_size() - acquire size of the memory region needed to load mdt
> * @fw: firmware object for the mdt file
> @@ -118,7 +138,7 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
> return ERR_PTR(-ENOMEM);
>
> /* Is the header and hash already packed */
> - if (ehdr_size + hash_size == fw->size)
> + if (qcom_mdt_bins_are_split(fw))
> hash_offset = phdrs[0].p_filesz;
> else
> hash_offset = phdrs[hash_index].p_offset;
> @@ -150,6 +170,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
> void *metadata;
> char *fw_name;
> bool relocate = false;
> + bool is_split;
> void *ptr;
> int ret = 0;
> int i;
> @@ -157,6 +178,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);
>
> @@ -238,28 +260,22 @@ 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) {
> - /* Firmware is large enough to be non-split */
> - if (phdr->p_offset + phdr->p_filesz > fw->size) {
> - dev_err(dev,
> - "failed to load segment %d from truncated file %s\n",
> - i, firmware);
> - ret = -EINVAL;
> - break;
> + if (phdr->p_filesz) {
> + if (!is_split) {
In an effort to reduce the diff size and avoid adding another level of
indentation, how about making the conditionals:
if (is_split && phdr->p_filesz) {
memcpy();
} else if (phdr->p_filesz) {
...
}
Apart from that I think this patch looks good!
Regards,
Bjorn
> + /* 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 */
> + snprintf(fw_name + fw_name_len - 3, 4, "b%02d", i);
> + ret = request_firmware_into_buf(&seg_fw, fw_name, dev,
> + ptr, phdr->p_filesz);
> + if (ret) {
> + dev_err(dev, "failed to load %s\n", fw_name);
> + break;
> + }
> +
> + release_firmware(seg_fw);
> }
> -
> - memcpy(ptr, fw->data + phdr->p_offset, phdr->p_filesz);
> - } else if (phdr->p_filesz) {
> - /* Firmware not large enough, load split-out segments */
> - sprintf(fw_name + fw_name_len - 3, "b%02d", i);
> - ret = request_firmware_into_buf(&seg_fw, fw_name, dev,
> - ptr, phdr->p_filesz);
> - if (ret) {
> - dev_err(dev, "failed to load %s\n", fw_name);
> - break;
> - }
> -
> - release_firmware(seg_fw);
> }
>
> if (phdr->p_memsz > phdr->p_filesz)
> --
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
