Re: [PATCH v2] firmware: qcom: scm: Peripheral Authentication Service

[Stephen Boyd <sboyd@codeaurora.org>]

On 07/15, Bjorn Andersson wrote:
> This adds the Peripheral Authentication Service (PAS) interface to the
> Qualcomm SCM interface. The API is used to authenticate and boot a range
> of external processors in various Qualcomm platforms.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---
> 
> Changes since v1:
> - Big endian compatibility

Did you try out a big endian kernel?

> diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
> index 1bd6f9c34331..81d9c59f3ccc 100644
> --- a/drivers/firmware/qcom_scm-32.c
> +++ b/drivers/firmware/qcom_scm-32.c
> @@ -501,3 +502,95 @@ int __qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp)
>  	return qcom_scm_call(QCOM_SCM_SVC_HDCP, QCOM_SCM_CMD_HDCP,
>  		req, req_cnt * sizeof(*req), resp, sizeof(*resp));
>  }
> +
> +bool __qcom_scm_pas_supported(u32 peripheral)
> +{
> +	u32 ret_val;

I guess we don't have to care about __le32 here because it's just
a zero or non-zero value?

> +	int ret;
> +
> +	ret = qcom_scm_call(QCOM_SCM_SVC_PIL, QCOM_SCM_PAS_IS_SUPPORTED_CMD,
> +			    &peripheral, sizeof(peripheral),

Peripheral needs cpu_to_le32() treatment.

> +			    &ret_val, sizeof(ret_val));
> +
> +	return ret ? false : !!ret_val;
> +}
> +
> +int __qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size)
> +{
> +	dma_addr_t mdata_phys;
> +	void *mdata_buf;
> +	__le32 scm_ret;
> +	int ret;
> +	struct pas_init_image_req {
> +		__le32 proc;
> +		__le32 image_addr;
> +	} request;
> +
> +	/*
> +	 * During the scm call memory protection will be enabled for the meta
> +	 * data blob, so make sure it's physically contiguous, 4K aligned and
> +	 * non-cachable to avoid XPU violations.
> +	 */
> +	mdata_buf = dma_alloc_coherent(NULL, size, &mdata_phys, GFP_KERNEL);

This should pass a device pointer instead of NULL here. Please
make struct device an argument of this function and pass
something there besides NULL in the calling driver. Or we should
make SCM into a platform device driver with a node in DT (named
firmware?). Then if we need to do anything special for DMA to the
firmware, etc. we have a struct device that can describe that.

Also, dma_alloc_coherent() doesn't do enough to prevent XPU
violations because memory returned from that function on ARM is
not guaranteed to be device memory and so we could speculatively
access the locked down metadata region. This is why we added the
strongly ordered mapping property and pass that to
dma_alloc_attrs in the downstream code so we can change the page
table attributes of the mapping to be device memory. Not doing
this can lead to random crashes when some read speculates on the
metadata and the secure world intercepts it and shuts the system
down.

I was going to say we could try to use the carveout/reserved
memory code but that doesn't look fool proof. From what I can
tell CMA doesn't use the same page table attributes for the
mapping that dma-coherent does, so if we use dma-coherent it will
use ioremap and work but if we use CMA it won't (at least it
looks like bufferable memory there). Can we add a way to request
memory doesn't allow speculatioan through the DMA APIs?

> +	if (!mdata_buf) {
> +		pr_err("Allocation of metadata buffer failed.\n");
> +		return -ENOMEM;
> +	}
> +	memcpy(mdata_buf, metadata, size);
> +
> +	request.proc = cpu_to_le32(peripheral);
> +	request.image_addr = cpu_to_le32(mdata_phys);
> +
> +	ret = qcom_scm_call(QCOM_SCM_SVC_PIL, QCOM_SCM_PAS_INIT_IMAGE_CMD,
> +			    &request, sizeof(request),
> +			    &scm_ret, sizeof(scm_ret));
> +
> +	dma_free_coherent(NULL, size, mdata_buf, mdata_phys);
> +
> +	return ret ? : le32_to_cpu(scm_ret);
> +}
> +
> +int __qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size)
> +{
> +	__le32 scm_ret;
> +	int ret;
> +	struct pas_init_image_req {

I thought this was going to be an unnamed structure?

	struct {
		__le32 proc;
		__le32 addr;
		__le32 len;
	} cmd;

> +		__le32 proc;
> +		__le32 addr;
> +		__le32 len;
> +	} request;
> +
> +	request.proc = cpu_to_le32(peripheral);
> +	request.addr = cpu_to_le32(addr);
> +	request.len = cpu_to_le32(size);
> +
> +	ret = qcom_scm_call(QCOM_SCM_SVC_PIL, QCOM_SCM_PAS_MEM_SETUP_CMD,
> +			    &request, sizeof(request),
> +			    &scm_ret, sizeof(scm_ret));
> +
> +	return ret ? : le32_to_cpu(scm_ret);
> +}
> +
> +int __qcom_scm_pas_auth_and_reset(u32 peripheral)
> +{
> +	__le32 scm_ret;
> +	int ret;
> +
> +	ret = qcom_scm_call(QCOM_SCM_SVC_PIL, QCOM_SCM_PAS_AUTH_AND_RESET_CMD,
> +			    &peripheral, sizeof(peripheral),

peripheral needs cpu_to_le32() treatment.

> +			    &scm_ret, sizeof(scm_ret));
> +
> +	return ret ? : le32_to_cpu(scm_ret);
> +}
> +
> +int __qcom_scm_pas_shutdown(u32 peripheral)
> +{
> +	__le32 scm_ret;
> +	int ret;
> +
> +	ret = qcom_scm_call(QCOM_SCM_SVC_PIL, QCOM_SCM_PAS_SHUTDOWN_CMD,
> +			    &peripheral, sizeof(peripheral),

peripheral needs cpu_to_le32() treatment.

> +			    &scm_ret, sizeof(scm_ret));
> +
> +	return ret ? : le32_to_cpu(scm_ret);
> +}

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project