Re: [PATCH 4/4] fs/iso9660: Incorrect check for entry boudary

[Thomas Schmitt]

Hi,

On Wed, 14 Dec 2022 18:55:05 +0000 Lidong Chen <lidong.chen@oracle.com> wrote:
> [PATCH 4/4] fs/iso9660: Incorrect check for entry boudary

s/boudary/boundary/

> An entry consists of the entry info and the component area.

Component area is specific to the RRIP SL entry. So:

s/An entry/An SL entry/


> The entry info should take up 5 bytes instead of sizeof (*entry).
> The area after the first 5 bytes is the component area. The code
> uses the sizeof (*entry) to check the boundary which is incorrect.
> Also, an entry may not have component record. Added a check for
> for the component length before reading the component record.
>
> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
> ---
>  grub-core/fs/iso9660.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
> index 67aa8451c..af432ee82 100644
> --- a/grub-core/fs/iso9660.c
> +++ b/grub-core/fs/iso9660.c
> @@ -662,10 +662,22 @@ susp_iterate_dir (struct grub_iso9660_susp_entry *entry,
>    else if (grub_strncmp ("SL", (char *) entry->sig, 2) == 0)
>      {
>        unsigned int pos = 1;
> +      unsigned int csize;
>
> -      /* The symlink is not stored as a POSIX symlink, translate it.  */
> -      while (pos + sizeof (*entry) < entry->len)
> +      /* The symlink is not stored as a POSIX symlink, translate it. */
> +      while ((pos + GRUB_ISO9660_SUSP_HEADER_SZ) < entry->len)

This loop is iterating over component records, not SUSP entries.
Minimum size of a component record is 2.

So i think the appropriate condition is:

         while (pos + 1 < entry->len)

Component records have no struct representation in GRUB. So no sizeof will
tell the correct value.

C-wise decisive is this line:

                add_part (ctx, (char *) &entry->data[pos + 2],
                          entry->data[pos + 1]);

which lower in this patch changes to

                if (entry->data[pos + 1] > 0)
                  {
                    add_part (ctx, (char *) &entry->data[pos + 2],
                              entry->data[pos + 1]);
                  }

This change will alter the program behavior in respect to empty link
target path components.

The validity of entry->data[pos + 1] is guaranteed by my test proposal.
If entry->data[pos + 1] is 0, then &entry->data[pos + 2] can be an illegal
address, but 0 bytes from that address will be requested to be added.
(A 0-byte will be added by add_part() as separator between link target path
components.)

I am undecided whether add_part() should be skipped if
  (pos + 2 == entry->len)
which indicates an empty path component.
If add_part() shall be performed with length 0, then we should skip in it
the call of
  grub_memcpy (ctx->symlink + size, part, len2);
in case of (len2 == 0).


>       {
> +       /*
> +        * entry->len is GRUB_ISO9660_SUSP_HEADER_SZ plus the
> +        * length of the 'Component Record'. The length of the

With GRUB_ISO9660_SUSP_HEADER_SZ == 4 it will be

  GRUB_ISO9660_SUSP_HEADER_SZ + 1 + length of the Component Area.

The "+ 1" stands for the "FLAGS" byte. (RRIP-1.12, 4.1.3)
It should be 'Component Area' or plural 'Component Records' because
'Component Record' is a single path component.


> +        * record is 2 (pos and pos + 1) plus the actual record
> +        * starting at pos + 2. pos stores the 'Component Flags',
> +        * pos + 1 specifies the length of actual record.
> +        */

This describes a single component record, of which there are as many as
needed to represent the link target path.

entry->data[pos + 1] gives the length of the component, not of the component
record, of which the length is entry->data[pos + 1] + 2.


> +          csize = entry->data[pos + 1] + 2;
> +          if (csize + GRUB_ISO9660_SUSP_HEADER_SZ > entry->len)

With GRUB_ISO9660_SUSP_HEADER_SZ == 4 it will be

             if (csize + GRUB_ISO9660_SUSP_HEADER_SZ + 1 > entry->len)


> +            break;
> +
>         /* The current position is the `Component Flag'.  */
>         switch (entry->data[pos] & 30)
>           {
> @@ -681,8 +693,11 @@ susp_iterate_dir (struct grub_iso9660_susp_entry *entry,
>                     return grub_errno;
>                 }
>
> -             add_part (ctx, (char *) &entry->data[pos + 2],
> -                       entry->data[pos + 1]);
> +             if (entry->data[pos + 1] > 0)
> +               {
> +                 add_part (ctx, (char *) &entry->data[pos + 2],
> +                           entry->data[pos + 1]);
> +               }

As written above, this changes program behavior if a link target path
contains an empty component.


>               ctx->was_continue = (entry->data[pos] & 1);
>               break;
>             }
> --
> 2.35.1
>

So no "Reviewed-by:" yet.

Open issues:

The commit message should mention that it is about SL and not about general
SUSP or RRIP entries.

The while-condition of the component loop should neither use sizeof (*entry)
nor GRUB_ISO9660_SUSP_HEADER_SZ, but plain 1, because that's the minimum
size of an SL component record minus 1.
Probably this should be a separate bug-fix patch to be applied before this
patch [4/4].

Remaining mentioning of GRUB_ISO9660_SUSP_HEADER_SZ needs to become
  GRUB_ISO9660_SUSP_HEADER_SZ + 1
when/if GRUB_ISO9660_SUSP_HEADER_SZ gets defined to 4.

The change of program behavior by ignoring empty link target path components
should be mentioned in the commit message.

-------------------------------------------------------------------------

... whew. All four reviews done.
Probably i made some mistakes in them. Please check all my statements
before basing code changes on them. Inform me if you find such mistakes.

Now i'm looking forward to the next round of review, decisions by Daniel
Kiper, and comments from bystanders.


Have a nice day :)

Thomas