Should LDM check be less aggressive?

[Andrey Borzenkov <arvidjaar@gmail.com>]

There are multiple reports of GRUB2 refusing to install on disk which
was used for LDM in the past. Example is:
https://bugs.launchpad.net/ubuntu/+source/grub2/+bug/1061255

I tested behavior of Windows and Linux on a disk with valid LDM
signature but manually modified partition table. Both ignore LDM labels
if disk does not contain SFS partition (0x42) and treat is as pure MBR
disk. This differs from GRUB2 behavior which always checks for LDM
label whether SFS partition exists or not.

Would the following be appropriate? It adds additional check for
partition 0x42 before checking for LDM labels.

-andrey

Index: grub-2.00/grub-core/disk/ldm.c
===================================================================
--- grub-2.00.orig/grub-core/disk/ldm.c
+++ grub-2.00/grub-core/disk/ldm.c
@@ -22,6 +22,7 @@
 #include <grub/err.h>
 #include <grub/misc.h>
 #include <grub/diskfilter.h>
+#include <grub/msdos_partition.h>
 #include <grub/gpt_partition.h>
 #include <grub/i18n.h>
 
@@ -103,6 +104,34 @@ read_int (grub_uint8_t *in, grub_size_t
   return ret;
 }
 
+static int
+msdos_has_ldm_partition (grub_disk_t dsk)
+{
+  grub_err_t err;
+  int has_sfs = 0;
+  auto int hook (grub_disk_t disk, const grub_partition_t p);
+  int hook (grub_disk_t disk __attribute__ ((unused)), const grub_partition_t p)
+  {
+    if (p->number >= 4)
+      return 1;
+    if (p->msdostype == GRUB_PC_PARTITION_TYPE_SFS)
+      {
+	has_sfs = 1;
+	return 1;
+      }
+    return 0;
+  }
+
+  err = grub_partition_msdos_iterate (dsk, hook);
+  if (err)
+    {
+      grub_errno = GRUB_ERR_NONE;
+      return 0;
+    }
+
+  return has_sfs;
+}
+
 static const grub_gpt_part_type_t ldm_type = GRUB_GPT_PARTITION_TYPE_LDM;
 
 static grub_disk_addr_t
@@ -756,17 +785,20 @@ grub_ldm_detect (grub_disk_t disk,
 
   {
     int i;
+    int has_sfs = msdos_has_ldm_partition (disk);
     for (i = 0; i < 3; i++)
       {
 	grub_disk_addr_t sector = LDM_LABEL_SECTOR;
 	switch (i)
 	  {
 	  case 0:
+	    if (!has_sfs)
+	      continue;
 	    sector = LDM_LABEL_SECTOR;
 	    break;
 	  case 1:
 	    /* LDM is never inside a partition.  */
-	    if (disk->partition)
+	    if (!has_sfs || disk->partition)
 	      continue;
 	    sector = grub_disk_get_size (disk);
 	    if (sector == GRUB_DISK_SIZE_UNKNOWN)
@@ -867,6 +899,7 @@ int
 grub_util_is_ldm (grub_disk_t disk)
 {
   int i;
+  int has_sfs = msdos_has_ldm_partition (disk);
   for (i = 0; i < 3; i++)
     {
       grub_disk_addr_t sector = LDM_LABEL_SECTOR;
@@ -876,11 +909,13 @@ grub_util_is_ldm (grub_disk_t disk)
       switch (i)
 	{
 	case 0:
+	  if (!has_sfs)
+	    continue;
 	  sector = LDM_LABEL_SECTOR;
 	  break;
 	case 1:
 	  /* LDM is never inside a partition.  */
-	  if (disk->partition)
+	  if (!has_sfs || disk->partition)
 	    continue;
 	  sector = grub_disk_get_size (disk);
 	  if (sector == GRUB_DISK_SIZE_UNKNOWN)
Index: grub-2.00/include/grub/msdos_partition.h
===================================================================
--- grub-2.00.orig/include/grub/msdos_partition.h
+++ grub-2.00/include/grub/msdos_partition.h
@@ -43,6 +43,7 @@
 #define GRUB_PC_PARTITION_TYPE_FAT16_LBA	0xe
 #define GRUB_PC_PARTITION_TYPE_WIN95_EXTENDED	0xf
 #define GRUB_PC_PARTITION_TYPE_PLAN9            0x39
+#define GRUB_PC_PARTITION_TYPE_SFS		0x42
 #define GRUB_PC_PARTITION_TYPE_EZD		0x55
 #define GRUB_PC_PARTITION_TYPE_MINIX		0x80
 #define GRUB_PC_PARTITION_TYPE_LINUX_MINIX	0x81
Index: grub-2.00/Makefile.util.def
===================================================================
--- grub-2.00.orig/Makefile.util.def
+++ grub-2.00/Makefile.util.def
@@ -32,6 +32,7 @@ library = {
   common = grub-core/disk/ldm.c;
   common = grub-core/disk/diskfilter.c;
   common = grub-core/partmap/gpt.c;
+  common = grub-core/partmap/msdos.c;
 };
 
 library = {
@@ -109,7 +110,6 @@ library = {
   common = grub-core/partmap/acorn.c;
   common = grub-core/partmap/amiga.c;
   common = grub-core/partmap/apple.c;
-  common = grub-core/partmap/msdos.c;
   common = grub-core/partmap/sun.c;
   common = grub-core/partmap/plan.c;
   common = grub-core/partmap/dvh.c;