Two Small Patches (x86 VolId & Sun Label Checking)

Two Small Patches (x86 VolId & Sun Label Checking)

[ehem+grub]

[-- Attachment #1: Type: text/plain, Size: 1465 bytes --]

I've got a system with an oddball, but valid setup that is outside the
bounds of what GRUB supports. As such I'm starting to work on
familiarizing myself with GRUB2's internals on the way to solving the
problem.

I've found two issues worth fixing so far and I'm attaching patches.
First, what is refered to as "a magic number used by Windows NT" is
apparently used as a volume identifier to identify disks. Current
versions of LILO in fact use it to locate the disk holding its second
stage. Second, perhaps minor, but I'm surprised someone chose to break
the checksum verification code in grub-core/partmap/sun.c and didn't
didn't keep the magic number check with it. I hereby release the two
attached patches under GPL version 3 and subsequent varients.

I'm not very familiar with Bazaar, shouldn't be a problem. Does seem git
is taking over from Subversion.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         EHeM@gremlin.m5p.com PGP F6B23DE0         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
2477\___\_|_/DC21 03A0 5D61 985B <-PGP-> F2BE 6526 ABD2 F6B2\_|_/___/3DE0




-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         EHeM@gremlin.m5p.com PGP F6B23DE0         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
2477\___\_|_/DC21 03A0 5D61 985B <-PGP-> F2BE 6526 ABD2 F6B2\_|_/___/3DE0



[-- Attachment #2: Type: text/x-patch, Size: 1843 bytes --]

=== modified file 'grub-core/boot/i386/pc/boot.S'
--- grub-core/boot/i386/pc/boot.S	2010-09-19 22:06:45 +0000
+++ grub-core/boot/i386/pc/boot.S	2010-12-17 04:27:12 +0000
@@ -407,11 +407,13 @@
 	ret
 
 	/*
-	 *  Windows NT breaks compatibility by embedding a magic
-	 *  number here.
+	 *  Windows NT uses this as a 32-bit Volume Id that is unique
+	 *  amoung disks connected to the system and needs to be
+	 *  preserved. LILO acts similarly, using this field to locate
+	 *  the disks with its second stage.
 	 */
 
-	. = _start + GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC
+	. = _start + GRUB_BOOT_MACHINE_X86_VOLID
 nt_magic:
 	.long 0
 	.word 0

=== modified file 'include/grub/i386/pc/boot.h'
--- include/grub/i386/pc/boot.h	2010-04-26 08:56:12 +0000
+++ include/grub/i386/pc/boot.h	2010-12-12 06:22:16 +0000
@@ -39,8 +39,8 @@
 /* The offset of BOOT_DRIVE_CHECK.  */
 #define GRUB_BOOT_MACHINE_DRIVE_CHECK	0x66
 
-/* The offset of a magic number used by Windows NT.  */
-#define GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC	0x1b8
+/* The offset of what is used as a Volume Id by Windows NT and LILO.	*/
+#define GRUB_BOOT_MACHINE_X86_VOLID	0x1b8
 
 /* The offset of the start of the partition table.  */
 #define GRUB_BOOT_MACHINE_PART_START	0x1be

=== modified file 'util/grub-setup.c'
--- util/grub-setup.c	2010-11-26 21:03:16 +0000
+++ util/grub-setup.c	2010-12-12 06:19:44 +0000
@@ -396,9 +396,9 @@
 
     /* Copy the partition table.  */
     if (dest_partmap)
-      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);
+      memcpy (boot_img + GRUB_BOOT_MACHINE_X86_VOLID,
+	      tmp_img + GRUB_BOOT_MACHINE_X86_VOLID,
+	      GRUB_BOOT_MACHINE_PART_END - GRUB_BOOT_MACHINE_X86_VOLID);
 
     free (tmp_img);
     


[-- Attachment #3: Type: text/x-patch, Size: 1662 bytes --]

=== modified file 'grub-core/partmap/sun.c'
--- grub-core/partmap/sun.c	2010-09-14 19:07:39 +0000
+++ grub-core/partmap/sun.c	2010-12-17 05:04:53 +0000
@@ -67,19 +67,27 @@
 
 static struct grub_partition_map grub_sun_partition_map;
 
-/* Verify checksum (true=ok).  */
-static int
-grub_sun_is_valid (struct grub_sun_block *label)
+/* test whether we're dealing with a valid Sun disklabel */
+static grub_err_t
+grub_sun_test (struct grub_sun_block *label)
 {
   grub_uint16_t *pos;
   grub_uint16_t sum = 0;
 
+  if (GRUB_PARTMAP_SUN_MAGIC != grub_be_to_cpu16 (label->magic))
+    return grub_error (GRUB_ERR_BAD_PART_TABLE, "not a sun partition table");
+
   for (pos = (grub_uint16_t *) label;
        pos < (grub_uint16_t *) (label + 1);
        pos++)
     sum ^= *pos;
 
-  return ! sum;
+  /* Maybe another error value would be better, because partition
+     table _is_ recognized but invalid.  */
+  if (sum)
+    return grub_error (GRUB_ERR_BAD_PART_TABLE, "invalid checksum");
+
+  return GRUB_ERR_NONE;
 }
 
 static grub_err_t
@@ -98,14 +106,9 @@
   if (err)
     return err;
 
-  if (GRUB_PARTMAP_SUN_MAGIC != grub_be_to_cpu16 (block.magic))
-    return grub_error (GRUB_ERR_BAD_PART_TABLE, "not a sun partition table");
+  if (GRUB_ERR_NONE != (err = grub_sun_test(&block)))
+    return err;
 
-  if (! grub_sun_is_valid (&block))
-      return grub_error (GRUB_ERR_BAD_PART_TABLE, "invalid checksum");
-  
-  /* Maybe another error value would be better, because partition
-     table _is_ recognized but invalid.  */
   for (partnum = 0; partnum < GRUB_PARTMAP_SUN_MAX_PARTS; partnum++)
     {
       struct grub_sun_partition_descriptor *desc;

Re: Two Small Patches (x86 VolId & Sun Label Checking)

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1565 bytes --]

On 12/17/2010 06:19 AM, ehem+grub@m5p.com wrote:
> I've got a system with an oddball, but valid setup that is outside the
> bounds of what GRUB supports. As such I'm starting to work on
> familiarizing myself with GRUB2's internals on the way to solving the
> problem.
>
> I've found two issues worth fixing so far and I'm attaching patches.
> First, what is refered to as "a magic number used by Windows NT" is
> apparently used as a volume identifier to identify disks. 
It's windows NT Volume ID. I don't see how it is a misnomer.
> Current
> versions of LILO in fact use it to locate the disk holding its second
> stage. Second, perhaps minor, but I'm surprised someone chose to break
> the checksum verification code in grub-core/partmap/sun.c and didn't
> didn't keep the magic number check with it.
This seems to be just moving the code around. There are many different
coding styles. Changing from one of them to another is waste of effort
unless it's done to uniformise with rest of codebase. I don't have
strong preference to one or another style so I prefer not to touch anything.
>  I hereby release the two
> attached patches under GPL version 3 and subsequent varients.
>
>   

> I'm not very familiar with Bazaar, shouldn't be a problem. Does seem git
> is taking over from Subversion.
>
>
>   
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>   


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: Two Small Patches (x86 VolId & Sun Label Checking)

[ehem+grub]

>From: Vladimir '?-coder/phcoder' Serbinenko <phcoder@gmail.com>
> On 12/17/2010 06:19 AM, ehem+grub@m5p.com wrote:
> > First, what is refered to as "a magic number used by Windows NT" is
> > apparently used as a volume identifier to identify disks. 

> It's windows NT Volume ID. I don't see how it is a misnomer.

Perhaps not a gigantic one, but I would argue the existing name is a
misnomer. "Magic number" generally refers to the various constants used
to flag the various style of disk header, things like 0x504D for Apple,
"RDSK" for Amiga, the 64-bit constant for GPT, or 0xDABE for Sun.

Meanwhile, as you agreed above, it is a volume identifier that needs to
be preserved; something rather different from a magic number. Also, as
I'd further noted, LILO also uses it to identify disks, so it isn't
confined to Windows NT at this point.


> > stage. Second, perhaps minor, but I'm surprised someone chose to break
> > the checksum verification code in grub-core/partmap/sun.c and didn't
> > didn't keep the magic number check with it.

> This seems to be just moving the code around. There are many different
> coding styles. Changing from one of them to another is waste of effort
> unless it's done to uniformise with rest of codebase. I don't have
> strong preference to one or another style so I prefer not to touch anything.

Leaving it alone for now is fine by me. I just found it rather strange to
see two distinct styles in the exact same bit of code. I'd expect either
all of the verification or none of the verification broken into a
distinct function.

I will in fact be implementing breaking detection functions into distinct
functions uniformly, because there is a deeper issue lurking here. Looks
to be pure luck no one ran into an unpleasant bug lurking in the existing
design.

(I can readily provide an illustration of the lurking landmine, later
though)


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         EHeM@gremlin.m5p.com PGP F6B23DE0         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
2477\___\_|_/DC21 03A0 5D61 985B <-PGP-> F2BE 6526 ABD2 F6B2\_|_/___/3DE0

Re: Two Small Patches (x86 VolId & Sun Label Checking)

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1631 bytes --]

On 12/26/2010 06:29 AM, ehem+grub@m5p.com wrote:
>
>>> stage. Second, perhaps minor, but I'm surprised someone chose to break
>>> the checksum verification code in grub-core/partmap/sun.c and didn't
>>> didn't keep the magic number check with it.
>>>       
>   
>> This seems to be just moving the code around. There are many different
>> coding styles. Changing from one of them to another is waste of effort
>> unless it's done to uniformise with rest of codebase. I don't have
>> strong preference to one or another style so I prefer not to touch anything.
>>     
> Leaving it alone for now is fine by me. I just found it rather strange to
> see two distinct styles in the exact same bit of code. I'd expect either
> all of the verification or none of the verification broken into a
> distinct function.
>
>   
checking magic number is a simple check (a == b) whereas checksum check
is a composite (you need to compute checksum first) so moving checksum
out in a separate function makes sense
> I will in fact be implementing breaking detection functions into distinct
> functions uniformly,
I'm not fond of "let's change it just because we can". There are much
more real tasks on the project. Come to #grub and I'm sure will find
something for you.
>  because there is a deeper issue lurking here. Looks
> to be pure luck no one ran into an unpleasant bug lurking in the existing
> design.
>
>   
Please show me the real problem then.
> (I can readily provide an illustration of the lurking landmine, later
> though)
>
>
>   


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: Two Small Patches (x86 VolId & Sun Label Checking)

[ehem+grub]

>From: Vladimir '?-coder/phcoder' Serbinenko <phcoder@gmail.com>
> On 12/26/2010 06:29 AM, ehem+grub@m5p.com wrote:
> > I will in fact be implementing breaking detection functions into distinct
> > functions uniformly,
> I'm not fond of "let's change it just because we can". There are much
> more real tasks on the project. Come to #grub and I'm sure will find
> something for you.
> >  because there is a deeper issue lurking here. Looks
> > to be pure luck no one ran into an unpleasant bug lurking in the existing
> > design.
> >
> >   
> Please show me the real problem then.

Quite simple, the disk slice scheme detection routines vary in the
quality of their detection. In particular, the MSDOS-style detection is
*extremely* brittle. The only even mildly distinguishing characteristic
it finds is ensuring only the msb of the boot-flag byte is set. The other
thing it looks for is the 0xAA55 signature, but that is merely a signal
to PC-BIOSes that the disk is bootable; as such *any* bootable disk for a
IBM-PC will have that signature, whether or not it is actually using the
MSDOS-style header. A 1 in 65536 chance of a false positive is bad.

Whereas most of the other schemes have actual magic numbers for the
disk-slice scheme, that is *not* merely a flag for whether it is okay to
boot from or not (plus checksums, which push them to 1 in 2^32 chance of
incorrect detection).

Take a look at the attached file, it is ment as a header for a 512KB
image (`dd if=/dev/zero count=1023 2>/dev/null | cat sample /dev/stdin >
full_sample`). The only reason it will be correctly detected as a
SunOS-style disk label is that routine gets tried first, the MSDOS-style
detection would take it as valid.


I do have a specific goal in mind. Perhaps oddball, but a definite goal.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         EHeM@gremlin.m5p.com PGP F6B23DE0         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
2477\___\_|_/DC21 03A0 5D61 985B <-PGP-> F2BE 6526 ABD2 F6B2\_|_/___/3DE0

Re: Two Small Patches (x86 VolId & Sun Label Checking)

[ehem+grub]

[-- Attachment #1: Type: text/plain, Size: 697 bytes --]

>From: ehem+grub@m5p.com
> Take a look at the attached file, it is ment as a header for a 512KB
> image (`dd if=/dev/zero count=1023 2>/dev/null | cat sample /dev/stdin >
> full_sample`). The only reason it will be correctly detected as a
> SunOS-style disk label is that routine gets tried first, the MSDOS-style
> detection would take it as valid.

Erk, pressed return too early, see attached.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         EHeM@gremlin.m5p.com PGP F6B23DE0         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
2477\___\_|_/DC21 03A0 5D61 985B <-PGP-> F2BE 6526 ABD2 F6B2\_|_/___/3DE0



[-- Attachment #2: Type: application/octet-stream, Size: 512 bytes --]

Re: Two Small Patches (x86 VolId & Sun Label Checking)

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 2891 bytes --]

On 12/26/2010 10:15 PM, ehem+grub@m5p.com wrote:
>> From: Vladimir '?-coder/phcoder' Serbinenko <phcoder@gmail.com>
>> On 12/26/2010 06:29 AM, ehem+grub@m5p.com wrote:
>>     
>>> I will in fact be implementing breaking detection functions into distinct
>>> functions uniformly,
>>>       
>> I'm not fond of "let's change it just because we can". There are much
>> more real tasks on the project. Come to #grub and I'm sure will find
>> something for you.
>>     
>>>  because there is a deeper issue lurking here. Looks
>>> to be pure luck no one ran into an unpleasant bug lurking in the existing
>>> design.
>>>
>>>   
>>>       
>> Please show me the real problem then.
>>     
> Quite simple, the disk slice scheme detection routines vary in the
> quality of their detection. In particular, the MSDOS-style detection is
> *extremely* brittle. The only even mildly distinguishing characteristic
> it finds is ensuring only the msb of the boot-flag byte is set. The other
> thing it looks for is the 0xAA55 signature, but that is merely a signal
> to PC-BIOSes that the disk is bootable; as such *any* bootable disk for a
> IBM-PC will have that signature, whether or not it is actually using the
> MSDOS-style header. A 1 in 65536 chance of a false positive is bad.
>
>   
Actually 1 in 2^(7*4+16) =2^44 in you take into account the both checks
and consider every possible sector equiprobable. While this is a
problem, it's design problem of this partitioning label. More sanity
checks are possible but they would be heuristics and increase the
possibility of false negative. So every additional sanity check is to be
considered on case-by-case basis.
> Whereas most of the other schemes have actual magic numbers for the
> disk-slice scheme, that is *not* merely a flag for whether it is okay to
> boot from or not (plus checksums, which push them to 1 in 2^32 chance of
> incorrect detection).
>
> Take a look at the attached file, it is ment as a header for a 512KB
> image (`dd if=/dev/zero count=1023 2>/dev/null | cat sample /dev/stdin >
> full_sample`). The only reason it will be correctly detected as a
> SunOS-style disk label is that routine gets tried first, the MSDOS-style
> detection would take it as valid.
>
>   
Recent GRUB don't reject multiple disklabels per disk and you can access
all the partitions described by all of them. E.g:
(hd0,msdos1) vs (hd0,sun1)
While false-positive looks ugly in ls and slows GRUB down (checking for
filesystem in ghost partitions) it's only mildly affected. False
negatives on the other hand may prevent GRUB from booting altogether
> I do have a specific goal in mind. Perhaps oddball, but a definite goal.
>
>
>   
Improving quality of partmap detection is a good goal but be aware of
the price of heuristics.


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: Two Small Patches (x86 VolId & Sun Label Checking)

[ehem+grub]

>From: Vladimir '?-coder/phcoder' Serbinenko <phcoder@gmail.com>
> On 12/26/2010 10:15 PM, ehem+grub@m5p.com wrote:
> > Quite simple, the disk slice scheme detection routines vary in the
> > quality of their detection. In particular, the MSDOS-style detection is
> > *extremely* brittle. The only even mildly distinguishing characteristic
> > it finds is ensuring only the msb of the boot-flag byte is set. The other
> > thing it looks for is the 0xAA55 signature, but that is merely a signal
> > to PC-BIOSes that the disk is bootable; as such *any* bootable disk for a
> > IBM-PC will have that signature, whether or not it is actually using the
> > MSDOS-style header. A 1 in 65536 chance of a false positive is bad.

> Actually 1 in 2^(7*4+16) =2^44 in you take into account the both checks
> and consider every possible sector equiprobable. While this is a
> problem, it's design problem of this partitioning label. More sanity
> checks are possible but they would be heuristics and increase the
> possibility of false negative. So every additional sanity check is to be
> considered on case-by-case basis.

Alas, seeing how every possible sector is very much *not* equiprobable,
there is real potential for problems here. Taking a look at the example I
very quickly generated. It came straight out of the generic Linux `fdisk`
program (press 's' for a Sun disklabel), with the only alteration being
to mark it bootable for PC-BIOSes (helpful if one wants a PC-BIOS to boot
from it) and adjusting the checksum to compensate. The result, no need
for anything extra, those 28 bits came set to zero for free. Effectively,
any disk that is bootable in a x86 PC will test positive for having the
MS-DOS-style table (most will in fact have one, but a few oddballs
won't).



> > Whereas most of the other schemes have actual magic numbers for the
> > disk-slice scheme, that is *not* merely a flag for whether it is okay to
> > boot from or not (plus checksums, which push them to 1 in 2^32 chance of
> > incorrect detection).
> >
> > Take a look at the attached file, it is ment as a header for a 512KB
> > image (`dd if=/dev/zero count=1023 2>/dev/null | cat sample /dev/stdin >
> > full_sample`). The only reason it will be correctly detected as a
> > SunOS-style disk label is that routine gets tried first, the MSDOS-style
> > detection would take it as valid.

> Recent GRUB don't reject multiple disklabels per disk and you can access
> all the partitions described by all of them. E.g:
> (hd0,msdos1) vs (hd0,sun1)
> While false-positive looks ugly in ls and slows GRUB down (checking for
> filesystem in ghost partitions) it's only mildly affected. False
> negatives on the other hand may prevent GRUB from booting altogether

> Improving quality of partmap detection is a good goal but be aware of
> the price of heuristics.

That isn't my thinking. I was thinking of having it test for the various
schemes first, then choosing the best fit. While trying multiple schemes
is viable for grub_partition_iterate(), it doesn't work when installing
boot code or attempting to do partition modification (since both of these
*must* know in order to function).



Organizational item here. Is the existing layout of <task>/<arch> for the
best? (task would be boot/partmap/parttool, arch is pretty much every .c
file in grub-core/partmap) I wonder if perhaps a structure like
<arch>/<task> would work better?

Seeing how the boot code/installation code *must* know the
disk-label/partitioning-type in order to fit within the limitations
thereof, the latter seems to make sense. Certainly the existing layout is
pretty conventional, but the latter seems to match how things need to be
shared better.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         EHeM@gremlin.m5p.com PGP F6B23DE0         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
2477\___\_|_/DC21 03A0 5D61 985B <-PGP-> F2BE 6526 ABD2 F6B2\_|_/___/3DE0

Re: Two Small Patches (x86 VolId & Sun Label Checking)

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 3145 bytes --]

On 12/29/2010 07:25 AM, ehem+grub@m5p.com wrote:
>> From: Vladimir '?-coder/phcoder' Serbinenko <phcoder@gmail.com>
>> On 12/26/2010 10:15 PM, ehem+grub@m5p.com wrote:
>>     
>>> Quite simple, the disk slice scheme detection routines vary in the
>>> quality of their detection. In particular, the MSDOS-style detection is
>>> *extremely* brittle. The only even mildly distinguishing characteristic
>>> it finds is ensuring only the msb of the boot-flag byte is set. The other
>>> thing it looks for is the 0xAA55 signature, but that is merely a signal
>>> to PC-BIOSes that the disk is bootable; as such *any* bootable disk for a
>>> IBM-PC will have that signature, whether or not it is actually using the
>>> MSDOS-style header. A 1 in 65536 chance of a false positive is bad.
>>>       
>   
>> Actually 1 in 2^(7*4+16) =2^44 in you take into account the both checks
>> and consider every possible sector equiprobable. While this is a
>> problem, it's design problem of this partitioning label. More sanity
>> checks are possible but they would be heuristics and increase the
>> possibility of false negative. So every additional sanity check is to be
>> considered on case-by-case basis.
>>     
> Alas, seeing how every possible sector is very much *not* equiprobable,
>   
Then you need to havethe information about distribution before even
speaking about any probabilities. Otherwise these "probabilities" are
good only for fortune-telling.
>
>> Improving quality of partmap detection is a good goal but be aware of
>> the price of heuristics.
>>     
> That isn't my thinking. I was thinking of having it test for the various
> schemes first, then choosing the best fit. While trying multiple schemes
> is viable for grub_partition_iterate(), it doesn't work when installing
> boot code or attempting to do partition modification (since both of these
> *must* know in order to function).
>
>   
When doing grub-setup then the philosophy is: "do no harm". It must
abort when it sees anything unusual. GRUB can't have a super cow
intelligence to magically sort all possible weird stuff. So sometimes we
have to abort rather than risk damaging any data (with optional override
options).
As for partmap the user tells explicitly which partition table he means.
E.g:
parttool hd0,msdos1 boot+
So there is no problem here.
> Organizational item here. Is the existing layout of <task>/<arch> for the
> best? (task would be boot/partmap/parttool, arch is pretty much every .c
> file in grub-core/partmap) I wonder if perhaps a structure like
> <arch>/<task> would work better?
>
>   
Shuffling the files around without anything more than "it looks slightly
better for me" is usually useless. We, the current developpers, find the
current layout convenient. If do-o-cratically the consensus on changing
of layout would be established it can be done, however I don't see it happen

While the sensible checks would be accepted, pushing tolerances of
heuristics is of little use. Tell the real stuff if you want to convince me


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: Two Small Patches (x86 VolId & Sun Label Checking)

[Brendan Trotter]

Hi,

2010/12/27 Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com>:
> On 12/26/2010 10:15 PM, ehem+grub@m5p.com wrote:
>> Quite simple, the disk slice scheme detection routines vary in the
>> quality of their detection. In particular, the MSDOS-style detection is
>> *extremely* brittle. The only even mildly distinguishing characteristic
>> it finds is ensuring only the msb of the boot-flag byte is set. The other
>> thing it looks for is the 0xAA55 signature, but that is merely a signal
>> to PC-BIOSes that the disk is bootable; as such *any* bootable disk for a
>> IBM-PC will have that signature, whether or not it is actually using the
>> MSDOS-style header. A 1 in 65536 chance of a false positive is bad.

If a disk is bootable then it should have the 0xAA55 signature,
regardless of whether or not there's a partition table.

If a disk is *not* bootable then it should *not* have the 0xAA55
signature, regardless of whether or not there's a partition table.
Lots of OSs (including Linux/fdisk) have the same stupid bug where
they put the 0xAA55 signature on pure data disks where it doesn't
belong. The worst case here is that the computer crashes during boot
if the disk ever becomes the "first" disk (rather than the BIOS moving
on to the next type of bootable device, and/or rather than the user
getting a "No OS found" BIOS error message), so it's usually just an
annoying bug (and not a serious bug, unless someone is perverted
enough to attempt to use a bootable CD-ROM or something as a type of
fail-safe in case their first drive goes missing), but it's still
"wrong".

To check if there is/isn't an "MBR partition table", first check for
GPT. If there's no GPT then; for each (potential) MBR partition table
entry that isn't "type = unused" check for "(boot-flag & 0x7F == 0)"
then check if the starting CHS address is sane (e.g. not an invalid
sector, and with work-arounds for "unrepresentable") and if the ending
CHS address is sane (e.g. not invalid and not less than or equal to
the starting CHS, and with work-arounds for "unrepresentable") and
then do the same for the starting and ending LBA addresses. Then you
could check for overlapping partitions.

If a partition is marked as "active" then you could check for the
0xAA55 signature in the first sector of that partition, but that is
the only case where checking the 0xAA55 signature makes any sense.

> Actually 1 in 2^(7*4+16) =2^44 in you take into account the both checks
> and consider every possible sector equiprobable.

Actually, if you take into account that the second check is worthless
and/or wrong; then for the current code; if there's 4 (assumed)
"usable" partitions there's 1 chance in 4*127 of thinking there's a
partition table when there isn't, and if there's only one (assumed)
"usable" partition then it drops to 1 chance in 127; and if the MBR
contains random data the former is much more likely than the latter (I
was too lazy to do the calculation properly).


Cheers,

Brendan

Re: Two Small Patches (x86 VolId & Sun Label Checking)

[ehem+grub]

>From: Vladimir '?-coder/phcoder' Serbinenko <phcoder@gmail.com>
> On 12/29/2010 07:25 AM, ehem+grub@m5p.com wrote:
> >> From: Vladimir '?-coder/phcoder' Serbinenko <phcoder@gmail.com>
> >> On 12/26/2010 10:15 PM, ehem+grub@m5p.com wrote:
> >>     
> >>> Quite simple, the disk slice scheme detection routines vary in the
> >>> quality of their detection. In particular, the MSDOS-style detection is
> >>> *extremely* brittle. The only even mildly distinguishing characteristic
> >>> it finds is ensuring only the msb of the boot-flag byte is set. The other
> >>> thing it looks for is the 0xAA55 signature, but that is merely a signal
> >>> to PC-BIOSes that the disk is bootable; as such *any* bootable disk for a
> >>> IBM-PC will have that signature, whether or not it is actually using the
> >>> MSDOS-style header. A 1 in 65536 chance of a false positive is bad.
> >>>       
> >   
> >> Actually 1 in 2^(7*4+16) =2^44 in you take into account the both checks
> >> and consider every possible sector equiprobable. While this is a
> >> problem, it's design problem of this partitioning label. More sanity
> >> checks are possible but they would be heuristics and increase the
> >> possibility of false negative. So every additional sanity check is to be
> >> considered on case-by-case basis.
> >>     
> > Alas, seeing how every possible sector is very much *not* equiprobable,
> >   
> Then you need to havethe information about distribution before even
> speaking about any probabilities. Otherwise these "probabilities" are
> good only for fortune-telling.

Brendan Trotter did some additional demolition. The real problem is those
extra bits are all set to *zero*. Unused space in most schemes will
likely be cleared to zero, so those are fairly likely to fall by default.
If they overlap structures, then the odds will vary.

Using the SunOS-style for comparison, since I've got samples handy. The
overlaps are on the start cylinders for slices #0, #2, #4, and #6. Of
those, #2 will almost certainly be zero (overlap slice, covering the
whole disk) and #0 will very likely be zero as often being the root FS.
#4 and #6 are more interesting, that is the 2nd least significant byte,
so they're fairly likely to have data there. Though not too many systems
will have six or more genuine slices, so #6 is pretty likely to go unused
and it will be left as zeros. So, only seven bits are semi-likely to be
notable.

Jumping to the real world. My fast example passed, though it reads as
completely empty to MS-DOS. Two other samples got lucky, both had lots of
slices and so hit both of the magic bytes. On the flip side, one only had
two bits set so it was pretty close...


> > Organizational item here. Is the existing layout of <task>/<arch> for the
> > best? (task would be boot/partmap/parttool, arch is pretty much every .c
> > file in grub-core/partmap) I wonder if perhaps a structure like
> > <arch>/<task> would work better?
> >
> >   
> Shuffling the files around without anything more than "it looks slightly
> better for me" is usually useless. We, the current developpers, find the
> current layout convenient. If do-o-cratically the consensus on changing
> of layout would be established it can be done, however I don't see it happen

It keeps associated structures closer together. Specifically, it means
things like the boot blocks for MS-DOS-style partitions
(grub-core/boot/i386) would be closer to the code for handling
MS-DOS-style partitions (grub-core/partmap/msdos.c,
grub-core/parttool/msdos.c). Seeing how all of those share some internal
portions that no other code cares about, it would seem keeping them
closer together would be a good thing.

Anyway, it isn't crucial for my purposes, I thought I'd suggest it, but
I can readily deal with it either way.


> While the sensible checks would be accepted, pushing tolerances of
> heuristics is of little use. Tell the real stuff if you want to convince me

Have I mentioned heuristics in any of my messages? I've merely been
writing that the existing methods are rather sub-optimal. The checks for
MS-DOS-style partitions will pretty much accept any old garbage, while
most of the other schemes have got pretty good exclusion checks.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         EHeM@gremlin.m5p.com PGP F6B23DE0         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
2477\___\_|_/DC21 03A0 5D61 985B <-PGP-> F2BE 6526 ABD2 F6B2\_|_/___/3DE0