[PATCH v2] isofs: validate directory records in isofs_read_level3_size()

[PATCH v2] isofs: validate directory records in isofs_read_level3_size()

[Matthias Goergens]

isofs_read_level3_size() walks the multi-extent directory records of a
file and dereferences de->size and de->flags for each one without ever
checking the record's length byte.  A record with a short length placed
near the end of a block makes those fixed-field reads run past the
record, and for a record at the end of the last block of a page, past
the buffer.

readdir, lookup and the NFS get_parent path have validated every record
with isofs_dir_record_valid() since commit e2ee4078ec58 ("isofs:
validate directory records consistently").  Do the same here, after
moving on to the next block when the previous record ended exactly at
the end of the block or the rest of the block is zero padding, as
commit 3c01d9263683 ("isofs: Fix handling of directories with tight
blocks") does for the other two walkers.

The check rejects a record that would run past its block, so the code
that reassembled such a record from two blocks can no longer run.
ECMA-119 does not allow directory records to straddle sector
boundaries, which is what commit b2eb2e288604 ("isofs: Drop support of
directory entries straddling blocks") relied on when it removed the
same code from readdir and lookup.  Remove it here too, along with its
temporary record buffer.

Found by fuzzing fs/isofs in a userspace harness with ASan
(heap-buffer-overflow reads in isonum_733(de->size)).

Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>
---
v2: squash the two patches into one, so the record is checked only
after the move to the next block, as Jan suggested.  In v1, 1/2 relied
on the straddle code that 2/2 then removed to handle a record ending
exactly at the end of a block.  The resulting code is the same as after
v1's 2/2.  Based on jack/linux-fs for_next, and cites the tight-block
fix as 3c01d9263683.

isofs_read_inode() still reassembles a first record that straddles a
block.  Only an NFS file handle can point it at one now, and the level
3 walk then returns -EIO.  I can send a follow-up that makes it reject
such records too.

v1: https://lore.kernel.org/all/20260922140137.1768064-1-matthias.goergens@gmail.com/

 fs/isofs/inode.c | 44 +++++++++++++-------------------------------
 1 file changed, 13 insertions(+), 31 deletions(-)

diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
index b766b5c9c593b..184350d2e6ad5 100644
--- a/fs/isofs/inode.c
+++ b/fs/isofs/inode.c
@@ -1176,7 +1176,6 @@ static int isofs_read_level3_size(struct inode *inode)
 	unsigned long block, offset, block_saved, offset_saved;
 	int i = 0;
 	int more_entries = 0;
-	struct iso_directory_record *tmpde = NULL;
 	struct iso_inode_info *ei = ISOFS_I(inode);
 
 	inode->i_size = 0;
@@ -1200,9 +1199,12 @@ static int isofs_read_level3_size(struct inode *inode)
 				goto out_noread;
 		}
 		de = (struct iso_directory_record *) (bh->b_data + offset);
-		de_len = *(unsigned char *) de;
 
-		if (de_len == 0) {
+		/*
+		 * If we are at the end of a block (or at its zero-padded
+		 * tail), move on to the next block.
+		 */
+		if (offset >= bufsize || de->length[0] == 0) {
 			brelse(bh);
 			bh = NULL;
 			++block;
@@ -1210,32 +1212,18 @@ static int isofs_read_level3_size(struct inode *inode)
 			continue;
 		}
 
+		if (!isofs_dir_record_valid(de, offset, bufsize)) {
+			printk(KERN_NOTICE "iso9660: Corrupted directory entry in block %lu of inode %llu\n",
+			       block, inode->i_ino);
+			brelse(bh);
+			return -EIO;
+		}
+
+		de_len = de->length[0];
 		block_saved = block;
 		offset_saved = offset;
 		offset += de_len;
 
-		/* Make sure we have a full directory entry */
-		if (offset >= bufsize) {
-			int slop = bufsize - offset + de_len;
-			if (!tmpde) {
-				tmpde = kmalloc(256, GFP_KERNEL);
-				if (!tmpde)
-					goto out_nomem;
-			}
-			memcpy(tmpde, de, slop);
-			offset &= bufsize - 1;
-			block++;
-			brelse(bh);
-			bh = NULL;
-			if (offset) {
-				bh = sb_bread(inode->i_sb, block);
-				if (!bh)
-					goto out_noread;
-				memcpy((void *)tmpde+slop, bh->b_data, offset);
-			}
-			de = tmpde;
-		}
-
 		inode->i_size += isonum_733(de->size);
 		if (i == 1) {
 			ei->i_next_section_block = block_saved;
@@ -1249,17 +1237,11 @@ static int isofs_read_level3_size(struct inode *inode)
 			goto out_toomany;
 	} while (more_entries);
 out:
-	kfree(tmpde);
 	brelse(bh);
 	return 0;
 
-out_nomem:
-	brelse(bh);
-	return -ENOMEM;
-
 out_noread:
 	printk(KERN_INFO "ISOFS: unable to read i-node block %lu\n", block);
-	kfree(tmpde);
 	return -EIO;
 
 out_toomany:

base-commit: c8437ca3d4386af1ae1f2869643e82fdb9c1f0f5
-- 
2.55.0

Re: [PATCH v2] isofs: validate directory records in isofs_read_level3_size()

[Jan Kara]

On Wed 23-09-26 23:32:20, Matthias Goergens wrote:
> isofs_read_level3_size() walks the multi-extent directory records of a
> file and dereferences de->size and de->flags for each one without ever
> checking the record's length byte.  A record with a short length placed
> near the end of a block makes those fixed-field reads run past the
> record, and for a record at the end of the last block of a page, past
> the buffer.
> 
> readdir, lookup and the NFS get_parent path have validated every record
> with isofs_dir_record_valid() since commit e2ee4078ec58 ("isofs:
> validate directory records consistently").  Do the same here, after
> moving on to the next block when the previous record ended exactly at
> the end of the block or the rest of the block is zero padding, as
> commit 3c01d9263683 ("isofs: Fix handling of directories with tight
> blocks") does for the other two walkers.
> 
> The check rejects a record that would run past its block, so the code
> that reassembled such a record from two blocks can no longer run.
> ECMA-119 does not allow directory records to straddle sector
> boundaries, which is what commit b2eb2e288604 ("isofs: Drop support of
> directory entries straddling blocks") relied on when it removed the
> same code from readdir and lookup.  Remove it here too, along with its
> temporary record buffer.
> 
> Found by fuzzing fs/isofs in a userspace harness with ASan
> (heap-buffer-overflow reads in isonum_733(de->size)).
> 
> Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>

Thanks! I've added the patch to my tree.

								Honza

> ---
> v2: squash the two patches into one, so the record is checked only
> after the move to the next block, as Jan suggested.  In v1, 1/2 relied
> on the straddle code that 2/2 then removed to handle a record ending
> exactly at the end of a block.  The resulting code is the same as after
> v1's 2/2.  Based on jack/linux-fs for_next, and cites the tight-block
> fix as 3c01d9263683.
> 
> isofs_read_inode() still reassembles a first record that straddles a
> block.  Only an NFS file handle can point it at one now, and the level
> 3 walk then returns -EIO.  I can send a follow-up that makes it reject
> such records too.
> 
> v1: https://lore.kernel.org/all/20260922140137.1768064-1-matthias.goergens@gmail.com/
> 
>  fs/isofs/inode.c | 44 +++++++++++++-------------------------------
>  1 file changed, 13 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
> index b766b5c9c593b..184350d2e6ad5 100644
> --- a/fs/isofs/inode.c
> +++ b/fs/isofs/inode.c
> @@ -1176,7 +1176,6 @@ static int isofs_read_level3_size(struct inode *inode)
>  	unsigned long block, offset, block_saved, offset_saved;
>  	int i = 0;
>  	int more_entries = 0;
> -	struct iso_directory_record *tmpde = NULL;
>  	struct iso_inode_info *ei = ISOFS_I(inode);
>  
>  	inode->i_size = 0;
> @@ -1200,9 +1199,12 @@ static int isofs_read_level3_size(struct inode *inode)
>  				goto out_noread;
>  		}
>  		de = (struct iso_directory_record *) (bh->b_data + offset);
> -		de_len = *(unsigned char *) de;
>  
> -		if (de_len == 0) {
> +		/*
> +		 * If we are at the end of a block (or at its zero-padded
> +		 * tail), move on to the next block.
> +		 */
> +		if (offset >= bufsize || de->length[0] == 0) {
>  			brelse(bh);
>  			bh = NULL;
>  			++block;
> @@ -1210,32 +1212,18 @@ static int isofs_read_level3_size(struct inode *inode)
>  			continue;
>  		}
>  
> +		if (!isofs_dir_record_valid(de, offset, bufsize)) {
> +			printk(KERN_NOTICE "iso9660: Corrupted directory entry in block %lu of inode %llu\n",
> +			       block, inode->i_ino);
> +			brelse(bh);
> +			return -EIO;
> +		}
> +
> +		de_len = de->length[0];
>  		block_saved = block;
>  		offset_saved = offset;
>  		offset += de_len;
>  
> -		/* Make sure we have a full directory entry */
> -		if (offset >= bufsize) {
> -			int slop = bufsize - offset + de_len;
> -			if (!tmpde) {
> -				tmpde = kmalloc(256, GFP_KERNEL);
> -				if (!tmpde)
> -					goto out_nomem;
> -			}
> -			memcpy(tmpde, de, slop);
> -			offset &= bufsize - 1;
> -			block++;
> -			brelse(bh);
> -			bh = NULL;
> -			if (offset) {
> -				bh = sb_bread(inode->i_sb, block);
> -				if (!bh)
> -					goto out_noread;
> -				memcpy((void *)tmpde+slop, bh->b_data, offset);
> -			}
> -			de = tmpde;
> -		}
> -
>  		inode->i_size += isonum_733(de->size);
>  		if (i == 1) {
>  			ei->i_next_section_block = block_saved;
> @@ -1249,17 +1237,11 @@ static int isofs_read_level3_size(struct inode *inode)
>  			goto out_toomany;
>  	} while (more_entries);
>  out:
> -	kfree(tmpde);
>  	brelse(bh);
>  	return 0;
>  
> -out_nomem:
> -	brelse(bh);
> -	return -ENOMEM;
> -
>  out_noread:
>  	printk(KERN_INFO "ISOFS: unable to read i-node block %lu\n", block);
> -	kfree(tmpde);
>  	return -EIO;
>  
>  out_toomany:
> 
> base-commit: c8437ca3d4386af1ae1f2869643e82fdb9c1f0f5
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2] isofs: validate directory records in isofs_read_level3_size()

[Matthias Goergens]

On Wed 23-09-26 19:13:03, Jan Kara wrote:
> Thanks! I've added the patch to my tree.

Thanks!  One correction to my commit message, if you're still able to
edit it: the out-of-bounds reads from all three fuzzer reproducers are
on de->flags, not in isonum_733(de->size).  The last paragraph would
then read:

  Found by fuzzing fs/isofs in a userspace harness with ASan
  (heap-buffer-overflow reads of de->flags).

Matthias

Re: [PATCH v2] isofs: validate directory records in isofs_read_level3_size()

[Jan Kara]

On Thu 24-09-26 01:52:11, Matthias Goergens wrote:
> On Wed 23-09-26 19:13:03, Jan Kara wrote:
> > Thanks! I've added the patch to my tree.
> 
> Thanks!  One correction to my commit message, if you're still able to
> edit it: the out-of-bounds reads from all three fuzzer reproducers are
> on de->flags, not in isonum_733(de->size).  The last paragraph would
> then read:
> 
>   Found by fuzzing fs/isofs in a userspace harness with ASan
>   (heap-buffer-overflow reads of de->flags).

No problem. Updated.

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