| Message ID | cover.1667389115.git.sweettea-kernel@dorminy.me (mailing list archive) |
|---|---|
| Headers | show |
| Series | btrfs: add fscrypt integration | expand |
Thank you for creating this! I'm told the design document [1] no longer reflects the current proposal in these patches. If that's so I think it's worth bringing the design document up to date so we can review the cryptography. Thanks! [1] https://docs.google.com/document/d/1iNnrqyZqJ2I5nfWKt7cd1T9xwU0iHhjhk9ALQW3XuII/edit
On Thu, Nov 3, 2022 at 3:54 PM Paul Crowley <paulcrowley@google.com> wrote: > > Thank you for creating this! I'm told the design document [1] no > longer reflects the current proposal in these patches. If that's so I > think it's worth bringing the design document up to date so we can > review the cryptography. Thanks! > > [1] https://docs.google.com/document/d/1iNnrqyZqJ2I5nfWKt7cd1T9xwU0iHhjhk9ALQW3XuII/edit So this might be my ignorance here, but when I look at the patch set, I don't really see any significant mathematics or cryptographic work going on here. This seems to be primarily just interacting with the fscrypt stuff that exists in the kernel already. Could you please clarify what you mean here?
On 11/3/22 15:22, Paul Crowley wrote: > Thank you for creating this! I'm told the design document [1] no > longer reflects the current proposal in these patches. If that's so I > think it's worth bringing the design document up to date so we can > review the cryptography. Thanks! > > [1] https://docs.google.com/document/d/1iNnrqyZqJ2I5nfWKt7cd1T9xwU0iHhjhk9ALQW3XuII/edit I apologize for the delay; I realized when this thread was bumped just now that my attempt to share the updated doc didn't seem to make it to the mailing list. https://docs.google.com/document/d/1janjxewlewtVPqctkWOjSa7OhCgB8Gdx7iDaCDQQNZA/edit?usp=sharing is an update of the design document that hopefully is what you're requesting.
On Wed, Nov 16, 2022 at 03:08:26PM -0500, Neal Gompa wrote: > On Thu, Nov 3, 2022 at 3:54 PM Paul Crowley <paulcrowley@google.com> wrote: > > > > Thank you for creating this! I'm told the design document [1] no > > longer reflects the current proposal in these patches. If that's so I > > think it's worth bringing the design document up to date so we can > > review the cryptography. Thanks! > > > > [1] https://docs.google.com/document/d/1iNnrqyZqJ2I5nfWKt7cd1T9xwU0iHhjhk9ALQW3XuII/edit > > So this might be my ignorance here, but when I look at the patch set, > I don't really see any significant mathematics or cryptographic work > going on here. This seems to be primarily just interacting with the > fscrypt stuff that exists in the kernel already. > > Could you please clarify what you mean here? There absolutely is significant cryptographic work going on here. There needs to be a new way to choose keys and IVs for file contents blocks, as the existing ways are not appropriate for btrfs. That is the main difficulty we are having. One idea is the one which this patchset is intended to implement. Other ideas that have been brought up involve deriving per-extent keys, using per-block IVs, or using authenticated encryption (or some combination of these). These ideas would be better cryptographically then the one that this patchset actually implements, so it needs to be properly documented why they've been ruled out. (Or maybe they haven't really been ruled out -- I'm not sure they have.) And as I've mentioned, if we do go with the current proposal, which results in some IV reuse, it needs to be decided whether we should try to ameliorate that by hashing part of the IV with a secret key, like IV_INO_LBLK_32 does. Another area where new cryptographic design is needed is the encryption of the fsverity metadata. ext4 and f2fs get encryption of the fsverity metadata "for free" since they store it past EOF, but btrfs doesn't. Anyway, I tried to get Paul's feedback on this patchset, but he (understandingly) didn't want to dig through random mailing list discussions, which don't really have all the information anyway. I think updating the design document to fully reflect the current proposal and the detailed reasoning behind it would be super helpful to get everyone on the right page and to make sure the right design is being chosen. - Eric
I appreciate the conversation happening on the doc; thank ya'll for reading and commenting. Would it be worth having a meeting on Zoom or the like this week to discuss the way forward for getting encryption for btrfs, be that one of the extent-based encryption variations or AEAD? Thanks! Sweet Tea On 11/16/22 15:19, Sweet Tea Dorminy wrote: > > > On 11/3/22 15:22, Paul Crowley wrote: >> Thank you for creating this! I'm told the design document [1] no >> longer reflects the current proposal in these patches. If that's so I >> think it's worth bringing the design document up to date so we can >> review the cryptography. Thanks! >> >> [1] >> https://docs.google.com/document/d/1iNnrqyZqJ2I5nfWKt7cd1T9xwU0iHhjhk9ALQW3XuII/edit > > I apologize for the delay; I realized when this thread was bumped just > now that my attempt to share the updated doc didn't seem to make it to > the mailing list. > > https://docs.google.com/document/d/1janjxewlewtVPqctkWOjSa7OhCgB8Gdx7iDaCDQQNZA/edit?usp=sharing is an update of the design document that hopefully is what you're requesting.
We had a very productive meeting and have greatly changed the design: we now plan to implement authenticated encryption, and have per-extent keys instead of per-extent nonces, which will greatly improve the cryptographic characteristics over this version. Many thanks for the discussion, both in the meeting and on the document. The document has been updated to hopefully reflect the discussion we had; further comments are always appreciated. https://docs.google.com/document/d/1janjxewlewtVPqctkWOjSa7OhCgB8Gdx7iDaCDQQNZA/edit?usp=sharing I look forward to working on implementing the new design; thanks to all! Sweet Tea On 11/21/22 12:26, Sweet Tea Dorminy wrote: > I appreciate the conversation happening on the doc; thank ya'll for > reading and commenting. > > Would it be worth having a meeting on Zoom or the like this week to > discuss the way forward for getting encryption for btrfs, be that one of > the extent-based encryption variations or AEAD? > > Thanks! > > Sweet Tea > > On 11/16/22 15:19, Sweet Tea Dorminy wrote: >> >> >> On 11/3/22 15:22, Paul Crowley wrote: >>> Thank you for creating this! I'm told the design document [1] no >>> longer reflects the current proposal in these patches. If that's so I >>> think it's worth bringing the design document up to date so we can >>> review the cryptography. Thanks! >>> >>> [1] >>> https://docs.google.com/document/d/1iNnrqyZqJ2I5nfWKt7cd1T9xwU0iHhjhk9ALQW3XuII/edit >> >> I apologize for the delay; I realized when this thread was bumped just >> now that my attempt to share the updated doc didn't seem to make it to >> the mailing list. >> >> https://docs.google.com/document/d/1janjxewlewtVPqctkWOjSa7OhCgB8Gdx7iDaCDQQNZA/edit?usp=sharing is an update of the design document that hopefully is what you're requesting.
On Wed, Nov 23, 2022 at 08:22:30PM -0500, Sweet Tea Dorminy wrote: > The document has been updated to hopefully reflect the discussion we had; > further comments are always appreciated. https://docs.google.com/document/d/1janjxewlewtVPqctkWOjSa7OhCgB8Gdx7iDaCDQQNZA/edit?usp=sharing How is this going to work with hardware encryption offload? I think the number of keys for UFS and eMMC inline encryption, but Eric may correct me.
On Sun, Nov 27, 2022 at 11:59:40PM -0800, Christoph Hellwig wrote: > On Wed, Nov 23, 2022 at 08:22:30PM -0500, Sweet Tea Dorminy wrote: > > The document has been updated to hopefully reflect the discussion we had; > > further comments are always appreciated. https://docs.google.com/document/d/1janjxewlewtVPqctkWOjSa7OhCgB8Gdx7iDaCDQQNZA/edit?usp=sharing > > How is this going to work with hardware encryption offload? I think > the number of keys for UFS and eMMC inline encryption, but Eric may > correct me. First, traditional crypto accelerators via the crypto API will work in any case. I think your question is specifically about inline encryption (https://www.kernel.org/doc/html/latest/block/inline-encryption.html). To use inline encryption hardware, consecutive blocks must use consecutive IVs, and the nonzero part of the IVs needs to fit within the hardware's DUN size. That's 64 bits for the UFS standard, and 32 bits for the eMMC standard. fscrypt's "default" setting of per-file keys satisfies both of those requirements. That means the current proposal for btrfs does too, since it's the same as that "default" setting -- just with extents instead of files. (For eMMC, extents would have to be limited to 2^32 blocks.) The other consideration, which seems to be what you're asking about, is a performance one: how well this performs on hardware where switching keys is very expensive. The answer is not very well. Of course, that's the answer for per-file keys too. Note that this is an issue for some inline encryption hardware (e.g. Qualcomm ICE), but not others (e.g. Exynos FMP, Mediatek UFS). The way this problem is "solved" in ext4 and f2fs is by also providing the (less than cryptographically ideal) settings IV_INO_LBLK_64 and IV_INO_LBLK_32. Those squeeze the inode number *and* file offset into a 64-bit or 32-bit IV, so that per-file keys aren't needed. There's a natural mapping of the IV_INO_LBLK_* settings onto extent-based encryption. A 32-bit extent number would just be used instead of an inode number. Or, if a 32-bit extent number is infeasible, an extent nonce of any length hashed with a secret key could be used instead. So yes, it would be possible to provide settings that optimize for hardware like Qualcomm ICE, as ext4 and f2fs do with IV_INO_LBLK_*. However, it makes sense to leave that for later until if/when someone actually has a use case for it. - Eric
The kind of inline encryption hardware we see on Android devices tends to have these limitations: - as you indicate, loading keys can incur latency, so if many keys are in use at once it can slow things down - it's limited to using AES-XTS - on UFS devices, the IV (transmitted in the DUN) must be zero in the 64 high bits - consecutive blocks in the same operation use consecutive IVs - there's no easy way to gather a checksum or MAC over the on-disk ciphertext short of re-reading after writing Android works around this with IV_INO_LBLK_64 policies, but these only work well on the relatively small storage devices we use on Android. In particular the IV limitation is very serious: - inode numbers must be four bytes - they must never change (so ext4 filesystem resizing is disabled) - files cannot be more than 2^32 blocks Things are worse on eMMC devices. Even without this IV limitation, the security proofs for most AES modes of operation start to look shaky as you approach the "birthday bound" of encrypting 2^68 bytes with the same key. If your attack model always assumes a point-in-time attack then you only have to worry if you use a single key to encrypt a multi-exabyte storage device; btrfs is designed to scale to such devices and more. If your attack model includes an attacker who repeatedly gets access to the storage device across time, then writing multiple exabytes with the same key can be a problem even if some of those are overwritten. This leads us to prefer per-extent AES keys (derived from a root key) if possible. It's a shame AES doesn't have a 256-bit blocksize. The way btrfs works also gives us some opportunities to do things a little better. In general disk encryption has to make sacrifices to deal with the limitation that IVs must be reused and there's no room for a MAC. But because btrfs writes in whole extents, with fresh metadata and checksum on each write, it becomes possible to use a fresh IV and MAC for every new write. This opens up the possibility of using an AEAD mode like AES-GCM. This combination gives us the strongest proofs of security even against very generous attack models. Our recommendation: btrfs should first build the ideal thing first since it will have reasonable performance for most users, then later design alternatives that make a few compromises for performance where there is demand. On Sun, 27 Nov 2022 at 23:59, Christoph Hellwig <hch@infradead.org> wrote: > > On Wed, Nov 23, 2022 at 08:22:30PM -0500, Sweet Tea Dorminy wrote: > > The document has been updated to hopefully reflect the discussion we had; > > further comments are always appreciated. https://docs.google.com/document/d/1janjxewlewtVPqctkWOjSa7OhCgB8Gdx7iDaCDQQNZA/edit?usp=sharing > > How is this going to work with hardware encryption offload? I think > the number of keys for UFS and eMMC inline encryption, but Eric may > correct me.
This is a changeset adding encryption to btrfs. Last October, Omar Sandoval sent out a design document for having fscrypt integration with btrfs [1]. In summary, it proposes btrfs storing its own encryption IVs on a per-file-extent basis. fscrypt usually encrypts files using an IV derived from per-inode information; this would prevent snapshotting or reflinking or data relocation for btrfs. We have refined this into a fscrypt extent context object, opaque to the filesystem, which fscrypt uses to generate an IV associated with each block in an extent. Thus, all the inodes sharing a particular key and file extent may decrypt the extent. This series implements this integration for non-inline extents and for verity items. Followup changes will allow encryption of inline extents, which requires a few preliminary rearrangements, and will adjust tests. This series should provide encryption, but this series should not be used in production, as there are likely bugs. This set hopefully reflects all of the feedback given on prior versions. This patchset is based on kdave/misc-next as of a few minutes ago; 698dd8deb7c144616da2bcf2534808931a6bb8e2 . [1] https://lore.kernel.org/linux-btrfs/YXGyq+buM79A1S0L@relinquished.localdomain/ Changelog: v5: - Added encrypting normal compressed extents. - Dropped the first few changes, around qstr/fscrypt_str usage, since they were taken; thanks! - Adjusted fscrypt_have_same_policy() to bubble up errors. - Stopped using division in create_io_em(). - https://lore.kernel.org/linux-btrfs/cover.1667389115.git.sweettea-kernel@dorminy.me v4: - Dropped the partial directory encryption trio of patches, as it's an easy and orthogonal followon. - fscrypt: Changed extent-based encryption to require a direct key policy. - fscrypt: Changed to allow direct key policies with mixed filename/contents encryption modes, to enable usage of existing AES modes with btrfs. - fscrypt: Changed terminology used for extent context contents to 'nonce' instead of 'IV', to match the nonces used in direct-key policies elsewhere. - fscrypt: Changed IV generation for extent-based encryption to generate 16-byte IVs (if needed) from a 16-byte nonce plus extent offset, hopefully as discussed. - fscrypt: Updated documentation change to remove partial directory encryption. - fscrypt: Fixed another place to refer to 'inode-based' encryption instead of a more generic term. - btrfs: Factored btrfs_fscrypt_set_extent_context() as per Josef's feedback. - btrfs: Fixed a couple of style nits. - https://lore.kernel.org/linux-btrfs/cover.1666651724.git.sweettea-kernel@dorminy.me v3: - fscrypt: changed to generate extent contexts the same way as IVs are generated for inode-based encryption, allowing use of any existing policy and making the difference in encryption more minimal. - Changed to use qstr's, then fscrypt_strs, then fscrypt_names only where absolutely necessary, rather than fscrypt_names everywhere. - Reordered changes to put partially-encrypted directories, and no-key name handling, in their own sections at the end of the patchset. - Expanded on descriptions of how no-key name handling works. - Added encryption of verity items (but see notes in change about outstanding questions there) - Stylistic fixes - Renamed flags from FSCRYPT to ENCRYPT in multiple instances. - Made the incompat encrypt superblock flag be set the first time a directory is set to be encrypted, so there isn't a need for a mkfs option. - Hopefully addressed all other minor feedback points. - https://lore.kernel.org/linux-btrfs/cover.1666281276.git.sweettea-kernel@dorminy.me v2: - Amended the fscrypt side to generically add extent contexts, hopefully as per Eric Biggers' past comments. IVs are now entirely abstracted within an extent context, and there is no longer a new encryption policy, as DIRECT_KEY sufficiently encapsulates the needs of extent-based encryption. Documented its usage in btrfs briefly in the documentation. - Adjusted the btrfs side to deal in opaque extent contexts. Improved optimization to skip storing inode contexts if they are the same as the inode's root item's inode context. - Combined 'add fscrypt operation table to superblock' into 'start using fscrypt hooks'. - https://lore.kernel.org/linux-btrfs/cover.1662420176.git.sweettea-kernel@dorminy.me - progs: https://lore.kernel.org/linux-btrfs/cover.1662417859.git.sweettea-kernel@dorminy.me - tests: https://lore.kernel.org/linux-btrfs/cover.1662417905.git.sweettea-kernel@dorminy.me v1: - Recombined the fscrypt changes back into this patchset. - Fixed several races and incorrectly ordered operations. - Improved IV retrieval to correctly distinguish between filename/symlink encryption and encryption of block 0 of a file. - https://lore.kernel.org/linux-btrfs/cover.1660744500.git.sweettea-kernel@dorminy.me - progs: https://lore.kernel.org/linux-btrfs/cover.1660729916.git.sweettea-kernel@dorminy.me - tests: https://lore.kernel.org/linux-btrfs/cover.1660729861.git.sweettea-kernel@dorminy.me RFC v2: - Fixed all warnings and known incorrectnesses. - Split fscrypt changes into their own patchset: https://lore.kernel.org/linux-fscrypt/cover.1658623235.git.sweettea-kernel@dorminy.me - Combined and reordered changes so that enabling fscrypt is the last change. - Removed unnecessary factoring. - Split a cleanup change off. - https://lore.kernel.org/linux-btrfs/cover.1658623319.git.sweettea-kernel@dorminy.me RFC v1: - https://lore.kernel.org/linux-btrfs/cover.1657707686.git.sweettea-kernel@dorminy.me Omar Sandoval (10): fscrypt: expose fscrypt_nokey_name fscrypt: add fscrypt_have_same_policy() to check inode compatibility btrfs: disable various operations on encrypted inodes btrfs: start using fscrypt hooks btrfs: add fscrypt_context items btrfs: translate btrfs encryption flags and encrypted inode flag btrfs: encrypt normal file extent data if appropriate btrfs: Add new FEATURE_INCOMPAT_ENCRYPT feature flag. btrfs: implement fscrypt ioctls btrfs: permit searching for nokey names for removal Sweet Tea Dorminy (8): fscrypt: allow fscrypt_generate_iv() to distinguish filenames fscrypt: add extent-based encryption fscrypt: extent direct key policies for extent-based encryption fscrypt: document btrfs' fscrypt quirks. btrfs: store a fscrypt extent context per normal file extent btrfs: use correct name hash for nokey names btrfs: encrypt verity items btrfs: allow encrypting compressed extents Documentation/filesystems/fscrypt.rst | 31 ++- fs/btrfs/Makefile | 1 + fs/btrfs/accessors.h | 29 +++ fs/btrfs/btrfs_inode.h | 3 +- fs/btrfs/compression.c | 23 ++ fs/btrfs/ctree.h | 4 + fs/btrfs/delayed-inode.c | 30 ++- fs/btrfs/delayed-inode.h | 4 +- fs/btrfs/dir-item.c | 81 +++++- fs/btrfs/dir-item.h | 13 +- fs/btrfs/extent_io.c | 94 ++++++- fs/btrfs/extent_io.h | 3 + fs/btrfs/extent_map.c | 7 + fs/btrfs/extent_map.h | 4 + fs/btrfs/file-item.c | 25 +- fs/btrfs/file.c | 11 +- fs/btrfs/fs.h | 5 +- fs/btrfs/fscrypt.c | 289 +++++++++++++++++++++ fs/btrfs/fscrypt.h | 64 +++++ fs/btrfs/inode.c | 358 ++++++++++++++++++++------ fs/btrfs/ioctl.c | 48 +++- fs/btrfs/ordered-data.c | 11 +- fs/btrfs/ordered-data.h | 4 +- fs/btrfs/reflink.c | 11 + fs/btrfs/root-tree.c | 8 +- fs/btrfs/root-tree.h | 2 +- fs/btrfs/super.c | 7 + fs/btrfs/tree-checker.c | 49 +++- fs/btrfs/tree-log.c | 16 +- fs/btrfs/verity.c | 124 +++++++-- fs/crypto/crypto.c | 40 ++- fs/crypto/fname.c | 43 +--- fs/crypto/fscrypt_private.h | 23 +- fs/crypto/inline_crypt.c | 28 +- fs/crypto/policy.c | 105 ++++++++ include/linux/fscrypt.h | 92 +++++++ include/uapi/linux/btrfs.h | 1 + include/uapi/linux/btrfs_tree.h | 21 ++ 38 files changed, 1509 insertions(+), 203 deletions(-) create mode 100644 fs/btrfs/fscrypt.c create mode 100644 fs/btrfs/fscrypt.h base-commit: 698dd8deb7c144616da2bcf2534808931a6bb8e2