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

[Glenn Washburn]

On Fri, 27 Jan 2023 11:56:50 +0100
"Thomas Schmitt" <scdbackup@gmx.net> wrote:

> 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'm not sure I completely understand. Why does only one suffice? It
sounds like they test different code paths. Is it possible that there
is a future code regression such that one iso succeeds and the other
fails? Doing both isn't a problem for me.

> 
> 
> > > 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.

Ok, so there should be no output on success then for both ce_loop and
ce_loop2, correct? (for "grub-fstest <iso> ls /" )

Glenn

> 
> 
> > 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
>