Re: [PATCH 2/2] firmware: qcom: scm: fall back to kcalloc() for no SCM device bound

[Elliot Berman]

On Mon, Sep 09, 2024 at 08:38:45PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> 
> Older platforms don't have an actual SCM device tied into the driver
> model and so there's no struct device which to use with the TZ Mem API.
> We need to fall-back to kcalloc() when allocating the buffer for
> additional SMC arguments on such platforms which don't even probe the SCM
> driver and never create the TZMem pool.
> 
> Fixes: 449d0d84bcd8 ("firmware: qcom: scm: smc: switch to using the SCM allocator")
> Reported-by: Rudraksha Gupta <guptarud@xxxxxxxxx>
> Closes: https://lore.kernel.org/lkml/692cfe9a-8c05-4ce4-813e-82b3f310019a@xxxxxxxxx/<S-Del>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> ---
>  drivers/firmware/qcom/qcom_scm-smc.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm-smc.c b/drivers/firmware/qcom/qcom_scm-smc.c
> index 2b4c2826f572..13f72541033c 100644
> --- a/drivers/firmware/qcom/qcom_scm-smc.c
> +++ b/drivers/firmware/qcom/qcom_scm-smc.c
> @@ -147,6 +147,15 @@ static int __scm_smc_do(struct device *dev, struct arm_smccc_args *smc,
>  	return 0;
>  }
>  
> +static void smc_args_free(void *ptr)
> +{
> +	if (qcom_scm_get_tzmem_pool())

I'm a little concerned about this check. I didn't think making SCM calls
without the SCM device probed was possible until this report. We do
worry about that in the downstream kernel. So, I'm not sure if this
scenario is currently possible in the upstream kernel.

It's possible that some driver makes SCM call in parallel to SCM device
probing. Then, it might be possible for qcom_scm_get_tzmem_pool() to
return NULL at beginning of function and then a valid pointer by the
time we're freeing the ptr.

- Elliot

> +		qcom_tzmem_free(ptr);
> +	else
> +		kfree(ptr);
> +}
> +
> +DEFINE_FREE(smc_args, void *, if (!IS_ERR_OR_NULL(_T)) smc_args_free(_T));
>  
>  int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>  		   enum qcom_scm_convention qcom_convention,
> @@ -155,7 +164,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>  	struct qcom_tzmem_pool *mempool = qcom_scm_get_tzmem_pool();
>  	int arglen = desc->arginfo & 0xf;
>  	int i, ret;
> -	void *args_virt __free(qcom_tzmem) = NULL;
> +	void *args_virt __free(smc_args) = NULL;
>  	gfp_t flag = atomic ? GFP_ATOMIC : GFP_KERNEL;
>  	u32 smccc_call_type = atomic ? ARM_SMCCC_FAST_CALL : ARM_SMCCC_STD_CALL;
>  	u32 qcom_smccc_convention = (qcom_convention == SMC_CONVENTION_ARM_32) ?
> @@ -173,9 +182,20 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>  		smc.args[i + SCM_SMC_FIRST_REG_IDX] = desc->args[i];
>  
>  	if (unlikely(arglen > SCM_SMC_N_REG_ARGS)) {
> -		args_virt = qcom_tzmem_alloc(mempool,
> -					     SCM_SMC_N_EXT_ARGS * sizeof(u64),
> -					     flag);
> +		/*
> +		 * Older platforms don't have an entry for SCM in device-tree
> +		 * and so no device is bound to the SCM driver. This means there
> +		 * is no struct device for the TZ Mem API. Fall back to
> +		 * kcalloc() on such platforms.
> +		 */
> +		if (mempool)
> +			args_virt = qcom_tzmem_alloc(
> +					mempool,
> +					SCM_SMC_N_EXT_ARGS * sizeof(u64),
> +					flag);
> +		else
> +			args_virt = kcalloc(SCM_SMC_N_EXT_ARGS, sizeof(u64),
> +					    flag);
>  		if (!args_virt)
>  			return -ENOMEM;
>  
> 
> -- 
> 2.43.0
>