| Message ID | 20211206225725.77512-1-quic_gaurkash@quicinc.com |
|---|---|
| Headers | show |
| Series | Add wrapped key support for Qualcomm ICE | expand |
On Mon, Dec 06, 2021 at 02:57:16PM -0800, Gaurav Kashyap wrote: > Add a new library which congregates all the ICE > functionality so that all storage controllers containing > ICE can utilize it. In commit messages and comments, please spell out "Inline Crypto Engine (ICE)" the first time it appears, so that people know what ICE means. > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig > index 79b568f82a1c..a900f5ab6263 100644 > --- a/drivers/soc/qcom/Kconfig > +++ b/drivers/soc/qcom/Kconfig > @@ -209,4 +209,11 @@ config QCOM_APR > application processor and QDSP6. APR is > used by audio driver to configure QDSP6 > ASM, ADM and AFE modules. > + > +config QTI_ICE_COMMON > + tristate "QTI common ICE functionality" Since this is a library, it shouldn't be user-selectable, but rather just be selected by the other options that need it. Putting a string after "tristate" makes it user-selectable; the string is the prompt string. The line should just be "tristate", without a string afterwards. > diff --git a/drivers/soc/qcom/qti-ice-common.c b/drivers/soc/qcom/qti-ice-common.c > new file mode 100644 > index 000000000000..0c5b529201c5 > --- /dev/null > +++ b/drivers/soc/qcom/qti-ice-common.c > @@ -0,0 +1,199 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Common ICE library for storage encryption. > + * > + * Copyright (c) 2021, Qualcomm Innovation Center. All rights reserved. > + */ > + > +#include <linux/qti-ice-common.h> > +#include <linux/module.h> > +#include <linux/qcom_scm.h> > +#include <linux/delay.h> > +#include "qti-ice-regs.h" > + > +#define QTI_ICE_MAX_BIST_CHECK_COUNT 100 > +#define QTI_AES_256_XTS_KEY_RAW_SIZE 64 > + > +static bool qti_ice_supported(const struct ice_mmio_data *mmio) > +{ > + u32 regval = qti_ice_readl(mmio->ice_mmio, QTI_ICE_REGS_VERSION); > + int major = regval >> 24; > + int minor = (regval >> 16) & 0xFF; > + int step = regval & 0xFFFF; > + > + /* For now this driver only supports ICE version 3 and higher. */ > + if (major < 3) { > + pr_warn("Unsupported ICE version: v%d.%d.%d\n", > + major, minor, step); > + return false; > + } For log messages associated with a device, the dev_*() functions should be used instead of pr_*(). How about including the relevant 'struct device *' in the struct ice_mmio_data? This comment applies to everywhere in qti-ice-common that is logging anything. > +/** > + * qti_ice_init() - Initialize ICE functionality > + * @ice_mmio_data: contains ICE register mapping for i/o > + * > + * Initialize ICE by checking the version for ICE support and > + * also checking the fuses blown. > + * > + * Return: 0 on success; -EINVAL on failure. > + */ > +int qti_ice_init(const struct ice_mmio_data *mmio) > +{ > + if (!qti_ice_supported(mmio)) > + return -EINVAL; > + return 0; > +} > +EXPORT_SYMBOL_GPL(qti_ice_init); Be more specific about what "failure" means here. It means that the driver doesn't support the ICE hardware, right? -ENODEV would be a more appropriate error code for this. -EINVAL is a very generic error. > +static void qti_ice_low_power_and_optimization_enable( > + const struct ice_mmio_data *mmio) > +{ > + u32 regval = qti_ice_readl(mmio->ice_mmio, > + QTI_ICE_REGS_ADVANCED_CONTROL); > + > + /* Enable low power mode sequence > + * [0]-0,[1]-0,[2]-0,[3]-7,[4]-0,[5]-0,[6]-0,[7]-0, > + * Enable CONFIG_CLK_GATING, STREAM2_CLK_GATING and STREAM1_CLK_GATING > + */ > + regval |= 0x7000; > + /* Optimization enable sequence*/ > + regval |= 0xD807100; > + qti_ice_writel(mmio->ice_mmio, regval, QTI_ICE_REGS_ADVANCED_CONTROL); > + /* Memory barrier - to ensure write completion before next transaction */ > + wmb(); > +} This part changed slightly from the original code in drivers/scsi/ufs/ufs-qcom-ice.c and drivers/mmc/host/sdhci-msm.c. What is the reason for these changes? To be fair, I can't properly explain this part of the original code; I just did what some existing ICE code was doing. But if it wasn't the correct or best way, it would be super useful to understand *why* this different version is really the correct/best way. Also note that the line "regval |= 0x7000;" is redundant. > +static int qti_ice_wait_bist_status(const struct ice_mmio_data *mmio) > +{ > + int count; > + u32 regval; > + > + for (count = 0; count < QTI_ICE_MAX_BIST_CHECK_COUNT; count++) { > + regval = qti_ice_readl(mmio->ice_mmio, > + QTI_ICE_REGS_BIST_STATUS); > + if (!(regval & QTI_ICE_BIST_STATUS_MASK)) > + break; > + udelay(50); > + } > + > + if (regval) { > + pr_err("%s: wait bist status failed, reg %d\n", > + __func__, regval); > + return -ETIMEDOUT; > + } > + > + return 0; > +} The copy of this in drivers/mmc/host/sdhci-msm.c is a bit nicer in that it uses the readl_poll_timeout() helper function, and it has a longer comment explaining it. So I think you should use that version rather than the UFS version. > +/** > + * qti_ice_keyslot_program() - Program a key to an ICE slot > + * @ice_mmio_data: contains ICE register mapping for i/o > + * @crypto_key: key to be program, this can be wrapped or raw > + * @crypto_key_size: size of the key to be programmed > + * @slot: the keyslot at which the key should be programmed. > + * @data_unit_mask: mask for the dun which is part of the > + * crypto configuration. > + * @capid: capability index indicating the algorithm for the > + * crypto configuration > + * > + * Program the passed in key to a slot in ICE. > + * The key that is passed in can either be a raw key or wrapped. > + * In both cases, due to access control of ICE for Qualcomm chipsets, > + * a scm call is used to program the key into ICE from trustzone. > + * Trustzone can differentiate between raw and wrapped keys. How does TrustZone differentiate between raw and wrapped keys? I thought you had mentioned that only one is supported at a time on a particular SoC. > +int qti_ice_keyslot_program(const struct ice_mmio_data *mmio, > + const u8 *crypto_key, unsigned int crypto_key_size, > + unsigned int slot, u8 data_unit_mask, int capid) > +{ > + int err = 0; > + int i = 0; > + union { > + u8 bytes[QTI_AES_256_XTS_KEY_RAW_SIZE]; > + u32 words[QTI_AES_256_XTS_KEY_RAW_SIZE / sizeof(u32)]; > + } key; > + > + memcpy(key.bytes, crypto_key, crypto_key_size); This is assuming that wrapped keys aren't longer than raw AES-256-XTS keys, which isn't necessarily true. > +#define qti_ice_writel(mmio, val, reg) \ > + writel_relaxed((val), mmio + (reg)) > +#define qti_ice_readl(mmio, reg) \ > + readl_relaxed(mmio + (reg)) Previously, writel() and readl() were used instead of writel_relaxed() and readl_relaxed(). What is the reason for this change? My understanding is that the *_relaxed() functions are harder to use correctly, so they shouldn't be used unless it's been carefully thought through and the extra performance is needed. - Eric
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
On Mon, Dec 06, 2021 at 02:57:20PM -0800, Gaurav Kashyap wrote: > Adds support in ufshcd-core for wrapped keys. > 1. Change program key vop to support wrapped key sizes by > using blk_crypto_key directly instead of using ufs_crypto_cfg > which is not suitable for wrapped keys. > 2. Add derive_sw_secret vop and derive_sw_secret crypto_profile op. > > Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com> > --- > drivers/scsi/ufs/ufshcd-crypto.c | 52 +++++++++++++++++++++++++------- > drivers/scsi/ufs/ufshcd.h | 9 +++++- > 2 files changed, 49 insertions(+), 12 deletions(-) There will be a build error if the patch series is applied just up to here, since this patch changes the prototype of ufs_hba_variant_ops::program_key but doesn't update ufs_qcom which implements it. Every intermediate step needs to be buildable, and that's more important than keeping changes to different drivers separate. So I recommend having one patch that does the program_key change, in both ufshcd-crypto.c and ufs-qcom-ice.c. Adding derive_sw_secret should be a separate patch, and maybe should be combined with the other new methods. > diff --git a/drivers/scsi/ufs/ufshcd-crypto.c b/drivers/scsi/ufs/ufshcd-crypto.c > index 0ed82741f981..9d68621a0eb4 100644 > --- a/drivers/scsi/ufs/ufshcd-crypto.c > +++ b/drivers/scsi/ufs/ufshcd-crypto.c > @@ -18,16 +18,23 @@ static const struct ufs_crypto_alg_entry { > }; > > static int ufshcd_program_key(struct ufs_hba *hba, > + const struct blk_crypto_key *key, > const union ufs_crypto_cfg_entry *cfg, int slot) > { > int i; > u32 slot_offset = hba->crypto_cfg_register + slot * sizeof(*cfg); > int err = 0; > + bool evict = false; > > ufshcd_hold(hba, false); > > if (hba->vops && hba->vops->program_key) { > - err = hba->vops->program_key(hba, cfg, slot); > + if (!(cfg->config_enable & UFS_CRYPTO_CONFIGURATION_ENABLE)) > + evict = true; > + err = hba->vops->program_key(hba, key, slot, > + cfg->data_unit_size, > + cfg->crypto_cap_idx, > + evict); > goto out; > } This is a little weird because here we've already gone through the trouble of creating a 'union ufs_crypto_cfg_entry', only to throw it away in the ->program_key case and just use the original blk_crypto_key instead. I think that this should be refactored a bit to make it so that a 'ufs_crypto_cfg_entry' is only be initialized if program_key is not implemented. Also, note that 'struct blk_crypto_key' includes the data unit size. So there's no need to pass the data unit size as a separate argument to program_key. > +static int ufshcd_crypto_derive_sw_secret(struct blk_crypto_profile *profile, > + const u8 *wrapped_key, > + unsigned int wrapped_key_size, > + u8 sw_secret[BLK_CRYPTO_SW_SECRET_SIZE]) > +{ > + struct ufs_hba *hba = > + container_of(profile, struct ufs_hba, crypto_profile); > + > + if (hba->vops && hba->vops->derive_secret) > + return hba->vops->derive_secret(hba, wrapped_key, > + wrapped_key_size, sw_secret); There's a weird double space here. > @@ -190,7 +215,12 @@ int ufshcd_hba_init_crypto_capabilities(struct ufs_hba *hba) > hba->crypto_profile.ll_ops = ufshcd_crypto_ops; > /* UFS only supports 8 bytes for any DUN */ > hba->crypto_profile.max_dun_bytes_supported = 8; > - hba->crypto_profile.key_types_supported = BLK_CRYPTO_KEY_TYPE_STANDARD; > + if (hba->hw_wrapped_keys_supported) > + hba->crypto_profile.key_types_supported = > + BLK_CRYPTO_KEY_TYPE_HW_WRAPPED; > + else > + hba->crypto_profile.key_types_supported = > + BLK_CRYPTO_KEY_TYPE_STANDARD; "hw_wrapped_keys_supported" is confusing because it doesn't just mean that wrapped keys are supported, but also that standard keys are *not* supported. "use_hw_wrapped_keys" would be clearer. However, given that wrapped keys aren't specified by the UFS standard, I think this better belongs as a bit in hba->quirks, like UFSHCD_QUIRK_USES_WRAPPED_CRYPTO_KEYS. > + int (*derive_secret)(struct ufs_hba *hba, const u8 *wrapped_key, > + unsigned int wrapped_key_size, > + u8 sw_secret[BLK_CRYPTO_SW_SECRET_SIZE]); This probably should be called derive_sw_secret, not just derive_secret. - Eric
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
Hey Eric Have you tested that QCOM_SCM_ES_DERIVE_SW_SECRET is working properly? - You will need updated trustzone build for that (as I was missing a minor detail in the original one pertaining to SW secret) , please request again on the same ticket for the updated build. - I have reminded the people in Qualcomm too to provide you the build. - Note that with the new build you should be using the correct directions, i.e QCOM_SCM_RO where intended Warm Regards Gaurav Kashyap -----Original Message----- From: Eric Biggers <ebiggers@kernel.org> Sent: Thursday, January 6, 2022 11:48 AM To: Gaurav Kashyap (QUIC) <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; Neeraj Soni (QUIC) <quic_neersoni@quicinc.com>; Dinesh Garg <dineshg@quicinc.com> Subject: Re: [PATCH 00/10] Add wrapped key support for Qualcomm ICE WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros. Hi Gaurav, On Mon, Dec 06, 2021 at 02:57:15PM -0800, Gaurav Kashyap wrote: > Testing: > Test platform: SM8350 HDK/MTP > Engineering trustzone image (based on sm8350) is required to test this > feature. This is because of version changes of HWKM. > HWKM version 2 and moving forward has a lot of restrictions on the key > management due to which the launched SM8350 solution (based on v1) > cannot be used and some modifications are required in trustzone. I've been trying to test this patchset on a SM8350 HDK using the TrustZone image you provided, but it's not completely working yet. This is the kernel branch I'm using: https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git/log/?h=wip-wrapped-keys. It has my v4 patchset with your patchset rebased on top of it, some qcom_scm.c fixes I had to make (see below), and some extra logging messages. This is how I'm building and booting a kernel on the board: https://github.com/ebiggers/fscryptctl/blob/wip-wrapped-keys/scripts/sm8350-buildkernel.sh And this is the test script I'm running: https://github.com/ebiggers/fscryptctl/blob/wip-wrapped-keys/scripts/wrappedkey-test.sh. It imports or generates a hardware-wrapped key, then tries to set up a directory on an ext4 filesystem that is encrypted with that key. This uses new 'fscryptctl' commands to access the new blk-crypto ioctls; the version of 'fscryptctl' on the branch the scripts are on has all the needed changes. QCOM_SCM_ES_IMPORT_ICE_KEY, QCOM_SCM_ES_GENERATE_ICE_KEY, QCOM_SCM_ES_PREPARE_ICE_KEY, all seem to work. However, QCOM_SCM_ES_DERIVE_SW_SECRET doesn't work; it always returns -EINVAL. For example: Importing hardware-wrapped key [ 187.606109] blk-crypto: entering BLKCRYPTOCREATEKEY [ 187.611648] calling QCOM_SCM_ES_IMPORT_ICE_KEY; raw_key=5858585858585858585858585858585858585858585858585858585858585858 [ 187.628180] QCOM_SCM_ES_IMPORT_ICE_KEY succeeded; longterm_wrapped_key=fab51aa07fb6c2bf2fea60a8120e8d35a9e53865b594e0fb6279e7951a34864591f1c1c4e26f9421039377c1ac311ff9241a0152030000000000000000000000 [ 187.646433] blk-crypto: exiting BLKCRYPTOCREATEKEY; ret=0 Preparing hardware-wrapped key [ 187.653129] blk-crypto: entering BLKCRYPTOPREPAREKEY [ 187.660356] calling QCOM_SCM_ES_PREPARE_ICE_KEY; longterm_wrapped_key=fab51aa07fb6c2bf2fea60a8120e8d35a9e53865b594e0fb6279e7951a34864591f1c1c4e26f9421039377c1ac311ff9241a0152030000000000000000000000 [ 187.680420] QCOM_SCM_ES_PREPARE_ICE_KEY succeeded; ephemeral_wrapped_key=1fbf5d501854858d6faaf52c9d22bebc576012e40485ba75e7d19e88f74b3400eb1a8836e28232939e990df6007659b1241a0152030000000000000000000000 [ 187.698791] blk-crypto: exiting BLKCRYPTOPREPAREKEY; ret=0 Adding hardware-wrapped key [ 187.705515] calling blk_crypto_derive_sw_secret(); wrapped_key_size=68 [ 187.714075] in qti_ice_derive_sw_secret() [ 187.718212] calling qti_ice_hwkm_init() [ 187.722157] calling qti_ice_hwkm_init_sequence(version=1) [ 187.727715] setting standard mode [ 187.731134] checking BIST status [ 187.734464] configuring ICE registers [ 187.738230] disabling CRC check [ 187.741479] setting RSP_FIFO_FULL bit [ 187.745247] calling qcom_scm_derive_sw_secret() [ 187.749920] calling QCOM_SCM_ES_DERIVE_SW_SECRET; wrapped_key=1fbf5d501854858d6faaf52c9d22bebc576012e40485ba75e7d19e88f74b3400eb1a8836e28232939e990df6007659b1241a0152030000000000000000000000, secret_size=32 [ 187.768834] QCOM_SCM_ES_DERIVE_SW_SECRET failed with error -22 [ 187.774838] blk_crypto_derive_sw_secret() returned -22 error: adding key to /mnt: Invalid argument You can see that the wrapped_key being passed to QCOM_SCM_ES_DERIVE_SW_SECRET matches the ephemeral_wrapped_key that was returned earlier by QCOM_SCM_ES_PREPARE_ICE_KEY, and that secret_size is 32. So the arguments are as expected. However, QCOM_SCM_ES_DERIVE_SW_SECRET still fails. This still occurs if QCOM_SCM_ES_GENERATE_ICE_KEY is used instead of QCOM_SCM_ES_IMPORT_ICE_KEY. Have you tested that QCOM_SCM_ES_DERIVE_SW_SECRET is working properly? For reference, these are the fixes I had to apply to qcom_scm.c to get things working until that point. This included fixing the direction of the first arguments to the SCM calls, and fixing the return values. Note, I also tested leaving QCOM_SCM_ES_DERIVE_SW_SECRET using QCOM_SCM_RO instead of QCOM_SCM_RW, but the result was still the same --- it still returned -EINVAL. diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c index d57f52015640..002b57a1473d 100644 --- a/drivers/firmware/qcom_scm.c +++ b/drivers/firmware/qcom_scm.c @@ -1087,7 +1087,7 @@ int qcom_scm_derive_sw_secret(const u8 *wrapped_key, u32 wrapped_key_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_RO, + .arginfo = QCOM_SCM_ARGS(4, QCOM_SCM_RW, QCOM_SCM_VAL, QCOM_SCM_RW, QCOM_SCM_VAL), .args[1] = wrapped_key_size, @@ -1148,7 +1148,7 @@ EXPORT_SYMBOL(qcom_scm_derive_sw_secret); * 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. + * Return: size of the resulting key on success; -errno on failure. */ int qcom_scm_generate_ice_key(u8 *longterm_wrapped_key, u32 longterm_wrapped_key_size) @@ -1188,7 +1188,7 @@ int qcom_scm_generate_ice_key(u8 *longterm_wrapped_key, dma_free_coherent(__scm->dev, longterm_wrapped_key_size, longterm_wrapped_keybuf, longterm_wrapped_key_phys); - return ret; + return ret ?: longterm_wrapped_key_size; } EXPORT_SYMBOL(qcom_scm_generate_ice_key); @@ -1209,7 +1209,7 @@ EXPORT_SYMBOL(qcom_scm_generate_ice_key); * with the secure environment to rewrap the wrapped key with an * ephemeral wrapping key. * - * Return: 0 on success; -errno on failure. + * Return: size of the resulting key on success; -errno on failure. */ int qcom_scm_prepare_ice_key(const u8 *longterm_wrapped_key, u32 longterm_wrapped_key_size, @@ -1219,7 +1219,7 @@ int qcom_scm_prepare_ice_key(const u8 *longterm_wrapped_key, struct qcom_scm_desc desc = { .svc = QCOM_SCM_SVC_ES, .cmd = QCOM_SCM_ES_PREPARE_ICE_KEY, - .arginfo = QCOM_SCM_ARGS(4, QCOM_SCM_RO, + .arginfo = QCOM_SCM_ARGS(4, QCOM_SCM_RW, QCOM_SCM_VAL, QCOM_SCM_RW, QCOM_SCM_VAL), .args[1] = longterm_wrapped_key_size, @@ -1270,7 +1270,7 @@ int qcom_scm_prepare_ice_key(const u8 *longterm_wrapped_key, dma_free_coherent(__scm->dev, longterm_wrapped_key_size, longterm_wrapped_keybuf, longterm_wrapped_key_phys); - return ret; + return ret ?: ephemeral_wrapped_key_size; } EXPORT_SYMBOL(qcom_scm_prepare_ice_key); @@ -1289,7 +1289,7 @@ EXPORT_SYMBOL(qcom_scm_prepare_ice_key); * the secure environment to import a raw key and generate a longterm * wrapped key. * - * Return: 0 on success; -errno on failure. + * Return: size of the resulting key on success; -errno on failure. */ int qcom_scm_import_ice_key(const u8 *imported_key, u32 imported_key_size, u8 *longterm_wrapped_key, @@ -1298,7 +1298,7 @@ int qcom_scm_import_ice_key(const u8 *imported_key, u32 imported_key_size, struct qcom_scm_desc desc = { .svc = QCOM_SCM_SVC_ES, .cmd = QCOM_SCM_ES_IMPORT_ICE_KEY, - .arginfo = QCOM_SCM_ARGS(4, QCOM_SCM_RO, + .arginfo = QCOM_SCM_ARGS(4, QCOM_SCM_RW, QCOM_SCM_VAL, QCOM_SCM_RW, QCOM_SCM_VAL), .args[1] = imported_key_size, @@ -1344,7 +1344,7 @@ int qcom_scm_import_ice_key(const u8 *imported_key, u32 imported_key_size, dma_free_coherent(__scm->dev, imported_key_size, imported_keybuf, imported_key_phys); - return ret; + return ret ?: longterm_wrapped_key_size; } EXPORT_SYMBOL(qcom_scm_import_ice_key);
Hi Gaurav, On Thu, Jan 06, 2022 at 09:14:22PM +0000, Gaurav Kashyap wrote: > Hey Eric > > > Have you tested that QCOM_SCM_ES_DERIVE_SW_SECRET is working properly? > > - You will need updated trustzone build for that (as I was missing a minor detail in the original one pertaining to SW secret) , please request again on the same ticket for the updated build. > - I have reminded the people in Qualcomm too to provide you the build. > - Note that with the new build you should be using the correct directions, i.e QCOM_SCM_RO where intended > > Warm Regards > Gaurav Kashyap > I verified that the latest TrustZone build is working; thanks for the help! Note, these are the branches I'm using for now: * Kernel patches: https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git/log/?h=wip-wrapped-keys * fscryptctl tool and test scripts: https://github.com/ebiggers/fscryptctl/tree/wip-wrapped-keys The kernel branch is based on v5.17-rc1. I haven't changed your patches from your latest series other than rebasing them and adding a fix "qcom_scm: fix return values" on top. Note that v5.16-rc5 and later have a bug where the UFS controller on SM8350 isn't recognized. Therefore, my branch contains a fix from Bjorn Andersson for that bug, which I applied from the mailing list. One oddity I noticed is that if I import the same raw key twice, the long-term wrapped key blob is the same. This implies that the key encryption algorithm used by the Qualcomm hardware is deterministic, which is unexpected. I would expect the wrapped key to contain a random nonce. Do you know why deterministic encryption is used? This probably isn't much of a problem, but it's unexpected. Besides that, I think the next steps are for you to address the comments I've left on this series, and for me to get started on adding ciphertext verification tests for this to xfstests (alongside the other fscrypt ciphertext verification tests that are already there) so that we can prove this feature is actually encrypting the data as intended. - Eric