Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 10 Apr 2023 14:52:23 -0700
From:   Bjorn Andersson <andersson@kernel.org>
To:     Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
Cc:     linux-arm-msm@vger.kernel.org, Andy Gross <agross@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        Trilok Soni <quic_tsoni@quicinc.com>,
        Satya Durga Srinivasu Prabhala <quic_satyap@quicinc.com>,
        Rajendra Nayak <quic_rjendra@quicinc.com>,
        Elliot Berman <quic_eberman@quicinc.com>,
        Guru Das Srinagesh <quic_gurus@quicinc.com>,
        Sibi Sankar <quic_sibis@quicinc.com>,
        Melody Olvera <quic_molvera@quicinc.com>
Subject: Re: [PATCH v5 1/1] soc: qcom: mdt_loader: Enhance split binary
 detection
Message-ID: <20230410215223.3xcctjsq5qqbqvs2@ripper>
References: <20230410171432.19046-1-quic_gokukris@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230410171432.19046-1-quic_gokukris@quicinc.com>
Precedence: bulk

On Mon, Apr 10, 2023 at 10:14:32AM -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.
> 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: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
> Signed-off-by: Melody Olvera <quic_molvera@quicinc.com>

Please read Documentation/process/submitting-patches.rst section about
Developer's Certificate of Origin 1.1, then apply that for each person
in the S-o-b chain from the top. Finally consider how Melody's name can
be the last one in this list, while the patch is coming from you.

> ---
> V5: Removes extra empty lines from V4.

That's nice, but what about the other changes I asked for?

Regards,
Bjorn

> 
> V4: Removes the unneceessary change in qcom_mdt_read_metadata(), the
> exisiting check holds good in case the hash segment is in the loaded image.
> 
> 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@quicinc.com/
> ---
>  drivers/soc/qcom/mdt_loader.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> index 33dd8c315eb7..814646ce41e5 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,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, fw_name);
>  	ehdr = (struct elf32_hdr *)fw->data;
>  	phdrs = (struct elf32_phdr *)(ehdr + 1);
>  
> @@ -330,8 +352,7 @@ 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) {
>  			/* 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
>