linux-kernel - Re: [PATCH v6 1/1] soc: qcom: mdt_loader: Enhance split binary detection

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <07beafde-19b6-2c25-6451-4c5871cb0012@quicinc.com>
Date:   Tue, 23 May 2023 10:28:54 -0700
From:   Gokul Krishna Krishnakumar <quic_gokukris@...cinc.com>
To:     <linux-arm-msm@...r.kernel.org>
CC:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
        Trilok Soni <quic_tsoni@...cinc.com>,
        "Satya Durga Srinivasu Prabhala" <quic_satyap@...cinc.com>,
        Rajendra Nayak <quic_rjendra@...cinc.com>,
        Elliot Berman <quic_eberman@...cinc.com>,
        "Guru Das Srinagesh" <quic_gurus@...cinc.com>,
        Sibi Sankar <quic_sibis@...cinc.com>,
        Melody Olvera <quic_molvera@...cinc.com>
Subject: Re: [PATCH v6 1/1] soc: qcom: mdt_loader: Enhance split binary
 detection



On 5/8/2023 5:18 PM, 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 and in this scenario the firmware authentication fails.
> 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.
> 
> Co-developed-by: Melody Olvera <quic_molvera@...cinc.com>
> Signed-off-by: Melody Olvera <quic_molvera@...cinc.com>
> Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@...cinc.com>
> 
> ---
> V6: Fixed format error in qcom_mdt_bins_are_split function definition and
> Correcting the s-o-b by keeping Melody as the co-developer.
> 
> V5: Removes extra empty lines from V4 and fixed the S-o-b by keeping Melody's
> name first.
> 
> V4: Change the commit text to include the scenario in which we see the problem.
> 
> V3: separated out from [1] and includes changes addressing comments
> from that patch set:
> 	1. Change the checking condition for non-split firmwares to
> 	(phr->p_filesz && !issplit) on line #352 for better readability.
> 	2. Removes an unncecessary check for split bins in qcom_mdt_read_metadata()/
> 
> [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 3f11554df2f3..0e35d29b4438 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",
Hi reviewers,
Could you please review this patch.

Thanks,
Gokul