search

diskfs / go-diskfs

MIT License
494 stars 112 forks source link

Wrong Boot Information Table for ElTorito BIOS boot image larger than 64k #210

Closed max-gotlib closed 4 months ago

max-gotlib commented 4 months ago

Wrong Boot Information Table is written into the boot sector of ElTorito covered BIOS boot image. According to https://wiki.osdev.org/El-Torito#A_BareBones_Boot_Image_with_Boot_Information_Table (and that's what xorriso does) the "boot file length" field should be provided as 32-bit byte count. But the ElToritoEntry struct has 16-bit 'size' field (which is written at offset 8 of the boot table). As a result, truncated byte count prevents normal boot with BIOS images larger than 64k.

The fix is obvious - change ElToritoEntry::size to 32-bit unsigned int type.

diff --git a/github.com/diskfs/go-diskfs/filesystem/iso9660/eltorito.go b/github.com/diskfs/go-diskfs/filesystem/iso9660/eltorito.go
index a2cb818b..8a89faa4 100644
--- a/vendor/github.com/diskfs/go-diskfs/filesystem/iso9660/eltorito.go
+++ b/vendor/github.com/diskfs/go-diskfs/filesystem/iso9660/eltorito.go
@@ -76,7 +76,7 @@ type ElToritoEntry struct {
    SystemType mbr.Type
    // LoadSize how many blocks of BootFile to load, equivalent to genisoimage option `-boot-load-size`
    LoadSize uint16
-   size     uint16
+   size     uint32
    location uint32
 }

@@ -127,7 +127,7 @@ func (e *ElToritoEntry) headerBytes(last bool, entries uint16) []byte {
 func (e *ElToritoEntry) entryBytes() []byte {
    blocks := e.LoadSize
    if blocks == 0 {
-       blocks = e.size / 512
+       blocks = uint16(e.size / 512)
        if e.size%512 > 1 {
            blocks++
        }
diff --git a/github.com/diskfs/go-diskfs/filesystem/iso9660/finalize.go b/github.com/diskfs/go-diskfs/filesystem/iso9660/finalize.go
index c055cf4d..f4cff559 100644
--- a/vendor/github.com/diskfs/go-diskfs/filesystem/iso9660/finalize.go
+++ b/vendor/github.com/diskfs/go-diskfs/filesystem/iso9660/finalize.go
@@ -504,7 +504,7 @@ func (fs *FileSystem) Finalize(options FinalizeOptions) error {
                }
            }
            // save the child so we can add location late
-           e.size = uint16(child.size)
+           e.size = uint32(child.size)
            child.elToritoEntry = e
        }
    }
deitch commented 4 months ago

Thanks for the bug report @max-gotlib .

I am having a hard time reconciling this with the link you referenced, as well as the full El Torito spec, thankfully available online at mit.edu.

The link you sent says:

The size of the boot file can be found (in bytes) in the bi_BootFileLength field

which aligns with what you wrote. Where do you see that it is 32-bit byte count (i.e. 4 bytes)? I see that the bi_Checksum is explicitly "32 bit checksum", but where does it say 32 bytes for the size?

The part I am finding more confusing is where it says:

A Boot Information Table may be put into a boot image. It starts at offset 8 and is 56 bytes long.

I cannot find a structure in the spec that starts at offset 8 of anything and is 56 bytes long.

Could we be referring to 2 distinct things? The code you referenced is for entries in the boot catalog. These are referenced in section 2.0-2.5 of the spec, specifically figure 5, "Section Entry". The size there is a 2-byte word, which is the number of blocks, not size of file in bytes. You can see that conversion in go-diskfs here.

max-gotlib commented 4 months ago

Could we be referring to 2 distinct things? The code you referenced is for entries in the boot catalog. These are referenced in section 2.0-2.5 of the spec, specifically figure 5, "Section Entry". The size there is a 2-byte word, which is the number of blocks, not size of file in bytes. You can see that conversion in go-diskfs here.

We are definitely referring to different things :)

The size of the boot image is referred in two different places (as you absolutely correctly mentioned): the ElTorito boot catalog entries AND in the Boot Into Table.

The "thing" is - in the Boot Into Table the size goes "directly" (as exact number of bytes), while ElTorito catalogue entry has (as you, again, correctly pointed out) the number of blocks.

During iso9660 filesystem finalization (https://github.com/diskfs/go-diskfs/blob/d67c42d83f8441f972e770521dd17aa732ee168a/filesystem/iso9660/finalize.go#L507) the boot image size ("in bytes") is preserved in the ElToritoEntry::size field.

Obviously, the ElToritoEntry::size field gets truncated (int64 -> uint16) value.

The ElToritoEntry::size field is later used to:

In both cases we get wrong boot image size recorded.

deitch commented 4 months ago

I think I see where you are going. The issue is not the boot entry per se; that is created and handle correctly. It doesn't much care if e.size is uint16, uint32 or uint64, as long as it is big enough to handle whatever size there is. In the end, that gets divided by blocksize and saved in the boot catalog as a boot entry with number of blocks.

The issue is here where we generate the boot table. We reuse the ElToritoEntry.size for this directly here as 4 bytes, just upconverting the 16 bits to 32 bits (essentially getting 2 bytes of 0x00 and 2 of actual data).

If we stored it in ElToritoEntry.size as a uint32 to begin with, then we would not need to upconvert, the data would be correct for the boot table, and we could continue to divide by blocksize to save in the actual Boot Entry.

The failure particular comes when we have a boot image larger than 64K, since 2^16 = 64K.

Is that it? Want to open a PR?

max-gotlib commented 4 months ago

I think I see where you are going.

Yep. The "generation" code is fine, it just uses insufficiently precise "model" (sorry for being too vague in the first place)

Is that it? Want to open a PR?

Yes, you correctly translated my words into user-friendly language :) I'll prepare the PR. Thanks!