Re: [PATCH v9 09/11] fscrypt: add inline encryption support

From: Eric Biggers <ebiggers@kernel.org>
To: Satya Tangirala <satyat@google.com>
Cc: linux-block@vger.kernel.org, linux-scsi@vger.kernel.org,
	linux-fscrypt@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-f2fs-devel@lists.sourceforge.net,
	linux-ext4@vger.kernel.org,
	Barani Muthukumaran <bmuthuku@qti.qualcomm.com>,
	Kuohong Wang <kuohong.wang@mediatek.com>,
	Kim Boojin <boojin.kim@samsung.com>
Subject: Re: [PATCH v9 09/11] fscrypt: add inline encryption support
Date: Wed, 25 Mar 2020 22:45:36 -0700	[thread overview]
Message-ID: <20200326054536.GD858@sol.localdomain> (raw)
In-Reply-To: <20200326030702.223233-10-satyat@google.com>

On Wed, Mar 25, 2020 at 08:07:00PM -0700, Satya Tangirala wrote:
> +/* Enable inline encryption for this file if supported. */
> +void fscrypt_select_encryption_impl(struct fscrypt_info *ci)
> +{
> +	const struct inode *inode = ci->ci_inode;
> +	struct super_block *sb = inode->i_sb;
> +
> +	/* The file must need contents encryption, not filenames encryption */
> +	if (!fscrypt_needs_contents_encryption(inode))
> +		return;
> +
> +	/* blk-crypto must implement the needed encryption algorithm */
> +	if (ci->ci_mode->blk_crypto_mode == BLK_ENCRYPTION_MODE_INVALID)
> +		return;
> +
> +	/* The filesystem must be mounted with -o inlinecrypt */
> +	if (!(sb->s_flags & SB_INLINECRYPT))
> +		return;
> +
> +	ci->ci_inlinecrypt = true;
> +}

A bug I came across last week when writing a new test is that '-o inlinecrypt'
can break some fscrypt settings because it enables blk-crypto even when
CONFIG_BLK_INLINE_ENCRYPTION_FALLBACK is unset and the hardware doesn't support
the algorithm.  For example, adding '-o inlinecrypt' can make Adiantum-encrypted
files stop working, due to the hardware only supporting AES-XTS.

That's undesirable.  Adding '-o inlinecrypt' should just make inline encryption
be used where it can, and not break anything.

To fix this, we should make fscrypt_select_encryption_impl() only set
->ci_inlinecrypt if either blk-crypto-fallback is enabled or if all the
filesystem's devices support the algorithm.

In v7+ of this patchset, this is a bit tricky because now
blk_ksm_crypto_key_supported() takes in a 'struct blk_crypto_key', which
fscrypt_select_encryption_impl() doesn't have available yet.  Perhaps make
blk_ksm_crypto_key_supported() a wrapper around a function like
blk_ksm_crypto_setting_supported() that takes a new struct:

	struct blk_crypto_setting {
	        enum blk_crypto_mode_num crypto_mode;
		unsigned int data_unit_size;
		unsigned int dun_bytes;
	};

Then maybe add blk_crypto_setting_supported() which returns true if either
blk_ksm_crypto_key_supported() *or* blk-crypto-fallback is enabled.

- Eric