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

[Daniel Kiper]

Hi,

Sorry for top posting...

I have just realized colleague of mine is doing some work in the ISO 9660
code including part which we are discussing here. I asked her to post the
patches on the grub-devel. You can expect them soon. Please take a look
at them and comment/review.

Daniel

On Tue, Nov 29, 2022 at 07:47:22PM +0100, Thomas Schmitt wrote:
> 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