Re: [PATCH v2 1/6] soc: qcom: ice: Add TZ agnostic re-direction functions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] soc: qcom: ice: broken CONFIG_QCOM_INLINE_CRYPTO_ENGINE=m guard
  in ice.h
- [High] soc: qcom: ice: broken TZ availability check in
  qcom_ice_tz_is_available()
- [Medium] soc: qcom: ice: qcom_ice_tz_has_wrapped_key_support() returns
  -ENODEV as bool
- [Low] soc: qcom: ice: non-atomic check-then-store in
  qcom_ice_tz_ops_register()
--

commit aaede1b942c524f288e35421c3d46bb0baa718ed
Author: Harshal Dev <harshal.dev@oss.qualcomm.com>

soc: qcom: ice: Add TZ agnostic re-direction functions

This commit adds TrustZone-agnostic redirection functions and a backend
operations registration interface so the Qualcomm ICE driver can route calls
to either QTEE SCM or an OP-TEE backend.

> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> index e256595b9826f..452896b1858f1 100644
> --- a/drivers/soc/qcom/ice.c
> +++ b/drivers/soc/qcom/ice.c
> @@ -118,6 +118,130 @@ struct qcom_ice {
>  static DEFINE_XARRAY(ice_handles);
>  static DEFINE_MUTEX(ice_mutex);
>  
> +static struct qcom_ice_tz_ops *ice_tz_ops;
> +
> +static bool qcom_ice_tz_ops_registered(void)
> +{
> +	/*
> +	 * The barrier is intended to synchronize the data stores for the
> +	 * ice_tz_ops data structure when client drivers (UFS/SDHC) are in
> +	 * parallel checking for ICE service availability.
> +	 */
> +	return !!smp_load_acquire(&ice_tz_ops);
> +}
> +
> +static bool qcom_ice_tz_is_available(void)
> +{
> +	return qcom_scm_is_available();
> +}

[Severity: High]
As reported by Eric Biggers on v1 [1], does qcom_ice_tz_is_available() cause
qcom_ice_create() to return -EPROBE_DEFER indefinitely on QTEE-only
platforms?

Later in this series, commit "tee: optee: Register TEE devices on the
platform bus" updates qcom_ice_tz_is_available() to return
(qcom_scm_is_available() & qcom_ice_tee_is_available()), and commit
"soc: qcom: ice: Migrate to the TZ agnostic ICE APIs" makes
qcom_ice_create() return -EPROBE_DEFER when qcom_ice_tz_is_available() is
false before checking qcom_ice_tz_ops_registered().  Because
qcom_ice_tee_is_available() only becomes true after an OP-TEE platform
device probes, it stays false on QTEE-only platforms (or when CONFIG_OPTEE
is disabled).  Since ufs_qcom_ice_init() and sdhci_msm_ice_init() only fall
back to running without inline encryption on -EOPNOTSUPP and propagate
-EPROBE_DEFER, UFS and eMMC/SDHC controllers never finish probing on those
platforms.

[ ... ]

> +static bool qcom_ice_tz_has_wrapped_key_support(void)
> +{
> +	struct qcom_ice_tz_ops *ops;
> +
> +	ops = ice_tz_ops;
> +	if (!ops)
> +		return -ENODEV;
> +
> +	return ops->ice_has_wrapped_key_support(ops->dev);
> +}

[Severity: Medium]
Does returning -ENODEV from a bool function evaluate to true when
ice_tz_ops is NULL?  Besides triggering a -Wint-in-bool-context compiler
warning, this reports that wrapped keys are supported when no TZ backend is
registered.

[ ... ]

> +int qcom_ice_tz_ops_register(struct qcom_ice_tz_ops *ops)
> +{
> +	if (!qcom_ice_tz_ops_registered())
> +		/* Paired with smp_load_acquire() in qcom_ice_tz_ops_registered() */
> +		smp_store_release(&ice_tz_ops, ops);
> +	else
> +		return -EBUSY;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(qcom_ice_tz_ops_register);
> +
> +void qcom_ice_tz_ops_unregister(struct qcom_ice_tz_ops *ops)
> +{
> +	if (READ_ONCE(ice_tz_ops) == ops)
> +		/* Paired with smp_load_acquire() in qcom_ice_tz_ops_registered() */
> +		smp_store_release(&ice_tz_ops, NULL);
> +}
> +EXPORT_SYMBOL_GPL(qcom_ice_tz_ops_unregister);

[Severity: Low]
Can two backends probing in parallel on the platform and TEE buses both pass
!qcom_ice_tz_ops_registered() and overwrite ice_tz_ops instead of returning
-EBUSY?  Using cmpxchg() or a mutex across register and unregister would make
the check and update atomic.

> diff --git a/include/soc/qcom/ice.h b/include/soc/qcom/ice.h
> index 4bee553f0a59d..89e62e151b85b 100644
> --- a/include/soc/qcom/ice.h
> +++ b/include/soc/qcom/ice.h

[ ... ]

> @@ -31,4 +72,19 @@ int qcom_ice_import_key(struct qcom_ice *ice,
>  			u8 lt_key[BLK_CRYPTO_MAX_HW_WRAPPED_KEY_SIZE]);
>  struct qcom_ice *devm_of_qcom_ice_get(struct device *dev);
>  
> +#ifdef CONFIG_QCOM_INLINE_CRYPTO_ENGINE
> +int qcom_ice_tz_ops_register(struct qcom_ice_tz_ops *ops);
> +void qcom_ice_tz_ops_unregister(struct qcom_ice_tz_ops *ops);
> +#else
> +static inline int qcom_ice_tz_ops_register(struct qcom_ice_tz_ops *__unused)
> +{
> +	return 0;
> +}
> +
> +static inline
> +void qcom_ice_tz_ops_unregister(struct qcom_ice_tz_ops *__unused)
> +{
> +}
> +#endif /* CONFIG_QCOM_INLINE_CRYPTO_ENGINE */
> +
>  #endif /* __QCOM_ICE_H__ */

[Severity: High]
Does #ifdef CONFIG_QCOM_INLINE_CRYPTO_ENGINE break the build when
CONFIG_QCOM_INLINE_CRYPTO_ENGINE=m?

QCOM_INLINE_CRYPTO_ENGINE is a tristate symbol (and defaults to =m in arm64
defconfig), so when built as a module only
CONFIG_QCOM_INLINE_CRYPTO_ENGINE_MODULE is defined.  The header then exposes
the static inline stubs, which conflict with the non-static definitions in
drivers/soc/qcom/ice.c and cause TZ backends to call no-op stubs.  Also, if
CONFIG_QCOM_SCM=y while CONFIG_QCOM_INLINE_CRYPTO_ENGINE=m, built-in
qcom_scm cannot link against symbols exported by qcom_ice.ko.

[1] https://lore.kernel.org/r/20261024185602.GF2553@quark

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-ice-migrate-tee-svc-v2-0-29d19f6c92ab@oss.qualcomm.com?part=1