Hi,
On Wed, 18 Jan 2023 08:23:58 +0000 Lidong Chen <lidong.c...@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 scdbac...@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: <31992389627932343...@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: <19021389617225107...@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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
