Re: [PATCH v3 0/5] fs/iso9660: Fix out-of-bounds read

[Thomas Schmitt]

Hi,

i wrote:
> > So it would be better to add one or two canned images:
> >   897 bytes of http://scdbackup.webframe.org/ce_loop.iso.gz
> >   904 bytes of http://scdbackup.webframe.org/ce_loop2.iso.gz

Glenn Washburn wrote:
> These are small, I'm good with adding these images to the repository.

After the fixes about CE, one of them would suffice.
ce_loop.iso is the easier hack and thus more probable to be created
by some hoaxer. It does not loop endlessly with GRUB before the fixes.
ce_loop2.iso loops endlessly already with the older GRUB versions.


> > I would want to run
> >   gunzip <ce_loop.iso.gz >ce_loop.iso
> >   run_grubfstest ls /
> > in the neighborhood of the xorriso runs and then bail out immediately.

> Why do you want to bail out at this point? I think what the case
> statements should look like is:
>
>                    x"iso9660_ce_loop")
>                        gunzip <"@srcdir@"/tests/ce_loop.iso.gz 
> >"${FSIMAGEP}0.img" ;;

The hoax ISOs do not contain the files which the further tests in
grub-fs-tester obviously expect. Like:

            if [ x$NOHARDLINK != xy ]; then
                if run_grubfstest cmp "$GRUBDIR/$BASEHARD" 
"$MNTPOINTRO/$OSDIR/$BASEFILE"  ; then

So i thought that a single test should be done immediately after
unpacking the iso.gz and then all other tests should be skipped.


> You were talking about the test endlessly looping above, since these
> are native tests we'd put a timeout in the wrapper script that calls
> grub-fs-tester. That would look like adding the following line to the end of
> tests/iso9660_test.in:
> timeout -s KILL "3600" "@builddir@/grub-fs-tester" iso9660_ce_loop

I was not aware of the timeout command.


> A better timeout value could probably be selected. It should be as short as
> possible, but also accounting for the fact that the tests may be run on
> slower machines or in virtual machines.

I think 60 seconds of timeout should suffice for 100000 loop cycles in C
with a function call and repeated reading of the same disk block.
If this lasts longer than a minute, then we should reduce the limit of
100,000 loop hops.

After applying Lidong Chen's patches i get on a 4 GHz Xeon with nvme disk:

  $ time ./grub-fstest /u/test/ce_loop.iso ls /

  real    0m0.086s
  ...
  $ time ./grub-fstest /u/test/ce_loop2.iso ls /

  real    0m0.088s
  ...

Regrettably there is no error message to see.
But the fact that grub-fstest neither loops endlessly nor shows a file
named "x" indicates that our intention is fulfilled by the patches.


> I see some other modifications that I'd like to
> make to grub-fs-tester, so I could make the changes to add this as well with
> your guidance.

I would be happy if you create the new test.
The only guidance i can offer are the download addresses and the SHA256s:

  http://scdbackup.webframe.org/ce_loop.iso.gz
  d86b73b0cc260968f50c30a5207b798f5fc2396233aee5eb3cedf9cef069f3c2

  http://scdbackup.webframe.org/ce_loop2.iso.gz
  a6bde0c1562de8959d783bca0a79ad750da2bc129bdea2362b4a7c3e83426b2c

If only one of them shall be tested, then i propose ce_loop2.iso .


Have a nice day :)

Thomas