Re: [PATCH 0/2] fs/iso9660: Delay CE hop until end of current SUSP area

[Daniel Kiper]

On Tue, Mar 07, 2023 at 05:56:49PM +0100, Thomas Schmitt wrote:
> Hi,
>
> SUSP 1.12 says:
>
>   The "CE" System Use Entry indicates a Continuation Area that shall be
>   processed after the current System Use field or Continuation Area is
>   processed.
>
> But GRUB rather takes an encountered CE entry as reason to immediately
> switch reading to the location that is given by the CE entry.
> This can skip over important information.
>
> The usual ISO 9660 producers on GNU/Linux write the CE entry as last
> entry of System Use field or Continuation Area. So the problem does not
> show up with their output. Nevertheless, Linux and libisofs obey the
> specs whereas GRUB does not.
>
> As demonstration i crafted a small ISO, where the CE entry comes before
> the NM entry which tells the Rock Ridge file name "RockRidgeName:x".
> Linux shows the NM name, nevertheless:
>   $ sudo mount iso9660_early_ce.iso /mnt/iso
>   mount: /mnt/iso: WARNING: source write-protected, mounted read-only.
>   $ ls /mnt/iso
>   RockRidgeName:x
>   $
>
> GRUB does not see the NM entry and thus shows the dull ISO 9660 name
> (which is actually "ROCKRIDG.;1"):
>   $ ./grub-fstest iso9660_early_ce.iso ls /
>   rockridg
>   $
>
> After the code change of my patch, i get:
>   $ ./grub-fstest iso9660_early_ce.iso ls /
>   RockRidgeName:x
>   $
>
> A new code block in tests/iso9660_test.in verifies that the patched code
> is in effect:
>   make check TESTS=iso9660_test
> detects the old code state and shows that the new code still has the
> capability to cope with endless CE loops.
>
> -------------------------------------------------------------------------
> How to create an ISO 9660 filesystem where CE is not the last SUSP entry
> of a file's directory record:
>
>    # Deliberately chosen names
>    iso=iso9660_early_ce.iso
>    # rr_path is longer than 8, mixed-case, with non-ISO-9660 character
>    rr_path=/RockRidgeName:x
>
>    # A dummy file as payload
>    echo x >x
>
>    # 250 fattr characters to surely exceed the size of a directory record
>    
> long_string=0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789
>
>    # Create ISO with the payload file and attached fattr named user.dummy .
>    # Make it small with the most restrictive ISO 9660 file name rules.
>    test -e "$iso" && rm "$iso"
>    xorriso -compliance no_emul_toc:iso_9660_level=1 \
>            -padding 0 \
>            -outdev "$iso" \
>            -xattr on \
>            -map x "$rr_path" \
>            -setfattr user.dummy "$long_string" "$rr_path" --
>
>    # Cut out the NM field and the CE field from the directory record
>    # of $rr_path. The numbers were determined by looking at a hex dump.
>    dd if="$iso" bs=1 skip=37198 count=20 of=nm_field
>    dd if="$iso" bs=1 skip=37218 count=28 of=ce_field
>
>    # Put them back in reverse sequence
>    dd conv=notrunc if=ce_field bs=1 seek=37198 of="$iso"
>    dd conv=notrunc if=nm_field bs=1 seek=37226 of="$iso"

Patch set LGTM. Though I would prefer to hear Glenn's opinion on the test
Thomas introduces because AIUI this [1] email presented some concerns.

Glenn?

Daniel

[1] https://lists.gnu.org/archive/html/grub-devel/2023-03/msg00022.html