Hi,
(Cc-ing t.feng in the hope that this issue can become part of the code
review.)
While reviewing "[PATCH 7/9]" by t.feng, i wonder whether there is a bug
in grub_iso9660_susp_iterate() in regard to the end of the SUSP data:
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))
{
I think the loop end condition should use 4 rather than 1:
(char *) entry < (char *) sua + sua_size - 4 && entry->len > 0
The entry struct has at least 4 bytes:
struct grub_iso9660_susp_entry
{
grub_uint8_t sig[2];
grub_uint8_t len;
grub_uint8_t version;
grub_uint8_t data[0];
} GRUB_PACKED;
Especially the expression
entry->len > 0
reads beyond the end of the allocated buffer sua if
entry >= sua + sua_size - 3
It does not look as if there are guard rails installed yet:
The size of the allocated space (parameter sua_size) is determined by the
callers of grub_iso9660_susp_iterate from struct grub_iso9660_dir objects,
which obviously represent ISO 9660 directory entry bytes as read from the
filesystem.
---------------------------------------------------------------------------
So the producer of the ISO filesystem can create in GRUB the impression
of trailing bytes which form no valid SUSP entry. This may even happen in
good faith because a ISO 9660 System Use field may hold data which are not
under control of the SUSP protocol.
There is a "ST" entry specified which explcitely ends the range of SUSP.
But the SUSP specs, both 1.10 and 1.12, mention that a System Use field
or a Continuation area may end without ST entry with up to 3 remaining
bytes which shall be ignored. From SUSP 1.12:
If the remaining allocated space following the last recorded System Use
Entry in a System Use field or Continuation Area is less than four bytes
long, it cannot contain a System Use Entry and shall be ignored.
Otherwise an "ST" System Use Entry (see section 5.4) shall be the last
System Use Entry recorded in a System Use field or Continuation Area.
Neither mkisofs/genisominage nor libisofs write any ST entries. At least
for libisofs i can state that there is no trailing non-SUSP data announced
by the size of the directory entry or the Continuation Area.
Have a nice day :)
Thomas
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
