fs/ecryptfs/crypto.c | 2 ++ 1 file changed, 2 insertions(+)
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'.
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 EAD4On 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
>
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;
}
On 11. Jan 2026, at 02:08, Al Viro wrote: > 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? I wanted to keep the fix simple, but I'm happy to refactor the function if that's preferred. Any preferences, Tyler? Thanks, Thorsten
On 2026-01-11 13:28:17, Thorsten Blum wrote: > On 11. Jan 2026, at 02:08, Al Viro wrote: > > 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? > > I wanted to keep the fix simple, but I'm happy to refactor the function > if that's preferred. Any preferences, Tyler? I typically like the multi-patch approach of a minimal, easy-to-backport fix first and then a more complete cleanup/improvement in the followup patch(es). Tyler > > Thanks, > Thorsten >
© 2016 - 2026 Red Hat, Inc.