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
>