Possible memory fault in fs/iso9660

* Possible memory fault in fs/iso9660
@ 2022-11-19 12:38 Thomas Schmitt
  2022-11-19 12:57 ` Possible memory fault in fs/iso9660 (correction) Thomas Schmitt
  0 siblings, 1 reply; 9+ messages in thread
From: Thomas Schmitt @ 2022-11-19 12:38 UTC (permalink / raw)
  To: grub-devel; +Cc: fengtao40

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

^ permalink raw reply	[flat|nested] 9+ messages in thread