Hi,

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


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

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


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

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

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


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

The ISO producing program writes this table into the boot image file.
ISOLINUX and GRUB boot images expect it.
Regrettably there is no defined indication for the presence of a boot info
table. 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)

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.

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


Reply via email to