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

[Thomas Schmitt]

Hi,

in summary: Your objections to my objections are correct.


On Wed, 14 Dec 2022 18:55:05 +0000 Lidong Chen <lidong.chen@oracle.com> wrote:
> >> +      /* The symlink is not stored as a POSIX symlink, translate it. */
> >> +      while ((pos + GRUB_ISO9660_SUSP_HEADER_SZ) < entry->len)

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

On Mon, 19 Dec 2022 21:00:23 +0000 Lidong Chen <lidong.chen@oracle.com> wrote:
> My understanding is the entry->len is the size of the System Use Field.
> 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)

You are right.

My "+ 1" would be correct if entry->len was the size of the SL entry
payload (FLAGS byte and COMPONENT AREA). But further "+ 4" are needed
because of the SUSP header size of the SL entry.

(One could use "+ GRUB_ISO9660_SUSP_HEADER_SZ" instead of "+ 4" in this
 case. But at other places you followed my proposal to refer with plain
 numbers to the description in the RRIP specs. So in the sum "+ 5" is
 good.)


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

I meant your change of program behavior:

-               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]);
+                 }

I obviously made some inappropriate thought jumps when writing
  (pos + 2 == entry->len)
Apologies for being confusing.

(It's not only about the end of the SL entry but about the size of any
component record, regardless of its position in the entry's payload.
Aggravating is that the condition i mentioned is wrong by the missing
"+ 4" for the SUSP header, even if it was about the SL entry's end.)

Whatever, you already decided not to change program behavior by
skipping over empty text components.


> > Open issues:
> > [...]
> > 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].

So i now retract this issue.

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

This issue is now obsolete.


Have a nice day :)

Thomas