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

[Eric Biggers <ebiggers@kernel.org>]

On Mon, Dec 06, 2021 at 02:57:18PM -0800, Gaurav Kashyap wrote:
> Storage encryption requires fscrypt deriving a sw secret from

As I mentioned on the previous version of this patchset, I think mentions of
"fscrypt" should generally be avoided at the driver level.  Drivers don't know
or care who is using block layer functionality.  It's better to think of the
sw_secret support as simply one of the requirements for hardware-wrapped keys,
alongside the other ones such as support for import_key, prepare_key, etc.

> +/**
> + * 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 which is at most the secret size
> + * @secret_size: maximum size of the secret that is derived
> + *
> + * Derive a SW secret to be used for inline encryption using Qualcomm ICE.

The SW secret isn't used for inline encryption.  It's actually the opposite; the
SW secret is used only for tasks that can't use inline encryption.

> + * 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. So, it makes sense
> + * for the secure EE to derive the sw secret and return to the kernel when
> + * wrapped keys are used.

The second sentence above seems to be saying the same as the first.

> + * 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)

Is @secret_size really the "maximum size of the secret that is derived"?  That
would imply that a shorter secret might be derived.  But if the return value is
0 on success, then there is no way for callers to know what length was derived.

Can't the semantics be "derive exactly secret_size bytes"?  That would make much
more sense.

> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
> index c0475d1c9885..ccd764bdc357 100644
> --- a/include/linux/qcom_scm.h
> +++ b/include/linux/qcom_scm.h
> @@ -103,6 +103,9 @@ extern int qcom_scm_ice_invalidate_key(u32 index);
>  extern int qcom_scm_ice_set_key(u32 index, const u8 *key, u32 key_size,
>  				enum qcom_scm_ice_cipher cipher,
>  				u32 data_unit_size);
> +extern int qcom_scm_derive_sw_secret(const u8 *wrapped_key,
> +				     u32 wrapped_key_size, u8 *sw_secret,
> +				     u32 secret_size);
>  
>  extern bool qcom_scm_hdcp_available(void);
>  extern int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt,
> @@ -169,6 +172,9 @@ static inline int qcom_scm_ice_invalidate_key(u32 index) { return -ENODEV; }
>  static inline int qcom_scm_ice_set_key(u32 index, const u8 *key, u32 key_size,
>  				       enum qcom_scm_ice_cipher cipher,
>  				       u32 data_unit_size) { return -ENODEV; }
> +static inline int qcom_scm_derive_sw_secret(const u8 *wrapped_key,
> +					u32 wrapped_key_size, u8 *sw_secret,
> +					u32 secret_size) { return -ENODEV; }
>  
>  static inline bool qcom_scm_hdcp_available(void) { return false; }
>  static inline int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt,

These "return -ENODEV" stubs don't exist in the latest kernel.  Can you make
sure that you've developing on top of the latest kernel?  It looks like you
based this patchset on top of my patchset
https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git/log/?h=wrapped-keys-v2.
But I had already sent out a newer version of it, based on v5.16-rc1:
https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git/log/?h=wrapped-keys-v4.
So please make sure you're using the most recent version.

- Eric