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

[Lidong Chen]

> On Dec 15, 2022, at 10:20 AM, Thomas Schmitt <scdbackup@gmx.net> wrote:
> 
> 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)

My understanding is the entry->len is the size of the System Use Field. If that 
is
correct,  the while-loop condition is to make sure the component records are
within the System Use Field boundary.  “Pos” was initially set to ‘1’ and 
incremented
by the size of each component record.  So, “Pos” is relative to the Component 
Area, 
not the System Use Field of “SL”.  I think the fix should include the 5 bytes:

    While (pos + 5 < 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.

My mistake. I will restore the original code. 
> 
> 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)
Do you mean this within the while-loop?:

    If (pos + 2 == entry->len)
       continue

> 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)

I will add plain ‘5’ and add the comment for it. 
> 
> 
>> +            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.
I will fix it.

> 
> 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.
Please see my response above.

> 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.
> 
Thank you for taking the time to explain and review!  Your explanation and 
comments
 are very helpful, appreciated it. 

Li

> 
> Have a nice day :)
> 
> Thomas

>