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

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

[Thomas Schmitt]

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"

-------------------------------------------------------------------------

Have a nice day :)

Thomas

Thomas Schmitt (2):
  fs/iso9660: Delay CE hop until end of current SUSP area
  tests: Add test for iso9660 delayed CE hop

 grub-core/fs/iso9660.c        |  84 ++++++++++++++++++----------------
 tests/iso9660_early_ce.iso.gz | Bin 0 -> 709 bytes
 tests/iso9660_test.in         |  24 ++++++++++
 3 files changed, 68 insertions(+), 40 deletions(-)
 create mode 100644 tests/iso9660_early_ce.iso.gz

--
2.30.2

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

[Thomas Schmitt]

The SUSP specs demand that the reading of the next SUSP area which is
depicted by a CE entry shall be delayed until reading of the current
SUSP area is completed.
Up to now GRUB immediately ends reading of the current area and loads the
new one.

So buffer the parameters of a found CE entry and perform checks and
reading of new data only after the reader loop has ended.

Signed-off-by: Thomas Schmitt <scdbackup@gmx.net>
---
 grub-core/fs/iso9660.c | 84 ++++++++++++++++++++++--------------------
 1 file changed, 44 insertions(+), 40 deletions(-)

diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
index acccf5fc7..adf960771 100644
--- a/grub-core/fs/iso9660.c
+++ b/grub-core/fs/iso9660.c
@@ -272,6 +272,9 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
   struct grub_iso9660_susp_entry *entry;
   grub_err_t err;
   int ce_counter = 0;
+  grub_ssize_t ce_sua_size = 0;
+  grub_off_t ce_off;
+  grub_disk_addr_t ce_block;

   if (sua_size <= 0)
     return GRUB_ERR_NONE;
@@ -293,6 +296,7 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,

   entry = (struct grub_iso9660_susp_entry *) sua;

+ next_susp_area:
   while (entry->len > 0)
     {
       /* Ensure the entry is within System Use Area. */
@@ -307,55 +311,21 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
       if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0)
 	{
 	  struct grub_iso9660_susp_ce *ce;
-	  grub_disk_addr_t ce_block;

-	  if (++ce_counter > GRUB_ISO9660_MAX_CE_HOPS)
+	  if (ce_sua_size > 0)
 	    {
 	      grub_free (sua);
 	      return grub_error (GRUB_ERR_BAD_FS,
-	                         "suspecting endless CE loop");
+				 "more than one CE entry in SUSP area");
 	    }

+	  /* Buffer CE parameters for use after the end of this loop.  */
 	  ce = (struct grub_iso9660_susp_ce *) entry;
-	  sua_size = grub_le_to_cpu32 (ce->len);
-	  off = grub_le_to_cpu32 (ce->off);
+	  ce_sua_size = grub_le_to_cpu32 (ce->len);
+	  ce_off = grub_le_to_cpu32 (ce->off);
 	  ce_block = grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ;
-
-	  if (sua_size <= 0)
-	    break;
-
-	  if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
-	    {
-	      grub_free (sua);
-	      return grub_error (GRUB_ERR_BAD_FS,
-			         "invalid continuation area in CE entry");
-	    }
-
-	  grub_free (sua);
-	  sua = grub_malloc (sua_size);
-	  if (!sua)
-	    return grub_errno;
-
-	  /* Load a part of the System Usage Area.  */
-	  err = grub_disk_read (node->data->disk, ce_block, off,
-				sua_size, sua);
-	  if (err)
-	    {
-	      grub_free (sua);
-	      return err;
-	    }
-
-	  entry = (struct grub_iso9660_susp_entry *) sua;
-	  /*
-	   * The hook function will not process CE or ST.
-	   * Advancing to the next entry would skip them.
-	   */
-	  if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0
-	      || grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
-	    continue;
 	}
-
-      if (hook (entry, hook_arg))
+      else if (hook (entry, hook_arg))
 	{
 	  grub_free (sua);
 	  return 0;
@@ -367,6 +337,40 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
         break;
     }

+  if (ce_sua_size > 0)
+    {
+      /* Load the next System Use Area by buffered CE entry parameters.  */
+      if (++ce_counter > GRUB_ISO9660_MAX_CE_HOPS)
+	{
+	  grub_free (sua);
+	  return grub_error (GRUB_ERR_BAD_FS, "suspecting endless CE loop");
+	}
+      if (ce_sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
+	{
+	  grub_free (sua);
+	  return grub_error (GRUB_ERR_BAD_FS,
+			     "invalid continuation area in CE entry");
+	}
+
+      grub_free (sua);
+      sua = grub_malloc (ce_sua_size);
+      if (!sua)
+	return grub_errno;
+
+      err = grub_disk_read (node->data->disk, ce_block, ce_off, ce_sua_size,
+			    sua);
+      if (err)
+	{
+	  grub_free (sua);
+	  return err;
+	}
+      entry = (struct grub_iso9660_susp_entry *) sua;
+      sua_size = ce_sua_size;
+      ce_sua_size = 0;
+
+      goto next_susp_area;
+    }
+
   grub_free (sua);
   return 0;
 }
--
2.30.2

[PATCH 2/2] tests: Add test for iso9660 delayed CE hop

[Thomas Schmitt]

The ISO filesystem image iso9660_early_ce.iso exposes the unusual
situation that the Rock Ridge name entry of its only file is located
after a CE entry which points to the next continuation area.

The correct behavior is to read the Rock Ridge name and to only then
load the next continuation area. If GRUB performs this correctly, then
the name "RockRidgeName:x" will be read and reported by grub-fstest.
If GRUB wrongly performs the CE hop immediatly when encountering the CE
entry, then the dull ISO 9660 name "rockridg" will not be overridden and
be put out by grub-fstest.

Signed-off-by: Thomas Schmitt <scdbackup@gmx.net>
---
 tests/iso9660_early_ce.iso.gz | Bin 0 -> 709 bytes
 tests/iso9660_test.in         |  24 ++++++++++++++++++++++++
 2 files changed, 24 insertions(+)
 create mode 100644 tests/iso9660_early_ce.iso.gz

diff --git a/tests/iso9660_early_ce.iso.gz b/tests/iso9660_early_ce.iso.gz
new file mode 100644
index 0000000000000000000000000000000000000000..df4ef5fb276944c41a8e355b96423ce7e935d585
GIT binary patch
literal 709
zcmb2|=3wyXVNPLSetW~$U)WLNz(+~1E!S8$9vm%@xUx8Jo9<Ir|I<dTowx6veKYNV
zXRkmqPt`@1!<|QtynitB{KMxDo%YO@xbwD**<%a)1gW3@FQvtOpE)z{^31gY45R^v
zU)t-Ur#tZ`Ogd4rC-m>`Z$|g_8lRLub~f&3bja#OVIfzS`OkVU;;7brX~~Lbo}q6-
z-PF_Gbx-{C%W?C+`?)UH(#oF&-p#bsy;>mcdpy^o*LC^5BJn#v+qDc%NB%Cl-sU=g
z)3Uj9wC0=eSKI#I@rSu=_PdzceH(A&>|AFN{_x+UnpTm+ajb2xjq0{FdKccg|DTcJ
zz}JtN4~)x%?oWx|-&ygv(dN@9y?cTGcNDz4GGW{MQyc$%O&5+oe`4jL%ON*^yU(<d
zH({^bcyNEm%|)l*cueejb29qsD<AuwJ-N#NSDI?CJ~z3_?$W`!dz<P5`br{BPCk`5
zQ%qS@ag*Y)yPOyG=f<V&+_N!G!cwek+0qT~<MwS_zdbIuUB7Ib{CmEA_6!VvcyGy_
zjQSNBmHjCF>(ZsWrmxGKn%|*yUvR5Y;;Oa#pS4EXZ3~q888{*R+Sx9@kW0OfPb^y{
z8!77Ec{3naIAdy#+RAq;T*~(tnAQog-}2t_X|ciAKOt*p90@Nhw=_68Yun!L$q~Po
z<<{t%2CkU2BD?nO3Y{jqtA^4>+}F)2&CaMLTFtu~@;dOs+l;Bc$IpvRbaOpWq$8ZR
z)K7oSV^-mtX)%(uzvFt+mUkuVTb$#wFxQHfFDtX_dY`xV;+*WW`Z{{C{d2^7ybFV5
xzJ&`i6tt$izd8FqFqn7vRayU6VJ9ZN=p}78dn3uf(6IWO@akK61q=d=3;-EpKr#RT

literal 0
HcmV?d00001

diff --git a/tests/iso9660_test.in b/tests/iso9660_test.in
index 44bc08c6d..d7022e061 100644
--- a/tests/iso9660_test.in
+++ b/tests/iso9660_test.in
@@ -28,3 +28,27 @@ for fs in iso9660_ce_loop iso9660_ce_loop2; do
   fi
 done
 echo "PASS"
+
+echo "Testing for proper handling of early CE ... "
+fs=iso9660_early_ce
+tempdir=`mktemp -d "${TMPDIR:-/tmp}/${0##*/}.$(date '+%Y%m%d%H%M%S%N').${fs}.XXX"` ||
+  { echo "Failed to make temporary directory"; exit 99; }
+gunzip <"$srcdir"/tests/${fs}.iso.gz >"${tempdir}/${fs}.iso" || exit 99
+ret=0
+output=$(LC_ALL=C timeout -s KILL "60" \
+  "@builddir@/grub-fstest" "${tempdir}/${fs}.iso" ls / ) || ret=$?
+rm -rf "$tempdir"
+if [ "${ret:-0}" -ne 0 ]; then
+  echo "... grub-fstest returns $ret"
+  echo "FAIL ($fs)"
+  exit 1
+fi
+# Before comparing: remove trailing blank added by grub-fstest
+output=$(echo -n $output)
+if [ x"$output" != x"RockRidgeName:x" ]; then
+  echo "... found: '$output' , expected: 'RockRidgeName:x'"
+  echo "FAIL ($fs)"
+  exit 1
+fi
+echo "PASS"
+
--
2.30.2

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

[Lidong Chen]

Hi,

Thanks for the detailed instruction for running the test! 
The patches look good to me. I ran the tests with and without the patches, I got the expected result.

Thanks,
Lidong 


> On Mar 7, 2023, at 8:56 AM, Thomas Schmitt <scdbackup@gmx.net> 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"
> 
> -------------------------------------------------------------------------
> 
> Have a nice day :)
> 
> Thomas
> 
> Thomas Schmitt (2):
>  fs/iso9660: Delay CE hop until end of current SUSP area
>  tests: Add test for iso9660 delayed CE hop
> 
> grub-core/fs/iso9660.c        |  84 ++++++++++++++++++----------------
> tests/iso9660_early_ce.iso.gz | Bin 0 -> 709 bytes
> tests/iso9660_test.in         |  24 ++++++++++
> 3 files changed, 68 insertions(+), 40 deletions(-)
> create mode 100644 tests/iso9660_early_ce.iso.gz
> 
> --
> 2.30.2
> 

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

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

[Thomas Schmitt]

Hi,


Daniel Kiper wrote:
> AIUI this [1] email presented some concerns.
> [1] https://lists.gnu.org/archive/html/grub-devel/2023-03/msg00022.html

This was about the trailing blank in the output of grub-fstest.

I decided to simply remove it because it is not expectable that any wrong
handling of a CE entry would produce blanks from the two filename
alternatives "ROCKRIDG.;1" and "RockRidgeName:x".
So i wrote in [PATCH 2/2]:

  +# Before comparing: remove trailing blank added by grub-fstest
  +output=$(echo -n $output)

(I remember to have found the origin of the blank somewhere in the deeper
levels of the code, possibly grub-core/commands/ls.c function print_files.
It did not look like it would be reasonable to ask for changing it.)


Have a nice day :)

Thomas

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

[Glenn Washburn]

On 3/30/23 18:30, Daniel Kiper wrote:
> 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

I had partially written a response to the above referenced email a 
couple weeks ago, but didn't sent it because it seemed unnecessary with 
the way Thomas changed the test to work with or without extra trailing 
spaces. So the concerns in the referenced email aren't a concern for 
this test patch.

As for a more general concern, not really either. If the trailing spaces 
are removed later on, tests will fail and then they can be fixed. To sum 
up what I was going to say in response to the referenced email, 
grub-fstest is a tool for running grub commands/code in user-space and 
has subcommands corresponding for specifying what code to run. I haven't 
verified, but I believe that in this case the grub "ls" command is 
creating the output, so its likely that the command is what is producing 
the extra space, not grub-fstest itself. Again, not something to worry 
about here, IMO.

So patch #2, the test patch, in the series LGTM.

Glenn

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

[Daniel Kiper]

On Fri, Mar 31, 2023 at 12:57:05AM +0000, Glenn Washburn wrote:
> On 3/30/23 18:30, Daniel Kiper wrote:
> > 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
>
> I had partially written a response to the above referenced email a couple
> weeks ago, but didn't sent it because it seemed unnecessary with the way
> Thomas changed the test to work with or without extra trailing spaces. So
> the concerns in the referenced email aren't a concern for this test patch.
>
> As for a more general concern, not really either. If the trailing spaces are
> removed later on, tests will fail and then they can be fixed. To sum up what
> I was going to say in response to the referenced email, grub-fstest is a
> tool for running grub commands/code in user-space and has subcommands
> corresponding for specifying what code to run. I haven't verified, but I
> believe that in this case the grub "ls" command is creating the output, so
> its likely that the command is what is producing the extra space, not
> grub-fstest itself. Again, not something to worry about here, IMO.
>
> So patch #2, the test patch, in the series LGTM.

Glenn, thank you for detailed response.

Then for both patches Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>...

Thomas, thank you for fixing this issue.

Daniel

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

[Lidong Chen]

[-- Attachment #1: Type: text/plain, Size: 3908 bytes --]


Tested-by: Lidong Chen <lidong.chen@oracle.com<mailto:lidong.chen@oracle.com>>

On Mar 8, 2023, at 10:28 PM, Lidong Chen <lidong.chen@oracle.com> wrote:

Hi,

Thanks for the detailed instruction for running the test!
The patches look good to me. I ran the tests with and without the patches, I got the expected result.

Thanks,
Lidong


On Mar 7, 2023, at 8:56 AM, Thomas Schmitt <scdbackup@gmx.net> 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"

-------------------------------------------------------------------------

Have a nice day :)

Thomas

Thomas Schmitt (2):
fs/iso9660: Delay CE hop until end of current SUSP area
tests: Add test for iso9660 delayed CE hop

grub-core/fs/iso9660.c        |  84 ++++++++++++++++++----------------
tests/iso9660_early_ce.iso.gz | Bin 0 -> 709 bytes
tests/iso9660_test.in         |  24 ++++++++++
3 files changed, 68 insertions(+), 40 deletions(-)
create mode 100644 tests/iso9660_early_ce.iso.gz

--
2.30.2




[-- Attachment #2: Type: text/html, Size: 5469 bytes --]