Re: [PATCH] fs/ext4: Prevent encryption/decryption of unaligned blocks in aes_encrypt

[Eric Biggers]

On Thu, Jun 13, 2024 at 03:48:25PM +0200, Norbert Kamiński wrote:
> syzbot is reporting an uninitialized value in aes_encrypt(). The block
> cipher expects the bytes to encrypt or decrypt to be a multiple of the
> cipher’s block size. However, when ext4_write_begin() is called and new
> folios are allocated, they might not be aligned to the required block
> size.

While the length of file content blocks does need to be a multiple of
FSCRYPT_CONTENTS_ALIGNMENT bytes, this has nothing to do with the syzbot report
that this patch is trying to fix, and this is always the case in ext4 anyway.

> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 4bae9ccf5fe0..965f790a9d36 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1156,6 +1156,9 @@ static int ext4_write_begin(struct file *file, struct address_space *mapping,
>  	 * the folio (if needed) without using GFP_NOFS.
>  	 */
>  retry_grab:
> +	if (IS_ENABLED(CONFIG_FS_ENCRYPTION))
> +		mapping_set_gfp_mask(mapping,
> +				     mapping_gfp_mask(mapping) | __GFP_ZERO);
>  	folio = __filemap_get_folio(mapping, index, FGP_WRITEBEGIN,
>  					mapping_gfp_mask(mapping));
>  	if (IS_ERR(folio))
> @@ -2882,6 +2885,9 @@ static int ext4_da_write_begin(struct file *file, struct address_space *mapping,
>  	}
>  
>  retry:
> +	if (IS_ENABLED(CONFIG_FS_ENCRYPTION))
> +		mapping_set_gfp_mask(mapping,
> +				     mapping_gfp_mask(mapping) | __GFP_ZERO);

No, it's not acceptable to force all pagecache pages to be zeroized in ext4
without opting into init_on_alloc.  This is also the wrong place to set the
mapping's gfp_mask, as the mapping has already been activated.

What actually needs to be done is root-cause this bug and fix the underlying
cause.  It looks like somehow data got marked as valid in the pagecache without
being initialized, which is never supposed to happen.

- Eric