isofs: advance to the next block when a record ends at the block end

[PATCH] isofs: advance to the next block when a record ends at the block end

Posted by Matthias Goergens 2 days, 10 hours ago

Since commit b2eb2e288604 ("isofs: Drop support of directory entries
straddling blocks") some ISO 9660 images list incomplete directories:
find returns fewer entries than before, and some of the names readdir
does return cannot be looked up.  The log fills with

  iso9660: Corrupted directory entry in block 0 of inode 55232

and on some images the walk reads a byte past the end of a buffer:

  BUG: KASAN: use-after-free in isofs_readdir+0x93b/0xc40

Affected are images with a directory record ending exactly at the end of
a logical block, which ECMA-119 allows and current tools produce.  On
the Debian 13.7.0 amd64 DVD-1, which has three, find returns 9163
entries instead of 9461:

  https://cdimage.debian.org/debian-cd/13.7.0/amd64/iso-dvd/debian-13.7.0-amd64-DVD-1.iso

The KASAN report is from a shareware CD with one in its root directory:

  https://archive.org/download/simtel-1-0295/SIMTEL1_0295.ISO

A local one, which lists 46 of its 65 files:

  mkdir tree
  for i in $(seq -f '%04g' 1 65); do echo hi > tree/FILE$i.TXT; done
  xorrisofs -iso-level 1 -o boundary.iso tree/

genisoimage pads the tail of a block instead of filling it exactly, so
its images have no such records.

That commit removed the reassembly of records crossing a block boundary.
Its condition was "offset >= bufsize", so it also handled a record
ending exactly at the end (offset == bufsize): drop the buffer,
increment the block, reset the offset.  Nothing does that now, so
do_isofs_readdir() and isofs_find_entry() read the next record's length
byte from bh->b_data + bufsize, one past the buffer.  Depending on that
byte the walk either treats the rest of the block as padding and skips
the next block of entries, or fails the record as corrupt and stops.

Straddling records stay rejected: isofs_dir_record_valid() fails them
because their length exceeds the remaining space, which is what that
commit relied on.  One ending exactly at the end fits in its block and
is accepted, so it is the advance, not the reassembly, that has to come
back.

Do it at the top of the loop: do_isofs_readdir() reaches the next
iteration from the multi-extent branch, "." and "..", the
hidden/associated skip (showassoc is off by default, so no mount option
is needed) and the fall-through, and all of them need covering.  The
record was validated to end within the block, so offset never exceeds
bufsize.

Fixes: b2eb2e288604 ("isofs: Drop support of directory entries straddling blocks")
Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>
---
Not tagged for stable: b2eb2e288604 is in 7.3-rc1..rc3 and has not
appeared in a released kernel.

Validated under qemu with KASAN and UBSAN against the images above,
using a kernel built from the commit before b2eb2e288604 as the
reference for correct behaviour.

do_isofs_readdir(), isofs_find_entry() and isofs_read_level3_size() all
open-code this same block-end transition.  Happy to follow up with a
helper that shares it, kept out of here to keep the regression fix
minimal.

 fs/isofs/dir.c   | 12 ++++++++++++
 fs/isofs/namei.c | 12 ++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/fs/isofs/dir.c b/fs/isofs/dir.c
index c7ca7603e97a..00a609c901dd 100644
--- a/fs/isofs/dir.c
+++ b/fs/isofs/dir.c
@@ -104,6 +104,18 @@ static int do_isofs_readdir(struct inode *inode, struct file *file,
 	while (ctx->pos < inode->i_size) {
 		int de_len;
 
+		/*
+		 * The previous record was validated to end within its
+		 * block; if it ended exactly at the end, the next record
+		 * starts at the beginning of the next block.
+		 */
+		if (offset == bufsize) {
+			brelse(bh);
+			bh = NULL;
+			block++;
+			offset = 0;
+		}
+
 		if (!bh) {
 			bh = isofs_bread(inode, block);
 			if (!bh)
diff --git a/fs/isofs/namei.c b/fs/isofs/namei.c
index 010682f5901a..5f5ed36d12e3 100644
--- a/fs/isofs/namei.c
+++ b/fs/isofs/namei.c
@@ -68,6 +68,18 @@ isofs_find_entry(struct inode *dir, struct dentry *dentry,
 		int de_len, match, i, dlen;
 		char *dpnt;
 
+		/*
+		 * The previous record was validated to end within its
+		 * block; if it ended exactly at the end, the next record
+		 * starts at the beginning of the next block.
+		 */
+		if (offset == bufsize) {
+			brelse(bh);
+			bh = NULL;
+			block++;
+			offset = 0;
+		}
+
 		if (!bh) {
 			bh = isofs_bread(dir, block);
 			if (!bh)
-- 
2.55.0

Re: [PATCH] isofs: advance to the next block when a record ends at the block end

Posted by Jan Kara 2 days, 3 hours ago

On Tue 22-09-26 13:48:53, Matthias Goergens wrote:
> Since commit b2eb2e288604 ("isofs: Drop support of directory entries
> straddling blocks") some ISO 9660 images list incomplete directories:
> find returns fewer entries than before, and some of the names readdir
> does return cannot be looked up.  The log fills with
> 
>   iso9660: Corrupted directory entry in block 0 of inode 55232
> 
> and on some images the walk reads a byte past the end of a buffer:
> 
>   BUG: KASAN: use-after-free in isofs_readdir+0x93b/0xc40
> 
> Affected are images with a directory record ending exactly at the end of
> a logical block, which ECMA-119 allows and current tools produce.  On
> the Debian 13.7.0 amd64 DVD-1, which has three, find returns 9163
> entries instead of 9461:
> 
>   https://cdimage.debian.org/debian-cd/13.7.0/amd64/iso-dvd/debian-13.7.0-amd64-DVD-1.iso
> 
> The KASAN report is from a shareware CD with one in its root directory:
> 
>   https://archive.org/download/simtel-1-0295/SIMTEL1_0295.ISO
> 
> A local one, which lists 46 of its 65 files:
> 
>   mkdir tree
>   for i in $(seq -f '%04g' 1 65); do echo hi > tree/FILE$i.TXT; done
>   xorrisofs -iso-level 1 -o boundary.iso tree/
> 
> genisoimage pads the tail of a block instead of filling it exactly, so
> its images have no such records.
> 
> That commit removed the reassembly of records crossing a block boundary.
> Its condition was "offset >= bufsize", so it also handled a record
> ending exactly at the end (offset == bufsize): drop the buffer,
> increment the block, reset the offset.  Nothing does that now, so
> do_isofs_readdir() and isofs_find_entry() read the next record's length
> byte from bh->b_data + bufsize, one past the buffer.  Depending on that
> byte the walk either treats the rest of the block as padding and skips
> the next block of entries, or fails the record as corrupt and stops.
> 
> Straddling records stay rejected: isofs_dir_record_valid() fails them
> because their length exceeds the remaining space, which is what that
> commit relied on.  One ending exactly at the end fits in its block and
> is accepted, so it is the advance, not the reassembly, that has to come
> back.
> 
> Do it at the top of the loop: do_isofs_readdir() reaches the next
> iteration from the multi-extent branch, "." and "..", the
> hidden/associated skip (showassoc is off by default, so no mount option
> is needed) and the fall-through, and all of them need covering.  The
> record was validated to end within the block, so offset never exceeds
> bufsize.
> 
> Fixes: b2eb2e288604 ("isofs: Drop support of directory entries straddling blocks")
> Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>

Thanks for the analysis and the fix! In fact I got notified about the
problem last week and fixed it (sligthly differently) [1]. I've now added
you as a reporter at least to give you some credit.

BTW, it is a good observation that there's yet another instance of
directory reading code in isofs_read_level3_size(). That deserves a similar
simplification as what commit b2eb2e288604 did (just without introducing
the bug this time :)).

								Honza

[1] https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git/commit/?id=bda8d8d49ca14e7f3bdc39fdc4330a48b32d6b6f


> ---
> Not tagged for stable: b2eb2e288604 is in 7.3-rc1..rc3 and has not
> appeared in a released kernel.
> 
> Validated under qemu with KASAN and UBSAN against the images above,
> using a kernel built from the commit before b2eb2e288604 as the
> reference for correct behaviour.
> 
> do_isofs_readdir(), isofs_find_entry() and isofs_read_level3_size() all
> open-code this same block-end transition.  Happy to follow up with a
> helper that shares it, kept out of here to keep the regression fix
> minimal.
> 
>  fs/isofs/dir.c   | 12 ++++++++++++
>  fs/isofs/namei.c | 12 ++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/fs/isofs/dir.c b/fs/isofs/dir.c
> index c7ca7603e97a..00a609c901dd 100644
> --- a/fs/isofs/dir.c
> +++ b/fs/isofs/dir.c
> @@ -104,6 +104,18 @@ static int do_isofs_readdir(struct inode *inode, struct file *file,
>  	while (ctx->pos < inode->i_size) {
>  		int de_len;
>  
> +		/*
> +		 * The previous record was validated to end within its
> +		 * block; if it ended exactly at the end, the next record
> +		 * starts at the beginning of the next block.
> +		 */
> +		if (offset == bufsize) {
> +			brelse(bh);
> +			bh = NULL;
> +			block++;
> +			offset = 0;
> +		}
> +
>  		if (!bh) {
>  			bh = isofs_bread(inode, block);
>  			if (!bh)
> diff --git a/fs/isofs/namei.c b/fs/isofs/namei.c
> index 010682f5901a..5f5ed36d12e3 100644
> --- a/fs/isofs/namei.c
> +++ b/fs/isofs/namei.c
> @@ -68,6 +68,18 @@ isofs_find_entry(struct inode *dir, struct dentry *dentry,
>  		int de_len, match, i, dlen;
>  		char *dpnt;
>  
> +		/*
> +		 * The previous record was validated to end within its
> +		 * block; if it ended exactly at the end, the next record
> +		 * starts at the beginning of the next block.
> +		 */
> +		if (offset == bufsize) {
> +			brelse(bh);
> +			bh = NULL;
> +			block++;
> +			offset = 0;
> +		}
> +
>  		if (!bh) {
>  			bh = isofs_bread(dir, block);
>  			if (!bh)
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR