* Re: [PATCH] ecryptfs: Add missing gotos in ecryptfs_read_metadata
2026-01-11 0:36 [PATCH] ecryptfs: Add missing gotos in ecryptfs_read_metadata Thorsten Blum
@ 2026-01-11 1:08 ` Al Viro
2026-01-11 12:28 ` Thorsten Blum
2026-01-12 19:21 ` Tyler Hicks
1 sibling, 1 reply; 5+ messages in thread
From: Al Viro @ 2026-01-11 1:08 UTC (permalink / raw)
To: Thorsten Blum
Cc: Tyler Hicks, Ard Biesheuvel, Zipeng Zhang, Christian Brauner,
Eric Biggers, Michael Halcrow, Andrew Morton, stable, ecryptfs,
linux-kernel
On Sun, Jan 11, 2026 at 01:36:52AM +0100, Thorsten Blum wrote:
> Add two missing goto statements to exit ecryptfs_read_metadata() when an
> error occurs.
>
> The first goto is required; otherwise ECRYPTFS_METADATA_IN_XATTR may be
> set when xattr metadata is enabled even though parsing the metadata
> failed. The second goto is not strictly necessary, but it makes the
> error path explicit instead of relying on falling through to 'out'.
Ugh... IMO the whole thing from the point we'd successfully allocated
the page to the point where we start to clear it ought to be in a separate
helper. Something like this, perhaps?
diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
index 260f8a4938b0..53fec5a3acaf 100644
--- a/fs/ecryptfs/crypto.c
+++ b/fs/ecryptfs/crypto.c
@@ -1272,6 +1272,43 @@ int ecryptfs_read_and_validate_xattr_region(struct dentry *dentry,
return rc;
}
+static int do_read_metadata(struct dentry *dentry, char *page,
+ struct ecryptfs_crypt_stat *crypt_stat)
+{
+ struct inode *inode = d_inode(dentry);
+
+ /* try to get it from file header */
+ if (ecryptfs_read_lower(page, 0, crypt_stat->extent_size, inode) >= 0 &&
+ ecryptfs_read_headers_virt(page, crypt_stat, dentry,
+ ECRYPTFS_VALIDATE_HEADER_SIZE) == 0)
+ return 0;
+
+ /* metadata is not in the file header, so try xattrs */
+ memset(page, 0, PAGE_SIZE);
+ if (ecryptfs_read_xattr_region(page, inode) < 0 ||
+ ecryptfs_read_headers_virt(page, crypt_stat, dentry,
+ ECRYPTFS_DONT_VALIDATE_HEADER_SIZE) != 0) {
+ printk(KERN_DEBUG "Valid eCryptfs headers not found in "
+ "file xattr region either, inode %lu\n", inode->i_ino);
+ return -EINVAL;
+ }
+
+ /* OK, it's in xattrs; are we allowed to use that? */
+ if (crypt_stat->mount_crypt_stat->flags
+ & ECRYPTFS_XATTR_METADATA_ENABLED) {
+ crypt_stat->flags |= ECRYPTFS_METADATA_IN_XATTR;
+ return 0;
+ }
+
+ printk(KERN_WARNING "Attempt to access file with "
+ "crypto metadata only in the extended attribute "
+ "region, but eCryptfs was mounted without "
+ "xattr support enabled. eCryptfs will not treat "
+ "this like an encrypted file, inode %lu\n",
+ inode->i_ino);
+ return -EINVAL;
+}
+
/*
* ecryptfs_read_metadata
*
@@ -1299,54 +1336,14 @@ int ecryptfs_read_metadata(struct dentry *ecryptfs_dentry)
mount_crypt_stat);
/* Read the first page from the underlying file */
page_virt = kmem_cache_alloc(ecryptfs_header_cache, GFP_USER);
- if (!page_virt) {
- rc = -ENOMEM;
- goto out;
- }
- rc = ecryptfs_read_lower(page_virt, 0, crypt_stat->extent_size,
- ecryptfs_inode);
- if (rc >= 0)
- rc = ecryptfs_read_headers_virt(page_virt, crypt_stat,
- ecryptfs_dentry,
- ECRYPTFS_VALIDATE_HEADER_SIZE);
- if (rc) {
- /* metadata is not in the file header, so try xattrs */
- memset(page_virt, 0, PAGE_SIZE);
- rc = ecryptfs_read_xattr_region(page_virt, ecryptfs_inode);
- if (rc) {
- printk(KERN_DEBUG "Valid eCryptfs headers not found in "
- "file header region or xattr region, inode %lu\n",
- ecryptfs_inode->i_ino);
- rc = -EINVAL;
- goto out;
- }
- rc = ecryptfs_read_headers_virt(page_virt, crypt_stat,
- ecryptfs_dentry,
- ECRYPTFS_DONT_VALIDATE_HEADER_SIZE);
- if (rc) {
- printk(KERN_DEBUG "Valid eCryptfs headers not found in "
- "file xattr region either, inode %lu\n",
- ecryptfs_inode->i_ino);
- rc = -EINVAL;
- }
- if (crypt_stat->mount_crypt_stat->flags
- & ECRYPTFS_XATTR_METADATA_ENABLED) {
- crypt_stat->flags |= ECRYPTFS_METADATA_IN_XATTR;
- } else {
- printk(KERN_WARNING "Attempt to access file with "
- "crypto metadata only in the extended attribute "
- "region, but eCryptfs was mounted without "
- "xattr support enabled. eCryptfs will not treat "
- "this like an encrypted file, inode %lu\n",
- ecryptfs_inode->i_ino);
- rc = -EINVAL;
- }
- }
-out:
- if (page_virt) {
- memset(page_virt, 0, PAGE_SIZE);
- kmem_cache_free(ecryptfs_header_cache, page_virt);
- }
+ if (!page_virt)
+ return -ENOMEM;
+
+ rc = do_read_metadata(ecryptfs_dentry, page_virt, crypt_stat);
+
+ memset(page_virt, 0, PAGE_SIZE);
+ kmem_cache_free(ecryptfs_header_cache, page_virt);
+
return rc;
}
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] ecryptfs: Add missing gotos in ecryptfs_read_metadata
2026-01-11 0:36 [PATCH] ecryptfs: Add missing gotos in ecryptfs_read_metadata Thorsten Blum
2026-01-11 1:08 ` Al Viro
@ 2026-01-12 19:21 ` Tyler Hicks
1 sibling, 0 replies; 5+ messages in thread
From: Tyler Hicks @ 2026-01-12 19:21 UTC (permalink / raw)
To: Thorsten Blum
Cc: Ard Biesheuvel, Zipeng Zhang, Christian Brauner, Eric Biggers,
Andrew Morton, stable, ecryptfs, linux-kernel
On 2026-01-11 01:36:52, Thorsten Blum wrote:
> Add two missing goto statements to exit ecryptfs_read_metadata() when an
> error occurs.
>
> The first goto is required; otherwise ECRYPTFS_METADATA_IN_XATTR may be
> set when xattr metadata is enabled even though parsing the metadata
> failed. The second goto is not strictly necessary, but it makes the
> error path explicit instead of relying on falling through to 'out'.
Hey Thorsten - It seems like there's a bug here but I don't think this
is the actual bug. At the top of ecryptfs_read_metadata(), we call
ecryptfs_copy_mount_wide_flags_to_inode_flags() to copy the mount-wide
crypt_stat flags to the inode's crypt_stat flags. Therefore, the current
code is already redundant in setting ECRYPTFS_METADATA_IN_XATTR after
ecryptfs_read_headers_virt(). No matter if it succeeds or fails.
This logic is confusing but, IIRC, the goal is to handle files with
header metadata and files with xattr metadata within the same mount. I
think the real bug may be that we're not clearing the inode's
ECRYPTFS_METADATA_IN_XATTR flag when the mount crypt stat has the
ECRYPTFS_XATTR_METADATA_ENABLED flag set and
ecryptfs_read_headers_virt() returns success. I haven't looked at what
impact that has elsewhere in the code.
However, it has been a long time since I've looked at this code. I'd
like your thoughts. Thanks!
Tyler
>
> Cc: stable@vger.kernel.org
> Fixes: dd2a3b7ad98f ("[PATCH] eCryptfs: Generalize metadata read/write")
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
> fs/ecryptfs/crypto.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
> index 260f8a4938b0..d49cdf7292ab 100644
> --- a/fs/ecryptfs/crypto.c
> +++ b/fs/ecryptfs/crypto.c
> @@ -1328,6 +1328,7 @@ int ecryptfs_read_metadata(struct dentry *ecryptfs_dentry)
> "file xattr region either, inode %lu\n",
> ecryptfs_inode->i_ino);
> rc = -EINVAL;
> + goto out;
> }
> if (crypt_stat->mount_crypt_stat->flags
> & ECRYPTFS_XATTR_METADATA_ENABLED) {
> @@ -1340,6 +1341,7 @@ int ecryptfs_read_metadata(struct dentry *ecryptfs_dentry)
> "this like an encrypted file, inode %lu\n",
> ecryptfs_inode->i_ino);
> rc = -EINVAL;
> + goto out;
> }
> }
> out:
> --
> Thorsten Blum <thorsten.blum@linux.dev>
> GPG: 1D60 735E 8AEF 3BE4 73B6 9D84 7336 78FD 8DFE EAD4
>
^ permalink raw reply [flat|nested] 5+ messages in thread