| Message ID | 20200304064942.371978-4-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Inline crypto support on DragonBoard 845c | expand |
Eric, I strongly recommend not to support the old mechanism of calling into TEE to set keys as this has been deprecated and will not work with newer hardware. There are few issues with this patch, it adds all the code within UFS and we would have to reimplement all the common ICE code for eMMC as well. For clearing a key, the patch uses program_key to set zeroes to the keyslot, without going into details (since newer hardware is not yet public) this will not work. We have a plan to upstream ICE support with the new hardware along with the framework to support wrapped keys and add sdhci/cqhci-crypto support. -----Original Message----- From: linux-fscrypt-owner@vger.kernel.org <linux-fscrypt-owner@vger.kernel.org> On Behalf Of Eric Biggers Sent: Tuesday, March 3, 2020 10:50 PM To: linux-scsi@vger.kernel.org; linux-arm-msm@vger.kernel.org Cc: linux-block@vger.kernel.org; linux-fscrypt@vger.kernel.org; Alim Akhtar <alim.akhtar@samsung.com>; Andy Gross <agross@kernel.org>; Avri Altman <avri.altman@wdc.com>; Barani Muthukumaran <bmuthuku@qti.qualcomm.com>; Bjorn Andersson <bjorn.andersson@linaro.org>; Can Guo <cang@codeaurora.org>; Elliot Berman <eberman@codeaurora.org>; Jaegeuk Kim <jaegeuk@kernel.org> Subject: [RFC PATCH v2 3/4] scsi: ufs: add program_key() variant op From: Eric Biggers <ebiggers@google.com> On Snapdragon SoCs, the Linux kernel isn't permitted to directly access the standard UFS crypto configuration registers. Instead, programming and evicting keys must be done through vendor-specific SMC calls. To support this hardware, add a ->program_key() method to 'struct ufs_hba_variant_ops'. This allows overriding the UFS standard key programming / eviction procedure. Signed-off-by: Eric Biggers <ebiggers@google.com> --- drivers/scsi/ufs/ufshcd-crypto.c | 34 +++++++++++++++++++++----------- drivers/scsi/ufs/ufshcd.h | 3 +++ 2 files changed, 25 insertions(+), 12 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd-crypto.c b/drivers/scsi/ufs/ufshcd-crypto.c index cd7ca50a1dd9..7c50d1d4f58c 100644 --- a/drivers/scsi/ufs/ufshcd-crypto.c +++ b/drivers/scsi/ufs/ufshcd-crypto.c @@ -131,14 +131,20 @@ static int ufshcd_crypto_cfg_entry_write_key(union ufs_crypto_cfg_entry *cfg, return -EINVAL; } -static void ufshcd_program_key(struct ufs_hba *hba, - const union ufs_crypto_cfg_entry *cfg, - int slot) +static int ufshcd_program_key(struct ufs_hba *hba, + const union ufs_crypto_cfg_entry *cfg, int slot) { int i; u32 slot_offset = hba->crypto_cfg_register + slot * sizeof(*cfg); + int err = 0; ufshcd_hold(hba, false); + + if (hba->vops && hba->vops->program_key) { + err = hba->vops->program_key(hba, cfg, slot); + goto out; + } + /* Ensure that CFGE is cleared before programming the key */ ufshcd_writel(hba, 0, slot_offset + 16 * sizeof(cfg->reg_val[0])); for (i = 0; i < 16; i++) { @@ -151,23 +157,28 @@ static void ufshcd_program_key(struct ufs_hba *hba, /* Dword 16 must be written last */ ufshcd_writel(hba, le32_to_cpu(cfg->reg_val[16]), slot_offset + 16 * sizeof(cfg->reg_val[0])); +out: ufshcd_release(hba); + return err; } -static void ufshcd_clear_keyslot(struct ufs_hba *hba, int slot) +static int ufshcd_clear_keyslot(struct ufs_hba *hba, int slot) { union ufs_crypto_cfg_entry cfg = { 0 }; - ufshcd_program_key(hba, &cfg, slot); + return ufshcd_program_key(hba, &cfg, slot); } /* Clear all keyslots at driver init time */ static void ufshcd_clear_all_keyslots(struct ufs_hba *hba) { int slot; + int err; - for (slot = 0; slot < ufshcd_num_keyslots(hba); slot++) - ufshcd_clear_keyslot(hba, slot); + for (slot = 0; slot < ufshcd_num_keyslots(hba); slot++) { + err = ufshcd_clear_keyslot(hba, slot); + WARN_ON_ONCE(err); + } } static int ufshcd_crypto_keyslot_program(struct keyslot_manager *ksm, @@ -203,10 +214,11 @@ static int ufshcd_crypto_keyslot_program(struct keyslot_manager *ksm, if (err) return err; - ufshcd_program_key(hba, &cfg, slot); + err = ufshcd_program_key(hba, &cfg, slot); memzero_explicit(&cfg, sizeof(cfg)); - return 0; + + return err; } static int ufshcd_crypto_keyslot_evict(struct keyslot_manager *ksm, @@ -223,9 +235,7 @@ static int ufshcd_crypto_keyslot_evict(struct keyslot_manager *ksm, * Clear the crypto cfg on the device. Clearing CFGE * might not be sufficient, so just clear the entire cfg. */ - ufshcd_clear_keyslot(hba, slot); - - return 0; + return ufshcd_clear_keyslot(hba, slot); } void ufshcd_crypto_enable(struct ufs_hba *hba) diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index c8f948aa5e3d..c2656575e24b 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -306,6 +306,7 @@ struct ufs_pwr_mode_info { * @dbg_register_dump: used to dump controller debug information * @phy_initialization: used to initialize phys * @device_reset: called to issue a reset pulse on the UFS device + * @program_key: program or evict an inline encryption key */ struct ufs_hba_variant_ops { const char *name; @@ -335,6 +336,8 @@ struct ufs_hba_variant_ops { void (*dbg_register_dump)(struct ufs_hba *hba); int (*phy_initialization)(struct ufs_hba *); void (*device_reset)(struct ufs_hba *hba); + int (*program_key)(struct ufs_hba *hba, + const union ufs_crypto_cfg_entry *cfg, int slot); }; /* clock gating state */ -- 2.25.1
Hi Barani, On Wed, Mar 04, 2020 at 12:18:20PM -0800, bmuthuku@codeaurora.org wrote: > Eric, I strongly recommend not to support the old mechanism of calling into > TEE to set keys as this has been deprecated and will not work with newer > hardware. Actually, I've also verified that this driver works nearly as-is on Snapdragon 865 and Snapdragon 765. (Those SoCs currently lack upstream support, but I tested with downstream kernels.) IIUC, this is the latest Qualcomm hardware in the real world. Lots of phones will be shipping with those SoCS this year. So your position is that crypto support for current Qualcomm hardware shouldn't be added, and instead we should wait years for new hardware that hasn't even been announced/released yet and has no upstream kernel support at all yet? Snapdragon 845 (via DragonBoard 845c) already has upstream support. We can easily support multiple hardware versions in the driver, so I don't see why this is an issue. In particular, if Qualcomm releases new hardware that just complies with the UFS standard and doesn't require these weird vendor-specific SCM calls, we can just conditionally skip the quirks when standard hardware is detected. > There are few issues with this patch, it adds all the code within > UFS and we would have to reimplement all the common ICE code for eMMC as > well. The downstream solution of having a separate ICE "device" and "driver" is really horrendous and results in lots of layering violations and unnecessary code, since as far as I can tell ICE is actually part of the UFS and eMMC controllers and not a separate device. So I think starting with the ICE logic in ufs-qcom is the right approach. If it turns out that sdhci-msm needs similar ICE management code, we can put it in a library. Though, note that after I removed all the unneeded stuff, we're only talking about ~200 lines of code, so it may not even be worthwhile. However, an issue with eMMC is that the eMMC crypto standard hasn't actually been published yet. And I haven't heard of any vendors implementing it besides Qualcomm, nor do I know of any upstream SoC we can use to develop it. So if you want eMMC crypto support, you're going to have to help with it. > For clearing a key, the patch uses program_key to set zeroes to the > keyslot, without going into details (since newer hardware is not yet public) > this will not work. That's incorrect; this patch uses the SCM call QCOM_SCM_ES_INVALIDATE_ICE_KEY to evict keys. That's exactly what the downstream driver does. Of course, when we add support for newer hardware we can use whatever keyslot management method works on that hardware. Hopefully, it's the standard UFS method and not another weird vendor-specific method. > We have a plan to upstream ICE support with the new hardware along with the > framework to support wrapped keys and add sdhci/cqhci-crypto support. Great, what's the timeline on this? Are there patches you can point to anywhere? How would one get ahold of this hardware, and does it / will it have upstream kernel support like sda845 does? - Eric
diff --git a/drivers/scsi/ufs/ufshcd-crypto.c b/drivers/scsi/ufs/ufshcd-crypto.c index cd7ca50a1dd9..7c50d1d4f58c 100644 --- a/drivers/scsi/ufs/ufshcd-crypto.c +++ b/drivers/scsi/ufs/ufshcd-crypto.c @@ -131,14 +131,20 @@ static int ufshcd_crypto_cfg_entry_write_key(union ufs_crypto_cfg_entry *cfg, return -EINVAL; } -static void ufshcd_program_key(struct ufs_hba *hba, - const union ufs_crypto_cfg_entry *cfg, - int slot) +static int ufshcd_program_key(struct ufs_hba *hba, + const union ufs_crypto_cfg_entry *cfg, int slot) { int i; u32 slot_offset = hba->crypto_cfg_register + slot * sizeof(*cfg); + int err = 0; ufshcd_hold(hba, false); + + if (hba->vops && hba->vops->program_key) { + err = hba->vops->program_key(hba, cfg, slot); + goto out; + } + /* Ensure that CFGE is cleared before programming the key */ ufshcd_writel(hba, 0, slot_offset + 16 * sizeof(cfg->reg_val[0])); for (i = 0; i < 16; i++) { @@ -151,23 +157,28 @@ static void ufshcd_program_key(struct ufs_hba *hba, /* Dword 16 must be written last */ ufshcd_writel(hba, le32_to_cpu(cfg->reg_val[16]), slot_offset + 16 * sizeof(cfg->reg_val[0])); +out: ufshcd_release(hba); + return err; } -static void ufshcd_clear_keyslot(struct ufs_hba *hba, int slot) +static int ufshcd_clear_keyslot(struct ufs_hba *hba, int slot) { union ufs_crypto_cfg_entry cfg = { 0 }; - ufshcd_program_key(hba, &cfg, slot); + return ufshcd_program_key(hba, &cfg, slot); } /* Clear all keyslots at driver init time */ static void ufshcd_clear_all_keyslots(struct ufs_hba *hba) { int slot; + int err; - for (slot = 0; slot < ufshcd_num_keyslots(hba); slot++) - ufshcd_clear_keyslot(hba, slot); + for (slot = 0; slot < ufshcd_num_keyslots(hba); slot++) { + err = ufshcd_clear_keyslot(hba, slot); + WARN_ON_ONCE(err); + } } static int ufshcd_crypto_keyslot_program(struct keyslot_manager *ksm, @@ -203,10 +214,11 @@ static int ufshcd_crypto_keyslot_program(struct keyslot_manager *ksm, if (err) return err; - ufshcd_program_key(hba, &cfg, slot); + err = ufshcd_program_key(hba, &cfg, slot); memzero_explicit(&cfg, sizeof(cfg)); - return 0; + + return err; } static int ufshcd_crypto_keyslot_evict(struct keyslot_manager *ksm, @@ -223,9 +235,7 @@ static int ufshcd_crypto_keyslot_evict(struct keyslot_manager *ksm, * Clear the crypto cfg on the device. Clearing CFGE * might not be sufficient, so just clear the entire cfg. */ - ufshcd_clear_keyslot(hba, slot); - - return 0; + return ufshcd_clear_keyslot(hba, slot); } void ufshcd_crypto_enable(struct ufs_hba *hba) diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index c8f948aa5e3d..c2656575e24b 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -306,6 +306,7 @@ struct ufs_pwr_mode_info { * @dbg_register_dump: used to dump controller debug information * @phy_initialization: used to initialize phys * @device_reset: called to issue a reset pulse on the UFS device + * @program_key: program or evict an inline encryption key */ struct ufs_hba_variant_ops { const char *name; @@ -335,6 +336,8 @@ struct ufs_hba_variant_ops { void (*dbg_register_dump)(struct ufs_hba *hba); int (*phy_initialization)(struct ufs_hba *); void (*device_reset)(struct ufs_hba *hba); + int (*program_key)(struct ufs_hba *hba, + const union ufs_crypto_cfg_entry *cfg, int slot); }; /* clock gating state */