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

[Eric Biggers]

On Mon, Jul 25, 2022 at 10:16:07PM -0400, Sweet Tea Dorminy wrote:
> 
> 
> 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. 

I don't think that's a good way to do it.  The fscrypt settings are supposed to
be very concrete, meaning that they specify a particular way of doing the
encryption, which can be reviewed for its security and which can be tested for
correctness of the on-disk format.  There shouldn't be cryptographic differences
between how different filesystems implement the same setting.

The fscrypt settings also shouldn't specify internal implementation details of
the code, as "IV_FROM_FS" does.  From userspace's perspective, *all* fscrypt
settings have IVs chosen by the filesystem; the division between the
"filesystem" and fs/crypto/ is an internal kernel implementation detail.

So I think you should go with something like RANDOM_IV or IV_PER_EXTENT.

> In practice, the btrfs side of the code is using a per-extent starting IV, as
> originally proposed. 

This is inconsistent with your commit message, which says that there is a random
IV for each block of data.  It's also inconsistent with your proposed change to
fscrypt_limit_io_blocks().  So I don't know which to believe.

Clearly this can't be properly reviewed on its own, so please send out the whole
patch series and not just the fs/crypto/ parts.

- Eric