Re: [PATCH 07/10] qcom_scm: scm call for create, prepare and import keys

From: Eric Biggers <ebiggers@kernel.org>
To: Gaurav Kashyap <quic_gaurkash@quicinc.com>
Cc: linux-scsi@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-mmc@vger.kernel.org, linux-block@vger.kernel.org,
	linux-fscrypt@vger.kernel.org, thara.gopinath@linaro.org,
	quic_neersoni@quicinc.com, dineshg@quicinc.com
Subject: Re: [PATCH 07/10] qcom_scm: scm call for create, prepare and import keys
Date: Mon, 13 Dec 2021 17:50:51 -0800	[thread overview]
Message-ID: <Ybf4ezgCbjugYsZz@gmail.com> (raw)
In-Reply-To: <20211206225725.77512-8-quic_gaurkash@quicinc.com>

On Mon, Dec 06, 2021 at 02:57:22PM -0800, Gaurav Kashyap wrote:
> +/**
> + * qcom_scm_generate_ice_key() - Generate a wrapped key for encryption.
> + * @longterm_wrapped_key: the wrapped key returned after key generation
> + * @longterm_wrapped_key_size: size of the wrapped key to be returned.
> + *
> + * Qualcomm wrapped keys need to be generated in a trusted environment.
> + * A generate key  IOCTL call is used to achieve this. These are longterm
> + * in nature as they need to be generated and wrapped only once per
> + * requirement.
> + *
> + * This SCM calls adds support for the generate key IOCTL to interface
> + * with the secure environment to generate and return a wrapped key..
> + *
> + * Return: 0 on success; -errno on failure.
> + */
> +int qcom_scm_generate_ice_key(u8 *longterm_wrapped_key,
> +			    u32 longterm_wrapped_key_size)

Isn't longterm_wrapped_key_size really a maximum size?  How does this function
indicate the size of the resulting key?

> +/**
> + * qcom_scm_prepare_ice_key() - Get per boot ephemeral wrapped key
> + * @longterm_wrapped_key: the wrapped key
> + * @longterm_wrapped_key_size: size of the wrapped key
> + * @ephemeral_wrapped_key: ephemeral wrapped key to be returned
> + * @ephemeral_wrapped_key_size: size of the ephemeral wrapped key
> + *
> + * Qualcomm wrapped keys (longterm keys) are rewrapped with a per-boot
> + * ephemeral key for added protection. These are ephemeral in nature as
> + * they are valid only for that boot. A create key IOCTL is used to
> + * achieve this. These are the keys that are installed into the kernel
> + * to be then unwrapped and programmed into ICE.
> + *
> + * This SCM call adds support for the create key IOCTL to interface
> + * with the secure environment to rewrap the wrapped key with an
> + * ephemeral wrapping key.
> + *
> + * Return: 0 on success; -errno on failure.
> + */
> +int qcom_scm_prepare_ice_key(const u8 *longterm_wrapped_key,
> +			     u32 longterm_wrapped_key_size,
> +			     u8 *ephemeral_wrapped_key,
> +			     u32 ephemeral_wrapped_key_size)

Similarly here.  Isn't ephemeral_wrapped_key_size really a maximum size?  How
does this function indicate the size of the resulting ephemeral wrapped key?

> +/**
> + * qcom_scm_import_ice_key() - Import a wrapped key for encryption
> + * @imported_key: the raw key that is imported
> + * @imported_key_size: size of the key to be imported

imported_key and imported_key_size should be called raw_key and raw_key_size.

> + * @longterm_wrapped_key: the wrapped key to be returned
> + * @longterm_wrapped_key_size: size of the wrapped key
> + *
> + * Conceptually, this is very similar to generate, the difference being,
> + * here we want to import a raw key and return a longterm wrapped key
> + * from it. THe same create key IOCTL is used to achieve this.
> + *
> + * This SCM call adds support for the create key IOCTL to interface with
> + * the secure environment to import a raw key and generate a longterm
> + * wrapped key.
> + *
> + * Return: 0 on success; -errno on failure.
> + */
> +int qcom_scm_import_ice_key(const u8 *imported_key, u32 imported_key_size,
> +			    u8 *longterm_wrapped_key,
> +			    u32 longterm_wrapped_key_size)

And likewise, isn't longterm_wrapped_key_size really a maximum size?  How does
this function indicate the size of the resulting key?

> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> index 08bb2a4c80db..efd0ede1fb37 100644
> --- a/drivers/firmware/qcom_scm.h
> +++ b/drivers/firmware/qcom_scm.h
> @@ -111,6 +111,9 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
>  #define QCOM_SCM_ES_INVALIDATE_ICE_KEY	0x03
>  #define QCOM_SCM_ES_CONFIG_SET_ICE_KEY	0x04
>  #define QCOM_SCM_ES_DERIVE_SW_SECRET	0x07
> +#define QCOM_SCM_ES_GENERATE_ICE_KEY	0x08
> +#define QCOM_SCM_ES_PREPARE_ICE_KEY	0x09
> +#define QCOM_SCM_ES_IMPORT_ICE_KEY	0xA

Writing "0xA" here looks weird.  It should be "0x0A" to match the others.

- Eric