linux-ext4 - Re: [PATCH] fs/ext4: Prevent encryption/decryption of unaligned blocks in aes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240621040049.GA4362@sol.localdomain>
Date: Thu, 20 Jun 2024 21:00:49 -0700
From: Eric Biggers <ebiggers@...nel.org>
To: Norbert Kamiński <norbert.kaminski@...ogain.com>
Cc: tytso@....edu, adilger.kernel@...ger.ca, linux-ext4@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	syzbot+aeb14e2539ffb6d21130@...kaller.appspotmail.com
Subject: Re: [PATCH] fs/ext4: Prevent encryption/decryption of unaligned
 blocks in aes_encrypt

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