Re: [PATCH v2 2/5] fs/iso9660: Prevent read past the end of system use area

[Thomas Schmitt]

Hi,

On Wed, 18 Jan 2023 08:23:55 +0000 Lidong Chen <lidong.chen@oracle.com> wrote:
> In the code, the for loop advanced the entry pointer to the
> next entry before checking if the next entry is within the
> system use area boundary. Another issue in the code was that
> there is no check for the size of system use area. For a
> corrupted system, the size of system use area can be less than
> the size of minimum SUSP entry size (4 bytes). These can cause
> buffer overrun. The fixes added the checks to ensure the read is
> valid and within the boundary.
>
> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
> ---
>  grub-core/fs/iso9660.c | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
> index 4f4cd6165..65c8862b6 100644
> --- a/grub-core/fs/iso9660.c
> +++ b/grub-core/fs/iso9660.c
> @@ -49,6 +49,8 @@ GRUB_MOD_LICENSE ("GPLv3+");
>  #define GRUB_ISO9660_VOLDESC_PART    3
>  #define GRUB_ISO9660_VOLDESC_END     255
>
> +#define GRUB_ISO9660_SUSP_HEADER_SZ  4
> +
>  /* The head of a volume descriptor.  */
>  struct grub_iso9660_voldesc
>  {
> @@ -272,6 +274,9 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, 
> grub_off_t off,
>    if (sua_size <= 0)
>      return GRUB_ERR_NONE;
>
> +  if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
> +    return grub_error (GRUB_ERR_BAD_FS, "invalid susp entry size");
> +
>    sua = grub_malloc (sua_size);
>    if (!sua)
>      return grub_errno;
> @@ -281,10 +286,14 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, 
> grub_off_t off,
>    if (err)
>      return err;
>
> -  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)
>      {
> +      /* Ensure the entry is within System Use Area */
> +      if ((char *) entry + entry->len > (sua + sua_size))
> +        break;
> +
>        /* The last entry.  */
>        if (grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
>       break;
> @@ -300,6 +309,16 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, 
> grub_off_t off,
>         off = grub_le_to_cpu32 (ce->off);
>         ce_block = grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ;
>
> +       if (sua_size <= 0)
> +         break;
> +
> +       if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
> +         {
> +           grub_free (sua);
> +           return grub_error (GRUB_ERR_BAD_FS,
> +                              "invalid continuation area in CE entry");
> +         }
> +
>         grub_free (sua);
>         sua = grub_malloc (sua_size);
>         if (!sua)
> @@ -319,6 +338,11 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, 
> grub_off_t off,
>         grub_free (sua);
>         return 0;
>       }
> +
> +      entry = (struct grub_iso9660_susp_entry *) ((char *) entry + 
> entry->len);
> +
> +      if (((sua + sua_size) - (char *) entry) < GRUB_ISO9660_SUSP_HEADER_SZ)
> +        break;
>      }
>
>    grub_free (sua);
> --
> 2.35.1

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


Have a nice day :)

Thomas