In isofs_read_level3_size(), check that de_len is at least sizeof(struct
iso_directory_record) + de->name_len[0] before advancing or inspecting
Rock Ridge extensions so corrupted ISO9660 directory records cannot
trigger out-of-bounds reads or infinite loops.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
index 337836a0a170..6007439a085a 100644
--- a/fs/isofs/inode.c
+++ b/fs/isofs/inode.c
@@ -1173,7 +1173,9 @@ static int isofs_read_level3_size(struct inode *inode)
 	struct buffer_head *bh = NULL;
 	unsigned long block, offset, block_saved, offset_saved;
 	int i = 0;
+	unsigned int empty_blocks = 0;
 	int more_entries = 0;
+	int ret = -EIO;
 	struct iso_directory_record *tmpde = NULL;
 	struct iso_inode_info *ei = ISOFS_I(inode);
 
@@ -1205,9 +1207,15 @@ static int isofs_read_level3_size(struct inode *inode)
 			bh = NULL;
 			++block;
 			offset = 0;
+			if (++empty_blocks > 100)
+				goto out;
 			continue;
 		}
 
+		if (de_len < sizeof(struct iso_directory_record))
+			goto out;
+		empty_blocks = 0;
+
 		block_saved = block;
 		offset_saved = offset;
 		offset += de_len;
@@ -1246,10 +1254,11 @@ static int isofs_read_level3_size(struct inode *inode)
 		if (i > 100)
 			goto out_toomany;
 	} while (more_entries);
+	ret = 0;
 out:
 	kfree(tmpde);
 	brelse(bh);
-	return 0;
+	return ret;
 
 out_nomem:
 	brelse(bh);
@@ -1264,6 +1273,7 @@ static int isofs_read_level3_size(struct inode *inode)
 	printk(KERN_INFO "%s: More than 100 file sections ?!?, aborting...\n"
 		"isofs_read_level3_size: inode=%llu\n",
 		__func__, inode->i_ino);
+	ret = 0;
 	goto out;
 }

Hi Hui,

I'm sorry, I missed your patch when I sent mine for the same function a
few days later ("[PATCH v2] isofs: validate directory records in
isofs_read_level3_size()"), and Jan has since applied mine to his tree.

I did test yours, on mainline and on Jan's for_next, running fs/isofs in
a userspace harness with ASan and UBSan: it fixes the three fuzzer
images that made isofs_read_level3_size() read past a record, and I
found no change on valid multi-extent images.

The part of yours that mine doesn't have is the limit on how many empty
blocks the walk skips.  Without it, a run of zero blocks is walked until
the end of the device.  On top of for_next that would be a small
follow-up, and if you'd like to send it I'm happy to test and review it.
If you'd rather I send it, I'll credit you with Suggested-by, or with
Co-developed-by followed by your Signed-off-by if you prefer.

Thanks,
Matthias

Hi!

On Thu 24-09-26 01:52:15, Matthias Goergens wrote:
> I'm sorry, I missed your patch when I sent mine for the same function a
> few days later ("[PATCH v2] isofs: validate directory records in
> isofs_read_level3_size()"), and Jan has since applied mine to his tree.
> 
> I did test yours, on mainline and on Jan's for_next, running fs/isofs in
> a userspace harness with ASan and UBSan: it fixes the three fuzzer
> images that made isofs_read_level3_size() read past a record, and I
> found no change on valid multi-extent images.

For record I think your fix was better because it used proper entry
validation helper which catches more problems.

> The part of yours that mine doesn't have is the limit on how many empty
> blocks the walk skips.  Without it, a run of zero blocks is walked until
> the end of the device.  On top of for_next that would be a small
> follow-up, and if you'd like to send it I'm happy to test and review it.
> If you'd rather I send it, I'll credit you with Suggested-by, or with
> Co-developed-by followed by your Signed-off-by if you prefer.

Based on the standard empty directory blocks are not allowed (there must be
at least one directory record in each directory block). So I'd perhaps just
add checks to refuse them. Then the limit on the number of sections of inode
description already present in isofs_read_level3_size() will naturally take
care of the rest.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Hi Honza,

On Thu 24-09-26 17:03:50, Jan Kara wrote:
> Based on the standard empty directory blocks are not allowed (there must be
> at least one directory record in each directory block). So I'd perhaps just
> add checks to refuse them. Then the limit on the number of sections of inode
> description already present in isofs_read_level3_size() will naturally take
> care of the rest.

That's simpler, thanks.

Hui, would you like to do that?  If not, I'll send it in a few days.
Either way, I'll first check a set of real images for empty directory
blocks, so we know the check doesn't reject discs that mount today.

Thanks,
Matthias