[PATCH v3 0/5] fs/iso9660: Fix out-of-bounds read

[PATCH v3 0/5] fs/iso9660: Fix out-of-bounds read

[Lidong Chen]

This v3 patches set addressed v2 review comments for patch 5.
There is no changes to patch 1 to 4. Tested patch 5, the fixes 
were able to detect the endless loop, and Grub exited accordingly.

Lidong Chen (5):
  fs/iso9660: Add check to prevent infinite loop
  fs/iso9660: Prevent read past the end of system use area
  fs/iso9660: Avoid reading past the entry boundary
  fs/iso9660: Incorrect check for entry boundary
  fs/iso9660: Prevent skipping CE or ST at start of continuation area

 grub-core/fs/iso9660.c | 100 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 93 insertions(+), 7 deletions(-)

-- 
2.35.1

[PATCH v3 1/5] fs/iso9660: Add check to prevent infinite loop

[Lidong Chen]

There is no check for the end of block when reading
directory extents. It resulted in read_node() always
read from the same offset in the while loop, thus
caused infinite loop. The fix added a check for the
end of the block and ensure the read is within directory
boundary.

Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
Reviewed-by: Thomas Schmitt <scdbackup@gmx.net>
---
 grub-core/fs/iso9660.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
index 91817ec1f..4f4cd6165 100644
--- a/grub-core/fs/iso9660.c
+++ b/grub-core/fs/iso9660.c
@@ -795,6 +795,15 @@ grub_iso9660_iterate_dir (grub_fshelp_node_t dir,
 	while (dirent.flags & FLAG_MORE_EXTENTS)
 	  {
 	    offset += dirent.len;
+
+	    /* offset should within the dir's len. */
+	    if (offset > len)
+	      {
+		if (ctx.filename_alloc)
+		  grub_free (ctx.filename);
+		return 0;
+	      }
+
 	    if (read_node (dir, offset, sizeof (dirent), (char *) &dirent))
 	      {
 		if (ctx.filename_alloc)
@@ -802,6 +811,18 @@ grub_iso9660_iterate_dir (grub_fshelp_node_t dir,
 		grub_free (node);
 		return 0;
 	      }
+
+	    /*
+	     * It is either the end of block or zero-padded sector,
+	     * skip to the next block.
+	     */
+	    if (!dirent.len)
+	      {
+		offset = (offset / GRUB_ISO9660_BLKSZ + 1) * GRUB_ISO9660_BLKSZ;
+		dirent.flags |= FLAG_MORE_EXTENTS;
+		continue;
+	      }
+
 	    if (node->have_dirents >= node->alloc_dirents)
 	      {
 		struct grub_fshelp_node *new_node;
-- 
2.35.1

Re: [PATCH v3 1/5] fs/iso9660: Add check to prevent infinite loop

[Daniel Kiper]

On Fri, Jan 20, 2023 at 07:39:38PM +0000, Lidong Chen wrote:
> There is no check for the end of block when reading
> directory extents. It resulted in read_node() always
> read from the same offset in the while loop, thus
> caused infinite loop. The fix added a check for the
> end of the block and ensure the read is within directory
> boundary.
>
> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
> Reviewed-by: Thomas Schmitt <scdbackup@gmx.net>
> ---
>  grub-core/fs/iso9660.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
> index 91817ec1f..4f4cd6165 100644
> --- a/grub-core/fs/iso9660.c
> +++ b/grub-core/fs/iso9660.c
> @@ -795,6 +795,15 @@ grub_iso9660_iterate_dir (grub_fshelp_node_t dir,
>  	while (dirent.flags & FLAG_MORE_EXTENTS)
>  	  {
>  	    offset += dirent.len;
> +
> +	    /* offset should within the dir's len. */
> +	    if (offset > len)
> +	      {
> +		if (ctx.filename_alloc)
> +		  grub_free (ctx.filename);

The Coverity discovered this hunk was leaking node memory. I have added
grub_free(node) call here and it stopped complaining. Now patches are in...

> +		return 0;
> +	      }
> +
>  	    if (read_node (dir, offset, sizeof (dirent), (char *) &dirent))
>  	      {
>  		if (ctx.filename_alloc)
> @@ -802,6 +811,18 @@ grub_iso9660_iterate_dir (grub_fshelp_node_t dir,
>  		grub_free (node);
>  		return 0;
>  	      }
> +
> +	    /*
> +	     * It is either the end of block or zero-padded sector,
> +	     * skip to the next block.
> +	     */
> +	    if (!dirent.len)
> +	      {
> +		offset = (offset / GRUB_ISO9660_BLKSZ + 1) * GRUB_ISO9660_BLKSZ;
> +		dirent.flags |= FLAG_MORE_EXTENTS;
> +		continue;
> +	      }
> +
>  	    if (node->have_dirents >= node->alloc_dirents)
>  	      {
>  		struct grub_fshelp_node *new_node;

Daniel

Re: [PATCH v3 1/5] fs/iso9660: Add check to prevent infinite loop

[Lidong Chen]

> On Feb 2, 2023, at 11:35 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> 
> On Fri, Jan 20, 2023 at 07:39:38PM +0000, Lidong Chen wrote:
>> There is no check for the end of block when reading
>> directory extents. It resulted in read_node() always
>> read from the same offset in the while loop, thus
>> caused infinite loop. The fix added a check for the
>> end of the block and ensure the read is within directory
>> boundary.
>> 
>> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
>> Reviewed-by: Thomas Schmitt <scdbackup@gmx.net>
>> ---
>> grub-core/fs/iso9660.c | 21 +++++++++++++++++++++
>> 1 file changed, 21 insertions(+)
>> 
>> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
>> index 91817ec1f..4f4cd6165 100644
>> --- a/grub-core/fs/iso9660.c
>> +++ b/grub-core/fs/iso9660.c
>> @@ -795,6 +795,15 @@ grub_iso9660_iterate_dir (grub_fshelp_node_t dir,
>> 	while (dirent.flags & FLAG_MORE_EXTENTS)
>> 	  {
>> 	    offset += dirent.len;
>> +
>> +	    /* offset should within the dir's len. */
>> +	    if (offset > len)
>> +	      {
>> +		if (ctx.filename_alloc)
>> +		  grub_free (ctx.filename);
> 
> The Coverity discovered this hunk was leaking node memory. I have added
> grub_free(node) call here and it stopped complaining. Now patches are in...

Ok, thanks Daniel!

Lidong

> 
>> +		return 0;
>> +	      }
>> +
>> 	    if (read_node (dir, offset, sizeof (dirent), (char *) &dirent))
>> 	      {
>> 		if (ctx.filename_alloc)
>> @@ -802,6 +811,18 @@ grub_iso9660_iterate_dir (grub_fshelp_node_t dir,
>> 		grub_free (node);
>> 		return 0;
>> 	      }
>> +
>> +	    /*
>> +	     * It is either the end of block or zero-padded sector,
>> +	     * skip to the next block.
>> +	     */
>> +	    if (!dirent.len)
>> +	      {
>> +		offset = (offset / GRUB_ISO9660_BLKSZ + 1) * GRUB_ISO9660_BLKSZ;
>> +		dirent.flags |= FLAG_MORE_EXTENTS;
>> +		continue;
>> +	      }
>> +
>> 	    if (node->have_dirents >= node->alloc_dirents)
>> 	      {
>> 		struct grub_fshelp_node *new_node;
> 
> Daniel

[PATCH v3 2/5] fs/iso9660: Prevent read past the end of system use area

[Lidong Chen]

In the code, the for loop advanced the entry pointer to the
next entry before checking if the next entry is within the
system use area boundary. Another issue in the code was that
there is no check for the size of system use area. For a
corrupted system, the size of system use area can be less than
the size of minimum SUSP entry size (4 bytes). These can cause
buffer overrun. The fixes added the checks to ensure the read is
valid and within the boundary.

Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
Reviewed-by: Thomas Schmitt <scdbackup@gmx.net>
---
 grub-core/fs/iso9660.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
index 4f4cd6165..65c8862b6 100644
--- a/grub-core/fs/iso9660.c
+++ b/grub-core/fs/iso9660.c
@@ -49,6 +49,8 @@ GRUB_MOD_LICENSE ("GPLv3+");
 #define GRUB_ISO9660_VOLDESC_PART	3
 #define GRUB_ISO9660_VOLDESC_END	255
 
+#define GRUB_ISO9660_SUSP_HEADER_SZ	4
+
 /* The head of a volume descriptor.  */
 struct grub_iso9660_voldesc
 {
@@ -272,6 +274,9 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
   if (sua_size <= 0)
     return GRUB_ERR_NONE;
 
+  if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
+    return grub_error (GRUB_ERR_BAD_FS, "invalid susp entry size");
+
   sua = grub_malloc (sua_size);
   if (!sua)
     return grub_errno;
@@ -281,10 +286,14 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
   if (err)
     return err;
 
-  for (entry = (struct grub_iso9660_susp_entry *) sua; (char *) entry < (char *) sua + sua_size - 1 && entry->len > 0;
-       entry = (struct grub_iso9660_susp_entry *)
-	 ((char *) entry + entry->len))
+  entry = (struct grub_iso9660_susp_entry *) sua;
+
+  while (entry->len > 0)
     {
+      /* Ensure the entry is within System Use Area */
+      if ((char *) entry + entry->len > (sua + sua_size))
+        break;
+
       /* The last entry.  */
       if (grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
 	break;
@@ -300,6 +309,16 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
 	  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)
@@ -319,6 +338,11 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
 	  grub_free (sua);
 	  return 0;
 	}
+
+      entry = (struct grub_iso9660_susp_entry *) ((char *) entry + entry->len);
+
+      if (((sua + sua_size) - (char *) entry) < GRUB_ISO9660_SUSP_HEADER_SZ)
+        break;
     }
 
   grub_free (sua);
-- 
2.35.1

[PATCH v3 3/5] fs/iso9660: Avoid reading past the entry boundary

[Lidong Chen]

Added a check for the SP entry data boundary before reading it.

Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
Reviewed-by: Thomas Schmitt <scdbackup@gmx.net>
---
 grub-core/fs/iso9660.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
index 65c8862b6..c6d65fc22 100644
--- a/grub-core/fs/iso9660.c
+++ b/grub-core/fs/iso9660.c
@@ -409,6 +409,9 @@ set_rockridge (struct grub_iso9660_data *data)
   if (!sua_size)
     return GRUB_ERR_NONE;
 
+  if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
+    return grub_error (GRUB_ERR_BAD_FS, "invalid rock ridge entry size");
+
   sua = grub_malloc (sua_size);
   if (! sua)
     return grub_errno;
@@ -435,8 +438,17 @@ set_rockridge (struct grub_iso9660_data *data)
       rootnode.have_symlink = 0;
       rootnode.dirents[0] = data->voldesc.rootdir;
 
-      /* The 2nd data byte stored how many bytes are skipped every time
-	 to get to the SUA (System Usage Area).  */
+      /* The size of SP (version 1) is fixed to 7. */
+      if (sua_size < 7 || entry->len < 7)
+	{
+	  grub_free (sua);
+	  return grub_error (GRUB_ERR_BAD_FS, "corrupted rock ridge entry");
+	}
+
+      /*
+       * The 2nd data byte stored how many bytes are skipped every time
+       * to get to the SUA (System Usage Area).
+       */
       data->susp_skip = entry->data[2];
       entry = (struct grub_iso9660_susp_entry *) ((char *) entry + entry->len);
 
-- 
2.35.1

[PATCH v3 4/5] fs/iso9660: Incorrect check for entry boundary

[Lidong Chen]

An SL entry consists of the entry info and the component area.
The entry info should take up 5 bytes instead of sizeof (*entry).
The area after the first 5 bytes is the component area. It is
incorrect to use the sizeof (*entry) to check the entry boundary.

Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
Reviewed-by: Thomas Schmitt <scdbackup@gmx.net>
---
 grub-core/fs/iso9660.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
index c6d65fc22..ca45b3424 100644
--- a/grub-core/fs/iso9660.c
+++ b/grub-core/fs/iso9660.c
@@ -663,10 +663,23 @@ susp_iterate_dir (struct grub_iso9660_susp_entry *entry,
   else if (grub_strncmp ("SL", (char *) entry->sig, 2) == 0)
     {
       unsigned int pos = 1;
+      unsigned int csize;
 
-      /* The symlink is not stored as a POSIX symlink, translate it.  */
-      while (pos + sizeof (*entry) < entry->len)
+      /* The symlink is not stored as a POSIX symlink, translate it. */
+      while ((pos + GRUB_ISO9660_SUSP_HEADER_SZ + 1) < entry->len)
 	{
+	  /*
+	   * entry->len is GRUB_ISO9660_SUSP_HEADER_SZ + 1 (the FLAGS)
+	   * + length of the "Component Area". The length of a component
+	   * record is 2 (pos and pos + 1) plus the "Component Content",
+	   * of which starts at pos + 2. entry->data[pos] is the
+	   * 'Component Flags'; entry->data[pos + 1] is the length
+	   * of the component.
+	   */
+          csize = entry->data[pos + 1] + 2;
+          if (GRUB_ISO9660_SUSP_HEADER_SZ + 1 + csize > entry->len)
+            break;
+
 	  /* The current position is the `Component Flag'.  */
 	  switch (entry->data[pos] & 30)
 	    {
-- 
2.35.1

[PATCH v3 5/5] fs/iso9660: Prevent skipping CE or ST at start of continuation area

[Lidong Chen]

If processing of a SUSP CE entry leads to a continuation area which
begins by entry CE or ST, then these entries were skipped without
interpretation. In case of CE this would lead to premature end of
processing the SUSP entries of the file. In case of ST this could
cause following non-SUSP bytes to be interpreted as SUSP entries.

Signed-off-by: Thomas Schmitt <scdbackup@gmx.net>
Tested-by: Lidong Chen <lidong.chen@oracle.com>
---
 grub-core/fs/iso9660.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
index ca45b3424..3ddb06ed4 100644
--- a/grub-core/fs/iso9660.c
+++ b/grub-core/fs/iso9660.c
@@ -50,6 +50,7 @@ GRUB_MOD_LICENSE ("GPLv3+");
 #define GRUB_ISO9660_VOLDESC_END	255
 
 #define GRUB_ISO9660_SUSP_HEADER_SZ	4
+#define GRUB_ISO9660_MAX_CE_HOPS	100000
 
 /* The head of a volume descriptor.  */
 struct grub_iso9660_voldesc
@@ -270,6 +271,7 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
   char *sua;
   struct grub_iso9660_susp_entry *entry;
   grub_err_t err;
+  int ce_counter = 0;
 
   if (sua_size <= 0)
     return GRUB_ERR_NONE;
@@ -304,6 +306,13 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
 	  struct grub_iso9660_susp_ce *ce;
 	  grub_disk_addr_t ce_block;
 
+	  if (++ce_counter > GRUB_ISO9660_MAX_CE_HOPS)
+	    {
+	      grub_free (sua);
+	      return grub_error (GRUB_ERR_BAD_FS,
+	                         "suspecting endless CE loop");
+	    }
+
 	  ce = (struct grub_iso9660_susp_ce *) entry;
 	  sua_size = grub_le_to_cpu32 (ce->len);
 	  off = grub_le_to_cpu32 (ce->off);
@@ -331,6 +340,13 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
 	    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))
-- 
2.35.1

Re: [PATCH v3 5/5] fs/iso9660: Prevent skipping CE or ST at start of continuation area

[Thomas Schmitt]

Hi,

On Fri, 20 Jan 2023 19:39:42 +0000 Lidong Chen <lidong.chen@oracle.com> wrote:
> If processing of a SUSP CE entry leads to a continuation area which
> begins by entry CE or ST, then these entries were skipped without
> interpretation. In case of CE this would lead to premature end of
> processing the SUSP entries of the file. In case of ST this could
> cause following non-SUSP bytes to be interpreted as SUSP entries.
>
> Signed-off-by: Thomas Schmitt <scdbackup@gmx.net>
> Tested-by: Lidong Chen <lidong.chen@oracle.com>
> ---
>  grub-core/fs/iso9660.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
> index ca45b3424..3ddb06ed4 100644
> --- a/grub-core/fs/iso9660.c
> +++ b/grub-core/fs/iso9660.c
> @@ -50,6 +50,7 @@ GRUB_MOD_LICENSE ("GPLv3+");
>  #define GRUB_ISO9660_VOLDESC_END	255
>
>  #define GRUB_ISO9660_SUSP_HEADER_SZ	4
> +#define GRUB_ISO9660_MAX_CE_HOPS	100000
>
>  /* The head of a volume descriptor.  */
>  struct grub_iso9660_voldesc
> @@ -270,6 +271,7 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
>    char *sua;
>    struct grub_iso9660_susp_entry *entry;
>    grub_err_t err;
> +  int ce_counter = 0;
>
>    if (sua_size <= 0)
>      return GRUB_ERR_NONE;
> @@ -304,6 +306,13 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
>  	  struct grub_iso9660_susp_ce *ce;
>  	  grub_disk_addr_t ce_block;
>
> +	  if (++ce_counter > GRUB_ISO9660_MAX_CE_HOPS)
> +	    {
> +	      grub_free (sua);
> +	      return grub_error (GRUB_ERR_BAD_FS,
> +	                         "suspecting endless CE loop");
> +	    }
> +
>  	  ce = (struct grub_iso9660_susp_ce *) entry;
>  	  sua_size = grub_le_to_cpu32 (ce->len);
>  	  off = grub_le_to_cpu32 (ce->off);
> @@ -331,6 +340,13 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
>  	    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))
> --
> 2.35.1

Reviewed-by: Thomas Schmitt <scdbackup@gmx.net>


Have a nice day :)

Thomas

Re: [PATCH v3 0/5] fs/iso9660: Fix out-of-bounds read

[Daniel Kiper]

On Fri, Jan 20, 2023 at 07:39:37PM +0000, Lidong Chen wrote:
> This v3 patches set addressed v2 review comments for patch 5.
> There is no changes to patch 1 to 4. Tested patch 5, the fixes
> were able to detect the endless loop, and Grub exited accordingly.
>
> Lidong Chen (5):
>   fs/iso9660: Add check to prevent infinite loop
>   fs/iso9660: Prevent read past the end of system use area
>   fs/iso9660: Avoid reading past the entry boundary
>   fs/iso9660: Incorrect check for entry boundary
>   fs/iso9660: Prevent skipping CE or ST at start of continuation area

For all the patches Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>...

Lidong, Thomas, I thank both of you for patiently working on these fixes!

Thomas, it would be nice if you could add the broken ISOs images which you
used for tests to the tests in the GRUB. If you do that please CC Glenn.

Daniel

Re: [PATCH v3 0/5] fs/iso9660: Fix out-of-bounds read

[Thomas Schmitt]

Hi,

Daniel Kiper wrote:
> Thomas, it would be nice if you could add the broken ISOs images which you
> used for tests to the tests in the GRUB. If you do that please CC Glenn.

Is it wise to have a test which will loop endlessly in case of failure ?
Is there a way to let a test time out ?


Whatever:

After poking in my memory and GRUB's tests directory i came to
tests/util/grub-fs-tester.in which produces its ISOs as needed.
It could be appropriate to create one or both CE loop ISOs there.
But this might become a problem in the future, because the post-production
hacks depend on correct byte addresses in the ISO image.

So it would be better to add one or two canned images:
  897 bytes of http://scdbackup.webframe.org/ce_loop.iso.gz
  904 bytes of http://scdbackup.webframe.org/ce_loop2.iso.gz

Next problem is that these images do not go well with the other tests in
grub-fs-tester.in. I would want to run

  gunzip <ce_loop.iso.gz >ce_loop.iso
  run_grubfstest ls /

in the neighborhood of the xorriso runs and then bail out immediately.
But i don't yet fully understand what the for-loops around the xorriso
runs mean:

  for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
      ...
      for BLKSIZE in $blksizes; do
          ...
          for NDEVICES in $(range "$MINDEVICES" "$MAXDEVICES" 1); do
                  ...
                  x"ziso9660")
                      FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
                      xorriso ...

So how to bail out properly at this point after e.g.

                  x"iso9660_ce_loop")
                      gunzip <ce_loop.iso.gz >ce_loop.iso
                      run_grubfstest ls /

?

And why do the ls tests in grub-fs-tester.in look like
   run_grubfstest ls -- -la
which i cannot decipher by help of the options[]-list in grub-fstest.c ?


I CC Glenn Washburn already now, in the hope that he can point me to
examples or states that these ISOs should not become part of the tests.
(Crossing fingers for the latter case ... ;-)


Have a nice day :)

Thomas

Re: [PATCH v3 0/5] fs/iso9660: Fix out-of-bounds read

[Glenn Washburn]

On Wed, 25 Jan 2023 21:24:00 +0100
"Thomas Schmitt" <scdbackup@gmx.net> wrote:

> Hi,
> 
> Daniel Kiper wrote:
> > Thomas, it would be nice if you could add the broken ISOs images
> > which you used for tests to the tests in the GRUB. If you do that
> > please CC Glenn.
> 
> Is it wise to have a test which will loop endlessly in case of
> failure ? Is there a way to let a test time out ?

If the test is a non-native one, ie. uses QEMU, there is a timeout, so
it would be fine. For native tests we'd want to write the test to be
stopped and fail after a certain amount of time.

> 
> Whatever:
> 
> After poking in my memory and GRUB's tests directory i came to
> tests/util/grub-fs-tester.in which produces its ISOs as needed.
> It could be appropriate to create one or both CE loop ISOs there.
> But this might become a problem in the future, because the
> post-production hacks depend on correct byte addresses in the ISO
> image.
> 
> So it would be better to add one or two canned images:
>   897 bytes of http://scdbackup.webframe.org/ce_loop.iso.gz
>   904 bytes of http://scdbackup.webframe.org/ce_loop2.iso.gz

These are small, I'm good with adding these images to the repository.

> 
> Next problem is that these images do not go well with the other tests
> in grub-fs-tester.in. I would want to run
> 
>   gunzip <ce_loop.iso.gz >ce_loop.iso
>   run_grubfstest ls /
> 
> in the neighborhood of the xorriso runs and then bail out immediately.
> But i don't yet fully understand what the for-loops around the xorriso
> runs mean:
> 
>   for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
>       ...
>       for BLKSIZE in $blksizes; do
>           ...
>           for NDEVICES in $(range "$MINDEVICES" "$MAXDEVICES" 1); do
>                   ...
>                   x"ziso9660")
>                       FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
>                       xorriso ...
> 
> So how to bail out properly at this point after e.g.
> 
>                   x"iso9660_ce_loop")
>                       gunzip <ce_loop.iso.gz >ce_loop.iso
>                       run_grubfstest ls /
> 
> ?

I'm not sure I completely understand the question. The grub-fs-tester
script runs a series of tests for various filesystems varying the block
size, sector size and number of devices. In this case ISO, that I'm
aware of, is not a multidevice filesystem/block device (unlike LVM, ZFS
or BTRFS). So I believe it would have MAXDEVICES == MINDEVICES == 1, which is default. For the *iso9660*, those 3 nested loops above only have one iteration each, so you can effectively ignore them.

Why do you want to bail out at this point? I think what the case
statements should look like is:

                   x"iso9660_ce_loop")
                       gunzip <"@srcdir@"/tests/ce_loop.iso.gz >"${FSIMAGEP}0.img" ;;

Then need to add "iso9660_ce_loop" to all the tests that it should be in, which I'm guessing would be the same ones that ziso9660 is in. Also I'd rather have @srcdir@ be $srcdir and have 'srcdir = "@srcdir@"' near the beginning of the script.

You were talking about the test endlessly looping above, since these
are native tests we'd put a timeout in the wrapper script that calls
grub-fs-tester. That would look like adding the following line to the end of tests/iso9660_test.in:

timeout -s KILL "3600" "@builddir@/grub-fs-tester" iso9660_ce_loop

A better timeout value could probably be selected. It should be as short as possible, but also accounting for the fact that the tests may be run on slower machines or in virtual machines. Maybe see how long the test takes to successfully run on your machine and double that for the timeout value.

> And why do the ls tests in grub-fs-tester.in look like
>    run_grubfstest ls -- -la
> which i cannot decipher by help of the options[]-list in
> grub-fstest.c ?

IIRC, every "command" for grub-fstest, which is the first arg to grub-fstest, can interpret its args differently. In the case of the ls command, GRUB's ls command is executed natively and provided as an argument list all args after the '--'. So the above is like running "ls -la" in grub, but for the architecture of the build machine, not the target machine.

> 
> I CC Glenn Washburn already now, in the hope that he can point me to
> examples or states that these ISOs should not become part of the
> tests. (Crossing fingers for the latter case ... ;-)

I'm generally for more tests, especially since you've got some corner cases that we've broken on before. I see some other modifications that I'd like to make to grub-fs-tester, so I could make the changes to add this as well with your guidance. Or if the above info is enough for you to get the tests running, send a patch to the list and I'll review it.

Glenn

Re: [PATCH v3 0/5] fs/iso9660: Fix out-of-bounds read

[Thomas Schmitt]

Hi,

i wrote:
> > So it would be better to add one or two canned images:
> >   897 bytes of http://scdbackup.webframe.org/ce_loop.iso.gz
> >   904 bytes of http://scdbackup.webframe.org/ce_loop2.iso.gz

Glenn Washburn wrote:
> These are small, I'm good with adding these images to the repository.

After the fixes about CE, one of them would suffice.
ce_loop.iso is the easier hack and thus more probable to be created
by some hoaxer. It does not loop endlessly with GRUB before the fixes.
ce_loop2.iso loops endlessly already with the older GRUB versions.


> > I would want to run
> >   gunzip <ce_loop.iso.gz >ce_loop.iso
> >   run_grubfstest ls /
> > in the neighborhood of the xorriso runs and then bail out immediately.

> Why do you want to bail out at this point? I think what the case
> statements should look like is:
>
>                    x"iso9660_ce_loop")
>                        gunzip <"@srcdir@"/tests/ce_loop.iso.gz >"${FSIMAGEP}0.img" ;;

The hoax ISOs do not contain the files which the further tests in
grub-fs-tester obviously expect. Like:

            if [ x$NOHARDLINK != xy ]; then
                if run_grubfstest cmp "$GRUBDIR/$BASEHARD" "$MNTPOINTRO/$OSDIR/$BASEFILE"  ; then

So i thought that a single test should be done immediately after
unpacking the iso.gz and then all other tests should be skipped.


> You were talking about the test endlessly looping above, since these
> are native tests we'd put a timeout in the wrapper script that calls
> grub-fs-tester. That would look like adding the following line to the end of
> tests/iso9660_test.in:
> timeout -s KILL "3600" "@builddir@/grub-fs-tester" iso9660_ce_loop

I was not aware of the timeout command.


> A better timeout value could probably be selected. It should be as short as
> possible, but also accounting for the fact that the tests may be run on
> slower machines or in virtual machines.

I think 60 seconds of timeout should suffice for 100000 loop cycles in C
with a function call and repeated reading of the same disk block.
If this lasts longer than a minute, then we should reduce the limit of
100,000 loop hops.

After applying Lidong Chen's patches i get on a 4 GHz Xeon with nvme disk:

  $ time ./grub-fstest /u/test/ce_loop.iso ls /

  real    0m0.086s
  ...
  $ time ./grub-fstest /u/test/ce_loop2.iso ls /

  real    0m0.088s
  ...

Regrettably there is no error message to see.
But the fact that grub-fstest neither loops endlessly nor shows a file
named "x" indicates that our intention is fulfilled by the patches.


> I see some other modifications that I'd like to
> make to grub-fs-tester, so I could make the changes to add this as well with
> your guidance.

I would be happy if you create the new test.
The only guidance i can offer are the download addresses and the SHA256s:

  http://scdbackup.webframe.org/ce_loop.iso.gz
  d86b73b0cc260968f50c30a5207b798f5fc2396233aee5eb3cedf9cef069f3c2

  http://scdbackup.webframe.org/ce_loop2.iso.gz
  a6bde0c1562de8959d783bca0a79ad750da2bc129bdea2362b4a7c3e83426b2c

If only one of them shall be tested, then i propose ce_loop2.iso .


Have a nice day :)

Thomas

Re: [PATCH v3 0/5] fs/iso9660: Fix out-of-bounds read

[Glenn Washburn]

On Fri, 27 Jan 2023 11:56:50 +0100
"Thomas Schmitt" <scdbackup@gmx.net> wrote:

> Hi,
> 
> i wrote:
> > > So it would be better to add one or two canned images:
> > >   897 bytes of http://scdbackup.webframe.org/ce_loop.iso.gz
> > >   904 bytes of http://scdbackup.webframe.org/ce_loop2.iso.gz
> 
> Glenn Washburn wrote:
> > These are small, I'm good with adding these images to the
> > repository.
> 
> After the fixes about CE, one of them would suffice.
> ce_loop.iso is the easier hack and thus more probable to be created
> by some hoaxer. It does not loop endlessly with GRUB before the fixes.
> ce_loop2.iso loops endlessly already with the older GRUB versions.

I'm not sure I completely understand. Why does only one suffice? It
sounds like they test different code paths. Is it possible that there
is a future code regression such that one iso succeeds and the other
fails? Doing both isn't a problem for me.

> 
> 
> > > I would want to run
> > >   gunzip <ce_loop.iso.gz >ce_loop.iso
> > >   run_grubfstest ls /
> > > in the neighborhood of the xorriso runs and then bail out
> > > immediately.
> 
> > Why do you want to bail out at this point? I think what the case
> > statements should look like is:
> >
> >                    x"iso9660_ce_loop")
> >                        gunzip <"@srcdir@"/tests/ce_loop.iso.gz
> > >"${FSIMAGEP}0.img" ;;
> 
> The hoax ISOs do not contain the files which the further tests in
> grub-fs-tester obviously expect. Like:
> 
>             if [ x$NOHARDLINK != xy ]; then
>                 if run_grubfstest cmp "$GRUBDIR/$BASEHARD"
> "$MNTPOINTRO/$OSDIR/$BASEFILE"  ; then
> 
> So i thought that a single test should be done immediately after
> unpacking the iso.gz and then all other tests should be skipped.
> 
> 
> > You were talking about the test endlessly looping above, since these
> > are native tests we'd put a timeout in the wrapper script that calls
> > grub-fs-tester. That would look like adding the following line to
> > the end of tests/iso9660_test.in:
> > timeout -s KILL "3600" "@builddir@/grub-fs-tester" iso9660_ce_loop
> 
> I was not aware of the timeout command.
> 
> 
> > A better timeout value could probably be selected. It should be as
> > short as possible, but also accounting for the fact that the tests
> > may be run on slower machines or in virtual machines.
> 
> I think 60 seconds of timeout should suffice for 100000 loop cycles
> in C with a function call and repeated reading of the same disk block.
> If this lasts longer than a minute, then we should reduce the limit of
> 100,000 loop hops.
> 
> After applying Lidong Chen's patches i get on a 4 GHz Xeon with nvme
> disk:
> 
>   $ time ./grub-fstest /u/test/ce_loop.iso ls /
> 
>   real    0m0.086s
>   ...
>   $ time ./grub-fstest /u/test/ce_loop2.iso ls /
> 
>   real    0m0.088s
>   ...
> 
> Regrettably there is no error message to see.
> But the fact that grub-fstest neither loops endlessly nor shows a file
> named "x" indicates that our intention is fulfilled by the patches.

Ok, so there should be no output on success then for both ce_loop and
ce_loop2, correct? (for "grub-fstest <iso> ls /" )

Glenn

> 
> 
> > I see some other modifications that I'd like to
> > make to grub-fs-tester, so I could make the changes to add this as
> > well with your guidance.
> 
> I would be happy if you create the new test.
> The only guidance i can offer are the download addresses and the
> SHA256s:
> 
>   http://scdbackup.webframe.org/ce_loop.iso.gz
>   d86b73b0cc260968f50c30a5207b798f5fc2396233aee5eb3cedf9cef069f3c2
> 
>   http://scdbackup.webframe.org/ce_loop2.iso.gz
>   a6bde0c1562de8959d783bca0a79ad750da2bc129bdea2362b4a7c3e83426b2c
> 
> If only one of them shall be tested, then i propose ce_loop2.iso .
> 
> 
> Have a nice day :)
> 
> Thomas
> 

Re: [PATCH v3 0/5] fs/iso9660: Fix out-of-bounds read

[Thomas Schmitt]

Hi,

Glenn Washburn wrote:
> Why does only one suffice? It
> sounds like they test different code paths. Is it possible that there
> is a future code regression such that one iso succeeds and the other
> fails?

They follow different code paths before hunk 4 of patch 5 fixes the
bug that CE and ST at the start of a continuation area are ignored:

@@ -331,6 +340,13 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
            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))

After this change, the first three hunks of patch 5 prevent that the
now common code path is an endless loop.

So a behavioral difference of ce_loop.iso and ce_loop2.iso is to expect
only if above patch hunk #4 gets reverted.


> Ok, so there should be no output on success then for both ce_loop and
>ce_loop2, correct? (for "grub-fstest <iso> ls /" )

Yes. Actually i would have expected an error message to be emitted.
But somehow grub-fstest does not show the text from:

+             return grub_error (GRUB_ERR_BAD_FS,
+                                "suspecting endless CE loop");


Have a nice day :)

Thomas