Re: [PATCH v4 1/1] soc: qcom: mdt_loader: Enhance split binary detection

[Gokul Krishna Krishnakumar]

On 4/4/2023 12:05 PM, Bjorn Andersson wrote:
> On Tue, Apr 04, 2023 at 10:54:17AM -0700, Gokul krishna Krishnakumar 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.
>
> What does "continue on to fail for split bins" actually mean? In what
> way does it fail?
> Authentication fails in the firmware for the split bins because the 
current conditional check do not go through the entire mdt file for a 
given firmware.

Thanks,
Gokul
> > 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 programs header's segment lies within the file or not.
> >
> > Signed-off-by: Melody Olvera <quic_molvera@xxxxxxxxxxx>
> > Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@xxxxxxxxxxx>
>
> The first S-o-b should be that of the author (unless Co-developed-by)
> and in [1] you're the author and Melody provide her S-o-b to certify
> the origin of her contribution.
>
> > ---
> > V4 addresses the comments from V3.
>
> That's nice, but not very helpful. Please spell out what you changed.

> > V3 is separated out from [1] and includes
> > changes addressing comments from that patch set.
> >
> > [1] https://lore.kernel.org/all/20230306231202.12223-5-quic_molvera@xxxxxxxxxxx/
> > ---
> >   drivers/soc/qcom/mdt_loader.c | 27 +++++++++++++++++++++++++--
> >   1 file changed, 25 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> > index 33dd8c315eb7..9270992728d4 100644
> > --- a/drivers/soc/qcom/mdt_loader.c
> > +++ b/drivers/soc/qcom/mdt_loader.c
> > @@ -258,6 +258,26 @@ int qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
> >   }
> >   EXPORT_SYMBOL_GPL(qcom_mdt_pas_init);
> >   
> > +static bool qcom_mdt_bins_are_split(const struct firmware *fw, const char* fw_name)
> > +{
> > +	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 int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
> >   			   const char *fw_name, int pas_id, void *mem_region,
> >   			   phys_addr_t mem_phys, size_t mem_size,
> > @@ -270,6 +290,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 +298,8 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
> >   	if (!fw || !mem_region || !mem_phys || !mem_size)
> >   		return -EINVAL;
> >   
> > +
>
> Double empty lines here?
>
> > +	is_split = qcom_mdt_bins_are_split(fw, fw_name);
> >   	ehdr = (struct elf32_hdr *)fw->data;
> >   	phdrs = (struct elf32_phdr *)(ehdr + 1);
> >   
> > @@ -330,8 +353,8 @@ 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) {
> > +
> > +		if (phdr->p_filesz && !is_split) {
>
> This looks much better now, thanks.
>
> Regards,
> Bjorn
>
> >   			/* 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",
> > --
> > 2.39.2