Re: Possible memory fault in fs/iso9660 (correction)

[Thomas Schmitt]

Hi,

Fengtao wrote:
> I think:
>         (char *) entry < (char *) sua + sua_size - 3 && entry->len > 0
> is ok, or:
>         (char *) entry <= (char *) sua + sua_size - 4 && entry->len > 0

"4" would be overdone. There are SUSP and RRIP entries of length 4,
which would be ignored if appearing at the end of the SUSP controlled area.


> I am also not familiar with ISO format.

I seem to be the last active programmer on that topic.

As stated on 24 Nov, i see other potential memory faults in the code of
grub-core/fs/iso9660.c if the ISO image contains incorrect SUSP entries.

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

I began to hack an ISO image so that it shows non-SUSP data after the
SUSP data. (See below.)

But now i am having noob problems with grub-fstest.

I pulled the source from git://git.savannah.gnu.org/grub on Debian 11 and
built it as prescribed in INSTALL.
Nevertheless grub-fstest does not show a memory fault with:

  valgrind ./grub-fstest /dvdbuffer/grub_test_non_susp.iso ls /

gdb says that the execution enters grub_iso9660_susp_iterate()
  Breakpoint 1, 0x000055555557dde4 in grub_iso9660_susp_iterate ()
but gives no further information, because
  (No debugging symbols found in ./grub-fstest)

Next i tried to insert visible messages into grub_iso9660_susp_iterate():
  grub_error (GRUB_ERR_BAD_NUMBER, "grub_iso9660_susp_iterate: called");
  ...
  grub_error (GRUB_ERR_BAD_NUMBER, "grub_iso9660_susp_iterate: before loop");
  ...
    grub_error (GRUB_ERR_BAD_NUMBER,
                "grub_iso9660_susp_iterate: sua + sua_size - entry = %ld",
                (long int) ((char *) sua + sua_size - (char *) entry));
I do not see any of them when running with above arguments.

So how can i make grub-core/fs/iso9660.c debuggable or let it emit visible
messages ?

---------------------------------------------------------------------------
The test ISO is made by these commands in bash:

  # Create an ISO with a playground of SUSP data.
  echo dummy >dummy_file
  test -e grub_test_non_susp.iso && rm grub_test_non_susp.iso
  xorriso \
     -xattr on \
     -outdev grub_test_non_susp.iso \
     -map dummy_file /dummy_file \
     -setfattr user.x y /dummy_file -- \
     -padding 0

  # Search for the AL entry of length 0x0c which holds attribute user.x.
  # (AL is the entry type of my invention AAIP. It gets ignored by all
  #  readers except xorriso. So it is a good playground for manipulations.)
  # (There is also another AL entry of size 0x10 in the CE area of the root
  #  directory.)
  al=$(grep -a -o -b 'AL'$'\x0c'$'\x01' grub_test_non_susp.iso | \
       sed -e 's/:/ /' | awk '{print $1}')

  # Replace length field value 0x0c by 0x0a.
  # This leaves the last 2 bytes of the AL entry as trailing non-SUSP data
  # in the System Use field of the directory entry.
  al_length_offset=$(expr $al + 2)
  echo $'\x0a' | \
    dd of=grub_test_non_susp.iso \
       bs=1 count=1 seek="$al_length_offset" conv=notrunc

Inspection by a hex dumper shows that the AL entry indeed announces the
desired (short) length of 10.

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

I also learned by doing and then by reading specs that a padding byte after
the System Use data is present if needed to get an even number of bytes
as size of the directory record.

This could explain the existing "- 1" in GRUB's code.

Above ISO is supposed to show 3 non-SUSP bytes at the end of the System Use
field: 2 from my dd manipulation, 1 as regular padding.


Have a nice day :)

Thomas