On Wed, Nov 03, 2021 at 04:18:38PM -0700, Gaurav Kashyap wrote:
>
> However, when keys are hardware wrapped, it can be only unwrapped
> by Qualcomm Trustzone.
Qualcomm Trustzone is software. There is a mode where it passes the key to the
actual HWKM hardware as intended, right?
> +/**
> + * qcom_scm_derive_sw_secret() - Derive SW secret from wrapped encryption key
> + * @wrapped_key: the wrapped key used for inline encryption
> + * @wrapped_key_size: size of the wrapped key
> + * @sw_secret: the secret to be derived
> + * @secret_size: size of the secret derived
Please make the semantics of the @secret_size parameter clear. Will the output
be at least @secret_size, exactly @secret_size, or at most @secret_size?
> + *
> + * Derive a SW secret to be used for inline encryption using Qualcomm ICE.
> + *
> + * Generally, for non-wrapped keys, fscrypt can derive a sw secret from the
> + * key in the clear in the linux keyring.
> + *
> + * However, for wrapped keys, the key needs to be unwrapped, which can be done
> + * only by the secure EE. So, it makes sense for the secure EE to derive the
> + * sw secret and return to the kernel when wrapped keys are used.
It's sort of a layering violation to mention fscrypt here, as this is low-level
driver code. fscrypt is just an example user. I recommend documenting this in
more general terms, and maybe referring to the "Hardware-wrapped keys" section
of Documentation/block/inline-encryption.rst (which my patchset adds) as that is
intended to explain derive_sw_secret already.
> +int qcom_scm_derive_sw_secret(const u8* wrapped_key, u32 wrapped_key_size,
> + u8 *sw_secret, u32 secret_size)
> +{
> + struct qcom_scm_desc desc = {
> + .svc = QCOM_SCM_SVC_ES,
> + .cmd = QCOM_SCM_ES_DERIVE_RAW_SECRET,
> + .arginfo = QCOM_SCM_ARGS(4, QCOM_SCM_RW,
wrapped_key is const. Should it use QCOM_SCM_RO instead of QCOM_SCM_RW?
> + keybuf = dma_alloc_coherent(__scm->dev, wrapped_key_size, &key_phys,
> + GFP_KERNEL);
> + if (!keybuf)
> + return -ENOMEM;
> + secretbuf = dma_alloc_coherent(__scm->dev, secret_size, &secret_phys,
> + GFP_KERNEL);
> + if (!secretbuf)
> + return -ENOMEM;
In the '!secretbuf' case, this leaks 'keybuf'.
Also, my understanding is that the use of dma_alloc_coherent() here is a bit
unusual. It would be helpful to leave a comment like:
/*
* Like qcom_scm_ice_set_key(), we use dma_alloc_coherent() to properly
* get a physical address, while guaranteeing that we can zeroize the
* key material later using memzero_explicit().
*/
> + ret = qcom_scm_call(__scm->dev, &desc, NULL);
> + memcpy(sw_secret, secretbuf, secret_size);
This is copying out the data even if the SCM call failed.
> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> index d92156ceb3ac..de5d4f8fd20d 100644
> --- a/drivers/firmware/qcom_scm.h
> +++ b/drivers/firmware/qcom_scm.h
> @@ -110,6 +110,7 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
> #define QCOM_SCM_SVC_ES 0x10 /* Enterprise Security */
> #define QCOM_SCM_ES_INVALIDATE_ICE_KEY 0x03
> #define QCOM_SCM_ES_CONFIG_SET_ICE_KEY 0x04
> +#define QCOM_SCM_ES_DERIVE_RAW_SECRET 0x07
Can this be renamed to DERIVE_SW_SECRET?
If not, then you probably should call the function qcom_scm_derive_raw_secret()
instead of qcom_scm_derive_sw_secret(), since the functions in qcom_scm.c are
intended to be thin wrappers around the SCM calls. The naming difference can be
dealt with at a higher level.
- Eric
