[PATCH] isofs: Fix handling of directories with small logical blocks

[PATCH] isofs: Fix handling of directories with small logical blocks

[Hengyu Liang]

Commit b2eb2e288604 ("isofs: Drop support of directory entries
straddling blocks") made do_isofs_readdir() and isofs_find_entry()
reject a directory record that does not end in the block it begins in.

However, the block is the logical block of the volume, which can be 512
or 1024 bytes. ECMA-119 only requires a record to end in the 2048-byte
logical sector in which it begins, so a record can cross a logical
block. As of now, ls stops at the first such record and the files after
it cannot be opened.

The issue can be reproduced with an image that has 1024-byte logical
blocks and 40 files in its root directory:

    mount -t iso9660 -o loop,ro lbs1024.iso /mnt
    ls /mnt | wc -l

Before commit b2eb2e288604 ("isofs: Drop support of directory entries
straddling blocks"), the result is 40.

After that commit, the result is 19, and the kernel logs:

    iso9660: Corrupted directory entry in block 0 of inode 1152

This patch will put such a record together from its two blocks again,
but only when it ends in the logical sector in which it begins.

Fixes: b2eb2e288604 ("isofs: Drop support of directory entries straddling blocks")
Signed-off-by: Hengyu Liang <hengyul@cs.unc.edu>
---
 fs/isofs/dir.c   | 22 +++++++++++++++++++++-
 fs/isofs/namei.c | 22 +++++++++++++++++++++-
 2 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/fs/isofs/dir.c b/fs/isofs/dir.c
index 28741251d56e..c72cfb08707c 100644
--- a/fs/isofs/dir.c
+++ b/fs/isofs/dir.c
@@ -95,7 +95,9 @@ static int do_isofs_readdir(struct inode *inode, struct file *file,
 	int map;
 	int first_de = 1;
 	char *p = NULL;		/* Quiet GCC */
+	u8 record[256];
 	struct iso_directory_record *de;
+	struct iso_directory_record *tmpde = (void *)record;
 	struct isofs_sb_info *sbi = ISOFS_SB(inode->i_sb);
 
 	offset = ctx->pos & (bufsize - 1);
@@ -129,7 +131,25 @@ static int do_isofs_readdir(struct inode *inode, struct file *file,
 		offset_saved = offset;
 		offset += de_len;
 
-		if (!isofs_dir_record_valid(de, offset_saved, bufsize)) {
+		/* A record may span blocks within a logical sector. */
+		if (offset > bufsize &&
+		    de_len <= ISOFS_BLOCK_SIZE -
+			      (ctx->pos & (ISOFS_BLOCK_SIZE - 1))) {
+			unsigned int slop = bufsize - offset_saved;
+
+			memcpy(tmpde, de, slop);
+			offset &= bufsize - 1;
+			block++;
+			brelse(bh);
+			bh = isofs_bread(inode, block);
+			if (!bh)
+				return -EIO;
+			memcpy((char *)tmpde + slop, bh->b_data, offset);
+			de = tmpde;
+		}
+
+		if (!isofs_dir_record_valid(de, de == tmpde ? 0 : offset_saved,
+					    de == tmpde ? de_len : bufsize)) {
 			printk(KERN_NOTICE "iso9660: Corrupted directory entry"
 			       " in block %lu of inode %llu\n", block,
 			       inode->i_ino);
diff --git a/fs/isofs/namei.c b/fs/isofs/namei.c
index 4fba1bf7f016..289efa60f3bf 100644
--- a/fs/isofs/namei.c
+++ b/fs/isofs/namei.c
@@ -54,6 +54,8 @@ isofs_find_entry(struct inode *dir, struct dentry *dentry,
 	unsigned char bufbits = ISOFS_BUFFER_BITS(dir);
 	unsigned long block, f_pos, offset, block_saved, offset_saved;
 	struct buffer_head *bh = NULL;
+	u8 record[256];
+	struct iso_directory_record *tmpde = (void *)record;
 	struct isofs_sb_info *sbi = ISOFS_SB(dir->i_sb);
 
 	if (!ISOFS_I(dir)->i_first_extent)
@@ -93,7 +95,25 @@ isofs_find_entry(struct inode *dir, struct dentry *dentry,
 		offset += de_len;
 		f_pos += de_len;
 
-		if (!isofs_dir_record_valid(de, offset_saved, bufsize)) {
+		/* A record may span blocks within a logical sector. */
+		if (offset > bufsize &&
+		    de_len <= ISOFS_BLOCK_SIZE -
+			      ((f_pos - de_len) & (ISOFS_BLOCK_SIZE - 1))) {
+			unsigned int slop = bufsize - offset_saved;
+
+			memcpy(tmpde, de, slop);
+			offset &= bufsize - 1;
+			block++;
+			brelse(bh);
+			bh = isofs_bread(dir, block);
+			if (!bh)
+				return 0;
+			memcpy((char *)tmpde + slop, bh->b_data, offset);
+			de = tmpde;
+		}
+
+		if (!isofs_dir_record_valid(de, de == tmpde ? 0 : offset_saved,
+					    de == tmpde ? de_len : bufsize)) {
 			printk(KERN_NOTICE "iso9660: Corrupted directory entry"
 			       " in block %lu of inode %llu\n", block,
 			       dir->i_ino);
-- 
2.53.0

Re: [PATCH] isofs: Fix handling of directories with small logical blocks

[Thomas Schmitt]

Hi,

Hengyu Liang wrote:
>     mount -t iso9660 -o loop,ro lbs1024.iso /mnt

Do i get it right that lbs1024.iso contains in the Primary Volume
Descriptor at BP 129 to 132 "Logical Block Size" the value 1024
instead of 2048 ?
(Where could i get such an ISO image ? And how was it created ?)

If so:
I agree with Hengyu Liang's assessment of ECMA-119 in regard to
Logical Sector (device-related unit inspired by CD sectors) and
Logical Block (filesystem addressing unit).
Assuming that my understanding of the isofs code is correct, i am
astonished and impressed to see in isofs support for
Logical Block < Logical Sector.


But i see a possible exotic consistency problem:

The mount option block={512|1024|2048} may set the address unit for
reading the volume descriptors in isofs_fill_super():
         opt->blocksize = sb_min_blocksize(s, opt->blocksize);
This means that if it gets through, it is the assumed Logical Sector
size.
Later the block size unit gets overridden by the logical block size
which was read from the chosen volume descriptor:
         if (!sb_set_blocksize(s, orig_zonesize))

The problem would be that during the check for allowed crossing of
logical blocks the Logical Sector size is hardcoded as ISOFS_BLOCK_SIZE
i.e. 2048.
So if the user is allowed to set an initial Sector size, shouldn't this
be used for the check in this patch, too ?

On the other hand, the default of opt->blocksize is 1024, which
obviously does not get into effect, even with 512-byte-per-block
USB sticks.
So can mount option block={512|1024|2048} still have an effect at all ?


Have a nice day :)

Thomas

Re: [PATCH] isofs: Fix handling of directories with small logical blocks

[Hengyu Liang]

Hi Thomas,

On Sun, Oct 11, 2026 at 10:29:07PM +0200, Thomas Schmitt wrote:
> Do i get it right that lbs1024.iso contains in the Primary Volume
> Descriptor at BP 129 to 132 "Logical Block Size" the value 1024
> instead of 2048 ?
> (Where could i get such an ISO image ? And how was it created ?)

Yes, BP 129 to 132 hold 1024.

I do not have such an image from a real medium, and I do not know a
program that writes one. I made it by hand with the script below
("python3 mkiso.py lbs1024.iso 1024"). It is minimal: a Primary Volume
Descriptor with only a few fields set, the terminator, and one sector
with a root directory of 40 empty files. To keep it short the script
writes no path tables. The kernel does not read them, but other
programs may reject the image for that.

    # mkiso.py <image> <logical block size>: ISO 9660 image, 40 empty files
    import struct, sys
    lbs = int(sys.argv[2])
    root = 18 * 2048 // lbs
    def b16(v): return struct.pack('<H', v) + struct.pack('>H', v)
    def b32(v): return struct.pack('<I', v) + struct.pack('>I', v)
    def pad(b): return b.ljust(2048, b'\0')
    def rec(name, block, size, flags):
        r = (bytes([0, 0]) + b32(block) + b32(size) + bytes(7) +
             bytes([flags, 0, 0]) + b16(1) + bytes([len(name)]) + name)
        r += bytes(len(r) % 2)
        return bytes([len(r)]) + r[1:]
    d = rec(b'\0', root, 2048, 2) + rec(b'\1', root, 2048, 2)
    for i in range(1, 41):
        d += rec(b'FILE%04d.TXT;1' % i, 0, 0, 0)
    pvd = bytearray(2048)
    pvd[0:7] = b'\1CD001\1'
    pvd[80:88] = b32(root + 2048 // lbs)
    pvd[120:132] = b16(1) + b16(1) + b16(lbs)
    pvd[156:190] = d[:34]
    img = bytes(16 * 2048) + pvd + pad(b'\xffCD001\1') + pad(d)
    open(sys.argv[1], 'wb').write(img)

> The mount option block={512|1024|2048} may set the address unit for
> reading the volume descriptors in isofs_fill_super():
>          opt->blocksize = sb_min_blocksize(s, opt->blocksize);
> This means that if it gets through, it is the assumed Logical Sector
> size.

As far as I can see, the option sets the size of the buffer that is
read, but not the place. The loop over the volume descriptors has

	block = iso_blknum << (ISOFS_BLOCK_BITS - s->s_blocksize_bits);

so sector N is read at byte N * 2048 for every value of the option.
The sector size is 2048 there as well.

> So if the user is allowed to set an initial Sector size, shouldn't this
> be used for the check in this patch, too ?

I think 2048 is the right value for the check. By ECMA-119 6.1.2 a
Logical Sector has 2048 bytes or more, and isofs_fill_super() fails
when the block size of the device is larger than 2048. So the Logical
Sector has 2048 bytes on every volume that isofs mounts.

If the check used the option, its default of 1024 would make the check
reject every record that crosses a 1024-byte block, which is what
happens now.

> So can mount option block={512|1024|2048} still have an effect at all ?

I see one effect. The mount fails when the logical block size of the
volume is smaller than the option:

	if (orig_zonesize < opt->blocksize)
		goto out_bad_size;

I tried images of the three sizes from the script with every value of
the option, on a loop device, on v7.2, on v7.3-rc6 and with the patch:

    lbs512.iso    mounts only with block=512
    lbs1024.iso   mounts without the option, with block=512 and with
                  block=1024, but not with block=2048
    lbs2048.iso   mounts with every value

A failed mount logs, for example:

    ISOFS: Logical zone size(512) < hardware blocksize(1024)

When the mount succeeds, the result of "ls /mnt | wc -l" does not
depend on the option. It is 40 on v7.2 and with the patch. On v7.3-rc6
it is 9, 19 and 40 for the three images.

Thanks,
Hengyu

Re: [PATCH] isofs: Fix handling of directories with small logical blocks

[Thomas Schmitt]

Hi,

Hengyu Liang wrote:
> As far as I can see, the option sets the size of the buffer that is
> read, but not the place. The loop over the volume descriptors has
> 
>         block = iso_blknum << (ISOFS_BLOCK_BITS - s->s_blocksize_bits);

I see. The code of fs/isofs surprises me every time i look at it.


> I think 2048 is the right value for the check.

I now agree.

(Actually i think 2048 is the only right value for Sector and Block.
Everything else is just weird.
libisofs did not check Logical Block Size at all. Yesterday i committed
"Prevented reading of filesystems with logical block size != 2048".)


Have a nice day :)

Thomas