| 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, 14 Jan 2021 11:46:10 -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 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?
> 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)
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