| Message ID | 1642097341-6521-1-git-send-email-israelr@nvidia.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add inline encryption support for dm-crypt | expand |
On 1/13/22 10:09, Israel Rukshin wrote: > I am working to add support for inline encryption/decryption > at storage protocols like nvmf over RDMA. The HW that I am > working with is ConnecX-6 Dx, which supports inline crypto > and can send the data on the fabric at the same time. > > This patchset is based on v5.16-rc4 with Eric Biggers patches > of the HW wrapped keys. > It can be retrieved from branch "wip-wrapped-keys" at > https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git > > I tested this patch with modified nvme-rdma as the block device > and created a DM crypt on top of it. I got performance enhancement > compared to SW crypto. I tested the HW wrapped inline mode with > the HW and the standard mode with a fallback algorithm. Hi Israel, Thank you for your work. For future patches related to inline encryption, please Cc Eric Biggers. Thanks, Bart. -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On 13/01/2022 19:09, Israel Rukshin wrote: > Hi, > > I am working to add support for inline encryption/decryption > at storage protocols like nvmf over RDMA. The HW that I am > working with is ConnecX-6 Dx, which supports inline crypto > and can send the data on the fabric at the same time. This idea comes from time to time, and it makes dm-crypt bloated, and mainly it moves responsibility for encryption to block layer crypto. It adds two completely different sector encryption paths there. Also, see my comments here (for similar patches) https://lore.kernel.org/dm-devel/c94d425a-bca4-8a8b-47bf-451239d29ebd@gmail.com/ I think dm-crypt should stay as SW crypto only (using kernel crypto API, so HW acceleration is done through crypto drivers there). A cleaner solution is to write a much simpler new dm-crypt-inline target, which will implement only inline encryption. (And userspace can decide which target to use.) Code should be just an extension to the dm-linear target, most of dm-crypt complexity is not needed here. Also, please think about configuration - how do you want to configure it? Just my opinion, it is, of course, up to DM maintainer if he takes such patches. Thanks, Milan -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Fri, Jan 14, 2022 at 09:51:20PM +0100, Milan Broz wrote: > On 13/01/2022 19:09, Israel Rukshin wrote: > > Hi, > > > > I am working to add support for inline encryption/decryption > > at storage protocols like nvmf over RDMA. The HW that I am > > working with is ConnecX-6 Dx, which supports inline crypto > > and can send the data on the fabric at the same time. > > This idea comes from time to time, and it makes dm-crypt bloated, > and mainly it moves responsibility for encryption to block layer > crypto. > It adds two completely different sector encryption paths there. > > Also, see my comments here (for similar patches) > https://lore.kernel.org/dm-devel/c94d425a-bca4-8a8b-47bf-451239d29ebd@gmail.com/ > > I think dm-crypt should stay as SW crypto only (using kernel crypto API, > so HW acceleration is done through crypto drivers there). > > A cleaner solution is to write a much simpler new dm-crypt-inline target, > which will implement only inline encryption. > (And userspace can decide which target to use.) > Code should be just an extension to the dm-linear target, most > of dm-crypt complexity is not needed here. > > Also, please think about configuration - how do you want to configure it? > > Just my opinion, it is, of course, up to DM maintainer if he takes such patches. > IMO, adding inline encryption support to dm-crypt would be fine. Normally, blk-crypto is just an alternate implementation of encryption/decryption. I'm not sure that a separate dm target is warranted just because of a different implementation, as opposed to different *behavior*. (Support for wrapped keys does complicate things a bit, as they do change behavior.) But, I'd also be fine with a separate dm target if the dm maintainers prefer that route. Note that in the Android Common Kernels, there is already a dm target called "dm-default-key" which uses dm-crypt compatible syntax but uses blk-crypto (inline encryption) rather than the crypto API: https://android.googlesource.com/kernel/common/+/refs/heads/android-mainline/drivers/md/dm-default-key.c It differs slightly from what is being proposed here in that dm-default-key's purpose is to implement filesystem "metadata encryption", so it has logic to skip encrypting blocks that have their encryption controlled at the filesystem level due to being part of an encrypted file's contents. I expect that logic would be unacceptable upstream, as it's a layering violation. (The long-term plan is to handle metadata encryption entirely at the filesystem level instead.) But with that potentially-controversial logic removed, it would basically be dm-inline-crypt already, so it would be a good starting point. - Eric -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Hi Israel, On Thu, Jan 13, 2022 at 08:09:00PM +0200, Israel Rukshin wrote: > Hi, > > I am working to add support for inline encryption/decryption > at storage protocols like nvmf over RDMA. The HW that I am > working with is ConnecX-6 Dx, which supports inline crypto > and can send the data on the fabric at the same time. > > This patchset is based on v5.16-rc4 with Eric Biggers patches > of the HW wrapped keys. > It can be retrieved from branch "wip-wrapped-keys" at > https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git > > I tested this patch with modified nvme-rdma as the block device > and created a DM crypt on top of it. I got performance enhancement > compared to SW crypto. I tested the HW wrapped inline mode with > the HW and the standard mode with a fallback algorithm. > > Israel Rukshin (1): > dm crypt: Add inline encryption support > > block/blk-crypto.c | 3 + > drivers/md/dm-crypt.c | 202 ++++++++++++++++++++++++++++++++++++------ > 2 files changed, 180 insertions(+), 25 deletions(-) I appreciate the interest in my patchset that adds support for hardware-wrapped inline encryption keys (https://lore.kernel.org/linux-block/20211116033240.39001-1-ebiggers@kernel.org). So far I've received very little feedback on it. One of the challenges I've been having is that I still have no platform on which I can actually test hardware-wrapped keys with the upstream kernel. The feature cannot be accepted upstream until there is a way to test it. It's almost working on the SM8350 SoC, but I am waiting for Qualcomm to fix some things. It sounds like you've implemented a block device driver that exposes support for hardware-wrapped keys. Can you please post that driver? Can you also elaborate on how wrapped keys work in your case? In particular, are they compatible with the whole design which I've documented in detail in my patchset? That would include implementing the ->import_key, ->generate_key, ->prepare_key, and ->derive_sw_secret operations. All the different parts are important. If something needs to be optional, that's something I can consider incorporating into the design, but it would restrict the use cases. Also, will your driver only support wrapped keys, or will it support standard keys too? - Eric -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On 14/01/2022 23:27, Eric Biggers wrote: > On Fri, Jan 14, 2022 at 09:51:20PM +0100, Milan Broz wrote: >> On 13/01/2022 19:09, Israel Rukshin wrote: >>> Hi, >>> >>> I am working to add support for inline encryption/decryption >>> at storage protocols like nvmf over RDMA. The HW that I am >>> working with is ConnecX-6 Dx, which supports inline crypto >>> and can send the data on the fabric at the same time. >> >> This idea comes from time to time, and it makes dm-crypt bloated, >> and mainly it moves responsibility for encryption to block layer >> crypto. >> It adds two completely different sector encryption paths there. >> >> Also, see my comments here (for similar patches) >> https://lore.kernel.org/dm-devel/c94d425a-bca4-8a8b-47bf-451239d29ebd@gmail.com/ >> >> I think dm-crypt should stay as SW crypto only (using kernel crypto API, >> so HW acceleration is done through crypto drivers there). >> >> A cleaner solution is to write a much simpler new dm-crypt-inline target, >> which will implement only inline encryption. >> (And userspace can decide which target to use.) >> Code should be just an extension to the dm-linear target, most >> of dm-crypt complexity is not needed here. >> >> Also, please think about configuration - how do you want to configure it? >> >> Just my opinion, it is, of course, up to DM maintainer if he takes such patches. >> > > IMO, adding inline encryption support to dm-crypt would be fine. Normally, > blk-crypto is just an alternate implementation of encryption/decryption. I'm > not sure that a separate dm target is warranted just because of a different > implementation, as opposed to different *behavior*. (Support for wrapped keys > does complicate things a bit, as they do change behavior.) But, I'd also be > fine with a separate dm target if the dm maintainers prefer that route. I would expect some issues with FIPS people here (currently, it is handled by enabling various crypto API drivers) about crypto boundaries and such stuff. But it is up to the corporate people, not me. Sadly, nobody did try to push some device-mapper functionality into the block layer. Then inline encryption can be just a block device configuration or whatever.... (discussed in 2010 or so... https://lwn.net/Articles/400589/) </sigh> I would prefer to separate SW FDE (dm-crypt, as it is) and a target that delegates encryption to the block layer (inline encryption). But it is just cryptsetup's maintainer view, as we have already too complex detection which features can be enabled on various kernel configs, etc. > Note that in the Android Common Kernels, there is already a dm target called > "dm-default-key" which uses dm-crypt compatible syntax but uses blk-crypto > (inline encryption) rather than the crypto API: > https://android.googlesource.com/kernel/common/+/refs/heads/android-mainline/drivers/md/dm-default-key.c Any plans to submit this to mainline? Or it is just too controversial? > > It differs slightly from what is being proposed here in that dm-default-key's > purpose is to implement filesystem "metadata encryption", so it has logic to > skip encrypting blocks that have their encryption controlled at the filesystem > level due to being part of an encrypted file's contents. I expect that logic > would be unacceptable upstream, as it's a layering violation. (The long-term > plan is to handle metadata encryption entirely at the filesystem level instead.) Well, I wish that we have strong authenticated encryption in filesystem even for metadata, where it fits better in the fist place.... But fscrypt is still not here (or am I mistaken?) Milan -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Hi Eric, On 1/15/2022 12:49 AM, Eric Biggers wrote: > Hi Israel, > > On Thu, Jan 13, 2022 at 08:09:00PM +0200, Israel Rukshin wrote: >> Hi, >> >> I am working to add support for inline encryption/decryption >> at storage protocols like nvmf over RDMA. The HW that I am >> working with is ConnecX-6 Dx, which supports inline crypto >> and can send the data on the fabric at the same time. >> >> This patchset is based on v5.16-rc4 with Eric Biggers patches >> of the HW wrapped keys. >> It can be retrieved from branch "wip-wrapped-keys" at >> https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git >> >> I tested this patch with modified nvme-rdma as the block device >> and created a DM crypt on top of it. I got performance enhancement >> compared to SW crypto. I tested the HW wrapped inline mode with >> the HW and the standard mode with a fallback algorithm. >> >> Israel Rukshin (1): >> dm crypt: Add inline encryption support >> >> block/blk-crypto.c | 3 + >> drivers/md/dm-crypt.c | 202 ++++++++++++++++++++++++++++++++++++------ >> 2 files changed, 180 insertions(+), 25 deletions(-) > I appreciate the interest in my patchset that adds support for hardware-wrapped > inline encryption keys > (https://lore.kernel.org/linux-block/20211116033240.39001-1-ebiggers@kernel.org). > So far I've received very little feedback on it. > > One of the challenges I've been having is that I still have no platform on which > I can actually test hardware-wrapped keys with the upstream kernel. The feature > cannot be accepted upstream until there is a way to test it. It's almost > working on the SM8350 SoC, but I am waiting for Qualcomm to fix some things. > > It sounds like you've implemented a block device driver that exposes support for > hardware-wrapped keys. Can you please post that driver? Yes, I implemented your inline callbacks at nvme-rdma driver. The driver communicates with the HW via a general ib_verbs layer, so I had to extend ib_verbs and mlx5_ib drivers. Those patches are at internal review and I will send the nvme-rdma patches afterwards. > > Can you also elaborate on how wrapped keys work in your case? In particular, > are they compatible with the whole design which I've documented in detail in my > patchset? That would include implementing the ->import_key, ->generate_key, > ->prepare_key, and ->derive_sw_secret operations. All the different parts are > important. If something needs to be optional, that's something I can consider > incorporating into the design, but it would restrict the use cases. In my case, the user should create wrapped keys by himself via a user space tool based on openssl library. Therefore, the ->import_key, ->generate_key and ->prepare_key are not necessary at my case. Regarding ->derive_sw_secret, it is not supported right now by ConnectX6 DX firmware (we plan to add support for this). To test fscrypt with wrapped keys, a temporary WA was added to the ->derive_sw_secret at nvme-rdma driver. The other callbacks you mentioned were left empty. > > Also, will your driver only support wrapped keys, or will it support standard > keys too? Our driver will support both modes. The first step is to support the standard keys, because of the gap I mentioned above. As I understand, ->derive_sw_secret is not needed for dm-crypt. Is that right? > > - Eric -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Fri, Jan 14, 2022 at 09:51:20PM +0100, Milan Broz wrote: > I think dm-crypt should stay as SW crypto only (using kernel crypto API, > so HW acceleration is done through crypto drivers there). > > A cleaner solution is to write a much simpler new dm-crypt-inline target, > which will implement only inline encryption. > (And userspace can decide which target to use.) > Code should be just an extension to the dm-linear target, most > of dm-crypt complexity is not needed here. Why do we even need a dm target for this as well? There should be no need to clone or remap bios, so I think hamdling inline crypto should be just a small addition to the core block layer. -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On 17/01/2022 08:52, Christoph Hellwig wrote: > On Fri, Jan 14, 2022 at 09:51:20PM +0100, Milan Broz wrote: >> I think dm-crypt should stay as SW crypto only (using kernel crypto API, >> so HW acceleration is done through crypto drivers there). >> >> A cleaner solution is to write a much simpler new dm-crypt-inline target, >> which will implement only inline encryption. >> (And userspace can decide which target to use.) >> Code should be just an extension to the dm-linear target, most >> of dm-crypt complexity is not needed here. > > Why do we even need a dm target for this as well? There should be no > need to clone or remap bios, so I think hamdling inline crypto should be > just a small addition to the core block layer. Well, yes, that was my question too :-) Maybe there is need to have some new userspace utility to configure that but otherwise I think that for inline encryption device mapper layer only increases complexity and reduces IO performance. Could anyone elaborate what it the reason for such DM extension? Compatibility with existing encryption/key management tools (like LUKS)? Easy support in LVM? ... Milan -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On 1/17/2022 12:50 PM, Milan Broz wrote: > On 17/01/2022 08:52, Christoph Hellwig wrote: >> On Fri, Jan 14, 2022 at 09:51:20PM +0100, Milan Broz wrote: >>> I think dm-crypt should stay as SW crypto only (using kernel crypto >>> API, >>> so HW acceleration is done through crypto drivers there). >>> >>> A cleaner solution is to write a much simpler new dm-crypt-inline >>> target, >>> which will implement only inline encryption. >>> (And userspace can decide which target to use.) >>> Code should be just an extension to the dm-linear target, most >>> of dm-crypt complexity is not needed here. >> >> Why do we even need a dm target for this as well? There should be no >> need to clone or remap bios, so I think hamdling inline crypto should be >> just a small addition to the core block layer. > > Well, yes, that was my question too :-) > > Maybe there is need to have some new userspace utility to configure that > but otherwise I think that for inline encryption device mapper layer > only increases complexity and reduces IO performance. > Regarding performance degradation, I added only one if condition at the data path (inside #ifdef). > Could anyone elaborate what it the reason for such DM extension? > Compatibility with existing encryption/key management tools (like LUKS)? > Easy support in LVM? ... DM extension gives us several capabilities: 1. Use the Linux keyring and other key management tools. - I used "keyctl padd user test-key @u < /tmp/wrapped_dek" at my tests 2. Split a single block device into several DMs. Allow us to use a different encryption key and encryption mode per DM. 3. Replace a key during I/O by using "dmsetup suspend /dev/dm-0" and "dmsetup resume /dev/dm-0". > > Milan -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Mon, Jan 17, 2022 at 04:00:59PM +0200, Israel Rukshin wrote: > DM extension gives us several capabilities: > > 1. Use the Linux keyring and other key management tools. > > - I used "keyctl padd user test-key @u < /tmp/wrapped_dek" at my tests Well, and kernel consumer can do that. > 2. Split a single block device into several DMs. Allow us to use a different > encryption key and encryption mode per DM. If we allow setting a default key for every block device you can still do that using normal dm-linear. > > 3. Replace a key during I/O by using "dmsetup suspend /dev/dm-0" and > "dmsetup resume /dev/dm-0". With a block layer ioctl that also works easily. -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Sat, Jan 15, 2022 at 10:22:26AM +0100, Milan Broz wrote: > > Note that in the Android Common Kernels, there is already a dm target called > > "dm-default-key" which uses dm-crypt compatible syntax but uses blk-crypto > > (inline encryption) rather than the crypto API: > > https://android.googlesource.com/kernel/common/+/refs/heads/android-mainline/drivers/md/dm-default-key.c > > Any plans to submit this to mainline? Or it is just too controversial? > > > > > It differs slightly from what is being proposed here in that dm-default-key's > > purpose is to implement filesystem "metadata encryption", so it has logic to > > skip encrypting blocks that have their encryption controlled at the filesystem > > level due to being part of an encrypted file's contents. I expect that logic > > would be unacceptable upstream, as it's a layering violation. (The long-term > > plan is to handle metadata encryption entirely at the filesystem level instead.) > > Well, I wish that we have strong authenticated encryption in filesystem even for > metadata, where it fits better in the fist place.... > But fscrypt is still not here (or am I mistaken?) > I doubt that people would find Android's dm-default-key to be acceptable, given that it's a layering violation, and a similar approach was rejected in the past (https://lore.kernel.org/dm-devel/20170614234040.4326-1-mhalcrow@google.com/T/#u). dm-default-key's purpose is filesystem metadata encryption; it encrypts all blocks that aren't already part of an encrypted file's contents. It differs from dm-crypt + fscrypt together (which the upstream kernel already supports) in that file contents aren't encrypted twice; this was a non-negotiable performance requirement. Obviously, this required a new flag in struct bio to indicate which bios are reading/writing from an encrypted file's contents. I doubt the block layer people would find that to be acceptable. In principle, the filesystem is the right place to implement metadata encryption in this way. This would also allow the kernel to enforce (via the key hierarchy) that fscrypt keys are never weaker than the metadata encryption key. Satya Tangirala previously implemented a proof of concept of this for f2fs (https://lore.kernel.org/linux-f2fs-devel/20201217150435.1505269-1-satyat@google.com/T/#u). Unfortunately, Satya has left Google and no one is currently working on this. But it is still the long-term plan. Authenticated encryption is certainly desirable, but not really feasible to retrofit into filesystems that overwrite data in-place. (Yes, dm-integrity exists, but its performance impact is too much for the vast majority of users.) Even f2fs isn't entirely log-structured; it often overwrites blocks in-place. - Eric -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Sun, Jan 16, 2022 at 12:15:22PM +0200, Israel Rukshin wrote: > > Yes, I implemented your inline callbacks at nvme-rdma driver. The driver > communicates with > > the HW via a general ib_verbs layer, so I had to extend ib_verbs and mlx5_ib > drivers. Those > > patches are at internal review and I will send the nvme-rdma patches > afterwards. > > > > > Can you also elaborate on how wrapped keys work in your case? In particular, > > are they compatible with the whole design which I've documented in detail in my > > patchset? That would include implementing the ->import_key, ->generate_key, > > ->prepare_key, and ->derive_sw_secret operations. All the different parts are > > important. If something needs to be optional, that's something I can consider > > incorporating into the design, but it would restrict the use cases. > > In my case, the user should create wrapped keys by himself via a user space > tool based > > on openssl library. Therefore, the ->import_key, ->generate_key and > ->prepare_key are > > not necessary at my case. Regarding ->derive_sw_secret, it is not supported > right now by > > ConnectX6 DX firmware (we plan to add support for this). To test fscrypt > with wrapped keys, > > a temporary WA was added to the ->derive_sw_secret at nvme-rdma driver. The > other callbacks > > you mentioned were left empty. So, what we need to think about is how userspace is expected to actually use and test the hardware-wrapped keys feature. My patchset proposed a design where if a block device declares support for hardware-wrapped keys, then the BLKCRYPTOCREATEKEY and BLKCRYPTOPREPAREKEY ioctls are available. Additionally, a specific hardware-internal key hierarchy and KDF is assumed (due to the need to support ->derive_sw_secret). While userspace doesn't need to know these details to use the feature normally, they *must* be known in order to test that it's actually working correctly. If we were to support variants of the feature, such as: * Hardware-wrapped keys must be created/prepared in some way other than BLKCRYPTOCREATEKEY and BLKCRYPTOPREPAREKEY. (Could you elaborate on what this way actually is, in your case?) * Hardware's key hierarchy or KDF is different, so userspace must do something else when verifying the on-disk ciphertext. ... then we would need to precisely specify these other variants, and define a way for userspace to discover what it should do. In https://lore.kernel.org/r/20211208013534.136590-1-ebiggers@kernel.org, I'm proposing exposing the crypto capabilities of block devices via sysfs. That could lead to a partial solution, e.g. the kernel could provide a file /sys/block/$disk/queue/crypto/wrapped_keys_variant ... which would allow userspace to discover the supported variant of hardware-wrapped keys. I was really hoping that one variant could be standardized, but that is one possibility. > > > > > Also, will your driver only support wrapped keys, or will it support standard > > keys too? > > Our driver will support both modes. The first step is to support the > standard keys, because of Okay, great. If you're adding inline encryption support to dm-crypt (or to dm-inline-crypt or to the block layer, depending on what people decide is the best approach), perhaps you should start with standard keys only, to avoid a dependency on the hardware-wrapped keys feature which is still being worked on? > > the gap I mentioned above. As I understand, ->derive_sw_secret is not needed > for dm-crypt. > > Is that right? That's correct. The larger issue is that if you don't support ->derive_sw_secret, then it's likely that your key hierarchy is different (probably you don't have a "hierarchy", but rather just one key), which would imply that the feature needs to be tested differently. As per the above, this could be accounted for in the design by allowing multiple variants of wrapped keys. Of course, that would add complexity. - Eric -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Tue, Jan 18, 2022 at 08:45:25AM -0800, Christoph Hellwig wrote: > On Mon, Jan 17, 2022 at 04:00:59PM +0200, Israel Rukshin wrote: > > DM extension gives us several capabilities: > > > > 1. Use the Linux keyring and other key management tools. > > > > - I used "keyctl padd user test-key @u < /tmp/wrapped_dek" at my tests > > Well, and kernel consumer can do that. > > > 2. Split a single block device into several DMs. Allow us to use a different > > encryption key and encryption mode per DM. > > If we allow setting a default key for every block device you can still > do that using normal dm-linear. > > > > > 3. Replace a key during I/O by using "dmsetup suspend /dev/dm-0" and > > "dmsetup resume /dev/dm-0". > > With a block layer ioctl that also works easily. > A while ago, I had looked into adding an ioctl to set a default key for a block device. There were a few things that led me to choose a dm target instead. I'm not sure how many of these are still relevant, but these are what I considered: * The block device for a partition doesn't have its own request_queue or queue_limits; those are properties of the disk, not the partition. But, setting an encryption key may require changes to the queue_limits. For example, discard_zeroes_data will no longer work, and the logical_block_size will need to become the crypto data unit size which may be larger than the original logical_block_size. * The block_device for a given partition didn't stay around while no one has it opened or mounted. This may have been addressed by Christoph's changes last year that merged block_device and hd_struct, but this used to be an issue. * There was some issue caused by the way the block layer maps partitions to disks; the knowledge of the original block device (and thus the key) was lost at this point. I'm not sure whether this is still an issue or not. * A block device ioctl to set a key would need to handle cases where the block device is already open (fail with EBUSY?), or already has pages cached in the pagecache (invalidate them?). A dm target avoids these concerns since a key would only be set up when the disk and block device are originally created. Finally, there's also the fact that this would really be more than "setting a default key". To precisely specify the encryption format, you also have to specify the algorithm, the key type, and the data unit size. (Also potentially more details about IV generation, if blk-crypto ever starts to support more IV generation methods, which I'd like to avoid but it might eventually happen.) These could all be passed in an ioctl, but dm-crypt already has a syntax defined for specifying encryption formats. So it could make sense to reuse it. Also as Israel indicated, people will want support for Linux keyring keys as an alternative to raw keys. A new ioctl could support this, though dm-crypt already has a defined way to specify such keys. If all these issues can be solved, then I'd be fine with the block device ioctl. - Eric -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Tue, Jan 18, 2022 at 12:27:33PM -0800, Eric Biggers wrote: > * The block device for a partition doesn't have its own request_queue or > queue_limits; those are properties of the disk, not the partition. But, > setting an encryption key may require changes to the queue_limits. For > example, discard_zeroes_data will no longer work, and the logical_block_size > will need to become the crypto data unit size which may be larger than the > original logical_block_size. If we need changes to the queue limits we're doing something wrong I think, as all these limitation only actually apply to bios that use inline encryption and thus should be dynamic decisions. Note that discard_zeroes_data is gone already, all zeroing must use REQ_OP_WRITE_ZEROES. > > * The block_device for a given partition didn't stay around while no one has it > opened or mounted. This may have been addressed by Christoph's changes last > year that merged block_device and hd_struct, but this used to be an issue. Yes, this is fixed now. > * There was some issue caused by the way the block layer maps partitions to > disks; the knowledge of the original block device (and thus the key) was lost > at this point. I'm not sure whether this is still an issue or not. Also fixed by the block_device/hd_struct merged as the lookup is gone entirely now. > * A block device ioctl to set a key would need to handle cases where the block > device is already open (fail with EBUSY?), or already has pages cached in the > pagecache (invalidate them?). A dm target avoids these concerns since a key > would only be set up when the disk and block device are originally created. An ioctl is by definition perfomed on an open file handle, so it will by definition be open. But I don't think that check really is needed to be so strict. We can require the ioctl to be on an FMODE_EXCL file handle which is a good sanity check and otherwise you get what you ask for. > > Finally, there's also the fact that this would really be more than "setting a > default key". To precisely specify the encryption format, you also have to > specify the algorithm, the key type, and the data unit size. (Also potentially > more details about IV generation, if blk-crypto ever starts to support more IV > generation methods, which I'd like to avoid but it might eventually happen.) > > These could all be passed in an ioctl, but dm-crypt already has a syntax defined > for specifying encryption formats. So it could make sense to reuse it. We could of course find a way to mostly reuse the dm-crypt text based setup syntax even on a block device if that helps. -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Tue, Jan 18, 2022 at 11:38:29AM -0800, Eric Biggers wrote: > I doubt that people would find Android's dm-default-key to be acceptable, given > that it's a layering violation, and a similar approach was rejected in the past > (https://lore.kernel.org/dm-devel/20170614234040.4326-1-mhalcrow@google.com/T/#u). > dm-default-key's purpose is filesystem metadata encryption; it encrypts all > blocks that aren't already part of an encrypted file's contents. It differs > from dm-crypt + fscrypt together (which the upstream kernel already supports) in > that file contents aren't encrypted twice; this was a non-negotiable performance > requirement. Obviously, this required a new flag in struct bio to indicate > which bios are reading/writing from an encrypted file's contents. I doubt the > block layer people would find that to be acceptable. Well, it was rejected because it pokes a hole into dm-crypt. In a purely inline crypto world a way to assign a key context if there is none before is a little different, especially if it requires a different setup than an unconditional encryption for the device. It would also not even require a flag. > > In principle, the filesystem is the right place to implement metadata encryption > in this way. This would also allow the kernel to enforce (via the key > hierarchy) that fscrypt keys are never weaker than the metadata encryption key. Yes. Especially in the inline crypto world it would seem just as trivial to assign a default key in the file system itself. -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On 1/18/2022 10:02 PM, Eric Biggers wrote: > On Sun, Jan 16, 2022 at 12:15:22PM +0200, Israel Rukshin wrote: >> Yes, I implemented your inline callbacks at nvme-rdma driver. The driver >> communicates with >> >> the HW via a general ib_verbs layer, so I had to extend ib_verbs and mlx5_ib >> drivers. Those >> >> patches are at internal review and I will send the nvme-rdma patches >> afterwards. >> >>> Can you also elaborate on how wrapped keys work in your case? In particular, >>> are they compatible with the whole design which I've documented in detail in my >>> patchset? That would include implementing the ->import_key, ->generate_key, >>> ->prepare_key, and ->derive_sw_secret operations. All the different parts are >>> important. If something needs to be optional, that's something I can consider >>> incorporating into the design, but it would restrict the use cases. >> In my case, the user should create wrapped keys by himself via a user space >> tool based >> >> on openssl library. Therefore, the ->import_key, ->generate_key and >> ->prepare_key are >> >> not necessary at my case. Regarding ->derive_sw_secret, it is not supported >> right now by >> >> ConnectX6 DX firmware (we plan to add support for this). To test fscrypt >> with wrapped keys, >> >> a temporary WA was added to the ->derive_sw_secret at nvme-rdma driver. The >> other callbacks >> >> you mentioned were left empty. > So, what we need to think about is how userspace is expected to actually use and > test the hardware-wrapped keys feature. > > My patchset proposed a design where if a block device declares support for > hardware-wrapped keys, then the BLKCRYPTOCREATEKEY and BLKCRYPTOPREPAREKEY > ioctls are available. > > Additionally, a specific hardware-internal key hierarchy and KDF is assumed (due > to the need to support ->derive_sw_secret). While userspace doesn't need to > know these details to use the feature normally, they *must* be known in order to > test that it's actually working correctly. > > If we were to support variants of the feature, such as: > > * Hardware-wrapped keys must be created/prepared in some way other than > BLKCRYPTOCREATEKEY and BLKCRYPTOPREPAREKEY. (Could you elaborate on > what this way actually is, in your case?) We currently assume that the same entity that configured the device the first time and pushed the first wrapping key to it is able to produce DEKs (Data Encryption keys) which are passed to the software in a wrapped form, to be loaded to that particular device as-is. At our case, we produce the wrapped DEKs at a different machine which may be placed at a more secured environment. > > * Hardware's key hierarchy or KDF is different, so userspace must do something > else when verifying the on-disk ciphertext. > > ... then we would need to precisely specify these other variants, and define a > way for userspace to discover what it should do. > > In https://lore.kernel.org/r/20211208013534.136590-1-ebiggers@kernel.org, I'm > proposing exposing the crypto capabilities of block devices via sysfs. That > could lead to a partial solution, e.g. the kernel could provide a file > > /sys/block/$disk/queue/crypto/wrapped_keys_variant > > ... which would allow userspace to discover the supported variant of > hardware-wrapped keys. I was really hoping that one variant could be > standardized, but that is one possibility. > >>> Also, will your driver only support wrapped keys, or will it support standard >>> keys too? >> Our driver will support both modes. The first step is to support the >> standard keys, because of > Okay, great. If you're adding inline encryption support to dm-crypt (or to > dm-inline-crypt or to the block layer, depending on what people decide is the > best approach), perhaps you should start with standard keys only, to avoid a > dependency on the hardware-wrapped keys feature which is still being worked on? Yes, I will do it. > >> the gap I mentioned above. As I understand, ->derive_sw_secret is not needed >> for dm-crypt. >> >> Is that right? > That's correct. The larger issue is that if you don't support > ->derive_sw_secret, then it's likely that your key hierarchy is different > (probably you don't have a "hierarchy", but rather just one key), which would > imply that the feature needs to be tested differently. We plan to support also ->derive_sw_secret. > > As per the above, this could be accounted for in the design by allowing multiple > variants of wrapped keys. Of course, that would add complexity. > > - Eric -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel