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

["Thomas Schmitt" <scdbackup@gmx.net>]

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