linux-ext4 - Re: [PATCH] quota: explicitly forbid quota files from being encrypted

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: <20230905091124.giawwm3tu6fm2buq@quack3>
Date:   Tue, 5 Sep 2023 11:11:24 +0200
From:   Jan Kara <jack@...e.cz>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     Jan Kara <jack@...e.com>, linux-fscrypt@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-ext4@...r.kernel.org,
        linux-f2fs-devel@...ts.sourceforge.net, stable@...r.kernel.org
Subject: Re: [PATCH] quota: explicitly forbid quota files from being encrypted

On Mon 04-09-23 17:32:27, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@...gle.com>
> 
> Since commit d7e7b9af104c ("fscrypt: stop using keyrings subsystem for
> fscrypt_master_key"), xfstest generic/270 causes a WARNING when run on
> f2fs with test_dummy_encryption in the mount options:
> 
> $ kvm-xfstests -c f2fs/encrypt generic/270
> [...]
> WARNING: CPU: 1 PID: 2453 at fs/crypto/keyring.c:240 fscrypt_destroy_keyring+0x1f5/0x260
> 
> The cause of the WARNING is that not all encrypted inodes have been
> evicted before fscrypt_destroy_keyring() is called, which violates an
> assumption.  This happens because the test uses an external quota file,
> which gets automatically encrypted due to test_dummy_encryption.
> 
> Encryption of quota files has never really been supported.  On ext4,
> ext4_quota_read() does not decrypt the data, so encrypted quota files
> are always considered invalid on ext4.  On f2fs, f2fs_quota_read() uses
> the pagecache, so trying to use an encrypted quota file gets farther,
> resulting in the issue described above being possible.  But this was
> never intended to be possible, and there is no use case for it.
> 
> Therefore, make the quota support layer explicitly reject using
> IS_ENCRYPTED inodes when quotaon is attempted.
> 
> Cc: stable@...r.kernel.org
> Signed-off-by: Eric Biggers <ebiggers@...gle.com>

Looks good. I'll queue this patch into my tree and send it to Linus for
RC2.

								Honza

> ---
>  fs/quota/dquot.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index 9e72bfe8bbad9..7e268cd2727cc 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -2339,6 +2339,20 @@ static int vfs_setup_quota_inode(struct inode *inode, int type)
>  	if (sb_has_quota_loaded(sb, type))
>  		return -EBUSY;
>  
> +	/*
> +	 * Quota files should never be encrypted.  They should be thought of as
> +	 * filesystem metadata, not user data.  New-style internal quota files
> +	 * cannot be encrypted by users anyway, but old-style external quota
> +	 * files could potentially be incorrectly created in an encrypted
> +	 * directory, hence this explicit check.  Some reasons why encrypted
> +	 * quota files don't work include: (1) some filesystems that support
> +	 * encryption don't handle it in their quota_read and quota_write, and
> +	 * (2) cleaning up encrypted quota files at unmount would need special
> +	 * consideration, as quota files are cleaned up later than user files.
> +	 */
> +	if (IS_ENCRYPTED(inode))
> +		return -EINVAL;
> +
>  	dqopt->files[type] = igrab(inode);
>  	if (!dqopt->files[type])
>  		return -EIO;
> 
> base-commit: 708283abf896dd4853e673cc8cba70acaf9bf4ea
> -- 
> 2.42.0
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR