| Message ID | 20241209045530.507833-1-ebiggers@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Support for hardware-wrapped inline encryption keys | expand |
On Mon, Dec 9, 2024 at 5:56 AM Eric Biggers <ebiggers@kernel.org> wrote: > > This patchset is also available in git via: > > git fetch https://git.kernel.org/pub/scm/fs/fscrypt/linux.git wrapped-keys-v9 > > This is a reworked version of the patchset > https://lore.kernel.org/linux-fscrypt/20241202-wrapped-keys-v7-0-67c3ca3f3282@linaro.org/T/#u > that was recently sent by Bartosz Golaszewski. It turned out there were a lot > of things I wanted to fix, and it would have taken much too long to address them > in a code review. For now this is build-tested only; I've errored on the side > of sending this out early since others are working on this too. Besides many > miscellaneous fixes and cleanups, I've made the following notable changes: > > - ufs-qcom and sdhci-msm now just initialize the blk_crypto_profile themselves, > like what ufs-exynos was doing. This avoids needing to add all the > host-specific hooks for wrapped key support to the MMC and UFS core drivers. > > - When passing the blk_crypto_key further down the stack, it now replaces > parameters like the algorithm ID, to avoid creating two sources of truth. > > - The module parameter qcom_ice.use_wrapped_keys should work correctly now. > > - The fscrypt support no longer uses a policy flag to indicate when a file is > protected by a HW-wrapped key, since it was already implied by the file's key > identifier being that of a HW-wrapped key. Originally there was an issue > where raw and HW-wrapped keys could share key identifiers, but I had fixed > that earlier by introducing a new HKDF context byte. > > - The term "standard keys" is no longer used. Now "raw keys" is consistently > used instead. I've found that people find the term "raw keys" to be more > intuitive. Also HW-wrapped keys could in principle be standardized. > > - I've reordered the patchset to place preparatory patches that don't depend on > the actual HW-wrapped key support first. > > My current thinking is that for 6.14 we should just aim to get the preparatory > patches 1-5 merged via the ufs and mmc trees, while the actual HW-wrapped key > support continues to be finalized and properly tested. But let me know if > anyone has any other thoughts. > > A quick intro to the patchset for anyone who hasn't been following along: > > This patchset adds support for hardware-wrapped inline encryption keys, a > security feature supported by some SoCs and that has already seen a lot of > real-world use downstream. It adds the block and fscrypt framework for the > feature as well as support for it with UFS on Qualcomm SoCs. > > This feature is described in full detail in the included Documentation changes. > But to summarize, hardware-wrapped keys are inline encryption keys that are > wrapped (encrypted) by a key internal to the hardware so that they can only be > unwrapped (decrypted) by the hardware. Initially keys are wrapped with a > permanent hardware key, but during actual use they are re-wrapped with a > per-boot ephemeral key for improved security. The hardware supports importing > keys as well as generating keys itself. > > This differs from the existing support for hardware-wrapped keys in the kernel > crypto API (which also goes by names such as "hardware-bound keys", depending on > the driver) in the same way that the crypto API differs from blk-crypto: the > crypto API is for general crypto operations, whereas blk-crypto is for inline > storage encryption. > > This feature is already being used by Android downstream for several years > (https://source.android.com/docs/security/features/encryption/hw-wrapped-keys), > but on other platforms userspace support will be provided via fscryptctl and > tests via xfstests (I have some old patches for this that need to be updated). > > Eric Biggers (10): > ufs: crypto: add ufs_hba_from_crypto_profile() > ufs: qcom: convert to use UFSHCD_QUIRK_CUSTOM_CRYPTO_PROFILE > mmc: crypto: add mmc_from_crypto_profile() > mmc: sdhci-msm: convert to use custom crypto profile > soc: qcom: ice: make qcom_ice_program_key() take struct blk_crypto_key > blk-crypto: add basic hardware-wrapped key support > blk-crypto: show supported key types in sysfs > blk-crypto: add ioctls to create and prepare hardware-wrapped keys > fscrypt: add support for hardware-wrapped keys > ufs: qcom: add support for wrapped keys > > Gaurav Kashyap (2): > firmware: qcom: scm: add calls for wrapped key support > soc: qcom: ice: add HWKM support to the ICE driver > > Documentation/ABI/stable/sysfs-block | 18 + > Documentation/block/inline-encryption.rst | 251 +++++++++++- > Documentation/filesystems/fscrypt.rst | 201 +++++++-- > .../userspace-api/ioctl/ioctl-number.rst | 2 + > block/blk-crypto-fallback.c | 7 +- > block/blk-crypto-internal.h | 10 + > block/blk-crypto-profile.c | 103 +++++ > block/blk-crypto-sysfs.c | 35 ++ > block/blk-crypto.c | 196 ++++++++- > block/ioctl.c | 5 + > drivers/firmware/qcom/qcom_scm.c | 214 ++++++++++ > drivers/firmware/qcom/qcom_scm.h | 4 + > drivers/md/dm-table.c | 1 + > drivers/mmc/host/cqhci-crypto.c | 38 +- > drivers/mmc/host/cqhci.h | 8 +- > drivers/mmc/host/sdhci-msm.c | 102 +++-- > drivers/soc/qcom/ice.c | 383 +++++++++++++++++- > drivers/ufs/core/ufshcd-crypto.c | 33 +- > drivers/ufs/host/ufs-exynos.c | 3 +- > drivers/ufs/host/ufs-qcom.c | 137 +++++-- > fs/crypto/fscrypt_private.h | 75 +++- > fs/crypto/hkdf.c | 4 +- > fs/crypto/inline_crypt.c | 42 +- > fs/crypto/keyring.c | 157 +++++-- > fs/crypto/keysetup.c | 63 ++- > fs/crypto/keysetup_v1.c | 4 +- > include/linux/blk-crypto-profile.h | 73 ++++ > include/linux/blk-crypto.h | 73 +++- > include/linux/firmware/qcom/qcom_scm.h | 8 + > include/linux/mmc/host.h | 8 + > include/soc/qcom/ice.h | 34 +- > include/uapi/linux/blk-crypto.h | 44 ++ > include/uapi/linux/fs.h | 6 +- > include/uapi/linux/fscrypt.h | 7 +- > include/ufs/ufshcd.h | 11 +- > 35 files changed, 2091 insertions(+), 269 deletions(-) > create mode 100644 include/uapi/linux/blk-crypto.h > > > base-commit: f486c8aa16b8172f63bddc70116a0c897a7f3f02 > -- > 2.47.1 > I haven't gotten to the bottom of this yet but the FS_IOC_ADD_ENCRYPTION_KEY ioctl doesn't work due to the SCM call returning EINVAL. Just FYI. I'm still figuring out what's wrong. Bart
On Mon, Dec 09, 2024 at 04:00:18PM +0100, Bartosz Golaszewski wrote: > > I haven't gotten to the bottom of this yet but the > FS_IOC_ADD_ENCRYPTION_KEY ioctl doesn't work due to the SCM call > returning EINVAL. Just FYI. I'm still figuring out what's wrong. > > Bart > Can you try the following? diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c index 180220d663f8b..36f3ddcb90207 100644 --- a/drivers/firmware/qcom/qcom_scm.c +++ b/drivers/firmware/qcom/qcom_scm.c @@ -1330,11 +1330,11 @@ int qcom_scm_derive_sw_secret(const u8 *eph_key, size_t eph_key_size, sw_secret_size, GFP_KERNEL); if (!sw_secret_buf) return -ENOMEM; - memcpy(eph_key_buf, eph_key_buf, eph_key_size); + memcpy(eph_key_buf, eph_key, eph_key_size); desc.args[0] = qcom_tzmem_to_phys(eph_key_buf); desc.args[1] = eph_key_size; desc.args[2] = qcom_tzmem_to_phys(sw_secret_buf); desc.args[3] = sw_secret_size;
On Mon, 9 Dec 2024 21:15:16 +0100, Eric Biggers <ebiggers@kernel.org> said: > On Mon, Dec 09, 2024 at 04:00:18PM +0100, Bartosz Golaszewski wrote: >> >> I haven't gotten to the bottom of this yet but the >> FS_IOC_ADD_ENCRYPTION_KEY ioctl doesn't work due to the SCM call >> returning EINVAL. Just FYI. I'm still figuring out what's wrong. >> >> Bart >> > > Can you try the following? > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c > index 180220d663f8b..36f3ddcb90207 100644 > --- a/drivers/firmware/qcom/qcom_scm.c > +++ b/drivers/firmware/qcom/qcom_scm.c > @@ -1330,11 +1330,11 @@ int qcom_scm_derive_sw_secret(const u8 *eph_key, size_t eph_key_size, > sw_secret_size, > GFP_KERNEL); > if (!sw_secret_buf) > return -ENOMEM; > > - memcpy(eph_key_buf, eph_key_buf, eph_key_size); > + memcpy(eph_key_buf, eph_key, eph_key_size); > desc.args[0] = qcom_tzmem_to_phys(eph_key_buf); > desc.args[1] = eph_key_size; > desc.args[2] = qcom_tzmem_to_phys(sw_secret_buf); > desc.args[3] = sw_secret_size; > > That's better, thanks. Now it's fscryptctl set_policy that fails like this: ioctl(3, FS_IOC_SET_ENCRYPTION_POLICY, 0xffffcaf8bb20) = -1 EINVAL (Invalid argument) Bartosz
On Mon, Dec 9, 2024 at 9:35 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Mon, 9 Dec 2024 21:15:16 +0100, Eric Biggers <ebiggers@kernel.org> said: > > On Mon, Dec 09, 2024 at 04:00:18PM +0100, Bartosz Golaszewski wrote: > >> > >> I haven't gotten to the bottom of this yet but the > >> FS_IOC_ADD_ENCRYPTION_KEY ioctl doesn't work due to the SCM call > >> returning EINVAL. Just FYI. I'm still figuring out what's wrong. > >> > >> Bart > >> > > > > Can you try the following? > > > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c > > index 180220d663f8b..36f3ddcb90207 100644 > > --- a/drivers/firmware/qcom/qcom_scm.c > > +++ b/drivers/firmware/qcom/qcom_scm.c > > @@ -1330,11 +1330,11 @@ int qcom_scm_derive_sw_secret(const u8 *eph_key, size_t eph_key_size, > > sw_secret_size, > > GFP_KERNEL); > > if (!sw_secret_buf) > > return -ENOMEM; > > > > - memcpy(eph_key_buf, eph_key_buf, eph_key_size); > > + memcpy(eph_key_buf, eph_key, eph_key_size); > > desc.args[0] = qcom_tzmem_to_phys(eph_key_buf); > > desc.args[1] = eph_key_size; > > desc.args[2] = qcom_tzmem_to_phys(sw_secret_buf); > > desc.args[3] = sw_secret_size; > > > > > > That's better, thanks. Now it's fscryptctl set_policy that fails like this: > > ioctl(3, FS_IOC_SET_ENCRYPTION_POLICY, 0xffffcaf8bb20) = -1 EINVAL (Invalid argument) > > Bartosz FYI: It fails the: `if (!fscrypt_supported_policy(policy, inode))` check in set_encryption_policy() in fs/crypto/policy.c. Bartosz
On Mon, Dec 09, 2024 at 02:35:29PM -0600, Bartosz Golaszewski wrote: > On Mon, 9 Dec 2024 21:15:16 +0100, Eric Biggers <ebiggers@kernel.org> said: > > On Mon, Dec 09, 2024 at 04:00:18PM +0100, Bartosz Golaszewski wrote: > >> > >> I haven't gotten to the bottom of this yet but the > >> FS_IOC_ADD_ENCRYPTION_KEY ioctl doesn't work due to the SCM call > >> returning EINVAL. Just FYI. I'm still figuring out what's wrong. > >> > >> Bart > >> > > > > Can you try the following? > > > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c > > index 180220d663f8b..36f3ddcb90207 100644 > > --- a/drivers/firmware/qcom/qcom_scm.c > > +++ b/drivers/firmware/qcom/qcom_scm.c > > @@ -1330,11 +1330,11 @@ int qcom_scm_derive_sw_secret(const u8 *eph_key, size_t eph_key_size, > > sw_secret_size, > > GFP_KERNEL); > > if (!sw_secret_buf) > > return -ENOMEM; > > > > - memcpy(eph_key_buf, eph_key_buf, eph_key_size); > > + memcpy(eph_key_buf, eph_key, eph_key_size); > > desc.args[0] = qcom_tzmem_to_phys(eph_key_buf); > > desc.args[1] = eph_key_size; > > desc.args[2] = qcom_tzmem_to_phys(sw_secret_buf); > > desc.args[3] = sw_secret_size; > > > > > > That's better, thanks. Now it's fscryptctl set_policy that fails like this: > > ioctl(3, FS_IOC_SET_ENCRYPTION_POLICY, 0xffffcaf8bb20) = -1 EINVAL > (Invalid argument) > Yes, as I mentioned I decided to drop the new encryption policy flag and go back to just relying on the key. I assume you were using https://github.com/ebiggers/fscryptctl/tree/wip-wrapped-keys? I have pushed out an updated version of that that should work. - Eric
On Mon, Dec 9, 2024 at 9:55 PM Eric Biggers <ebiggers@kernel.org> wrote: > > On Mon, Dec 09, 2024 at 02:35:29PM -0600, Bartosz Golaszewski wrote: > > On Mon, 9 Dec 2024 21:15:16 +0100, Eric Biggers <ebiggers@kernel.org> said: > > > On Mon, Dec 09, 2024 at 04:00:18PM +0100, Bartosz Golaszewski wrote: > > >> > > >> I haven't gotten to the bottom of this yet but the > > >> FS_IOC_ADD_ENCRYPTION_KEY ioctl doesn't work due to the SCM call > > >> returning EINVAL. Just FYI. I'm still figuring out what's wrong. > > >> > > >> Bart > > >> > > > > > > Can you try the following? > > > > > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c > > > index 180220d663f8b..36f3ddcb90207 100644 > > > --- a/drivers/firmware/qcom/qcom_scm.c > > > +++ b/drivers/firmware/qcom/qcom_scm.c > > > @@ -1330,11 +1330,11 @@ int qcom_scm_derive_sw_secret(const u8 *eph_key, size_t eph_key_size, > > > sw_secret_size, > > > GFP_KERNEL); > > > if (!sw_secret_buf) > > > return -ENOMEM; > > > > > > - memcpy(eph_key_buf, eph_key_buf, eph_key_size); > > > + memcpy(eph_key_buf, eph_key, eph_key_size); > > > desc.args[0] = qcom_tzmem_to_phys(eph_key_buf); > > > desc.args[1] = eph_key_size; > > > desc.args[2] = qcom_tzmem_to_phys(sw_secret_buf); > > > desc.args[3] = sw_secret_size; > > > > > > > > > > That's better, thanks. Now it's fscryptctl set_policy that fails like this: > > > > ioctl(3, FS_IOC_SET_ENCRYPTION_POLICY, 0xffffcaf8bb20) = -1 EINVAL > > (Invalid argument) > > > > Yes, as I mentioned I decided to drop the new encryption policy flag and go back > to just relying on the key. I assume you were using > https://github.com/ebiggers/fscryptctl/tree/wip-wrapped-keys? I have pushed out > an updated version of that that should work. > > - Eric Thanks, with that and the memcpy() fix: Tested-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> # sm8650
This patchset is also available in git via: git fetch https://git.kernel.org/pub/scm/fs/fscrypt/linux.git wrapped-keys-v9 This is a reworked version of the patchset https://lore.kernel.org/linux-fscrypt/20241202-wrapped-keys-v7-0-67c3ca3f3282@linaro.org/T/#u that was recently sent by Bartosz Golaszewski. It turned out there were a lot of things I wanted to fix, and it would have taken much too long to address them in a code review. For now this is build-tested only; I've errored on the side of sending this out early since others are working on this too. Besides many miscellaneous fixes and cleanups, I've made the following notable changes: - ufs-qcom and sdhci-msm now just initialize the blk_crypto_profile themselves, like what ufs-exynos was doing. This avoids needing to add all the host-specific hooks for wrapped key support to the MMC and UFS core drivers. - When passing the blk_crypto_key further down the stack, it now replaces parameters like the algorithm ID, to avoid creating two sources of truth. - The module parameter qcom_ice.use_wrapped_keys should work correctly now. - The fscrypt support no longer uses a policy flag to indicate when a file is protected by a HW-wrapped key, since it was already implied by the file's key identifier being that of a HW-wrapped key. Originally there was an issue where raw and HW-wrapped keys could share key identifiers, but I had fixed that earlier by introducing a new HKDF context byte. - The term "standard keys" is no longer used. Now "raw keys" is consistently used instead. I've found that people find the term "raw keys" to be more intuitive. Also HW-wrapped keys could in principle be standardized. - I've reordered the patchset to place preparatory patches that don't depend on the actual HW-wrapped key support first. My current thinking is that for 6.14 we should just aim to get the preparatory patches 1-5 merged via the ufs and mmc trees, while the actual HW-wrapped key support continues to be finalized and properly tested. But let me know if anyone has any other thoughts. A quick intro to the patchset for anyone who hasn't been following along: This patchset adds support for hardware-wrapped inline encryption keys, a security feature supported by some SoCs and that has already seen a lot of real-world use downstream. It adds the block and fscrypt framework for the feature as well as support for it with UFS on Qualcomm SoCs. This feature is described in full detail in the included Documentation changes. But to summarize, hardware-wrapped keys are inline encryption keys that are wrapped (encrypted) by a key internal to the hardware so that they can only be unwrapped (decrypted) by the hardware. Initially keys are wrapped with a permanent hardware key, but during actual use they are re-wrapped with a per-boot ephemeral key for improved security. The hardware supports importing keys as well as generating keys itself. This differs from the existing support for hardware-wrapped keys in the kernel crypto API (which also goes by names such as "hardware-bound keys", depending on the driver) in the same way that the crypto API differs from blk-crypto: the crypto API is for general crypto operations, whereas blk-crypto is for inline storage encryption. This feature is already being used by Android downstream for several years (https://source.android.com/docs/security/features/encryption/hw-wrapped-keys), but on other platforms userspace support will be provided via fscryptctl and tests via xfstests (I have some old patches for this that need to be updated). Eric Biggers (10): ufs: crypto: add ufs_hba_from_crypto_profile() ufs: qcom: convert to use UFSHCD_QUIRK_CUSTOM_CRYPTO_PROFILE mmc: crypto: add mmc_from_crypto_profile() mmc: sdhci-msm: convert to use custom crypto profile soc: qcom: ice: make qcom_ice_program_key() take struct blk_crypto_key blk-crypto: add basic hardware-wrapped key support blk-crypto: show supported key types in sysfs blk-crypto: add ioctls to create and prepare hardware-wrapped keys fscrypt: add support for hardware-wrapped keys ufs: qcom: add support for wrapped keys Gaurav Kashyap (2): firmware: qcom: scm: add calls for wrapped key support soc: qcom: ice: add HWKM support to the ICE driver Documentation/ABI/stable/sysfs-block | 18 + Documentation/block/inline-encryption.rst | 251 +++++++++++- Documentation/filesystems/fscrypt.rst | 201 +++++++-- .../userspace-api/ioctl/ioctl-number.rst | 2 + block/blk-crypto-fallback.c | 7 +- block/blk-crypto-internal.h | 10 + block/blk-crypto-profile.c | 103 +++++ block/blk-crypto-sysfs.c | 35 ++ block/blk-crypto.c | 196 ++++++++- block/ioctl.c | 5 + drivers/firmware/qcom/qcom_scm.c | 214 ++++++++++ drivers/firmware/qcom/qcom_scm.h | 4 + drivers/md/dm-table.c | 1 + drivers/mmc/host/cqhci-crypto.c | 38 +- drivers/mmc/host/cqhci.h | 8 +- drivers/mmc/host/sdhci-msm.c | 102 +++-- drivers/soc/qcom/ice.c | 383 +++++++++++++++++- drivers/ufs/core/ufshcd-crypto.c | 33 +- drivers/ufs/host/ufs-exynos.c | 3 +- drivers/ufs/host/ufs-qcom.c | 137 +++++-- fs/crypto/fscrypt_private.h | 75 +++- fs/crypto/hkdf.c | 4 +- fs/crypto/inline_crypt.c | 42 +- fs/crypto/keyring.c | 157 +++++-- fs/crypto/keysetup.c | 63 ++- fs/crypto/keysetup_v1.c | 4 +- include/linux/blk-crypto-profile.h | 73 ++++ include/linux/blk-crypto.h | 73 +++- include/linux/firmware/qcom/qcom_scm.h | 8 + include/linux/mmc/host.h | 8 + include/soc/qcom/ice.h | 34 +- include/uapi/linux/blk-crypto.h | 44 ++ include/uapi/linux/fs.h | 6 +- include/uapi/linux/fscrypt.h | 7 +- include/ufs/ufshcd.h | 11 +- 35 files changed, 2091 insertions(+), 269 deletions(-) create mode 100644 include/uapi/linux/blk-crypto.h base-commit: f486c8aa16b8172f63bddc70116a0c897a7f3f02