Re: [PATCH v2 5/6] soc: qcom: ice: Migrate to the TZ agnostic ICE APIs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] soc: qcom: ice: infinite probe deferral in qcom_ice_create()
  without OP-TEE
- [High] soc: qcom: ice: circular module dependency and build failure in
  qcom_ice_create()
- [Medium] soc: qcom: ice: -ENODEV converts to true in
  qcom_ice_tz_has_wrapped_key_support()
- [Medium] soc: qcom: ice: unsynchronized ice_tz_ops access races with
  backend teardown
--

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

soc: qcom: ice: Migrate to the TZ agnostic ICE APIs

This commit switches the Qualcomm Inline Crypto Engine driver from calling
qcom_scm_*() directly to using the qcom_ice_tz_*() wrapper functions.

> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> index 4844621b74970..9bab177f7ce55 100644
> --- a/drivers/soc/qcom/ice.c
> +++ b/drivers/soc/qcom/ice.c
> @@ -307,7 +307,7 @@ static bool qcom_ice_check_supported(struct qcom_ice *ice)
>  	 * advertise support for raw keys or wrapped keys, HWKM cannot be used
>  	 * unconditionally.  A module parameter is used to opt into using it.
>  	 */
> -	if (ice->hwkm_version && qcom_scm_has_wrapped_key_support()) {
> +	if (ice->hwkm_version && qcom_ice_tz_has_wrapped_key_support()) {

[Severity: Medium]
Does qcom_ice_tz_has_wrapped_key_support() evaluate to true when ice_tz_ops
is NULL?

drivers/soc/qcom/ice.c:qcom_ice_tz_has_wrapped_key_support() {
	struct qcom_ice_tz_ops *ops;

	ops = ice_tz_ops;
	if (!ops)
		return -ENODEV;

	return ops->ice_has_wrapped_key_support(ops->dev);
}

Because the return type is bool, returning -ENODEV converts to true,
causing qcom_ice_check_supported() to falsely enable ice->use_hwkm when
ice_tz_ops is NULL.

[ ... ]

> @@ -553,9 +553,9 @@ int qcom_ice_program_key(struct qcom_ice *ice, unsigned int slot,
>  	for (i = 0; i < ARRAY_SIZE(key.words); i++)
>  		__cpu_to_be32s(&key.words[i]);
>  
> -	err = qcom_scm_ice_set_key(slot, key.bytes, AES_256_XTS_KEY_SIZE,
> -				   QCOM_SCM_ICE_CIPHER_AES_256_XTS,
> -				   blk_key->crypto_cfg.data_unit_size / 512);
> +	err = qcom_ice_tz_set_key(slot, key.bytes, AES_256_XTS_KEY_SIZE,
> +				  QCOM_ICE_CIPHER_AES_256_XTS,
> +				  blk_key->crypto_cfg.data_unit_size / 512);

[Severity: Medium]
Can qcom_ice_tz_set_key() and the other qcom_ice_tz_*() wrappers race with
qcom_ice_tz_ops_unregister() during backend teardown?

CPU0                              CPU1
qcom_ice_program_key()
  qcom_ice_tz_set_key()
    ops = ice_tz_ops;
    if (!ops)
                                  qcom_ice_tee_remove()
                                    qcom_ice_tz_ops_unregister(...)
                                      smp_store_release(&ice_tz_ops, NULL)
                                    tee_client_close_session(...)
                                    tee_client_close_context(...)
                                    /* devm frees qcom_ice_tee_private */
    ops->ice_set_key(ops->dev, ...)
      ...                         <-- use-after-free on ops->dev data

Each qcom_ice_tz_*() wrapper reads ice_tz_ops without holding a lock or
reference, and qcom_ice_tz_ops_unregister() clears ice_tz_ops without
waiting for in-flight calls to complete before qcom_ice_tee_remove() tears
down the TEE session and frees its devm-managed driver data.

[ ... ]

> @@ -696,11 +696,11 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
>  {
>  	struct qcom_ice *engine;
>  
> -	if (!qcom_scm_is_available())
> +	if (!qcom_ice_tz_is_available())
>  		return ERR_PTR(-EPROBE_DEFER);

[Severity: High]
Does checking qcom_ice_tz_is_available() here cause qcom_ice_create() to
return -EPROBE_DEFER forever on platforms that do not have both QTEE SCM
and OP-TEE available at the same time?

drivers/soc/qcom/ice.c:qcom_ice_tz_is_available() {
	...
	return (qcom_scm_is_available() & qcom_ice_tee_is_available());
}

qcom_ice_tee_is_available() only becomes true when an OP-TEE platform
device binds to qcom_ice_plat_probe().  On any QTEE/SCM platform without
OP-TEE (or on an OP-TEE platform without qcom_scm),
qcom_ice_tz_is_available() never becomes true even after ice_tz_ops is
registered, so UFS and SDHCI controllers calling devm_of_qcom_ice_get()
defer probe indefinitely and cannot mount the root filesystem.

Conversely, on OP-TEE platforms, qcom_ice_plat_probe() and
qcom_ice_tee_probe() bind on different buses; if qcom_ice_plat_probe() sets
__teedev before qcom_ice_tee_probe() registers qcom_ice_tee_ops,
qcom_ice_tz_is_available() returns true while qcom_ice_tz_ops_registered()
is still false, causing qcom_ice_create() to fail permanently with
-EOPNOTSUPP instead of deferring probe.

>  
> -	if (!qcom_scm_ice_available()) {
> -		dev_warn(dev, "ICE SCM interface not found\n");
> +	if (!qcom_ice_tz_ops_registered()) {
> +		dev_warn(dev, "ICE firmware service interface not found\n");
>  		return ERR_PTR(-EOPNOTSUPP);
>  	}

[Severity: High]
Does calling qcom_ice_tz_is_available() and qcom_ice_tz_ops_registered() in
qcom_ice_create() introduce a circular module dependency and break modular
builds?

Making qcom_ice_tz_is_available() live causes qcom_ice to reference
qcom_ice_tee_is_available() from qcom_ice_tee and qcom_scm_is_available()
from qcom_scm, while both of those modules call qcom_ice_tz_ops_register()
and qcom_ice_tz_ops_unregister() exported by qcom_ice.  This also fails to
link when CONFIG_QCOM_INLINE_CRYPTO_ENGINE=y and CONFIG_QCOM_ICE_TEE is
modular or disabled.

In addition, include/soc/qcom/ice.h guards qcom_ice_tz_ops_register() and
qcom_ice_tz_ops_unregister() with #ifdef CONFIG_QCOM_INLINE_CRYPTO_ENGINE
instead of IS_ENABLED(CONFIG_QCOM_INLINE_CRYPTO_ENGINE):

include/soc/qcom/ice.h:
#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;
}

When CONFIG_QCOM_INLINE_CRYPTO_ENGINE=m (such as in arm64 defconfig), the
static inline stubs in include/soc/qcom/ice.h conflict with the definitions
in drivers/soc/qcom/ice.c and turn registration in the backends into a
no-op, leaving qcom_ice_tz_ops_registered() permanently false.

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