Re: [PATCH v1 03/11] soc: qcom: crypto_virt: add support for create, prepare and import keys

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing QCOM SCM dependency check leads to NULL pointer dereference
- [Medium] Global state inheritance of `g_wrapped_key_size` across devices
- [Low] Unconditional registration of unsupported hardware-wrapped key operations
--

commit 4b3400c30fba3c871f1ea78eace978c0fff73f5a
Author: linlzhan <[email protected]>
soc: qcom: crypto_virt: add support for create, prepare and import keys

This commit adds support for generating, preparing, and importing hardware
wrapped keys in the crypto_virt driver. It obtains the wrapped key size
from the "qcom,wrapped-key-size" property and enables SCM helpers.

> diff --git a/drivers/soc/qcom/crypto_virt.c b/drivers/soc/qcom/crypto_virt.c
> index 4ee2a36af6c1f..93c7993fb4a5d 100644
> --- a/drivers/soc/qcom/crypto_virt.c
> +++ b/drivers/soc/qcom/crypto_virt.c
[ ... ]
> +static int crypto_virt_probe(struct platform_device *pdev)
>  {
> +	int ret;
> +

[Severity: High]
Should this probe function verify that the QCOM SCM driver is available
before proceeding?

Since the driver registers virtio block crypto operations here, userspace can
issue block crypto ioctls as soon as this completes. If SCM is not yet ready,
a call to a function like qcom_scm_generate_ice_key() will unconditionally
dereference the uninitialized __scm pointer:

qcom_scm_generate_ice_key()
   void *lt_key_buf = qcom_tzmem_alloc(__scm->mempool, ...);

Could we add a check for qcom_scm_is_available() and return -EPROBE_DEFER
if it is not yet ready?

> +	ret = of_property_read_u32(pdev->dev.of_node, "qcom,wrapped-key-size",
> +				    &g_wrapped_key_size);
> +	if (ret)
> +		dev_warn(&pdev->dev, "qcom,wrapped-key-size not found\n");

[Severity: Medium]
Could this leak state between different probed devices?

Because g_wrapped_key_size is a static global variable, it retains its value
from previous probes. If a newly probed device is missing the
qcom,wrapped-key-size property, of_property_read_u32() returns an error
without modifying the output variable.

Would it be safer to zero-initialize g_wrapped_key_size before the read,
so that a device without this property doesn't accidentally inherit the
wrapped key size from a previously probed device?

> +
> +	if (!g_wrapped_key_size ||
> +	    g_wrapped_key_size > BLK_CRYPTO_MAX_HW_WRAPPED_KEY_SIZE) {
> +		dev_err(&pdev->dev,
> +			"invalid qcom,wrapped-key-size %u, won't support generate/import/prepare hardware wrapped key\n",
> +			g_wrapped_key_size);
> +		g_wrapped_key_size = 0;
> +	}
> +
>  	virtblk_set_crypto_ops(&virtblk_crypto_qcom_vops);

[Severity: Low]
Does this code unconditionally register the hardware-wrapped key operations,
even when they are unsupported?

By registering virtblk_crypto_qcom_vops with non-NULL function pointers for
generate_key, prepare_key, and import_key, the block layer framework will
assume these capabilities are supported.

When invoked, the unsupported operations check for !g_wrapped_key_size and
return -EINVAL:

crypto_virt_generate_key() {
    ...
    if (!g_wrapped_key_size) {
        ...
        return -EINVAL;
    }
}

Should these operations either return the standard -EOPNOTSUPP, or avoid
being registered altogether when the feature is locally disabled?

>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3