Re: Proposal v2: fs/iso9660: Prevent skipping CE or ST at start of continuation area

[Lidong Chen]

> On Dec 16, 2022, at 4:57 AM, Thomas Schmitt <scdbackup@gmx.net> wrote:
> 
> Hi,
> 
> i realize that my previous proposal opens a possibility for regression with
> a very bad ISO image.
> The danger is in an endless loop by a CE entry which points to itself.
> The bug which i want to see fixed currently prevents this special pitfall.
> 
> (Other endless loops by CE are possible and not prevented. It's much like
> with symbolic link loops. In the end only a hard limit on the number of
> CE hops would help.)
> 
> So i now propose with the same sketch of a commit message, this change
> (compiles but was but not tested):
> ------------------------------------------------------------------------
> 
> fs/iso9660: Prevent skipping CE or ST at start of continuation area
> 
> 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.lidong_chen_patch_2        2022-12-15 
> 11:46:50.654295924 +0100
> +++ grub-core/fs/iso9660.c.lidong_chen_patch_2_ce_fix_v2      2022-12-16 
> 13:54:55.654651173 +0100
> @@ -336,6 +336,21 @@ grub_iso9660_susp_iterate (grub_fshelp_n
>           }
> 
>         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)
> +         {
> +           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 as was done before december 2022. */
> +         }
> +       if (grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
> +         break;
>       }
> 
>       if (hook (entry, hook_arg))
> ————————————————————————————————————
Thank you for providing the diff! I will create a new patch for the changes.

Thanks,
Lidong 
> 
> 
> Have a nice day :)
> 
> Thomas
>