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;
Message-ID: <c1cac217-e655-916b-2889-4f389f73c6f4@quicinc.com>
Date:   Thu, 6 Apr 2023 10:52:30 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.1
Subject: Re: [PATCH v4 1/1] soc: qcom: mdt_loader: Enhance split binary
 detection
Content-Language: en-US
To:     Bjorn Andersson <andersson@kernel.org>
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>
References: <20230404175417.31185-1-quic_gokukris@quicinc.com>
 <20230404190510.dvsmkroyvu6437rb@ripper>
From:   Gokul Krishna Krishnakumar <quic_gokukris@quicinc.com>
In-Reply-To: <20230404190510.dvsmkroyvu6437rb@ripper>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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@quicinc.com>
>> Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
> 
> 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@quicinc.com/
>> ---
>>   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
>>