Re: [PATCH v2 02/10] qcom_scm: scm call for deriving a software secret

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jul 19, 2023 at 10:04:16AM -0700, Gaurav Kashyap wrote:

The $subject prefix does not match other changes in the scm driver.
Please use git log --oneline drivers/fimware/qcom_scm.c and follow what
other changes uses.

> Inline storage encryption requires deriving a sw secret from
> the hardware wrapped keys. For non-wrapped keys, this can be
> directly done as keys are in the clear.
> 
> However, when keys are hardware wrapped, it can be unwrapped
> by HWKM (Hardware Key Manager) which is accessible only from Qualcomm
> Trustzone. Hence, it also makes sense that the software secret is also
> derived there and returned to the linux kernel . This can be invoked by
> using the crypto profile APIs provided by the block layer.
> 
> Signed-off-by: Gaurav Kashyap <quic_gaurkash@xxxxxxxxxxx>
> ---
>  drivers/firmware/qcom_scm.c            | 70 ++++++++++++++++++++++++++
>  drivers/firmware/qcom_scm.h            |  1 +
>  include/linux/firmware/qcom/qcom_scm.h |  3 ++
>  3 files changed, 74 insertions(+)
> 
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index fde33acd46b7..51062d5c7f7b 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -1140,6 +1140,76 @@ int qcom_scm_ice_set_key(u32 index, const u8 *key, u32 key_size,
>  }
>  EXPORT_SYMBOL(qcom_scm_ice_set_key);
>  
> +/**
> + * qcom_scm_derive_sw_secret() - Derive SW secret from wrapped key
> + * @wrapped_key: the wrapped key used for inline encryption
> + * @wrapped_key_size: size of the wrapped key

Following my reply on patch 7, how about dropping the "_key" suffix on
these.

> + * @sw_secret: the secret to be derived which is exactly the secret size

Similarly the "sw_" prefix doesn't see to add value, please omit it.

> + * @secret_size: size of the secret
> + *
> + * Derive a SW secret from a HW Wrapped key for non HW key operations.
> + * For wrapped keys, the key needs to be unwrapped, in order to derive a
> + * SW secret, which can be done only by the secure EE.

Please spell out SW, HW, and EE in this sentence.

> + *
> + * For more information on sw secret, please refer to "Hardware-wrapped keys"
> + * section of Documentation/block/inline-encryption.rst.
> + *
> + * Return: 0 on success; -errno on failure.
> + */
> +int qcom_scm_derive_sw_secret(const u8 *wrapped_key, u32 wrapped_key_size,
> +			      u8 *sw_secret, u32 secret_size)

size_t is a better data type for "a size". (Think I forgot to mention
this in the other patch as well).

> +{
> +	struct qcom_scm_desc desc = {
> +		.svc = QCOM_SCM_SVC_ES,
> +		.cmd =  QCOM_SCM_ES_DERIVE_SW_SECRET,
> +		.arginfo = QCOM_SCM_ARGS(4, QCOM_SCM_RW,
> +					 QCOM_SCM_VAL, QCOM_SCM_RW,
> +					 QCOM_SCM_VAL),
> +		.args[1] = wrapped_key_size,
> +		.args[3] = secret_size,
> +		.owner = ARM_SMCCC_OWNER_SIP,
> +	};
> +
> +	void *keybuf, *secretbuf;
> +	dma_addr_t key_phys, secret_phys;
> +	int ret;
> +
> +	/*
> +	 * 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().
> +	 *

Unnecessary empty line.

> +	 */
> +	keybuf = dma_alloc_coherent(__scm->dev, wrapped_key_size, &key_phys,
> +				    GFP_KERNEL);

Unwrapping this line takes you to 89 characters, which is less than 100
and easier to read. So please bend the 80-char recommendation for this.

> +	if (!keybuf)
> +		return -ENOMEM;
> +	secretbuf = dma_alloc_coherent(__scm->dev, secret_size, &secret_phys,
> +				    GFP_KERNEL);

Same here

> +	if (!secretbuf) {
> +		ret = -ENOMEM;
> +		goto bail_keybuf;
> +	}
> +
> +	memcpy(keybuf, wrapped_key, wrapped_key_size);
> +	desc.args[0] = key_phys;
> +	desc.args[2] = secret_phys;
> +
> +	ret = qcom_scm_call(__scm->dev, &desc, NULL);
> +	if (!ret)
> +		memcpy(sw_secret, secretbuf, secret_size);
> +
> +	memzero_explicit(secretbuf, secret_size);

Empty line here would provide a nice separation between the zeroing and
the freeing.

> +	dma_free_coherent(__scm->dev, secret_size, secretbuf, secret_phys);
> +
> +bail_keybuf:

Both buffers are keys

err_free_wrapped:

> +	memzero_explicit(keybuf, wrapped_key_size);

And it seems sufficient to move this up together with the other
memzero_explicit() as well.

Regards,
Bjorn



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux