Re: [PATCH v2 02/34] ecryptfs: fall back to a read-only lower file

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ecryptfs: ECRYPTFS_KEY_VALID left set after ecryptfs_initialize_file() fails
--

commit f7dbc785a33f0a188b62f1d0ae6c24e5fc32a62c
Author: Christian Brauner <brauner@kernel.org>

ecryptfs: fall back to a read-only lower file

This commit falls back to opening the lower file read-only when opening it
read-write fails with EROFS, and rejects writes to a read-only lower file in
ecryptfs_write_lower() and __ecryptfs_truncate() with EROFS.

> diff --git a/fs/ecryptfs/read_write.c b/fs/ecryptfs/read_write.c
> index b3b451c2b9410..b3956ae50ff66 100644
> --- a/fs/ecryptfs/read_write.c
> +++ b/fs/ecryptfs/read_write.c
> @@ -33,6 +33,9 @@ int ecryptfs_write_lower(struct inode *ecryptfs_inode, char *data,
>  	lower_file = ecryptfs_inode_to_private(ecryptfs_inode)->lower_file;
>  	if (!lower_file)
>  		return -EIO;
> +	/* ecryptfs_open_lower() may have fallen back to a read-only file. */
> +	if (!(lower_file->f_mode & FMODE_WRITE))
> +		return -EROFS;

[Severity: Medium]
When opening an existing 0-byte lower file with O_RDONLY on a read-only lower
mount (or a lower filesystem in an error state) with default mount options,
ecryptfs_open() sets ECRYPTFS_POLICY_APPLIED | ECRYPTFS_ENCRYPTED on
crypt_stat->flags and calls read_or_initialize_metadata().

Because the lower file is empty, ecryptfs_read_metadata() fails and
read_or_initialize_metadata() calls ecryptfs_initialize_file():

fs/ecryptfs/inode.c:ecryptfs_initialize_file() {
	rc = ecryptfs_new_file_context(ecryptfs_inode);
	if (rc) {
		...
		goto out;
	}
	rc = ecryptfs_get_lower_file(ecryptfs_dentry, ecryptfs_inode);
	...
	rc = ecryptfs_write_metadata(ecryptfs_dentry, ecryptfs_inode);
	if (rc)
		printk(KERN_ERR "Error writing headers; rc = [%d]\n", rc);
	ecryptfs_put_lower_file(ecryptfs_inode);
out:
	return rc;
}

ecryptfs_new_file_context() sets ECRYPTFS_ENCRYPTED | ECRYPTFS_KEY_VALID in
crypt_stat->flags before ecryptfs_write_metadata() calls
ecryptfs_write_lower() and fails with -EROFS. Neither
ecryptfs_initialize_file() nor read_or_initialize_metadata() clears
ECRYPTFS_KEY_VALID (or ECRYPTFS_POLICY_APPLIED) on failure.

On a second open(O_RDONLY) or ecryptfs_setattr() on the same cached inode,
read_or_initialize_metadata() checks the flags at the start of the function:

fs/ecryptfs/file.c:read_or_initialize_metadata() {
	if (crypt_stat->flags & ECRYPTFS_POLICY_APPLIED &&
	    crypt_stat->flags & ECRYPTFS_KEY_VALID) {
		rc = 0;
		goto out;
	}
	...
}

Because both flags remain set from the failed initialization, does the second
open short-circuit metadata validation and succeed on the uninitialized empty
file (and if the lower mount is later remounted read-write, allow writes using
an in-memory key whose header was never written to disk)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-work-fs-sb_rdonly-v2-0-2f44f64f0b66@kernel.org?part=2