Skip to contentSkip to conversation

fsverity: Be more careful with return value of ->get_verity_descriptor()

PATCH Applied Eric Biggers 2 participants 3 messages Oct 25 – Oct 28

Subsystems (3)
Stable branch Btrfs file system fs-verity: read-only file-based authenticity protection
lore
Share & tools
lore

Eric Biggers

author maintainer
#0 lore raw

PATCH 1/1 fsverity: Be more careful with return value of ->get_verity_descriptor()

If fsverity_operations::get_verity_descriptor() returns that the verity descriptor has 0 size, then a zero-size buffer is allocated to hold it, and it's called again with bufsize=0.

But bufsize=0 is a special case that says to return the actual size. If in the meantime the filesystem has decided that the size is some actually valid size, it will return that size, and then validate_fsverity_descriptor() will read from ZERO_SIZE_PTR and crash.

Of course, for the filesystem to misbehave in that strange way, it generally would require direct writes to the block device.

Regardless, fsverity_get_descriptor() might as well guard against it. Make it do so. Specifically, validate that the first call to ->get_verity_descriptor() returns at least the minimum descriptor size, and validate that the second call returns the same size as the first.

Fixes fd2d1acfcadf ("fs-verity: add the hook for file ->open()")
Cc [email protected]
Signed-off-by Eric Biggers <[email protected]>
---
 fs/verity/open.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)
fs/verity/open.c +19 -19
index 6646290430380..78a65154b8957 100644
@@ -270,12 +270,6 @@ static bool validate_fsverity_descriptor(struct inode *inode,
 					 const struct fsverity_descriptor *desc,
 					 size_t desc_size)
 {
-	if (desc_size < sizeof(*desc)) {
-		fsverity_err(inode, "Unrecognized descriptor size: %zu bytes",
-			     desc_size);
-		return false;
-	}
-
 	if (desc->version != 1) {
 		fsverity_err(inode, "Unrecognized descriptor version: %u",
 			     desc->version);

@@ -314,31 +308,37 @@ static bool validate_fsverity_descriptor(struct inode *inode,
 int fsverity_get_descriptor(struct inode *inode,
 			    struct fsverity_descriptor **desc_ret)
 {
-	int res;
+	int size, res;
 	struct fsverity_descriptor *desc;
 
-	res = inode->i_sb->s_vop->get_verity_descriptor(inode, NULL, 0);
-	if (res < 0) {
-		fsverity_err(inode,
-			     "Error %d getting verity descriptor size", res);
-		return res;
+	size = inode->i_sb->s_vop->get_verity_descriptor(inode, NULL, 0);
+	if (size < 0) {
+		fsverity_err(inode, "Error %d getting verity descriptor size",
+			     size);
+		return size;
 	}
-	if (res > FS_VERITY_MAX_DESCRIPTOR_SIZE) {
-		fsverity_err(inode, "Verity descriptor is too large (%d bytes)",
-			     res);
+	if (size < sizeof(*desc) || size > FS_VERITY_MAX_DESCRIPTOR_SIZE) {
+		fsverity_err(inode,
+			     "Verity descriptor has invalid size (%d bytes)",
+			     size);
 		return -EMSGSIZE;
 	}
-	desc = kmalloc(res, GFP_KERNEL);
+	desc = kmalloc(size, GFP_KERNEL);
 	if (!desc)
 		return -ENOMEM;
-	res = inode->i_sb->s_vop->get_verity_descriptor(inode, desc, res);
+	res = inode->i_sb->s_vop->get_verity_descriptor(inode, desc, size);
 	if (res < 0) {
 		fsverity_err(inode, "Error %d reading verity descriptor", res);
 		kfree(desc);
 		return res;
 	}
-
-	if (!validate_fsverity_descriptor(inode, desc, res)) {
+	if (res != size) {
+		fsverity_err(inode, "Verity descriptor size changed (%d => %d)",
+			     size, res);
+		kfree(desc);
+		return -EINVAL;
+	}
+	if (!validate_fsverity_descriptor(inode, desc, size)) {
 		kfree(desc);
 		return -EINVAL;
 	}

base-commit: 21a7f44fd0765fcc5194130b28767786eba1d1e7

Signature · 1 line
2.56.0
AI review
#1 lore raw
Sashiko has reviewed this patch and found no issues.
Quoted 12 lines

If fsverity_operations::get_verity_descriptor() returns that the verity descriptor has 0 size, then a zero-size buffer is allocated to hold it, and it's called again with bufsize=0.

But bufsize=0 is a special case that says to return the actual size. If in the meantime the filesystem has decided that the size is some actually valid size, it will return that size, and then validate_fsverity_descriptor() will read from ZERO_SIZE_PTR and crash.

Of course, for the filesystem to misbehave in that strange way, it generally would require direct writes to the block device.

Quoted 8 lines

Regardless, fsverity_get_descriptor() might as well guard against it. Make it do so. Specifically, validate that the first call to ->get_verity_descriptor() returns at least the minimum descriptor size, and validate that the second call returns the same size as the first.

Fixes fd2d1acfcadf ("fs-verity: add the hook for file ->open()")
Cc [email protected]
Signed-off-by Eric Biggers <[email protected]>

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
author maintainer Applied
#2 lore raw
In reply to Eric Biggers
Quoted 12 lines · Eric Biggers

If fsverity_operations::get_verity_descriptor() returns that the verity descriptor has 0 size, then a zero-size buffer is allocated to hold it, and it's called again with bufsize=0.

But bufsize=0 is a special case that says to return the actual size. If in the meantime the filesystem has decided that the size is some actually valid size, it will return that size, and then validate_fsverity_descriptor() will read from ZERO_SIZE_PTR and crash.

Of course, for the filesystem to misbehave in that strange way, it generally would require direct writes to the block device.

Quoted 8 lines

Regardless, fsverity_get_descriptor() might as well guard against it. Make it do so. Specifically, validate that the first call to ->get_verity_descriptor() returns at least the minimum descriptor size, and validate that the second call returns the same size as the first.

Fixes fd2d1acfcadf ("fs-verity: add the hook for file ->open()")
Cc [email protected]
Signed-off-by Eric Biggers <[email protected]>

Applied to https://git.kernel.org/pub/scm/fs/fsverity/linux.git/log/?h=for-current

- Eric