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