grub-setup Modify the conditionality of the copy of the partition table

ChangeLog line 7

Quoted 6 lines · Mario Limonciello

ChangeLog +6 -0

+        * util/grub-setup.c: Conditionalize the partition map copy on floppy
+          support, not on whether the target contains partitions.
+
+          Otherwise, the BIOS on Dell Latitude E series laptops will freeze 
+          during POST if an invalid partition table is contained in the PBR
+          of the active partition when GRUB is installed to a partition.

It's wrong to assume that no floppies contain partition tables. Also --allow-floppy is usually used for USB sticks which sometimes appear as floppies on some BIOSes and they pretty much contain the partition table. Bottom line is: if you see a partition table: preserve it.

But it's ok to preserve this contents if floppy support is disabled even if no partition table is discovered.

A small logic lecture:

Floppy support is: allow_floppy || grub_util_biosdisk_is_floppy (dest_dev->disk)
Negation of it is either !(allow_floppy || grub_util_biosdisk_is_floppy (dest_dev->disk)) or !allow_floppy && !grub_util_biosdisk_is_floppy (dest_dev->disk), not what you wrote.

Please don't move around unrelated parts.

Quoted 11 lines · Vladimir 'φ-coder/phcoder' Serbinenko

Review

Quoted 6 lines · Mario Limonciello

(quoted diff) +6 -0

+        * util/grub-setup.c: Conditionalize the partition map copy on floppy
+          support, not on whether the target contains partitions.
+
+          Otherwise, the BIOS on Dell Latitude E series laptops will freeze 
+          during POST if an invalid partition table is contained in the PBR
+          of the active partition when GRUB is installed to a partition.

It's wrong to assume that no floppies contain partition tables. Also --allow-floppy is usually used for USB sticks which sometimes appear as floppies on some BIOSes and they pretty much contain the partition table. Bottom line is: if you see a partition table: preserve it.

Reading from floppies requires the floppy_probe function in boot.S, doesn't it, which collides with the partition table? But it makes sense to keep dest_partmap in the condition for safety anyway (and it saves a call to grub_util_biosdisk_is_floppy), so we'd end up with:

    if (dest_partmap ||
        (!allow_floppy && !grub_util_biosdisk_is_floppy (dest_dev->disk)))
      memcpy (boot_img + GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC,
              tmp_img + GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC,
              GRUB_BOOT_MACHINE_PART_END - GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC);

?

Quoted 2 lines

But it's ok to preserve this contents if floppy support is disabled even if no partition table is discovered.

In the case Mario discovered, there was indeed no partition table (all zeroes). Writing the floppy-probing code into that area caused it to fail.

Quoted 4 lines

A small logic lecture:

Floppy support is: allow_floppy || grub_util_biosdisk_is_floppy (dest_dev->disk)
Negation of it is either !(allow_floppy || grub_util_biosdisk_is_floppy (dest_dev->disk)) or !allow_floppy && !grub_util_biosdisk_is_floppy (dest_dev->disk), not what you wrote.

Agreed. The latter form is found elsewhere in grub-setup.

Quoted 1 line

Please don't move around unrelated parts.

The parts Mario moved around weren't unrelated. Since the case at hand is that of installing to a partition boot record, this if block will run:

    if (! dest_partmap)
      {
        grub_util_warn (_("Attempting to install GRUB to a partitionless disk or to a partition.  This is a BAD idea."));
        goto unable_to_embed;
      }

In order to preserve the partition table (or lack thereof) in this case, it was necessary to move the memcpy up above that test.

Cheers,