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

[Glenn Washburn]

On Wed, 25 Jan 2023 21:24:00 +0100
"Thomas Schmitt" <scdbackup@gmx.net> wrote:

> Hi,
> 
> Daniel Kiper wrote:
> > Thomas, it would be nice if you could add the broken ISOs images
> > which you used for tests to the tests in the GRUB. If you do that
> > please CC Glenn.
> 
> Is it wise to have a test which will loop endlessly in case of
> failure ? Is there a way to let a test time out ?

If the test is a non-native one, ie. uses QEMU, there is a timeout, so
it would be fine. For native tests we'd want to write the test to be
stopped and fail after a certain amount of time.

> 
> Whatever:
> 
> After poking in my memory and GRUB's tests directory i came to
> tests/util/grub-fs-tester.in which produces its ISOs as needed.
> It could be appropriate to create one or both CE loop ISOs there.
> But this might become a problem in the future, because the
> post-production hacks depend on correct byte addresses in the ISO
> image.
> 
> 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

These are small, I'm good with adding these images to the repository.

> 
> Next problem is that these images do not go well with the other tests
> in grub-fs-tester.in. 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.
> But i don't yet fully understand what the for-loops around the xorriso
> runs mean:
> 
>   for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
>       ...
>       for BLKSIZE in $blksizes; do
>           ...
>           for NDEVICES in $(range "$MINDEVICES" "$MAXDEVICES" 1); do
>                   ...
>                   x"ziso9660")
>                       FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
>                       xorriso ...
> 
> So how to bail out properly at this point after e.g.
> 
>                   x"iso9660_ce_loop")
>                       gunzip <ce_loop.iso.gz >ce_loop.iso
>                       run_grubfstest ls /
> 
> ?

I'm not sure I completely understand the question. The grub-fs-tester
script runs a series of tests for various filesystems varying the block
size, sector size and number of devices. In this case ISO, that I'm
aware of, is not a multidevice filesystem/block device (unlike LVM, ZFS
or BTRFS). So I believe it would have MAXDEVICES == MINDEVICES == 1, which is 
default. For the *iso9660*, those 3 nested loops above only have one iteration 
each, so you can effectively ignore them.

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

Then need to add "iso9660_ce_loop" to all the tests that it should be in, which 
I'm guessing would be the same ones that ziso9660 is in. Also I'd rather have 
@srcdir@ be $srcdir and have 'srcdir = "@srcdir@"' near the beginning of the 
script.

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

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. Maybe see how long the test takes to 
successfully run on your machine and double that for the timeout value.

> And why do the ls tests in grub-fs-tester.in look like
>    run_grubfstest ls -- -la
> which i cannot decipher by help of the options[]-list in
> grub-fstest.c ?

IIRC, every "command" for grub-fstest, which is the first arg to grub-fstest, 
can interpret its args differently. In the case of the ls command, GRUB's ls 
command is executed natively and provided as an argument list all args after 
the '--'. So the above is like running "ls -la" in grub, but for the 
architecture of the build machine, not the target machine.

> 
> I CC Glenn Washburn already now, in the hope that he can point me to
> examples or states that these ISOs should not become part of the
> tests. (Crossing fingers for the latter case ... ;-)

I'm generally for more tests, especially since you've got some corner cases 
that we've broken on before. 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. Or if the above info is enough for you to get the tests running, 
send a patch to the list and I'll review it.

Glenn