| 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: <9139706a-708c-4be6-a994-120cce0cd0e6@linaro.org>
Date: Sun, 21 Sep 2025 22:40:55 +0100
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Mukesh Ojha <mukesh.ojha@....qualcomm.com>,
Bjorn Andersson <andersson@...nel.org>,
Mathieu Poirier <mathieu.poirier@...aro.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Manivannan Sadhasivam <mani@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>
Cc: linux-arm-msm@...r.kernel.org, linux-remoteproc@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 03/12] firmware: qcom_scm: Introduce PAS context
initialization and destroy helper
On 20/09/2025 20:41, Mukesh Ojha wrote:
> When Secure Peripheral Authentication Service (PAS) method runs on a
> SoC where Linux runs at EL2 (Gunyah absence) where reset sequences
"i.e. runs without the Gynyah Hypervisor then, reset sequences"
> move to EL3 and Linux need to do some extra stuff before calling PAS
> SMC calls like creating SHMbridge. So, PAS SMC call need awareness and
> need handling of things required when Linux run at EL2.
"Therefore the PAS SMC call"
>
> Currently, remoteproc and non-remoteproc subsystems use different
"Currently remoteproc"
> variants of the MDT loader helper API, primarily due to the handling of
> the metadata context. Remoteproc subsystems retain metadata context
> until authentication and reset is done, while non-remoteproc subsystems
> (e.g., video, graphics, ipa etc.) do not need to retain it and can free
"do not need to retain metadata context"
> the context right inside qcom_scm_pas_init() call based on passed context
> parameter as NULL.
>
> So, in an attempt to unify the metadata API process for both remoteproc
"In an attempt to unify"
> and non-remoteproc subsystems and to make the SMC helper function
> cleaner whether SoC running with Gunyah presence or absence by introducing
> a dedicated PAS context initialization and destroy function. Context
> initialization beforehand would help all SMC function to carry it and do
> the right thing whether SoC is running with Gunyah presence or absence.
Since you need to do another version of this patch re: below, please
tidy up the commit log here a bit too.
> Signed-off-by: Mukesh Ojha <mukesh.ojha@....qualcomm.com>
> ---
> drivers/firmware/qcom/qcom_scm.c | 53 ++++++++++++++++++++++++++++++++++
> include/linux/firmware/qcom/qcom_scm.h | 11 +++++++
> 2 files changed, 64 insertions(+)
>
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 3379607eaf94..1c6b4c6f5513 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -558,6 +558,59 @@ static void qcom_scm_set_download_mode(u32 dload_mode)
> dev_err(__scm->dev, "failed to set download mode: %d\n", ret);
> }
>
> +/**
> + * qcom_scm_pas_ctx_init() - Initialize peripheral authentication service
> + * context for a given peripheral and it can be
> + * destroyed with qcom_scm_pas_ctx_destroy() to
> + * release the context
> + *
> + * @dev: PAS firmware device
> + * @pas_id: peripheral authentication service id
> + * @mem_phys: Subsystem reserve memory start address
> + * @mem_size: Subsystem reserve memory size
> + *
> + * Upon successful, returns the PAS context or ERR_PTR() of the error otherwise.
> + */
> +void *qcom_scm_pas_ctx_init(struct device *dev, u32 pas_id, phys_addr_t mem_phys,
> + size_t mem_size)
> +{
> + struct qcom_scm_pas_ctx *ctx;
> +
> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return ERR_PTR(-ENOMEM);
> +
> + ctx->dev = dev;
> + ctx->pas_id = pas_id;
> + ctx->mem_phys = mem_phys;
> + ctx->mem_size = mem_size;
> +
> + ctx->metadata = kzalloc(sizeof(*ctx->metadata), GFP_KERNEL);
> + if (!ctx->metadata) {
> + kfree(ctx);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + return ctx;
> +}
> +EXPORT_SYMBOL_GPL(qcom_scm_pas_ctx_init);
> +
> +/**
> + * qcom_scm_pas_ctx_destroy() - release PAS context
> + * @ctx: PAS context
> + */
> +void qcom_scm_pas_ctx_destroy(struct qcom_scm_pas_ctx *ctx)
> +{
> + kfree(ctx->metadata);
> + ctx->metadata = NULL;
> + ctx->dev = NULL;
> + ctx->pas_id = 0;
> + ctx->mem_phys = 0;
> + ctx->mem_size = 0;
> + kfree(ctx);
> +}
This looks a bit strange, manually destructing an object you then free.
I get the argument you might make about use-after-free but, I don't
think this level of defensive coding is necessary.
> +EXPORT_SYMBOL_GPL(qcom_scm_pas_ctx_destroy);
> +
> /**
> * qcom_scm_pas_init_image() - Initialize peripheral authentication service
> * state machine for a given peripheral, using the
> diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
> index a13f703b16cd..e3e9e9e9077f 100644
> --- a/include/linux/firmware/qcom/qcom_scm.h
> +++ b/include/linux/firmware/qcom/qcom_scm.h
> @@ -72,6 +72,17 @@ struct qcom_scm_pas_metadata {
> ssize_t size;
> };
>
> +struct qcom_scm_pas_ctx {
> + struct device *dev;
> + u32 pas_id;
> + phys_addr_t mem_phys;
> + size_t mem_size;
> + struct qcom_scm_pas_metadata *metadata;
> +};
> +
> +void *qcom_scm_pas_ctx_init(struct device *dev, u32 pas_id, phys_addr_t mem_phys,
> + size_t mem_size);
> +void qcom_scm_pas_ctx_destroy(struct qcom_scm_pas_ctx *ctx);
> int qcom_scm_pas_init_image(u32 pas_id, const void *metadata, size_t size,
> struct qcom_scm_pas_metadata *ctx);
> void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx);
>
> --
> 2.50.1
>
>
Once fixed.
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
---
bod
Powered by blists - more mailing lists