Date:   Tue, 21 Mar 2023 10:42:09 -0700
From:   Gokul Krishna Krishnakumar <quic_gokukris@...cinc.com>
To:     Bjorn Andersson <andersson@...nel.org>,
        Melody Olvera <quic_molvera@...cinc.com>
CC:     Andy Gross <agross@...nel.org>, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Jassi Brar <jassisinghbrar@...il.com>,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        Robert Marko <robimarko@...il.com>,
        Guru Das Srinagesh <quic_gurus@...cinc.com>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Manivannan Sadhasivam <mani@...nel.org>,
        <linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-remoteproc@...r.kernel.org>
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 <quic_gokukris@...cinc.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@...cinc.com>
>> Signed-off-by: Melody Olvera <quic_molvera@...cinc.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

Powered by blists - more mailing lists