| 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
| ||
|
Message-ID: <5efad446-cf46-02b3-461d-a67e12a4b1c0@codeaurora.org>
Date: Sun, 17 Jan 2021 15:03:22 -0800
From: Siddharth Gupta <sidgup@...eaurora.org>
To: Bjorn Andersson <bjorn.andersson@...aro.org>
Cc: agross@...nel.org, ohad@...ery.com, linux-arm-msm@...r.kernel.org,
linux-remoteproc@...r.kernel.org, linux-kernel@...r.kernel.org,
psodagud@...eaurora.org, rishabhb@...eaurora.org
Subject: Re: [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob
On 1/14/2021 9:46 AM, Bjorn Andersson wrote:
> On Wed 13 Jan 17:01 CST 2021, Siddharth Gupta wrote:
>
>> On 1/7/2021 4:21 PM, Bjorn Andersson wrote:
>>> On Wed 06 Jan 15:23 CST 2021, Siddharth Gupta wrote:
>>>
>>>> Since the split elf blobs will always contain the hash segment, we rely on
>>> I think it will sounds better if we add "should" in "we should rely on..."
>> Sure
>>>> the blob file to get the hash rather than assume that it will be present in
>>>> the mdt file. This change uses the hash index to read the appropriate elf
>>>> blob to get the hash segment.
>>>>
>>>> Signed-off-by: Siddharth Gupta<sidgup@...eaurora.org>
>>>> ---
>>>> drivers/remoteproc/qcom_q6v5_mss.c | 4 ++--
>>>> drivers/soc/qcom/mdt_loader.c | 38 +++++++++++++++++++++++++++----------
>>>> include/linux/soc/qcom/mdt_loader.h | 3 ++-
>>>> 3 files changed, 32 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
>>>> index 66106ba..74c0229 100644
>>>> --- a/drivers/remoteproc/qcom_q6v5_mss.c
>>>> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
>>>> @@ -4,7 +4,7 @@
>>>> *
>>>> * Copyright (C) 2016 Linaro Ltd.
>>>> * Copyright (C) 2014 Sony Mobile Communications AB
>>>> - * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
>>>> + * Copyright (c) 2012-2013, 2020 The Linux Foundation. All rights reserved.
>>>> */
>>>> #include <linux/clk.h>
>>>> @@ -828,7 +828,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>>>> void *ptr;
>>>> int ret;
>>>> - metadata = qcom_mdt_read_metadata(fw, &size);
>>>> + metadata = qcom_mdt_read_metadata(qproc->dev, fw, qproc->hexagon_mdt_image, &size);
>>>> if (IS_ERR(metadata))
>>>> return PTR_ERR(metadata);
>>>> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
>>>> index c9bbd8c..6876c0b 100644
>>>> --- a/drivers/soc/qcom/mdt_loader.c
>>>> +++ b/drivers/soc/qcom/mdt_loader.c
>>>> @@ -103,15 +103,18 @@ EXPORT_SYMBOL_GPL(qcom_mdt_get_size);
>>>> *
>>>> * Return: pointer to data, or ERR_PTR()
>>>> */
>>>> -void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
>>>> +void *qcom_mdt_read_metadata(struct device *dev, const struct firmware *fw, const char *firmware,
>>>> + size_t *data_len)
>>>> {
>>>> const struct elf32_phdr *phdrs;
>>>> const struct elf32_hdr *ehdr;
>>>> - size_t hash_offset;
>>>> + const struct firmware *seg_fw;
>>>> size_t hash_index;
>>>> size_t hash_size;
>>>> size_t ehdr_size;
>>>> + char *fw_name;
>>>> void *data;
>>>> + int ret;
>>>> ehdr = (struct elf32_hdr *)fw->data;
>>>> phdrs = (struct elf32_phdr *)(ehdr + 1);
>>>> @@ -137,14 +140,29 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
>>>> if (!data)
>>>> return ERR_PTR(-ENOMEM);
>>>> - /* Is the header and hash already packed */
>>>> - if (qcom_mdt_bins_are_split(fw))
>>>> - hash_offset = phdrs[0].p_filesz;
>>>> - else
>>>> - hash_offset = phdrs[hash_index].p_offset;
>>>> -
>>>> + /* copy elf header */
>>>> memcpy(data, fw->data, ehdr_size);
>>>> - memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
>>>> +
>>> This seems to duplicates parts of the loop in __qcom_mdt_load(), how
>>> about breaking this out to a separate
>>>
>>> static int mdt_load_segment(struct device *dev, const struct firmware *fw,
>>> int idx, void *buf, size_t len, bool is_split)
>>>
>>> Which either just memcpy from @fw or does the filename and loading
>>> dance, based on @is_split?
>> Since mdt_load_segment won't know the name of the firmware without a global
>> variable
>> (which in turn will make it non-reentrant), the idea of creating such a
>> function and not passing
>> the actual name of the firmware seemed wrong.
>>
> Wouldn't you be able to pass "firmware" as an argument to the
> load_segment function?
We can, which is what I guess you meant in the first place. What I thought
was that if we are creating something like mdt_load_segments passing
"firmware.mdt" with the index instead of "firmware.b<index>" seemed
kind of wrong, but I guess that is just a matter of the naming convention.
>> If we want to pass the firmware name in this function the code size will be
>> more or equal to
>> what we started with. If that is not a problem I can make the changes.
>>
> Perhaps I'm missing something here, I do expect that you would end with
> code similar to the hunk you add here. But in doing so we should be able
> to reuse that in the __qcom_mdt_load(). Or am I too optimistic?
>
> (In particular I'm not fond of the fw_name dance and doing it twice is
> worse)
If I am creating the firmware name inside this function then we should
definitely
see a code reduction. I'll make the changes and push the new patchset soon.
Thanks,
Sid
>
> Regards,
> Bjorn
>
>> Thanks,
>> Sid
>>> Regards,
>>> Bjorn
>>>
>>>> + if (qcom_mdt_bins_are_split(fw)) {
>>>> + fw_name = kstrdup(firmware, GFP_KERNEL);
>>>> + if (!fw_name) {
>>>> + kfree(data);
>>>> + return ERR_PTR(-ENOMEM);
>>>> + }
>>>> + snprintf(fw_name + strlen(fw_name) - 3, 4, "b%02d", hash_index);
>>>> +
>>>> + ret = request_firmware_into_buf(&seg_fw, fw_name, dev, data + ehdr_size, hash_size);
>>>> + kfree(fw_name);
>>>> +
>>>> + if (ret) {
>>>> + kfree(data);
>>>> + return ERR_PTR(ret);
>>>> + }
>>>> +
>>>> + release_firmware(seg_fw);
>>>> + } else {
>>>> + memcpy(data + ehdr_size, fw->data + phdrs[hash_index].p_offset, hash_size);
>>>> + }
>>>> *data_len = ehdr_size + hash_size;
>>>> @@ -191,7 +209,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
>>>> return -ENOMEM;
>>>> if (pas_init) {
>>>> - metadata = qcom_mdt_read_metadata(fw, &metadata_len);
>>>> + metadata = qcom_mdt_read_metadata(dev, fw, firmware, &metadata_len);
>>>> if (IS_ERR(metadata)) {
>>>> ret = PTR_ERR(metadata);
>>>> goto out;
>>>> diff --git a/include/linux/soc/qcom/mdt_loader.h b/include/linux/soc/qcom/mdt_loader.h
>>>> index e600bae..04ba5e8 100644
>>>> --- a/include/linux/soc/qcom/mdt_loader.h
>>>> +++ b/include/linux/soc/qcom/mdt_loader.h
>>>> @@ -21,6 +21,7 @@ int qcom_mdt_load_no_init(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,
>>>> phys_addr_t *reloc_base);
>>>> -void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len);
>>>> +void *qcom_mdt_read_metadata(struct device *dev, const struct firmware *fw, const char *firmware,
>>>> + size_t *data_len);
>>>> #endif
>>>> --
>>>> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>>>> a Linux Foundation Collaborative Project
>>>>
Powered by blists - more mailing lists