Re: [Libcdio-devel] [PATCH 2/4] Add El Torito virtual boot image support

[Pete Batard]

Hi Thomas,

On 2024.01.24 19:52, Thomas Schmitt via Libcdio-devel wrote:
> > Enables the El Torito boot images to be listed and extracted from a virtual
> > "[BOOT]/" root directory.
>
> In [PATCH 3/4]:
> > +    "  --no-el-torito            Don't use El-Torito extension information\n"
>
> Maybe you should not make it default but enable it only on demand ?
> Then people would not be surprised by files which do not show up in the
> mounted filesystem.

I disagree.

The files (the boot images) are present on the media and IMO, they 
should therefore be made available by default because it makes sense to 
me that, by default, people do want to be able to access every single 
bit of data from the ISO. In other words, it's not because libcdio was 
missing a capability until now that it should be disabled for 
"compatibility" reasons.

Again, people using 7-zip do get these entries, which means that, 
instead, the surprise should be that libcdio-based applications do not 
have them.

> Some nitpicking about [PATCH 2/4]:
>
> > +  \brief ISO-9660 Boot Record Volume Descriptor.
> > + */
> > +struct iso9660_br_s {
> > +    uint8_t          boot_id;                      /**< Boot indicator - 0x88 
> > */
>
> This looks like the Initial/Default Entry of the boot catalog, not
> like a "ISO-9660 Boot Record Volume Descriptor".

Yeah, this is obviously a comment copy/pasted from the previous 
structure that wasn't altered as it should have been.

  s/Boot Record Volume Descriptor/Section (Header) Entry/

Since it is used for both "Section Header Entry" and "Section Entry" per 
"Figure 4 - Section Header Entry" and "Figure 5 - Section Entry" of 
https://pdos.csail.mit.edu/6.828/2014/readings/boot-cdrom.pdf

> (I wonder how you represent the other record types of the boot catalog:
> Validation Entry, Section Header, Section Entry, Section Entry Extension.)

I will refer you to my note:

> Note that because I don't have the scope to cover the full El Torito
> specs, we limit ourselves to listing a maximum of 8 virtual images,
> and only ones that have type Bootable and No Emulation.

In other words I am not going to ever bother with those. The 
cost/benefit of processing these sections is simply not worth it when 
99.9% of usage scenarios will be to access Boot NoEmul images to further 
process them as flat disk images, and that's about it.

I just don't have the time to agonise over the 0.1% scenarios.
But I'll happily let others do... ;)

> > +       iso9660_br_t br[ISO_BLOCKSIZE / sizeof(iso9660_br_t)];
> > +       if (iso9660_iso_seek_read(p_iso, &br, p_brvd->boot_catalog_sector, 1) 
> > == ISO_BLOCKSIZE) {
> > +         for (j = 0, k = 0;
> > +              j < (ISO_BLOCKSIZE / sizeof(iso9660_br_t)) && k < 
> > MAX_BOOT_IMAGES;
> > +              j++) {
> > +           if (br[j].boot_id == 0x88 && br[j].media_type == 0) {
> > +             p_iso->boot_img[k].lsn = br[j].image_lsn;
> > +             p_iso->boot_img[k++].num_sectors = br[j].num_sectors;
> > +           }
> > +         }
>
> This works only by the fact that the El torito specs prescribe the
> values 0x90 and 0x91 for byte 0 of Section Header Entries, value 1 for
> byte 0 of the Validation Entry, value 0x44 for Section Entry Extension,
> and that Section Entry has nearly the same layout as Initial/Default
> Entry. All have 32 bytes of size, to your luck.

Not luck. I read the specs, that specifies 32 bytes for each section and 
0x88 for the first byte of the sections that are of interest to us, and 
then cut to the chase, since I have zero interest to invest another week 
of my time to cover the unlikely 0.1% usage scenario or to ensure that 
the checksum from the Validation Entry matches for instance.

> The loop reads possibly into an undefined remainder of the first catalog
> block and relies on no finding no trailing garbage there.

Yes, as I said, I don't care about the < .1% scenario of people who will 
go out of their way to find (or most likely hand modify) image creation 
software that leaves garbage that can *potentially* (but again, way too 
unlikely) create issues.

> You should at least install a warning sign, that this reading procedure
> only vaguely reflects the specs.

There's a comment that states:

/* Perform basic parsing of boot entries to fill an image table */

"basic" is the key word here.

Because if you want to be comprehensive over the many ways in which the 
proposal will trip, and that I am quite aware of, I could also point to 
a scenario where someone feeds a "./././[BOOT]/..." path...

> In the light of the reading loop, i revisit these previous lines:
> > +  struct {
> > +    uint32_t lsn;          /**< Start LSN of an El-Torito bootable image */
> > +    uint32_t num_sectors;   /**< Number of virtual sectors occupied by the
> > +                               bootable image */
> > +  } boot_img[MAX_BOOT_IMAGES];
>
> Be aware that especially with no-emulation boot images for legacy BIOS
> it is tradition

Gotta love how on one hand I'm kind of being chastised for taking 
shortcuts with the specs, but, when I follow the specs exactly, that 
make absolutely zero mention about a lower limit for num_sectors, I'm 
being told about "tradition".

Just so you know, I hand modified the ISO used for the tests to not have 
num_sectors that was a multiple of 4, precisely to make sure we were 
specs compliant there...

> to set num_sectors to 4 (= 1 CD-ROM sector) and to let
> these first 2048 bytes of code load the rest of the boot image as code.
> The boot info table tells this first code stage where to find itself and
> the rest of the boot program.

Well, if it's tradition, then the mastering application will use 
multiple of 4 for that image, and I don't see what the issue is.

Note that, in case you are being thrown off by the test ISO being used, 
that only has a single bootable image, please be mindful that, on most 
El Torito ISOs I have seen, there are 2 boot images (0 and 1), with, 
from what I gather, 0 being the 2048-byte loader you seem to be alluding 
to, and 1 the main image. The current proposal does handle these 2 
images as expected (i.e. one image per entry), so, on real-life 
El-Torito media, you will find:

- [BOOT]/0-Boot-NoEmul.img (the initial 2048 byte loader)
- [BOOT]/1-Boot-NoEmul.img (the rest of the image)

The current code proposal does properly list and process these 2 
entries, as separate entries, since they are separate entries in the 
boot catalog.

> The ISO producing program writes this table into the boot image file.

As a separate El Torito image, since it's what I gather makes the most 
sense.

> ISOLINUX and GRUB boot images expect it.

And I fail to see how this matters to us. For one thing, as I indicate 
above, all the data you mention should be accessible with this patchset 
(as long as it does follow the El Torito specs) and last time I checked, 
neither SysLinux or GRUB had any intention to switch to using libcdio, 
so I'm not really grasping what you're getting at here. Why do we need 
to care about what ISOLINUX and GRUB boot images expect, when any data 
provided by libcdio will never be used in such a context.

> Regrettably there is no defined indication for the presence of a boot info
> table.

My understanding is that, de facto, it'll be set as an intial 2048-byte 
image in the El Torito boot catalog. But, again, I am really not sure if 
I get what you're really talking about here because, either that data 
*is* included in an El Torito image and specs compliant, in which case 
libcdio will have no trouble extracting with this patchset if you need 
it, or it is *not* included in an El Torito image, and is therefore 
subject to some other ill-defined spec, in which case I don't see how 
this matter for an implementation that is about trying to follow the El 
Torito specs, and only the El Torito specs.

> One could take matching file_lba iand checksum as indication (but
> should be careful not to read too much due to a bogus file_len if it is
> not a boot info table).
>  From libisofs/boot_sectors.txt:
> ============================================================================
> The Boot Info Table begins at byte 8 of the boot image content.
>
> Byte Range | Value      | Meaning
> ---------- | ---------- | ----------------------------------------------------
>     8 -  11 |    pvd_lba | Block address of the Primary Volume Descriptor.
>             |            | This is the session start LBA + 16.
>             |            |
>    12 -  15 |   file_lba | Block address of the start of the boot image file
>             |            | content. Block size is 2048.
>             |            |
>    16 -  19 |   file_len | Number of bytes in boot image file content.
>             |            |
>    20 -  23 |   checksum | The sum of all 32-bit words of the file content
>             |            | from byte 64 to file end.
>             |            |
>    24 -  63 |          0 | Reserved
> ---------- | ---------- | ----------------------------------------------------
> All numbers are stored little-endian.
> ============================================================================
>
>
> > +    if ((_cdio_strnicmp(path, "[BOOT]/", 7) == 0) &&
> > +       (_cdio_stricmp(&path[8], "-Boot-NoEmul.img") == 0)) {
> > +      int index = path[7] - '0';
>
> I think it would be valuable if the file name would reflect the platform
> id from byte 1 of Validation Entry (byte0 == 1) and Section Header Entry
> (byte0 == 0x90 or byte0 == 0x91):
>      0= x86 legacy PC-BIOS (no emulation = plain x86 machine code)
>      1= PowerPC
>      2= Mac
>   0xef= EFI (always no emulation, FAT filesytem image)

I think is is much more valuable to follow suit with what other popular 
applications do, in this case 7-zip, and follow their naming 
conventions, so as *not to confuse end-users*.

7-zip uses "[BOOT]" as the name of the virtual directory and 
"#-Boot-NoEmul.img" for each 0x88/0x00 entry, with no platform 
indication (at least for x86, which is also what we limit the current 
proposal to). So I would insist that it would be very unwise to go "NIH" 
in libcdio, and reinvent the naming scheme, when virtual image listing 
has very much been invented elsewhere.

> Recognizing Section Header Entries would be valuable also because they
> tell the number of following Section Entries in bytes 2 and 3 and they
> tell whether another Section Header Entry will follow (byte0 = 0x90)
> or whether this is the last one (byte0 = 0x91).
> So you won't read into undefined territory.

Unless you can provide a real life scenario where the current proposal 
breaks (i.e. from standard ISO mastering software that somehow writes 
garbage into the boot catalog), I have to dispute the "valuable" above. 
Code is only valuable if it solves a real-life problem and I am very 
doubtful that what you describe can ever be one of these. Which is the 
precise reason why I deliberately skipped any form of boot record 
validation.

Now, as always, I very much appreciate your input and your review (It 
may sound like I am, but I am actually not bothered by your feedback, 
and I agree that at the very least the "Boot Record Volume Descriptor" 
in the comment should be fixed), but I also estimate that I have spent 
way more time than I would have liked on this proposal, especially as my 
initial plan was to just implement what I needed for Rufus (which meant 
no need for str(n)icmp, not bothering with tests or iso-info flags and 
whatnot) and move on, so, I agree that, in the absolute, if you want to 
be 100.0% specs compliant, there's a lot more that needs to be done with 
this proposal.

However, I also have to be very explicit that, I have exactly zero plan 
to tackle what I consider to be super corner case scenarios, as I'd much 
rather do that *if/after* someone comes to us with a real-life example 
of where the current proposal trips for them.

So, outside of the comment (which I am hoping Rocky can fix), I see 
nothing in what's being raised above that strikes me as something that I 
need/want to invest any more time on.

Regards,

/Pete

> The specs prescribe this catalog structure:
> - 1 Validation Entry (byte0 == 1, byte14=0x55 , byte15= 0xAA),
>    tells platform id
> - 1 Initial/Default Entry (byte0 == 0x88 or 0x00), defines first boot image
> - Section Header Entry (byte == 0x90 or 0x91),,
>    tells number N of Section Entries in bytes 2 and 3,
>    tells whether last Section,
>    tells platform id
> - Section Entry 1, (byte0= 0x88  or 0x00),
>    defines second boot image
>    [- Section Entry Extensions of Entry 1, byte0=0x44
>       (i have never seen this in the wild)]
> - ... more Section Entries [and their extensions]
> - Section Entry N [and extensions],
>    defines N+1st boot image
> - Section Header Entry (if previous Section Header Entry byte0 == 0x90)
> - ... Section Entries [and their extensions]
> - ...
>
>
> Have a nice day :)
>
> Thomas