Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 25 Jul 2022 16:32:34 -0700
From:   Eric Biggers <ebiggers@kernel.org>
To:     Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
Cc:     "Theodore Y . Ts'o " <tytso@mit.edu>,
        Jaegeuk Kim <jaegeuk@kernel.org>,
        linux-fscrypt@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-btrfs@vger.kernel.org, osandov@osandov.com,
        kernel-team@fb.com
Subject: Re: [PATCH RFC 4/4] fscrypt: Add new encryption policy for btrfs.
Message-ID: <Yt8oEiN6AkglKfIc@sol.localdomain>
References: <cover.1658623235.git.sweettea-kernel@dorminy.me>
 <675dd03f1a4498b09925fbf93cc38b8430cb7a59.1658623235.git.sweettea-kernel@dorminy.me>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <675dd03f1a4498b09925fbf93cc38b8430cb7a59.1658623235.git.sweettea-kernel@dorminy.me>
Precedence: bulk

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?  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?

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?

> diff --git a/fs/crypto/inline_crypt.c b/fs/crypto/inline_crypt.c
> index 90f3e68f166e..8a8330caadfa 100644
> --- a/fs/crypto/inline_crypt.c
> +++ b/fs/crypto/inline_crypt.c
> @@ -476,14 +476,22 @@ u64 fscrypt_limit_io_blocks(const struct inode *inode, u64 lblk, u64 nr_blocks)
>  		return nr_blocks;
>  
>  	ci = inode->i_crypt_info;
> -	if (!(fscrypt_policy_flags(&ci->ci_policy) &
> -	      FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32))
> -		return nr_blocks;
>  
> -	/* With IV_INO_LBLK_32, the DUN can wrap around from U32_MAX to 0. */
> +	if (fscrypt_policy_flags(&ci->ci_policy) &
> +	    FSCRYPT_POLICY_FLAG_IV_FROM_FS) {
> +		return 1;
> +	}

This effectively means that this IV generation method is incompatible with
inline encryption.  I assume this is okay with you?

- Eric