Re: [PATCH RFC 4/4] fscrypt: Add new encryption policy for btrfs.

[Sweet Tea Dorminy <sweettea-kernel@dorminy.me>]

On 7/25/22 19:32, Eric Biggers wrote:
> On Sat, Jul 23, 2022 at 08:52:28PM -0400, Sweet Tea Dorminy wrote:
>> Certain filesystems may want to use IVs generated and stored outside of
>> fscrypt's inode-based IV generation policies.  In particular, btrfs can
>> have multiple inodes referencing a single block of data, and moves
>> logical data blocks to different physical locations on disk; these two
>> features mean inode or physical-location-based IV generation policies
>> will not work for btrfs. For these or similar reasons, such filesystems
>> may want to implement their own IV generation and storage for data
>> blocks.
>>
>> Plumbing each such filesystem's internals into fscrypt for IV generation
>> would be ungainly and fragile. Thus, this change adds a new policy,
>> IV_FROM_FS, and a new operation function pointer, get_fs_derived_iv.  If
>> this policy is selected, the filesystem is required to provide the
>> function pointer, which populates the IV for a particular data block.
>> The IV buffer passed to get_fs_derived_iv() is pre-populated with the
>> inode contexts' nonce, in case the filesystem would like to use this
>> information; for btrfs, this is used for filename encryption.  Any
>> filesystem using this policy is expected to appropriately generate and
>> store a persistent random IV for each block of data.
> 
> This is changed from the original proposal to store just a random "starting IV"
> per extent, right? 

This is intended to be a generic interface that doesn't require any 
particular IV scheme from the filesystem. In practice, the btrfs side of 
the code is using a per-extent starting IV, as originally proposed. I 
don't see a way for the interface to require IVs per extent, but maybe 
there is a better way than this. Or, is there more detail I can add to 
the change description to clarify that the filesystem doesn't 
necessarily have to store an IV for each individual data block?

> Given that this new proposal uses per-block metadata, has
> support for authenticated encryption been considered? Has space been reserved
> in the per-block metadata for authentication tags so that authenticated
> encryption support could be added later even if it's not in the initial version?

I don't know sufficiently much about authenticated encryption to have 
considered it. As currently drafted, btrfs encrypts before checksumming 
if checksums are enabled, and checks against checksums before 
decrypting. Although at present we haven't discussed authentication 
tags, btrfs could store them in a separate itemtype which could be added 
at any time, much as we currently store fsverity data. We do have 
sufficient room saved for adding other encryption types, if necessary; 
we could use some of that to indicate the existence of authentication 
tags for the extents' data.

> 
> Also, could the new IV generation method just be defined as RANDOM_IV instead of
> IV_FROM_FS?  Why do individual filesystems have to generate the IVs?  Shouldn't
> IV generation happen in common code, with filesystems just storing and
> retrieving the IVs?
I think you're imagining an interface similar to get/set_context, where 
the first time a block is written the filesystem's set_IV method is 
called, and subsequent encryption/decryption calls get_IV, which is 
definitely elegant in its symmetry. But I'm not sure how to have a 
per-block set_IV and also only store an IV per extent, and it would be a 
significant cost to store an IV per block.

I would be happy to add a fscrypt_get_random_iv() method, instead of 
having the filesystem call get_random_bytes() itself, if you'd like.

Thank you!

Sweet Tea