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.

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