[PATCH 0/2] isofs: simplify the level 3 directory record walk

[PATCH 0/2] isofs: simplify the level 3 directory record walk

[Matthias Goergens]

You mentioned isofs_read_level3_size() deserves the same treatment as
commit b2eb2e288604, so here it is: validate every record with
isofs_dir_record_valid() first, then drop the straddling-record
reassembly and the scratch buffer it needed.

The one thing to watch is that the code being removed did two jobs.  It
ran on "offset >= bufsize", so as well as reassembling a straddling
record it advanced the block for a record ending exactly at the end of
one.  Only the first job is dead now, so I folded the second into the
zero-length check, the way you did in bda8d8d49ca1 ("isofs: Fix
handling of directories with tight blocks").

I left isofs_read_inode() alone.  Its condition is
"offset + de_len > bufsize", so it only ever reassembled and never did
the block advance.  Cleaning that one up is a separate patch.

Both patches touch only fs/isofs/inode.c, so they apply on top of
bda8d8d49ca1, and they list the same entries on every image I tested.

I ran bda8d8d49ca1 on its own against them too.  Entries listed,
driving fs/isofs from a userspace harness:

  image                                     before  bda8d8d49ca1
  xorrisofs -iso-level 1, 65 files              46            65
  same, one boundary record marked assoc        45            64
  Debian 13.7.0 amd64 DVD-1 (directories)     9340          9460
  Joliet, Rock Ridge + Joliet                    2             2

Tested-by: Matthias Goergens <matthias.goergens@gmail.com>

Unrelated, and only because you are a VFS maintainer.  Two regressions
I have chased this month were introduced by patches sent to
linux-fsdevel without a linux-kernel copy, and linux-fsdevel is not
among the lists Sashiko monitors, so it never saw either of them.
Enabling it for the list was proposed in July and seems to have
stalled.

For what it's worth, I'm in favour of adding Sashiko reviews.  Sashiko
ain't perfect, but I find its signal-to-noise ratio good enough to be a
net positive.  In fact I find it useful enough that I often run it
locally before sending patches out, despite the high token costs.

Matthias Goergens (2):
  isofs: validate directory records in isofs_read_level3_size()
  isofs: drop support for level 3 records straddling blocks

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


base-commit: 40288c9206c17eb66a603262e06a58d300d0f279
-- 
2.55.0

[PATCH 1/2] 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").  Use the same helper here.
It rejects records shorter than the fixed part, records whose name
would not fit, and records that would run past the block, so the
straddling-record copy below can no longer be reached with a bad
length.

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>
---
 fs/isofs/inode.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
index 337836a0a170..c9bc1f479161 100644
--- a/fs/isofs/inode.c
+++ b/fs/isofs/inode.c
@@ -1208,6 +1208,14 @@ 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);
+			kfree(tmpde);
+			return -EIO;
+		}
+
 		block_saved = block;
 		offset_saved = offset;
 		offset += de_len;
-- 
2.55.0

Re: [PATCH 1/2] isofs: validate directory records in isofs_read_level3_size()

[Jan Kara]

On Tue 22-09-26 22:01:36, 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").  Use the same helper here.
> It rejects records shorter than the fixed part, records whose name
> would not fit, and records that would run past the block, so the
> straddling-record copy below can no longer be reached with a bad
> length.
> 
> 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>
> ---
>  fs/isofs/inode.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
> index 337836a0a170..c9bc1f479161 100644
> --- a/fs/isofs/inode.c
> +++ b/fs/isofs/inode.c
> @@ -1208,6 +1208,14 @@ static int isofs_read_level3_size(struct inode *inode)
>  			continue;
>  		}
>  
> +		if (!isofs_dir_record_valid(de, offset, bufsize)) {

This is going to trigger when the block is fully packed, won't it? I think
the check belongs to a moment after we've handled transition to the next
block...

								Honza

> +			printk(KERN_NOTICE "iso9660: Corrupted directory entry in block %lu of inode %llu\n",
> +			       block, inode->i_ino);
> +			brelse(bh);
> +			kfree(tmpde);
> +			return -EIO;
> +		}
> +
>  		block_saved = block;
>  		offset_saved = offset;
>  		offset += de_len;
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/2] isofs: validate directory records in isofs_read_level3_size()

[Matthias Goergens]

Hi Jan,

Thanks for looking at it.

I don't see how it can trigger there, but I'm probably missing
something, so here is my reasoning.  With 1/2 alone, offset is always
below bufsize at the top of the loop: a record that ends exactly at the
end of the block takes the existing "offset >= bufsize" branch further
down, which leaves offset & (bufsize - 1) == 0 and bumps block, so the
next iteration starts at offset 0 of the next block.  And
isofs_dir_record_valid() accepts a record that ends exactly at bufsize:
its test "len > bufsize - offset" is false when len equals the room
left.

I also tried it, running fs/isofs in a userspace harness on level 3
images where a record of a multi-extent file ends exactly at the end of
a block, including xorrisofs -iso-level 3 output with a sparse 8 GiB
file.  Neither 1/2 alone nor 1/2+2/2 rejects the directory, and the file
sizes come out right; a record whose length runs past the block is
rejected.

If you have a reproducer, or just a hint at the case you have in mind,
I'd be really happy to look at it.

Even if the code might be technically correct, it's confusing.  1/2 only
works because of a branch further down that 2/2 then deletes, so I'll
send a v2 that squashes the two: move on to the next block first, then
check the record, as you suggest, with the straddle code gone.  The end
result is the same code as after 2/2.

Matthias

Re: [PATCH 1/2] isofs: validate directory records in isofs_read_level3_size()

[Jan Kara]

On Wed 23-09-26 23:32:17, Matthias Goergens wrote:
> Thanks for looking at it.
> 
> I don't see how it can trigger there, but I'm probably missing
> something, so here is my reasoning.  With 1/2 alone, offset is always
> below bufsize at the top of the loop: a record that ends exactly at the
> end of the block takes the existing "offset >= bufsize" branch further
> down, which leaves offset & (bufsize - 1) == 0 and bumps block, so the
> next iteration starts at offset 0 of the next block.  And
> isofs_dir_record_valid() accepts a record that ends exactly at bufsize:
> its test "len > bufsize - offset" is false when len equals the room
> left.

Doh, you're right. I got confused by the flow in the loop. Your patches are
correct.

> I also tried it, running fs/isofs in a userspace harness on level 3
> images where a record of a multi-extent file ends exactly at the end of
> a block, including xorrisofs -iso-level 3 output with a sparse 8 GiB
> file.  Neither 1/2 alone nor 1/2+2/2 rejects the directory, and the file
> sizes come out right; a record whose length runs past the block is
> rejected.
> 
> If you have a reproducer, or just a hint at the case you have in mind,
> I'd be really happy to look at it.
> 
> Even if the code might be technically correct, it's confusing.  1/2 only
> works because of a branch further down that 2/2 then deletes, so I'll
> send a v2 that squashes the two: move on to the next block first, then
> check the record, as you suggest, with the straddle code gone.  The end
> result is the same code as after 2/2.

Yeah, I was also wondering if squashing the two commits won't be less
confusing.

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

[PATCH 2/2] isofs: drop support for level 3 records straddling blocks

[Matthias Goergens]

isofs_read_level3_size() is the third copy of the directory record walk
and the last one that still reassembles a record spanning two blocks.
Now that it validates every record with isofs_dir_record_valid(), such a
record is rejected before the copy can run.  ECMA-119 does not allow
directory records to straddle sector boundaries, the same assumption
commit b2eb2e288604 ("isofs: Drop support of directory entries
straddling blocks") relied on when it removed the equivalent code from
readdir and lookup.

Remove it here too, along with the temporary record buffer it needed,
and fold the end-of-block case into the existing zero-length check the
way commit bda8d8d49ca1 ("isofs: Fix handling of directories with tight
blocks") did for the other two walkers.  That check has to cover both
cases.  The code being removed here ran on "offset >= bufsize", so it
was also doing the block advance for a record that ends exactly at the
end of a block, and dropping it without replacing that is what went
wrong last time.

Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>
---
 fs/isofs/inode.c | 38 ++++++--------------------------------
 1 file changed, 6 insertions(+), 32 deletions(-)

diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
index c9bc1f479161..70097456b721 100644
--- a/fs/isofs/inode.c
+++ b/fs/isofs/inode.c
@@ -1174,7 +1174,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;
@@ -1198,9 +1197,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;
@@ -1212,36 +1214,14 @@ static int isofs_read_level3_size(struct inode *inode)
 			printk(KERN_NOTICE "iso9660: Corrupted directory entry in block %lu of inode %llu\n",
 			       block, inode->i_ino);
 			brelse(bh);
-			kfree(tmpde);
 			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;
@@ -1255,17 +1235,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:
-- 
2.55.0