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

[Gokul Krishna Krishnakumar]

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@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > Signed-off-by: Melody Olvera <quic_molvera@xxxxxxxxxxx>
> > ---
> >   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