linux-kernel - Re: [PATCH] ecryptfs: Add missing gotos in ecryptfs_read

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20260111010825.GG3634291@ZenIV>
Date: Sun, 11 Jan 2026 01:08:25 +0000
From: Al Viro <viro@...iv.linux.org.uk>
To: Thorsten Blum <thorsten.blum@...ux.dev>
Cc: Tyler Hicks <code@...icks.com>, Ard Biesheuvel <ardb@...nel.org>,
	Zipeng Zhang <zhangzipeng0@...mail.com>,
	Christian Brauner <brauner@...nel.org>,
	Eric Biggers <ebiggers@...nel.org>,
	Michael Halcrow <mhalcrow@...ibm.com>,
	Andrew Morton <akpm@...ux-foundation.org>, stable@...r.kernel.org,
	ecryptfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ecryptfs: Add missing gotos in ecryptfs_read_metadata

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;
 }