| 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
| ||
|
Date: Thu, 7 Jan 2021 18:21:20 -0600
From: Bjorn Andersson <bjorn.andersson@...aro.org>
To: Siddharth Gupta <sidgup@...eaurora.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 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..."
> 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?
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