| 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: <20250822165238.ayslhgrjrbipojjk@hu-mojha-hyd.qualcomm.com>
Date: Fri, 22 Aug 2025 22:22:38 +0530
From: Mukesh Ojha <mukesh.ojha@....qualcomm.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
Cc: Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>,
Vikash Garodia <quic_vgarodia@...cinc.com>,
Dikshita Agarwal <quic_dikshita@...cinc.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Mathieu Poirier <mathieu.poirier@...aro.org>,
Abhinav Kumar <abhinav.kumar@...ux.dev>, linux-kernel@...r.kernel.org,
linux-arm-msm@...r.kernel.org, linux-media@...r.kernel.org,
linux-remoteproc@...r.kernel.org
Subject: Re: [PATCH v2 05/11] firmware: qcom_scm: Add shmbridge support to
pas_init/release function
On Thu, Aug 21, 2025 at 10:33:37PM +0530, Mukesh Ojha wrote:
> On Thu, Aug 21, 2025 at 04:23:53PM +0100, Bryan O'Donoghue wrote:
> > On 19/08/2025 17:54, Mukesh Ojha wrote:
> > > Qualcomm SoCs running with QHEE (Qualcomm Hypervisor Execution
> > > Environment—a library present in the Gunyah hypervisor) utilize the
> > > Peripheral Authentication Service (PAS) from Qualcomm TrustZone (TZ)
> > > also called QTEE(Qualcomm Trusted Execution Environment firmware)
> > > to securely authenticate and reset remote processors via a sequence
> > > of SMC calls such as qcom_scm_pas_init_image(), qcom_scm_pas_mem_setup(),
> > > and qcom_scm_pas_auth_and_reset().
> > >
> > > For memory passed to Qualcomm TrustZone, it must either be part of a
> > > pool registered with TZ or be directly registered via SHMbridge SMC
> > > calls.
> > >
> > > When QHEE is present, PAS SMC calls from Linux running at EL1 are
> > > trapped by QHEE (running at EL2), which then creates or retrieves memory
> > > from the SHM bridge for both metadata and remoteproc carveout memory
> > > before passing them to TZ. However, when the SoC runs with a
> > > non-QHEE-based hypervisor, Linux must create the SHM bridge for both
> > > metadata (before it is passed to TZ in qcom_scm_pas_init_image()) and
> > > for remoteproc memory (before the call is made to TZ in
> > > qcom_scm_pas_auth_and_reset()).
> > >
> > > For the qcom_scm_pas_init_image() call, metadata content must be copied
> > > to a buffer allocated from the SHM bridge before making the SMC call.
> > > This buffer should be freed either immediately after the call or during
> > > the qcom_scm_pas_metadata_release() function, depending on the context
> > > parameter passed to qcom_scm_pas_init_image(). Convert the metadata
> > > context parameter to use PAS context data structure so that it will also
> > > be possible to decide whether to get memory from SHMbridge pool or not.
> > >
> > > When QHEE is present, it manages the IOMMU translation context so, in
> > > absence of it device driver will be aware through device tree that its
> > > translation context is managed by Linux and it need to create SHMbridge
> > > before passing any buffer to TZ, So, remote processor driver should
> > > appropriately set ctx->has_iommu to let PAS SMC function to take care of
> > > everything ready for the call to work.
> > >
> > > Lets convert qcom_scm_pas_init_image() and qcom_scm_pas_metadata_release()
> > > to have these awareness.
> >
> > I like the effort in the commit log here but its also a bit too long.
> >
> > Please go through these paragraphs and try to reduce down the amount of text
> > you are generating.
>
> I was writing to set context for each commit and for the record and hence, the
> repetition of text you would see in some of the lines used.
>
> I will take your suggestion and reduce it.
>
> >
> > >
> > > Signed-off-by: Mukesh Ojha <mukesh.ojha@....qualcomm.com>
> > > ---
> > > drivers/firmware/qcom/qcom_scm.c | 71 +++++++++++++++++++++-----
> > > drivers/remoteproc/qcom_q6v5_pas.c | 14 ++---
> > > drivers/soc/qcom/mdt_loader.c | 4 +-
> > > include/linux/firmware/qcom/qcom_scm.h | 9 ++--
> > > 4 files changed, 73 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> > > index 7827699e277c..301d440f62f3 100644
> > > --- a/drivers/firmware/qcom/qcom_scm.c
> > > +++ b/drivers/firmware/qcom/qcom_scm.c
> > > @@ -616,6 +616,35 @@ static int __qcom_scm_pas_init_image(u32 peripheral, dma_addr_t mdata_phys,
> > > return ret;
> > > }
> > > +static int qcom_scm_pas_prep_and_init_image(struct qcom_scm_pas_ctx *ctx,
> > > + const void *metadata, size_t size)
> > > +{
> > > + struct qcom_scm_pas_metadata *mdt_ctx;
> > > + struct qcom_scm_res res;
> > > + phys_addr_t mdata_phys;
> > > + void *mdata_buf;
> > > + int ret;
> > > +
> > > + mdt_ctx = ctx->metadata;
> > > + mdata_buf = qcom_tzmem_alloc(__scm->mempool, size, GFP_KERNEL);
> > > + if (!mdata_buf)
> > > + return -ENOMEM;
> > > +
> > > + memcpy(mdata_buf, metadata, size);
> > > + mdata_phys = qcom_tzmem_to_phys(mdata_buf);
> > > +
> > > + ret = __qcom_scm_pas_init_image(ctx->peripheral, mdata_phys, mdata_buf, size, &res);
> > > + if (ret < 0 || !mdt_ctx) {
> >
> > if ret is an error or mdt_ctx is null free the memory
> >
> > > + qcom_tzmem_free(mdata_buf);
> > > + } else if (mdt_ctx) {
> >
> > if mdt_ctx is valid do this
>
> Nothing change, it is similar to the earlier code.
>
> >
> > > + mdt_ctx->ptr = mdata_buf;
> > > + mdt_ctx->addr.phys_addr = mdata_phys;
> > > + mdt_ctx->size = size;
> > > + }
> > > +
> > > + return ret ? : res.result[0];
> >
> > so we can have ctx_mtd valid but return the value at ret but also mtd valid
> > and return the res.result[0]
> >
> > That seems like an odd choice - surely if you are enumerating the
> > data-structure the result code we care about is res.result[0] instead of ret
> > ?
> >
> > OK I see this return logic comes from below..
> >
> > But
> >
> > drivers/soc/qcom/mdt_loader.c::qcom_mdt_pas_init
> >
> > ret = qcom_scm_pas_init_image(pas_id, metadata, metadata_len, ctx);
> > kfree(metadata);
> > if (ret) {
> > /* Invalid firmware metadata */
> > dev_err(dev, "error %d initializing firmware %s\n", ret, fw_name);
> > goto out;
> > }
> >
> > So if ret as returned from your function is > 0 you will leak the memory
> > allocated @ mdata_buf ..
> >
> > Do you expect something else to come along and call
> > qcom_scm_pas_metadata_release() ?
>
> You just identified a bug in the existing code where qcom_mdt_pas_init()
> does not call qcom_scm_pas_metadata_release() for firmware image for
> failure case from qcom_q6v5_pas().
This could be bug only if some odd firmware return > 0 response code
even if the SMC was a success.
>
>
> >
> > > +}
> > > +
> > > /**
> > > * qcom_scm_pas_init_image() - Initialize peripheral authentication service
> > > * state machine for a given peripheral, using the
> > > @@ -625,7 +654,7 @@ static int __qcom_scm_pas_init_image(u32 peripheral, dma_addr_t mdata_phys,
> > > * and optional blob of data used for authenticating the metadata
> > > * and the rest of the firmware
> > > * @size: size of the metadata
> > > - * @ctx: optional metadata context
> > > + * @ctx: optional pas context
> > > *
> > > * Return: 0 on success.
> > > *
> > > @@ -634,13 +663,19 @@ static int __qcom_scm_pas_init_image(u32 peripheral, dma_addr_t mdata_phys,
> > > * qcom_scm_pas_metadata_release() by the caller.
> > > */
> > > int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
> > > - struct qcom_scm_pas_metadata *ctx)
> > > + struct qcom_scm_pas_ctx *ctx)
> > > {
> > > + struct qcom_scm_pas_metadata *mdt_ctx;
> > > struct qcom_scm_res res;
> > > dma_addr_t mdata_phys;
> > > void *mdata_buf;
> > > int ret;
> > > + if (ctx && ctx->has_iommu) {
> > > + ret = qcom_scm_pas_prep_and_init_image(ctx, metadata, size);
> > > + return ret;
> > > + }
> > > +
> > > /*
> > > * During the scm call memory protection will be enabled for the meta
> > > * data blob, so make sure it's physically contiguous, 4K aligned and
> > > @@ -663,10 +698,11 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
> > > ret = __qcom_scm_pas_init_image(peripheral, mdata_phys, mdata_buf, size, &res);
> > > if (ret < 0 || !ctx) {
> > > dma_free_coherent(__scm->dev, size, mdata_buf, mdata_phys);
> > > - } else if (ctx) {
> > > - ctx->ptr = mdata_buf;
> > > - ctx->phys = mdata_phys;
> > > - ctx->size = size;
> > > + } else if (ctx->metadata) {
> > > + mdt_ctx = ctx->metadata;
> > > + mdt_ctx->ptr = mdata_buf;
> > > + mdt_ctx->addr.dma_addr = mdata_phys;
> > > + mdt_ctx->size = size;
> > > }
> > > return ret ? : res.result[0];
> >
> > is this return path still valid now that you've functionally decomposed into
> > qcom_sm_pas_prep_and_init ?
Yes, should be the same as it was earlier.
I believe, there is a duplication but its worth it to avoid confusion to
among different allocators used here one is DMA and other is TZmem.
> >
> > > @@ -675,18 +711,27 @@ EXPORT_SYMBOL_GPL(qcom_scm_pas_init_image);
> > > /**
> > > * qcom_scm_pas_metadata_release() - release metadata context
> > > - * @ctx: metadata context
> > > + * @ctx: pas context
> > > */
> > > -void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx)
> > > +void qcom_scm_pas_metadata_release(struct qcom_scm_pas_ctx *ctx)
> > > {
> > > - if (!ctx->ptr)
> > > + struct qcom_scm_pas_metadata *mdt_ctx;
> > > +
> > > + mdt_ctx = ctx->metadata;
> > > + if (!mdt_ctx->ptr)
> > > return;
> > > - dma_free_coherent(__scm->dev, ctx->size, ctx->ptr, ctx->phys);
> > > + if (ctx->has_iommu) {
> > > + qcom_tzmem_free(mdt_ctx->ptr);
> > > + mdt_ctx->addr.phys_addr = 0;
> > > + } else {
> > > + dma_free_coherent(__scm->dev, mdt_ctx->size, mdt_ctx->ptr,
> > > + mdt_ctx->addr.dma_addr);
> > > + mdt_ctx->addr.dma_addr = 0;
> > > + }
> > > - ctx->ptr = NULL;
> > > - ctx->phys = 0;
> > > - ctx->size = 0;
> > > + mdt_ctx->ptr = NULL;
> > > + mdt_ctx->size = 0;
> > > }
> > > EXPORT_SYMBOL_GPL(qcom_scm_pas_metadata_release);
> > > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> > > index e376c0338576..09cada92dfd5 100644
> > > --- a/drivers/remoteproc/qcom_q6v5_pas.c
> > > +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> > > @@ -209,9 +209,9 @@ static int qcom_pas_unprepare(struct rproc *rproc)
> > > * auth_and_reset() was successful, but in other cases clean it up
> > > * here.
> > > */
> > > - qcom_scm_pas_metadata_release(pas->pas_ctx->metadata);
> > > + qcom_scm_pas_metadata_release(pas->pas_ctx);
> > > if (pas->dtb_pas_id)
> > > - qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
> > > + qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
> > > return 0;
> > > }
> > > @@ -244,7 +244,7 @@ static int qcom_pas_load(struct rproc *rproc, const struct firmware *fw)
> > > return 0;
> > > release_dtb_metadata:
> > > - qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
> > > + qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
> > > release_firmware(pas->dtb_firmware);
> > > return ret;
> > > @@ -313,9 +313,9 @@ static int qcom_pas_start(struct rproc *rproc)
> > > goto release_pas_metadata;
> > > }
> > > - qcom_scm_pas_metadata_release(pas->pas_ctx->metadata);
> > > + qcom_scm_pas_metadata_release(pas->pas_ctx);
> > > if (pas->dtb_pas_id)
> > > - qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
> > > + qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
> > > /* firmware is used to pass reference from qcom_pas_start(), drop it now */
> > > pas->firmware = NULL;
> > > @@ -323,9 +323,9 @@ static int qcom_pas_start(struct rproc *rproc)
> > > return 0;
> > > release_pas_metadata:
> > > - qcom_scm_pas_metadata_release(pas->pas_ctx->metadata);
> > > + qcom_scm_pas_metadata_release(pas->pas_ctx);
> > > if (pas->dtb_pas_id)
> > > - qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
> > > + qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
> > > disable_px_supply:
> > > if (pas->px_supply)
> > > regulator_disable(pas->px_supply);
> > > diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> > > index 509ff85d9bf6..a1718db91b3e 100644
> > > --- a/drivers/soc/qcom/mdt_loader.c
> > > +++ b/drivers/soc/qcom/mdt_loader.c
> > > @@ -240,7 +240,7 @@ EXPORT_SYMBOL_GPL(qcom_mdt_read_metadata);
> > > */
> > > static int __qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
> > > const char *fw_name, int pas_id, phys_addr_t mem_phys,
> > > - struct qcom_scm_pas_metadata *ctx)
> > > + struct qcom_scm_pas_ctx *ctx)
> > > {
> > > const struct elf32_phdr *phdrs;
> > > const struct elf32_phdr *phdr;
> > > @@ -491,7 +491,7 @@ int qcom_mdt_pas_load(struct qcom_scm_pas_ctx *ctx, const struct firmware *fw,
> > > int ret;
> > > ret = __qcom_mdt_pas_init(ctx->dev, fw, firmware, ctx->peripheral,
> > > - ctx->mem_phys, ctx->metadata);
> > > + ctx->mem_phys, ctx);
> > > if (ret)
> > > return ret;
> > > diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
> > > index a31006fe49a9..bd3417d9c3f9 100644
> > > --- a/include/linux/firmware/qcom/qcom_scm.h
> > > +++ b/include/linux/firmware/qcom/qcom_scm.h
> > > @@ -68,7 +68,10 @@ int qcom_scm_set_remote_state(u32 state, u32 id);
> > > struct qcom_scm_pas_metadata {
> > > void *ptr;
> > > - dma_addr_t phys;
> > > + union {
> > > + dma_addr_t dma_addr;
> > > + phys_addr_t phys_addr;
> > > + } addr;
> > > ssize_t size;
> > > };
> > > @@ -85,8 +88,8 @@ struct qcom_scm_pas_ctx {
> > > void *qcom_scm_pas_ctx_init(struct device *dev, u32 peripheral, phys_addr_t mem_phys,
> > > size_t mem_size, bool save_mdt_ctx);
> > > int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
> > > - struct qcom_scm_pas_metadata *ctx);
> > > -void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx);
> > > + struct qcom_scm_pas_ctx *ctx);
> > > +void qcom_scm_pas_metadata_release(struct qcom_scm_pas_ctx *ctx);
> > > int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size);
> > > int qcom_scm_pas_prepare_and_auth_reset(struct qcom_scm_pas_ctx *ctx);
> > > int qcom_scm_pas_auth_and_reset(u32 peripheral);
> >
> > Please review the error paths here especially WRT to qcom_mdt_pas_init();
>
> Sure, will send the fix patch for the existing bug.
For existing code, consider a bug only if it is buggy firmware.
>
> >
> > ---
> > bod
>
> --
> -Mukesh Ojha
--
-Mukesh Ojha
Powered by blists - more mailing lists