| Message ID | 20230719170423.220033-3-quic_gaurkash@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Hardware wrapped key support for qcom ice and ufs | expand |
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@quicinc.com> > --- > 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
On Fri, Jul 21, 2023 at 08:50:56PM -0700, Bjorn Andersson wrote: > > +/** > > + * 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. The name 'sw_secret' comes from the block layer support (https://lore.kernel.org/linux-block/20221216203636.81491-2-ebiggers@kernel.org/). It is helpful to call it 'sw_secret' instead of 'secret', as there are other types of secrets involved that are not accessible to software (Linux). - Eric
On Fri, Jul 21, 2023 at 09:18:24PM -0700, Eric Biggers wrote: > On Fri, Jul 21, 2023 at 08:50:56PM -0700, Bjorn Andersson wrote: > > > +/** > > > + * 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. > > The name 'sw_secret' comes from the block layer support > (https://lore.kernel.org/linux-block/20221216203636.81491-2-ebiggers@kernel.org/). > It is helpful to call it 'sw_secret' instead of 'secret', as there are other > types of secrets involved that are not accessible to software (Linux). > I'm happy with that motivation. My OCD would like for the length of that to be sw_secret_size (or perhaps _len instead...), and not "secret_size", then. Thanks Eric, Bjorn
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 + * @sw_secret: the secret to be derived which is exactly the secret size + * @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. + * + * 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) +{ + 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(). + * + */ + 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) { + 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); + dma_free_coherent(__scm->dev, secret_size, secretbuf, secret_phys); + +bail_keybuf: + memzero_explicit(keybuf, wrapped_key_size); + dma_free_coherent(__scm->dev, wrapped_key_size, keybuf, key_phys); + + return ret; +} +EXPORT_SYMBOL(qcom_scm_derive_sw_secret); + /** * qcom_scm_hdcp_available() - Check if secure environment supports HDCP. * diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h index e6e512bd57d1..c145cdc71ff8 100644 --- a/drivers/firmware/qcom_scm.h +++ b/drivers/firmware/qcom_scm.h @@ -119,6 +119,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_SW_SECRET 0x07 #define QCOM_SCM_SVC_HDCP 0x11 #define QCOM_SCM_HDCP_INVOKE 0x01 diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h index 250ea4efb7cb..20f5d0b7dfd4 100644 --- a/include/linux/firmware/qcom/qcom_scm.h +++ b/include/linux/firmware/qcom/qcom_scm.h @@ -109,6 +109,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,
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@quicinc.com> --- 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(+)