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:
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
linux-stable-mirror@lists.linaro.org
-
Al Viro -
Thorsten Blum