Re: [PATCH v3 5/5] fs/iso9660: Prevent skipping CE or ST at start of continuation area

[Thomas Schmitt]

Hi,

On Fri, 20 Jan 2023 19:39:42 +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>
> Tested-by: Lidong Chen <lidong.chen@oracle.com>
> ---
>  grub-core/fs/iso9660.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
> index ca45b3424..3ddb06ed4 100644
> --- a/grub-core/fs/iso9660.c
> +++ b/grub-core/fs/iso9660.c
> @@ -50,6 +50,7 @@ GRUB_MOD_LICENSE ("GPLv3+");
>  #define GRUB_ISO9660_VOLDESC_END     255
>
>  #define GRUB_ISO9660_SUSP_HEADER_SZ  4
> +#define GRUB_ISO9660_MAX_CE_HOPS     100000
>
>  /* The head of a volume descriptor.  */
>  struct grub_iso9660_voldesc
> @@ -270,6 +271,7 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, 
> grub_off_t off,
>    char *sua;
>    struct grub_iso9660_susp_entry *entry;
>    grub_err_t err;
> +  int ce_counter = 0;
>
>    if (sua_size <= 0)
>      return GRUB_ERR_NONE;
> @@ -304,6 +306,13 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, 
> grub_off_t off,
>         struct grub_iso9660_susp_ce *ce;
>         grub_disk_addr_t ce_block;
>
> +       if (++ce_counter > GRUB_ISO9660_MAX_CE_HOPS)
> +         {
> +           grub_free (sua);
> +           return grub_error (GRUB_ERR_BAD_FS,
> +                              "suspecting endless CE loop");
> +         }
> +
>         ce = (struct grub_iso9660_susp_ce *) entry;
>         sua_size = grub_le_to_cpu32 (ce->len);
>         off = grub_le_to_cpu32 (ce->off);
> @@ -331,6 +340,13 @@ 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.
> +        */
> +       if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0
> +           || grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
> +         continue;
>       }
>
>        if (hook (entry, hook_arg))
> --
> 2.35.1

Reviewed-by: Thomas Schmitt <scdbackup@gmx.net>


Have a nice day :)

Thomas