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

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

[Lidong Chen]

This is the v2 patches set which addressed the review comments 
from Thomas Schmitt. Many thanks to Thomas for the review
comments as well as the detailed explanation and test instruction. 

Patch 0005 is a new patch addressing an old bug pointed out
by Thomas. Thanks Thomas for providing the fix.

Thomas also pointed out the issue of the potential endless
loops by CE. Since the sugguested fix requires a bit more 
investigation, and as Thomas pointed out that it should be
handled in a separate patch, the fix is not included in this
this v2 patches set. Because I am not an expert, it would 
be better that someone else can work on it. For the background
info and the comments, please see this email. The bottom half
of the email addressed the endless loop issue:

https://www.mail-archive.com/grub-devel@gnu.org/msg35785.html

For the testing, it passed grub-fstest and make check. The fuzz
test (ran for 2 days) confirmed that the patches fixed the issues. 

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 | 96 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 89 insertions(+), 7 deletions(-)

-- 
2.35.1

[PATCH v2 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

[PATCH v2 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>
---
 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 v2 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 v2 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 v2 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
---
 grub-core/fs/iso9660.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
index ca45b3424..c3ed11f04 100644
--- a/grub-core/fs/iso9660.c
+++ b/grub-core/fs/iso9660.c
@@ -331,6 +331,18 @@ 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.
+	   */
+	  ce = (struct grub_iso9660_susp_ce *) entry;
+	  if (ce_block != grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ
+              || off != grub_le_to_cpu32 (ce->off))
+	    continue;
+	  /*
+	   * Ending up here indicates an endless loop by self reference.
+	   * So skip this bad CE.
+	   */
 	}
 
       if (hook (entry, hook_arg))
-- 
2.35.1

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

[Thomas Schmitt]

Hi,

On Wed, 18 Jan 2023 08:23:54 +0000 Lidong Chen <lidong.chen@oracle.com> 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);
> +		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

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

(I'm not sure whether is appropriate to add another Reviewed-by after it
was already given and only a minor cosmetic change was made to the patch.
If this is not ok, then please give me a note.)


Have a nice day :)

Thomas

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

[Thomas Schmitt]

Hi,

On Wed, 18 Jan 2023 08:23:55 +0000 Lidong Chen <lidong.chen@oracle.com> wrote:
> 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>
> ---
>  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

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


Have a nice day :)

Thomas

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

[Thomas Schmitt]

Hi,

On Wed, 18 Jan 2023 08:23:56 +0000 Lidong Chen <lidong.chen@oracle.com> wrote:
> 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

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

My minor objections towards v1 are now addressed.


Have a nice day :)

Thomas

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

[Thomas Schmitt]

Hi,

On  Wed, 18 Jan 2023 08:23:57 +0000 Lidong Chen <lidong.chen@oracle.com> wrote:
> 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

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

Most of my initial objections towards patch 4 were wrong. What remained
is taken into respect now.


Have a nice day :)

Thomas

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

[Thomas Schmitt]

Hi,

On Wed, 18 Jan 2023 08:23:58 +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
> ---
>  grub-core/fs/iso9660.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
> index ca45b3424..c3ed11f04 100644
> --- a/grub-core/fs/iso9660.c
> +++ b/grub-core/fs/iso9660.c
> @@ -331,6 +331,18 @@ 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.
> +	   */
> +	  ce = (struct grub_iso9660_susp_ce *) entry;
> +	  if (ce_block != grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ
> +              || off != grub_le_to_cpu32 (ce->off))
> +	    continue;
> +	  /*
> +	   * Ending up here indicates an endless loop by self reference.
> +	   * So skip this bad CE.
> +	   */
>  	}
>
>        if (hook (entry, hook_arg))
> --
> 2.35.1

This looks like the part of my retracted v2 proposal which was intended to
break the possible endless loop by self-referring CE entries:
  Date: Fri, 16 Dec 2022 13:57:04 +0100
  Message-Id: <31992389627932343306@scdbackup.webframe.org>
  Subject: Proposal v2: fs/iso9660: Prevent skipping CE or ST at start of
           continuation area

But the problem of CE and ST at the start of a continuation area is not
fixed by the above.
What remains after postponing the loop breaker is (hopefully) addressed by
my proposal v1:
  Date: Fri, 16 Dec 2022 10:42:13 +0100
  Message-Id: <19021389617225107434@scdbackup.webframe.org>
  Subject: Proposal: fs/iso9660: Prevent skipping CE or ST at start of
           continuation area

--- grub-core/fs/iso9660.c.lidong_chen_patch_2  2022-12-15 11:46:50.654295924 +0
100
+++ grub-core/fs/iso9660.c.lidong_chen_patch_2_ce_fix   2022-12-16 10:05:52.9905
99283 +0100
@@ -336,6 +336,12 @@ grub_iso9660_susp_iterate (grub_fshelp_n
            }

          entry = (struct grub_iso9660_susp_entry *) sua;
+
+         if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0
+             || grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
+             /* The hook function will not process CE or ST.
+                Advancing to the next entry would skip them. */
+             continue;
        }

       if (hook (entry, hook_arg))
---------------------------------------------------------------------------


Bystanders please note that the correctness of my "continue" depends
on Lidong Chen's patch 2, which changes the "for"-loop to a "while"-loop
with incrementation at the loop's end:

-  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)
     {

... loop content ...

+      entry = (struct grub_iso9660_susp_entry *) ((char *) entry + entry->len);
+
+      if (((sua + sua_size) - (char *) entry) < GRUB_ISO9660_SUSP_HEADER_SZ)
+        break;
     }

In current grub-core/fs/iso9660.c we would have to goto the start of
the loop content, circumventing the incrementation step of "for".


Have a nice day :)

Thomas

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

[Thomas Schmitt]

Hi,

On Wed, 18 Jan 2023 08:23:53 +0000 Lidong Chen <lidong.chen@oracle.com> wrote:
> Thomas also pointed out the issue of the potential endless
> loops by CE. Since the sugguested fix requires a bit more
> investigation, and as Thomas pointed out that it should be
> handled in a separate patch, the fix is not included in this
> this v2 patches set. Because I am not an expert, it would
> be better that someone else can work on it. For the background
> info and the comments, please see this email. The bottom half
> of the email addressed the endless loop issue:
>
> https://www.mail-archive.com/grub-devel@gnu.org/msg35785.html

I had a look at Linux fs/isofs/rock.c about the handling of CE loops.

It works with a hop counter like my current untested proposal for GRUB,
which i made in above mail.
But Linux is very restrictive:

  /* Maximum number of Rock Ridge continuation entries */
  #define RR_MAX_CE_ENTRIES 32
  ...
                ret = -EIO;
                if (++rs->cont_loops >= RR_MAX_CE_ENTRIES)
                        goto out;

So probably my proposed limit of a million is just oversized. But i think
that 32 would be too low.

The Linux limit is bad news for people who want to put larger data blobs
into SUSP controlled blocks, because the size of a continuation area is
also restricted by Linux to a single block of 2048 bytes.
The EIO in case of too much CE entries does not show up in the system log
or on the terminal. Just the file is not listed and not accessible:

  $ wc /mnt/iso/x
  wc: /mnt/iso/x: No such file or directory

I understand from source code and experiments that the actual maximum
number of CE hops in Linux is 31 rather than 32.

libisofs and xorriso are in the process of getting an adjustable curb to
prevent the production of ISO filesystems with files which would not show
up in Linux. I decided for 100,000 hops as hard limit but set the default
to 31.
(SUSP prescribes that entries of unknown type be "ignored and skipped",
which Linux normally does fine. Here it could do better.)


Have a nice day :)

Thomas

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

[Lidong Chen]

Hi,

> On Jan 18, 2023, at 8:31 AM, Thomas Schmitt <scdbackup@gmx.net> wrote:
> 
> Hi,
> 
> On Wed, 18 Jan 2023 08:23:53 +0000 Lidong Chen <lidong.chen@oracle.com> wrote:
>> Thomas also pointed out the issue of the potential endless
>> loops by CE. Since the sugguested fix requires a bit more
>> investigation, and as Thomas pointed out that it should be
>> handled in a separate patch, the fix is not included in this
>> this v2 patches set. Because I am not an expert, it would
>> be better that someone else can work on it. For the background
>> info and the comments, please see this email. The bottom half
>> of the email addressed the endless loop issue:
>> 
>> https://urldefense.com/v3/__https://www.mail-archive.com/grub-devel@gnu.org/msg35785.html__;!!ACWV5N9M2RV99hQ!PnDkQnGTDLOfxl9IryXHlAoUSWaeNHsXVtkVb_R8FghhG18JFoFQGCllHBnL9fpX3MJUFMN2vjoj6pcyPqE$ 
> 
> I had a look at Linux fs/isofs/rock.c about the handling of CE loops.
> 
> It works with a hop counter like my current untested proposal for GRUB,
> which i made in above mail.
> But Linux is very restrictive:
> 
>  /* Maximum number of Rock Ridge continuation entries */
>  #define RR_MAX_CE_ENTRIES 32
>  ...
>                ret = -EIO;
>                if (++rs->cont_loops >= RR_MAX_CE_ENTRIES)
>                        goto out;
> 
> So probably my proposed limit of a million is just oversized. But i think
> that 32 would be too low.
> 
> The Linux limit is bad news for people who want to put larger data blobs
> into SUSP controlled blocks, because the size of a continuation area is
> also restricted by Linux to a single block of 2048 bytes.
> The EIO in case of too much CE entries does not show up in the system log
> or on the terminal. Just the file is not listed and not accessible:
> 
>  $ wc /mnt/iso/x
>  wc: /mnt/iso/x: No such file or directory
> 
> I understand from source code and experiments that the actual maximum
> number of CE hops in Linux is 31 rather than 32.
> 
> libisofs and xorriso are in the process of getting an adjustable curb to
> prevent the production of ISO filesystems with files which would not show
> up in Linux. I decided for 100,000 hops as hard limit but set the default
> to 31.
I am not sure I understand the hard limit vs the default in terms of checking the number of hops.
Will checking against the hard limit be enough?  Since the limit of CE hops has been decided,
if the previous proposed fix still stands, I can create a patch for it. The tough part for me is the
testing. 

Regards,
Lidong
 

> (SUSP prescribes that entries of unknown type be "ignored and skipped",
> which Linux normally does fine. Here it could do better.)
> 
> 
> Have a nice day :)
> 
> Thomas
> 

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

[Lidong Chen]

> On Jan 18, 2023, at 8:21 AM, Thomas Schmitt <scdbackup@gmx.net> wrote:
> 
> Hi,
> 
> On Wed, 18 Jan 2023 08:23:58 +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
>> ---
>> grub-core/fs/iso9660.c | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>> 
>> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
>> index ca45b3424..c3ed11f04 100644
>> --- a/grub-core/fs/iso9660.c
>> +++ b/grub-core/fs/iso9660.c
>> @@ -331,6 +331,18 @@ 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.
>> +	   */
>> +	  ce = (struct grub_iso9660_susp_ce *) entry;
>> +	  if (ce_block != grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ
>> +              || off != grub_le_to_cpu32 (ce->off))
>> +	    continue;
>> +	  /*
>> +	   * Ending up here indicates an endless loop by self reference.
>> +	   * So skip this bad CE.
>> +	   */
>> 	}
>> 
>>       if (hook (entry, hook_arg))
>> --
>> 2.35.1
> 
> This looks like the part of my retracted v2 proposal which was intended to
> break the possible endless loop by self-referring CE entries:
>  Date: Fri, 16 Dec 2022 13:57:04 +0100
>  Message-Id: <31992389627932343306@scdbackup.webframe.org>
>  Subject: Proposal v2: fs/iso9660: Prevent skipping CE or ST at start of
>           continuation area
> 
> But the problem of CE and ST at the start of a continuation area is not
> fixed by the above.
> What remains after postponing the loop breaker is (hopefully) addressed by
> my proposal v1:
>  Date: Fri, 16 Dec 2022 10:42:13 +0100
>  Message-Id: <19021389617225107434@scdbackup.webframe.org>
>  Subject: Proposal: fs/iso9660: Prevent skipping CE or ST at start of
>           continuation area
> 
> --- grub-core/fs/iso9660.c.lidong_chen_patch_2  2022-12-15 11:46:50.654295924 +0
> 100
> +++ grub-core/fs/iso9660.c.lidong_chen_patch_2_ce_fix   2022-12-16 10:05:52.9905
> 99283 +0100
> @@ -336,6 +336,12 @@ grub_iso9660_susp_iterate (grub_fshelp_n
>            }
> 
>          entry = (struct grub_iso9660_susp_entry *) sua;
> +
> +         if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0
> +             || grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
> +             /* The hook function will not process CE or ST.
> +                Advancing to the next entry would skip them. */
> +             continue;
>        }
> 
>       if (hook (entry, hook_arg))
> —————————————————————————————————————
Sorry about that. You are right, I will fix it.

Thanks,
Lidong

> 
> 
> Bystanders please note that the correctness of my "continue" depends
> on Lidong Chen's patch 2, which changes the "for"-loop to a "while"-loop
> with incrementation at the loop's end:
> 
> -  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)
>     {
> 
> ... loop content ...
> 
> +      entry = (struct grub_iso9660_susp_entry *) ((char *) entry + entry->len);
> +
> +      if (((sua + sua_size) - (char *) entry) < GRUB_ISO9660_SUSP_HEADER_SZ)
> +        break;
>     }
> 
> In current grub-core/fs/iso9660.c we would have to goto the start of
> the loop content, circumventing the incrementation step of "for".
> 
> 
> Have a nice day :)
> 
> Thomas
> 

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

[Lidong Chen]

> On Jan 18, 2023, at 8:07 AM, Thomas Schmitt <scdbackup@gmx.net> wrote:
> 
> Hi,
> 
> On Wed, 18 Jan 2023 08:23:54 +0000 Lidong Chen <lidong.chen@oracle.com> 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);
>> +		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
> 
> Reviewed-by: Thomas Schmitt <scdbackup@gmx.net>
> 
> (I'm not sure whether is appropriate to add another Reviewed-by after it
> was already given and only a minor cosmetic change was made to the patch.
> If this is not ok, then please give me a note.)
> 
> 
To me, having another ‘Reviewed-by’ is a confirmation that the v2 change is accepted. 

Thanks,
Lidong

> Have a nice day :)
> 
> Thomas
> 

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

[Thomas Schmitt]

Hi,

i wrote:
> > libisofs and xorriso are in the process of getting an adjustable curb to
> > prevent the production of ISO filesystems with files which would not show
> > up in Linux. I decided for 100,000 hops as hard limit but set the default
> > to 31.

Lidong Chen wrote:
> I am not sure I understand the hard limit vs the default in terms of
> checking the number of hops.

xorriso produces and reads ISO 9660 filesystems.

At production time, the limit will be adjustable. The default will be 31
to prevent files which don't show up when the filesystem is mounted by
Linux. The maximum adjustable limit at production time will be 100,000.

At read time, e.g. for extracting files or for adding another session to
the ISO, the limit will be 100,000.


> Since the limit of CE hops has been decided,

I meanwhile deem the proposed 1 million allowed hops quite high.
An opinion by experienced GRUB developers would be helpful.


> if the previous proposed fix still stands, I can create a patch for it.

The fix by hop counting is the right thing to do.


> The tough part for me is the testing.

You can use for testing
  http://scdbackup.webframe.org/ce_loop.iso.gz
  SHA256: d86b73b0cc260968f50c30a5207b798f5fc2396233aee5eb3cedf9cef069f3c2
and
  http://scdbackup.webframe.org/ce_loop2.iso.gz
  SHA256: a6bde0c1562de8959d783bca0a79ad750da2bc129bdea2362b4a7c3e83426b2c

ce_loop.iso currently causes no endless loop in grub-fstest, because
the CE entry at the start of the (bad) continuation area is ignored,
against the prescriptions of SUSP.
It will cause an endless loop after patch 5/5 is applied and the
self-pointing CE entry is not ignored any more by mistake.
  ./grub-fstest ce_loop.iso ls /

ce_loop2.iso already now causes an endless loop with
  ./grub-fstest ce_loop2.iso ls /

Both endless loops should be detected and cause a GRUB error when the
CE hop counter and loop breaker is in effect.


(I can meanwhile provide ISOs which have 32+ CE hops without loop, i.e.
righteously storing 64+ KiB of data in the chain of SUSP entries of a
file. But that's mainly interesting for testing Linux, not for GRUB.)


Have a nice day :)

Thomas

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

[Lidong Chen]

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



On Jan 19, 2023, at 3:58 AM, Thomas Schmitt <scdbackup@gmx.net<mailto:scdbackup@gmx.net>> wrote:

Hi,

i wrote:
libisofs and xorriso are in the process of getting an adjustable curb to
prevent the production of ISO filesystems with files which would not show
up in Linux. I decided for 100,000 hops as hard limit but set the default
to 31.

Lidong Chen wrote:
I am not sure I understand the hard limit vs the default in terms of
checking the number of hops.

xorriso produces and reads ISO 9660 filesystems.

At production time, the limit will be adjustable. The default will be 31
to prevent files which don't show up when the filesystem is mounted by
Linux. The maximum adjustable limit at production time will be 100,000.

At read time, e.g. for extracting files or for adding another session to
the ISO, the limit will be 100,000.


Since the limit of CE hops has been decided,

I meanwhile deem the proposed 1 million allowed hops quite high.
An opinion by experienced GRUB developers would be helpful.


if the previous proposed fix still stands, I can create a patch for it.

The fix by hop counting is the right thing to do.


The tough part for me is the testing.

You can use for testing
 https://urldefense.com/v3/__http://scdbackup.webframe.org/ce_loop.iso.gz__;!!ACWV5N9M2RV99hQ!OXku0fFl7fyFeTon9H7cdACNDHk8OrsvozKrR5iTCzbjwS90Ys6gZI8kpfDlpAgRKh2q3YKxK7ROWT5K7cg$
 SHA256: d86b73b0cc260968f50c30a5207b798f5fc2396233aee5eb3cedf9cef069f3c2
and
 https://urldefense.com/v3/__http://scdbackup.webframe.org/ce_loop2.iso.gz__;!!ACWV5N9M2RV99hQ!OXku0fFl7fyFeTon9H7cdACNDHk8OrsvozKrR5iTCzbjwS90Ys6gZI8kpfDlpAgRKh2q3YKxK7RO7e3E4Lw$
 SHA256: a6bde0c1562de8959d783bca0a79ad750da2bc129bdea2362b4a7c3e83426b2c

ce_loop.iso currently causes no endless loop in grub-fstest, because
the CE entry at the start of the (bad) continuation area is ignored,
against the prescriptions of SUSP.
It will cause an endless loop after patch 5/5 is applied and the
self-pointing CE entry is not ignored any more by mistake.
 ./grub-fstest ce_loop.iso ls /

ce_loop2.iso already now causes an endless loop with
 ./grub-fstest ce_loop2.iso ls /

Both endless loops should be detected and cause a GRUB error when the
CE hop counter and loop breaker is in effect.


I ran grub-fastest with both ce_loop ISO files. The endless loops were detected
and Grub exited accordingly. I didn't know where the grub error message
were stored in case of grub-fastest. But, I traced with gdb, and saw the
code reported the error. If the diff looks good, I will send the v3 patches set.

+#define GRUB_ISO9660_MAX_CE_HOPS       100000



   struct grub_iso9660_susp_entry *entry;
   grub_err_t err;
+  int ce_counter = 0;



          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;

          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))

Thanks,
Lidong



(I can meanwhile provide ISOs which have 32+ CE hops without loop, i.e.
righteously storing 64+ KiB of data in the chain of SUSP entries of a
file. But that's mainly interesting for testing Linux, not for GRUB.)


Have a nice day :)

Thomas



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

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

[Thomas Schmitt]

Hi,

Lidong Chen wrote:
> I ran grub-fastest with both ce_loop ISO files. The endless loops were
> detected and Grub exited accordingly.

Good.


> I didn't know where the grub error message
> were stored in case of grub-fastest.

So you don't see an error message ?

I had the same problem a while ago, when i tried to check that my thoughts
about the loop end condition in grub_iso9660_susp_iterate() are correct.
(This is now covered by your patch 2.)


> But, I traced with gdb, and saw the  code reported the error.

It's on my todo list to learn how to prepare grub-fstest for working with
gdb. Currently gdb says "No debugging symbols found in ./grub-fstest".


> If the diff looks good, I will send the v3 patches set.

I have no objections.

If patches 1 to 4 are included in v3, please tell whether they have changed
towards v2. (I see no reason why they should change. But if they do, i'll
have to compare them with the earlier versions.)

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

I still riddle about how the error message can become visible to the user.
I don't get ideas for that from
  https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Error-Handling

I wonder what is supposed to happen to the "textual message" component of
a grub_error() call. Under which conditions will it be displayed ?
And where ?


Have a nice day :)

Thomas

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

[Lidong Chen]

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



On Jan 20, 2023, at 3:49 AM, Thomas Schmitt <scdbackup@gmx.net<mailto:scdbackup@gmx.net>> wrote:

Hi,

Lidong Chen wrote:
I ran grub-fastest with both ce_loop ISO files. The endless loops were
detected and Grub exited accordingly.

Good.


I didn't know where the grub error message
were stored in case of grub-fastest.

So you don't see an error message ?

No, I don’t see the error message. In this test case, it looks like the calling function
commands/ls.c::grub_ls_list_files() only cares about “GRUB_ERR_BAD_FILE_TYPE”.
So, “GRUB_ERR_BAD_FS” is ignored, thus, no error message.
In general, I looks like grub-fstest.c doesn’t handle grub_errno.



I had the same problem a while ago, when i tried to check that my thoughts
about the loop end condition in grub_iso9660_susp_iterate() are correct.
(This is now covered by your patch 2.)


But, I traced with gdb, and saw the  code reported the error.

It's on my todo list to learn how to prepare grub-fstest for working with
gdb. Currently gdb says "No debugging symbols found in ./grub-fstest”.

I ran make with "-g -O0”,  make -j $(nproc) CFLAGS="-g -O0”



If the diff looks good, I will send the v3 patches set.

I have no objections.

If patches 1 to 4 are included in v3, please tell whether they have changed
towards v2. (I see no reason why they should change. But if they do, i'll
have to compare them with the earlier versions.)

Sure, I will do that. There is no changes to patch 1 to 4, I will mention it in the cover letter.


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

I still riddle about how the error message can become visible to the user.
I don't get ideas for that from
 https://urldefense.com/v3/__https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html*Error-Handling__;Iw!!ACWV5N9M2RV99hQ!JjeG7gWQlfqBsZdUXHliLSgI3HTR5KbRjtS65N8n4eGAomjdhltzK-vqyt7EMz-aEBuFvZWpGZEDQ9PYbtQ$

I read this "If exception is not handled before prompt is displayed, error message will be shown to user.”
Does it imply that the error message is only visible when Grub is running?

Thanks,
Lidong



I wonder what is supposed to happen to the "textual message" component of
a grub_error() call. Under which conditions will it be displayed ?
And where ?


Have a nice day :)

Thomas



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