| Message ID | 20230306231202.12223-5-quic_molvera@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | remoteproc: qcom_q6v5_pas: Add support for QDU1000/QRU1000 mpss | expand |
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 <quic_gokukris@quicinc.com> >> >> 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 <quic_gokukris@quicinc.com> >> Signed-off-by: Melody Olvera <quic_molvera@quicinc.com> >> --- >> 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
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); - if (ehdr_size + hash_size == fw->size) { - /* 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) { - /* 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 { - /* Hash is in its own segment, beyond the loaded file */ - ret = mdt_load_split_segment(data + ehdr_size, phdrs, hash_segment, fw_name, dev); - 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 (!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)