> On Jan 18, 2023, at 8:21 AM, Thomas Schmitt <scdbac...@gmx.net> wrote:
>
> 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))
> —————————————————————————————————————
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
>
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
