Re: [PATCH 2/4] qcom_scm: scm call for deriving a software secret

[Eric Biggers <ebiggers@kernel.org>]

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