[PATCH] Fix when installing on pationless but partionable medium

[PATCH] Fix when installing on pationless but partionable medium

[Vladimir 'phcoder' Serbinenko]

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

Sometimes a media that can be partioned isn't really partioned. E.g.
usb sticks. This is a patch to handle this situation. Unfortunately
such medium is often formated with a flavour of FAT which shares its
signature with MBR so it may be easily misidentified as
pc_partition_table. Furthermore the same signature is shared with
bootsectors including grub. One possibility is to try interpret disk
as known filesystems and see if we succeed. But the problem is that
the check for FAT are light and may result in false positives too. The
only more or less advanced check there is a check for FATXX string.
But I was about to propose to eliminate this check since I encountered
a FAT filesystem without this string on friend's SD card formatted
with symbian which he wanted to use as liveusb. Does anyone has a
better idea?

-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

[-- Attachment #2: installfix.dif --]
[-- Type: video/dv, Size: 1134 bytes --]

Re: [PATCH] Fix when installing on pationless but partionable medium

[Colin Watson]

On Fri, Jul 17, 2009 at 06:41:59PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> Sometimes a media that can be partioned isn't really partioned. E.g.
> usb sticks. This is a patch to handle this situation. Unfortunately
> such medium is often formated with a flavour of FAT which shares its
> signature with MBR so it may be easily misidentified as
> pc_partition_table. Furthermore the same signature is shared with
> bootsectors including grub. One possibility is to try interpret disk
> as known filesystems and see if we succeed. But the problem is that
> the check for FAT are light and may result in false positives too. The
> only more or less advanced check there is a check for FATXX string.
> But I was about to propose to eliminate this check since I encountered
> a FAT filesystem without this string on friend's SD card formatted
> with symbian which he wanted to use as liveusb. Does anyone has a
> better idea?

When checking for an MBR filesystem label, parted checks whether each of
partitions 1-4 has a boot indicator that's either 0 or 0x80, since as
you point out checking for the FAT signature suffers false positives; I
believe this algorithm matches that in the Linux kernel. Look at
libparted/labels/dos.c:msdos_probe(), which is already FSF-copyrighted
and GPLv3+. GRUB should use the same algorithm, and then the worst case
is that things will fail consistently.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] Fix when installing on pationless but partionable medium

[Robert Millan]

On Fri, Jul 17, 2009 at 05:51:40PM +0100, Colin Watson wrote:
> On Fri, Jul 17, 2009 at 06:41:59PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> > Sometimes a media that can be partioned isn't really partioned. E.g.
> > usb sticks. This is a patch to handle this situation. Unfortunately
> > such medium is often formated with a flavour of FAT which shares its
> > signature with MBR so it may be easily misidentified as
> > pc_partition_table. Furthermore the same signature is shared with
> > bootsectors including grub. One possibility is to try interpret disk
> > as known filesystems and see if we succeed. But the problem is that
> > the check for FAT are light and may result in false positives too. The
> > only more or less advanced check there is a check for FATXX string.
> > But I was about to propose to eliminate this check since I encountered
> > a FAT filesystem without this string on friend's SD card formatted
> > with symbian which he wanted to use as liveusb. Does anyone has a
> > better idea?
> 
> When checking for an MBR filesystem label, parted checks whether each of
> partitions 1-4 has a boot indicator that's either 0 or 0x80, since as
> you point out checking for the FAT signature suffers false positives; I
> believe this algorithm matches that in the Linux kernel. Look at
> libparted/labels/dos.c:msdos_probe(), which is already FSF-copyrighted
> and GPLv3+. GRUB should use the same algorithm, and then the worst case
> is that things will fail consistently.

I might be missing something about this check, but GRUB doesn't require that
the boot flag is present.  Therefore, its non presence doesn't imply this is
not a real msdos label.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] Fix when installing on pationless but partionable medium

[Vladimir 'phcoder' Serbinenko]

On Sat, Jul 18, 2009 at 8:45 PM, Robert Millan<rmh@aybabtu.com> wrote:
> On Fri, Jul 17, 2009 at 05:51:40PM +0100, Colin Watson wrote:
>> On Fri, Jul 17, 2009 at 06:41:59PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>> > Sometimes a media that can be partioned isn't really partioned. E.g.
>> > usb sticks. This is a patch to handle this situation. Unfortunately
>> > such medium is often formated with a flavour of FAT which shares its
>> > signature with MBR so it may be easily misidentified as
>> > pc_partition_table. Furthermore the same signature is shared with
>> > bootsectors including grub. One possibility is to try interpret disk
>> > as known filesystems and see if we succeed. But the problem is that
>> > the check for FAT are light and may result in false positives too. The
>> > only more or less advanced check there is a check for FATXX string.
>> > But I was about to propose to eliminate this check since I encountered
>> > a FAT filesystem without this string on friend's SD card formatted
>> > with symbian which he wanted to use as liveusb. Does anyone has a
>> > better idea?
>>
>> When checking for an MBR filesystem label, parted checks whether each of
>> partitions 1-4 has a boot indicator that's either 0 or 0x80, since as
>> you point out checking for the FAT signature suffers false positives; I
>> believe this algorithm matches that in the Linux kernel. Look at
>> libparted/labels/dos.c:msdos_probe(), which is already FSF-copyrighted
>> and GPLv3+. GRUB should use the same algorithm, and then the worst case
>> is that things will fail consistently.
>
> I might be missing something about this check, but GRUB doesn't require that
> the boot flag is present.  Therefore, its non presence doesn't imply this is
> not a real msdos label.
>
He refers to boot flag as a byte in msdos structure which can only be
0x00 (not set) or 0x80 (set)
> --
> Robert Millan
>
>  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
>  how) you may access your data; but nobody's threatening your freedom: we
>  still allow you to remove your data and not access it at all."
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH] Fix when installing on pationless but partionable medium

[Robert Millan]

On Sat, Jul 18, 2009 at 09:01:38PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> On Sat, Jul 18, 2009 at 8:45 PM, Robert Millan<rmh@aybabtu.com> wrote:
> > On Fri, Jul 17, 2009 at 05:51:40PM +0100, Colin Watson wrote:
> >> On Fri, Jul 17, 2009 at 06:41:59PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> >> > Sometimes a media that can be partioned isn't really partioned. E.g.
> >> > usb sticks. This is a patch to handle this situation. Unfortunately
> >> > such medium is often formated with a flavour of FAT which shares its
> >> > signature with MBR so it may be easily misidentified as
> >> > pc_partition_table. Furthermore the same signature is shared with
> >> > bootsectors including grub. One possibility is to try interpret disk
> >> > as known filesystems and see if we succeed. But the problem is that
> >> > the check for FAT are light and may result in false positives too. The
> >> > only more or less advanced check there is a check for FATXX string.
> >> > But I was about to propose to eliminate this check since I encountered
> >> > a FAT filesystem without this string on friend's SD card formatted
> >> > with symbian which he wanted to use as liveusb. Does anyone has a
> >> > better idea?
> >>
> >> When checking for an MBR filesystem label, parted checks whether each of
> >> partitions 1-4 has a boot indicator that's either 0 or 0x80, since as
> >> you point out checking for the FAT signature suffers false positives; I
> >> believe this algorithm matches that in the Linux kernel. Look at
> >> libparted/labels/dos.c:msdos_probe(), which is already FSF-copyrighted
> >> and GPLv3+. GRUB should use the same algorithm, and then the worst case
> >> is that things will fail consistently.
> >
> > I might be missing something about this check, but GRUB doesn't require that
> > the boot flag is present.  Therefore, its non presence doesn't imply this is
> > not a real msdos label.
> >
> He refers to boot flag as a byte in msdos structure which can only be
> 0x00 (not set) or 0x80 (set)

Yes.  GRUB's boot.img doesn't do anything with it AFAICT.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] Fix when installing on pationless but partionable medium

[Colin Watson]

On Sat, Jul 18, 2009 at 09:17:41PM +0200, Robert Millan wrote:
> On Sat, Jul 18, 2009 at 09:01:38PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> > On Sat, Jul 18, 2009 at 8:45 PM, Robert Millan<rmh@aybabtu.com> wrote:
> > > I might be missing something about this check, but GRUB doesn't require that
> > > the boot flag is present.  Therefore, its non presence doesn't imply this is
> > > not a real msdos label.
> > 
> > He refers to boot flag as a byte in msdos structure which can only be
> > 0x00 (not set) or 0x80 (set)
> 
> Yes.  GRUB's boot.img doesn't do anything with it AFAICT.

That's as may be, sure; but checking that that byte is one of the two
permitted values in all four partitions happens to be a good sanity
check for whether it's really an MS-DOS label or in fact something else.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] Fix when installing on pationless but partionable medium

[Robert Millan]

On Sun, Jul 19, 2009 at 11:07:41AM +0100, Colin Watson wrote:
> On Sat, Jul 18, 2009 at 09:17:41PM +0200, Robert Millan wrote:
> > On Sat, Jul 18, 2009 at 09:01:38PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> > > On Sat, Jul 18, 2009 at 8:45 PM, Robert Millan<rmh@aybabtu.com> wrote:
> > > > I might be missing something about this check, but GRUB doesn't require that
> > > > the boot flag is present.  Therefore, its non presence doesn't imply this is
> > > > not a real msdos label.
> > > 
> > > He refers to boot flag as a byte in msdos structure which can only be
> > > 0x00 (not set) or 0x80 (set)
> > 
> > Yes.  GRUB's boot.img doesn't do anything with it AFAICT.
> 
> That's as may be, sure; but checking that that byte is one of the two
> permitted values in all four partitions happens to be a good sanity
> check for whether it's really an MS-DOS label or in fact something else.

As a bootloader, most of the decisions GRUB takes have a critical effect.  We
can't make GRUB take those decisions based on heuristic.  If we determine
that something ought to be done, and we do it, we're taking responsibility for
it.  Bootloader-related breakage tends to be severe, and we can't afford to
be blamed for it just because "our heuristic got it wrong".

Going down this path, if our heuristic is not good enough, we'll have to
improve it.  We'd make it more complex and check for more things.  But no
matter what we do, it can never be completely reliable.

Similarly, and as I said before (in this thread or another, I'm not sure), the
situation in which someone using a _sane_ MBR label with a sane filesystem in
it is our primary use case, and the ability of GRUB to install in it must not
depend on heuristic.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] Fix when installing on pationless but partionable medium

[Pavel Roskin]

On Wed, 2009-07-22 at 19:16 +0200, Robert Millan wrote:
> As a bootloader, most of the decisions GRUB takes have a critical effect.  We
> can't make GRUB take those decisions based on heuristic.

Agreed.

Sorry for being late in joining this discussion.

It's not enough to make sure that there is some partitioning on the hard
driver.

boot.img has holes for FAT and PC MBR.  That's two configurations we
support.  No other partition tables or filesystems are supported.

-- 
Regards,
Pavel Roskin

Re: [PATCH] Fix when installing on pationless but partionable medium

[Vladimir 'phcoder' Serbinenko]

On Wed, Jul 22, 2009 at 7:36 PM, Pavel Roskin<proski@gnu.org> wrote:
> On Wed, 2009-07-22 at 19:16 +0200, Robert Millan wrote:
> boot.img has holes for FAT and PC MBR.  That's two configurations we
> support.  No other partition tables or filesystems are supported.
Many filesystems leave first block free. These are supported too
>
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH] Fix when installing on pationless but partionable medium

[Pavel Roskin]

On Wed, 2009-07-22 at 19:43 +0200, Vladimir 'phcoder' Serbinenko wrote:
> On Wed, Jul 22, 2009 at 7:36 PM, Pavel Roskin<proski@gnu.org> wrote:
> > On Wed, 2009-07-22 at 19:16 +0200, Robert Millan wrote:
> > boot.img has holes for FAT and PC MBR.  That's two configurations we
> > support.  No other partition tables or filesystems are supported.
> Many filesystems leave first block free. These are supported too

Fine with me if we can identify them.

-- 
Regards,
Pavel Roskin

Re: [PATCH] Fix when installing on pationless but partionable medium

[Vladimir 'phcoder' Serbinenko]

On Wed, Jul 22, 2009 at 7:46 PM, Pavel Roskin<proski@gnu.org> wrote:
> On Wed, 2009-07-22 at 19:43 +0200, Vladimir 'phcoder' Serbinenko wrote:
>> On Wed, Jul 22, 2009 at 7:36 PM, Pavel Roskin<proski@gnu.org> wrote:
>> > On Wed, 2009-07-22 at 19:16 +0200, Robert Millan wrote:
>> > boot.img has holes for FAT and PC MBR.  That's two configurations we
>> > support.  No other partition tables or filesystems are supported.
>> Many filesystems leave first block free. These are supported too
>
> Fine with me if we can identify them.
>
We could have an explicit list of them in util/i386/pc/grub-setup.c
same way as we currently have a list of supported partition tables.
This approach has a drawback of needing to manually add new
filesystems to yet another list but it has a safeguqrd to install only
on configurations which we have tested. If user needs to install to
another FS he can test and submit a patch. The only problem I see is
if FS is in grub-extras. In this case grub-setup wouldn't be able to
identify it and would fail installing to prevent FS damage. This is
part of larger problem of grub-utils with external modules. As a
possibility grub-setup can ask host OS about the target filesystem
(cumbersome) or we could implement module loading in grub-setup (using
shared library approach perhaps?)
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH] Fix when installing on pationless but partionable medium

[Robert Millan]

On Fri, Jul 17, 2009 at 06:41:59PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> Sometimes a media that can be partioned isn't really partioned. E.g.
> usb sticks. This is a patch to handle this situation.

But we had a check for this already, is it not working?

  if (! dest_dev->disk->has_partitions)
    { 
      grub_util_warn ("Attempting to install GRUB to a partitionless disk.  This is a BAD idea.");
      goto unable_to_embed;
    }

> Unfortunately
> such medium is often formated with a flavour of FAT which shares its
> signature with MBR so it may be easily misidentified as
> pc_partition_table. Furthermore the same signature is shared with
> bootsectors including grub. One possibility is to try interpret disk
> as known filesystems and see if we succeed. But the problem is that
> the check for FAT are light and may result in false positives too. The
> only more or less advanced check there is a check for FATXX string.
> But I was about to propose to eliminate this check since I encountered
> a FAT filesystem without this string on friend's SD card formatted
> with symbian which he wanted to use as liveusb. Does anyone has a
> better idea?

I'm not sure there's much we can do about this.  Using heuristics sounds like
it will make the solution worse than the problem.  I don't care much about
Microsoft filesystems, but I'd hate to see GRUB fail on a completely sane
ext3 inside msdos label because it happened to look like FAT in raw disk at
the same time.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] Fix when installing on pationless but partionable medium

[Vladimir 'phcoder' Serbinenko]

On Sat, Jul 18, 2009 at 8:42 PM, Robert Millan<rmh@aybabtu.com> wrote:
> On Fri, Jul 17, 2009 at 06:41:59PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>> Sometimes a media that can be partioned isn't really partioned. E.g.
>> usb sticks. This is a patch to handle this situation.
>
> But we had a check for this already, is it not working?
>
>  if (! dest_dev->disk->has_partitions)
>    {
>      grub_util_warn ("Attempting to install GRUB to a partitionless disk.  This is a BAD idea.");
>      goto unable_to_embed;
>    }
has_partitions is set by driver and has_partitions is a misnomer and
it should be really can_be_partitioned. As a matter of fact this is
even more problematic since whether has_partition is set or no often
depends whether author know about partitioned media of given kind. I
think this field should be ditched altogether
>
> I'm not sure there's much we can do about this.  Using heuristics sounds like
> it will make the solution worse than the problem.  I don't care much about
> Microsoft filesystems, but I'd hate to see GRUB fail on a completely sane
> ext3 inside msdos label because it happened to look like FAT in raw disk at
> the same time.
The approach proposed by Collin avoids such problems since correct
pc_partition_map is always detected as such. Also if fs is misdetected
as pc_partition_map it's still acessible by it's name (e.g. use (hd0)
even if (hd0,1) is present). With current misdetection problem
grub-setup may embed in internal FS structures corrupting it in the
passage
>
> --
> Robert Millan
>
>  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
>  how) you may access your data; but nobody's threatening your freedom: we
>  still allow you to remove your data and not access it at all."
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH] Fix when installing on pationless but partionable medium

[Robert Millan]

On Sat, Jul 18, 2009 at 09:00:36PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> On Sat, Jul 18, 2009 at 8:42 PM, Robert Millan<rmh@aybabtu.com> wrote:
> > On Fri, Jul 17, 2009 at 06:41:59PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> >> Sometimes a media that can be partioned isn't really partioned. E.g.
> >> usb sticks. This is a patch to handle this situation.
> >
> > But we had a check for this already, is it not working?
> >
> >  if (! dest_dev->disk->has_partitions)
> >    {
> >      grub_util_warn ("Attempting to install GRUB to a partitionless disk.  This is a BAD idea.");
> >      goto unable_to_embed;
> >    }
> has_partitions is set by driver and has_partitions is a misnomer and
> it should be really can_be_partitioned.

You're right.  I think it's fine if we rename it.

> As a matter of fact this is
> even more problematic since whether has_partition is set or no often
> depends whether author know about partitioned media of given kind. I
> think this field should be ditched altogether

I don't understand what you mean here.

> > I'm not sure there's much we can do about this.  Using heuristics sounds like
> > it will make the solution worse than the problem.  I don't care much about
> > Microsoft filesystems, but I'd hate to see GRUB fail on a completely sane
> > ext3 inside msdos label because it happened to look like FAT in raw disk at
> > the same time.
> The approach proposed by Collin avoids such problems since correct
> pc_partition_map is always detected as such.

I haven't looked at the source code, but what he said is we can determine if
an MBR is valid by checking the bootable flag, and this is not always so.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] Fix when installing on pationless but partionable medium

[Vladimir 'phcoder' Serbinenko]

> I don't understand what you mean here.
Let's take a common example of cdrom. Most of the users and developers
are accustomed to a cdrom holding one filesystem. On macs however cds
are partitioned and not being able to access all the partitions is a
problem for end user. Such situations are probably common. If we ditch
has_partitions altogether the only negative side effect will be that
in some weird configurations unpartitioned media may appear to have
partitions but whole media is still accessible. Additionally it
simplifies and makes kernel smaller
>
>> > I'm not sure there's much we can do about this.  Using heuristics sounds like
>> > it will make the solution worse than the problem.  I don't care much about
>> > Microsoft filesystems, but I'd hate to see GRUB fail on a completely sane
>> > ext3 inside msdos label because it happened to look like FAT in raw disk at
>> > the same time.
>> The approach proposed by Collin avoids such problems since correct
>> pc_partition_map is always detected as such.
>
> I haven't looked at the source code, but what he said is we can determine if
> an MBR is valid by checking the bootable flag, and this is not always so.
I don't see any problem. He said: checking that bootable flags of all
partitions are either set (0x80) or unset (0x0) and not another value
>
> --
> Robert Millan
>
>  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
>  how) you may access your data; but nobody's threatening your freedom: we
>  still allow you to remove your data and not access it at all."
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH] Fix when installing on pationless but partionable medium

[Robert Millan]

On Sat, Jul 18, 2009 at 11:28:58PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> > I don't understand what you mean here.
> Let's take a common example of cdrom. Most of the users and developers
> are accustomed to a cdrom holding one filesystem. On macs however cds
> are partitioned and not being able to access all the partitions is a
> problem for end user. Such situations are probably common. If we ditch
> has_partitions altogether the only negative side effect will be that
> in some weird configurations unpartitioned media may appear to have
> partitions but whole media is still accessible. Additionally it
> simplifies and makes kernel smaller

I'm not sure they're so weird.  But we could still do it.  Pavel, what do
you think?

> >> > I'm not sure there's much we can do about this.  Using heuristics sounds like
> >> > it will make the solution worse than the problem.  I don't care much about
> >> > Microsoft filesystems, but I'd hate to see GRUB fail on a completely sane
> >> > ext3 inside msdos label because it happened to look like FAT in raw disk at
> >> > the same time.
> >> The approach proposed by Collin avoids such problems since correct
> >> pc_partition_map is always detected as such.
> >
> > I haven't looked at the source code, but what he said is we can determine if
> > an MBR is valid by checking the bootable flag, and this is not always so.
> I don't see any problem. He said: checking that bootable flags of all
> partitions are either set (0x80) or unset (0x0) and not another value

Oh, that's different.  I think it's fine provided that:

  - None of the commonly used free partitioning tools uses an illegal value.

  - We fail gracefully and let the user know why.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] Fix when installing on pationless but partionable medium

[Pavel Roskin]

On Wed, 2009-07-22 at 19:22 +0200, Robert Millan wrote:
> On Sat, Jul 18, 2009 at 11:28:58PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> > > I don't understand what you mean here.
> > Let's take a common example of cdrom. Most of the users and developers
> > are accustomed to a cdrom holding one filesystem. On macs however cds
> > are partitioned and not being able to access all the partitions is a
> > problem for end user. Such situations are probably common. If we ditch
> > has_partitions altogether the only negative side effect will be that
> > in some weird configurations unpartitioned media may appear to have
> > partitions but whole media is still accessible. Additionally it
> > simplifies and makes kernel smaller
> 
> I'm not sure they're so weird.  But we could still do it.  Pavel, what do
> you think?

We don't use grub-setup to install GRUB on CDROM.  I don't know if we
can eliminate has_partitions.  If we can, I'm fine.

I suggest that we start with known safe cases, such are PC partition
table and FAT filesystem.  If we can positively identify the place where
we install the bootsector as either of that, we can install without
--force.  We can extend the rules as we check if other filesystems allow
overwriting the bootsector.

I don't think dumping floppy support would be right at this point.
Maybe five years from now.

> > > I haven't looked at the source code, but what he said is we can determine if
> > > an MBR is valid by checking the bootable flag, and this is not always so.
> > I don't see any problem. He said: checking that bootable flags of all
> > partitions are either set (0x80) or unset (0x0) and not another value
> 
> Oh, that's different.  I think it's fine provided that:
> 
>   - None of the commonly used free partitioning tools uses an illegal value.
> 
>   - We fail gracefully and let the user know why.

I'm fine with extra checks.

-- 
Regards,
Pavel Roskin

Re: [PATCH] Fix when installing on pationless but partionable medium

[Vladimir 'phcoder' Serbinenko]

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

On Wed, Jul 22, 2009 at 7:22 PM, Robert Millan<rmh@aybabtu.com> wrote:
> On Sat, Jul 18, 2009 at 11:28:58PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>> > I don't understand what you mean here.
>> Let's take a common example of cdrom. Most of the users and developers
>> are accustomed to a cdrom holding one filesystem. On macs however cds
>> are partitioned and not being able to access all the partitions is a
>> problem for end user. Such situations are probably common. If we ditch
>> has_partitions altogether the only negative side effect will be that
>> in some weird configurations unpartitioned media may appear to have
>> partitions but whole media is still accessible. Additionally it
>> simplifies and makes kernel smaller

See nopart.diff. Once Pavel's patch for partitions is committed they
can integrate nicely in util/i386/pc/grub-setup.c

>> He said: checking that bootable flags of all
>> partitions are either set (0x80) or unset (0x0) and not another value
>
> Oh, that's different.  I think it's fine provided that:
>
>  - None of the commonly used free partitioning tools uses an illegal value.
>
>  - We fail gracefully and let the user know why.

See mbr.diff

>
> --
> Robert Millan
>
>  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
>  how) you may access your data; but nobody's threatening your freedom: we
>  still allow you to remove your data and not access it at all."
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

[-- Attachment #2: mbr.diff --]
[-- Type: text/plain, Size: 933 bytes --]

diff --git a/ChangeLog b/ChangeLog
index 752bde8..0d42f3c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2009-07-26  Vladimir Serbinenko  <phcoder@gmail.com>
+
+	* partmap/pc.c (pc_partition_map_iterate): Check that boot flags are
+	valid.
+
 2009-07-25  Felix Zielcke  <fzielcke@z-51.de>
 
 	* kern/file.c (grub_file_open): Revert to previous check with
diff --git a/partmap/pc.c b/partmap/pc.c
index 6f68ecf..e3ea53a 100644
--- a/partmap/pc.c
+++ b/partmap/pc.c
@@ -121,6 +121,13 @@ pc_partition_map_iterate (grub_disk_t disk,
       if (mbr.signature != grub_cpu_to_le16 (GRUB_PC_PARTITION_SIGNATURE))
 	return grub_error (GRUB_ERR_BAD_PART_TABLE, "no signature");
 
+      for (i = 0; i < 4; i++)
+	if (mbr.entries[i].flag & 0x7f)
+	  break;
+
+      if (i != 4)
+	return grub_error (GRUB_ERR_BAD_PART_TABLE, "bad boot flag");
+
       /* Analyze DOS partitions.  */
       for (p.index = 0; p.index < 4; p.index++)
 	{

[-- Attachment #3: nopart.diff --]
[-- Type: text/plain, Size: 12856 bytes --]

diff --git a/ChangeLog b/ChangeLog
index 752bde8..bda13e9 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2009-07-26  Vladimir Serbinenko  <phcoder@gmail.com>
+
+	* include/grub/disk.h (grub_disk): Remove has_partitions.
+	All users updated.
+	* disk/loopback.c (grub_loopback): Remove has_partitions.
+	All users updated.
+	* util/grub-fstest.c (fstest): Don't pass "-p" to loopback.
+	(options): Remove partitions. All users updated.
+	* util/i386/pc/grub-setup.c (setup): copy parition table only when
+	actual partition table is found.
+
 2009-07-25  Felix Zielcke  <fzielcke@z-51.de>
 
 	* kern/file.c (grub_file_open): Revert to previous check with
diff --git a/disk/ata.c b/disk/ata.c
index 78d3965..73b07be 100644
--- a/disk/ata.c
+++ b/disk/ata.c
@@ -683,7 +683,6 @@ grub_ata_open (const char *name, grub_disk_t disk)
 
   disk->id = (unsigned long) dev;
 
-  disk->has_partitions = 1;
   disk->data = dev;
 
   return 0;
diff --git a/disk/efi/efidisk.c b/disk/efi/efidisk.c
index de84859..fd1fae4 100644
--- a/disk/efi/efidisk.c
+++ b/disk/efi/efidisk.c
@@ -514,16 +514,12 @@ grub_efidisk_open (const char *name, struct grub_disk *disk)
   switch (name[0])
     {
     case 'f':
-      disk->has_partitions = 0;
       d = get_device (fd_devices, num);
       break;
     case 'c':
-      /* FIXME: a CDROM should have partitions, but not implemented yet.  */
-      disk->has_partitions = 0;
       d = get_device (cd_devices, num);
       break;
     case 'h':
-      disk->has_partitions = 1;
       d = get_device (hd_devices, num);
       break;
     default:
diff --git a/disk/fs_file.c b/disk/fs_file.c
index e095682..19dabef 100644
--- a/disk/fs_file.c
+++ b/disk/fs_file.c
@@ -76,7 +76,6 @@ grub_fs_file_open (const char *name, grub_disk_t disk)
     return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "no matching file found");
 
   disk->total_sectors = dev->disk->total_sectors;
-  disk->has_partitions = 0;
   if (dev->disk->partition)
     {
       disk->partition = grub_malloc (sizeof (*disk->partition));
diff --git a/disk/fs_uuid.c b/disk/fs_uuid.c
index 6901dba..aabebdf 100644
--- a/disk/fs_uuid.c
+++ b/disk/fs_uuid.c
@@ -88,7 +88,6 @@ grub_fs_uuid_open (const char *name, grub_disk_t disk)
     return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "no matching UUID found");
 
   disk->total_sectors = dev->disk->total_sectors;
-  disk->has_partitions = 0;
   if (dev->disk->partition)
     {
       disk->partition = grub_malloc (sizeof (*disk->partition));
diff --git a/disk/host.c b/disk/host.c
index c4f3e71..c519662 100644
--- a/disk/host.c
+++ b/disk/host.c
@@ -43,7 +43,6 @@ grub_host_open (const char *name, grub_disk_t disk)
   disk->total_sectors = 0;
   disk->id = (unsigned long) "host";
 
-  disk->has_partitions = 0;
   disk->data = 0;
 
   return GRUB_ERR_NONE;
diff --git a/disk/i386/pc/biosdisk.c b/disk/i386/pc/biosdisk.c
index 0a6137f..115e2c1 100644
--- a/disk/i386/pc/biosdisk.c
+++ b/disk/i386/pc/biosdisk.c
@@ -106,7 +106,6 @@ grub_biosdisk_open (const char *name, grub_disk_t disk)
   if (drive < 0)
     return grub_errno;
 
-  disk->has_partitions = ((drive & 0x80) && (drive != cd_drive));
   disk->id = drive;
 
   data = (struct grub_biosdisk_data *) grub_zalloc (sizeof (*data));
diff --git a/disk/ieee1275/nand.c b/disk/ieee1275/nand.c
index 37427f8..9d96d51 100644
--- a/disk/ieee1275/nand.c
+++ b/disk/ieee1275/nand.c
@@ -113,7 +113,6 @@ grub_nand_open (const char *name, grub_disk_t disk)
 
   disk->id = dev_ihandle;
 
-  disk->has_partitions = 0;
   disk->data = data;
 
   return 0;
diff --git a/disk/ieee1275/ofdisk.c b/disk/ieee1275/ofdisk.c
index ca257d6..e749259 100644
--- a/disk/ieee1275/ofdisk.c
+++ b/disk/ieee1275/ofdisk.c
@@ -208,8 +208,6 @@ grub_ofdisk_open (const char *name, grub_disk_t disk)
 
   disk->id = (unsigned long) op;
 
-  /* XXX: Read this, somehow.  */
-  disk->has_partitions = 1;
   disk->data = (void *) (unsigned long) dev_ihandle;
   return 0;
 
diff --git a/disk/loopback.c b/disk/loopback.c
index 2980518..d98dbb2 100644
--- a/disk/loopback.c
+++ b/disk/loopback.c
@@ -28,7 +28,6 @@ struct grub_loopback
 {
   char *devname;
   char *filename;
-  int has_partitions;
   struct grub_loopback *next;
 };
 
@@ -37,7 +36,6 @@ static struct grub_loopback *loopback_list;
 static const struct grub_arg_option options[] =
   {
     {"delete", 'd', 0, "delete the loopback device entry", 0, 0},
-    {"partitions", 'p', 0, "simulate a hard drive with partitions", 0, 0},
     {0, 0, 0, 0, 0, 0}
   };
 
@@ -107,9 +105,6 @@ grub_cmd_loopback (grub_extcmd_t cmd, int argc, char **args)
       grub_free (newdev->filename);
       newdev->filename = newname;
 
-      /* Set has_partitions when `--partitions' was used.  */
-      newdev->has_partitions = state[1].set;
-
       return 0;
     }
 
@@ -133,9 +128,6 @@ grub_cmd_loopback (grub_extcmd_t cmd, int argc, char **args)
       return grub_errno;
     }
 
-  /* Set has_partitions when `--partitions' was used.  */
-  newdev->has_partitions = state[1].set;
-
   /* Add the new entry to the list.  */
   newdev->next = loopback_list;
   loopback_list = newdev;
@@ -178,7 +170,6 @@ grub_loopback_open (const char *name, grub_disk_t disk)
 			 / GRUB_DISK_SECTOR_SIZE);
   disk->id = (unsigned long) dev;
 
-  disk->has_partitions = dev->has_partitions;
   disk->data = file;
 
   return 0;
@@ -245,7 +236,7 @@ GRUB_MOD_INIT(loop)
 {
   cmd = grub_register_extcmd ("loopback", grub_cmd_loopback,
 			      GRUB_COMMAND_FLAG_BOTH,
-			      "loopback [-d|-p] DEVICENAME FILE",
+			      "loopback [-d] DEVICENAME FILE",
 			      "Make a device of a file.", options);
   grub_disk_dev_register (&grub_loopback_dev);
 }
diff --git a/disk/lvm.c b/disk/lvm.c
index 6707a40..f3c153b 100644
--- a/disk/lvm.c
+++ b/disk/lvm.c
@@ -97,7 +97,6 @@ grub_lvm_open (const char *name, grub_disk_t disk)
   if (! lv)
     return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "Unknown LVM device %s", name);
 
-  disk->has_partitions = 0;
   disk->id = lv->number;
   disk->data = lv;
   disk->total_sectors = lv->size;
diff --git a/disk/memdisk.c b/disk/memdisk.c
index 4a04708..d93752f 100644
--- a/disk/memdisk.c
+++ b/disk/memdisk.c
@@ -42,7 +42,6 @@ grub_memdisk_open (const char *name, grub_disk_t disk)
 
   disk->total_sectors = memdisk_size / GRUB_DISK_SECTOR_SIZE;
   disk->id = (unsigned long) "mdsk";
-  disk->has_partitions = 0;
 
   return GRUB_ERR_NONE;
 }
diff --git a/disk/raid.c b/disk/raid.c
index c4d0857..f9aaf28 100644
--- a/disk/raid.c
+++ b/disk/raid.c
@@ -126,7 +126,6 @@ grub_raid_open (const char *name, grub_disk_t disk)
     return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "Unknown RAID device %s",
                        name);
 
-  disk->has_partitions = 1;
   disk->id = array->number;
   disk->data = array;
 
diff --git a/disk/scsi.c b/disk/scsi.c
index 24ebdb6..85d53b3 100644
--- a/disk/scsi.c
+++ b/disk/scsi.c
@@ -283,11 +283,6 @@ grub_scsi_open (const char *name, grub_disk_t disk)
 			     "unknown SCSI device");
 	}
 
-      if (scsi->devtype == grub_scsi_devtype_cdrom)
-	disk->has_partitions = 0;
-      else
-	disk->has_partitions = 1;
-
       err = grub_scsi_read_capacity (scsi);
       if (err)
 	{
diff --git a/fs/i386/pc/pxe.c b/fs/i386/pc/pxe.c
index 4032e12..1a99ad4 100644
--- a/fs/i386/pc/pxe.c
+++ b/fs/i386/pc/pxe.c
@@ -65,7 +65,6 @@ grub_pxe_open (const char *name, grub_disk_t disk)
   disk->total_sectors = 0;
   disk->id = (unsigned long) "pxe";
 
-  disk->has_partitions = 0;
   disk->data = 0;
 
   return GRUB_ERR_NONE;
diff --git a/include/grub/disk.h b/include/grub/disk.h
index de71bb5..31d0e1a 100644
--- a/include/grub/disk.h
+++ b/include/grub/disk.h
@@ -98,9 +98,6 @@ struct grub_disk
   /* The total number of sectors.  */
   grub_uint64_t total_sectors;
 
-  /* If partitions can be stored.  */
-  int has_partitions;
-
   /* The id used by the disk cache manager.  */
   unsigned long id;
 
diff --git a/kern/device.c b/kern/device.c
index 83ae3dc..b9c340e 100644
--- a/kern/device.c
+++ b/kern/device.c
@@ -100,7 +100,7 @@ grub_device_iterate (int (*hook) (const char *name))
       if (! dev)
 	return 0;
 
-      if (dev->disk && dev->disk->has_partitions)
+      if (dev->disk)
 	{
 	  struct part_ent *p;
 	  int ret = 0;
diff --git a/kern/disk.c b/kern/disk.c
index e463626..5fe1bcf 100644
--- a/kern/disk.c
+++ b/kern/disk.c
@@ -281,12 +281,6 @@ grub_disk_open (const char *name)
       goto fail;
     }
 
-  if (p && ! disk->has_partitions)
-    {
-      grub_error (GRUB_ERR_BAD_DEVICE, "no partition on this disk");
-      goto fail;
-    }
-
   disk->dev = dev;
 
   if (p)
diff --git a/normal/completion.c b/normal/completion.c
index 4b38e33..405976a 100644
--- a/normal/completion.c
+++ b/normal/completion.c
@@ -160,18 +160,9 @@ iterate_dev (const char *devname)
   dev = grub_device_open (devname);
 
   if (dev)
-    {
-      if (dev->disk && dev->disk->has_partitions)
-	{
-	  if (add_completion (devname, ",", GRUB_COMPLETION_TYPE_DEVICE))
-	    return 1;
-	}
-      else
-	{
-	  if (add_completion (devname, ")", GRUB_COMPLETION_TYPE_DEVICE))
-	    return 1;
-	}
-    }
+    if (add_completion (devname, ",", GRUB_COMPLETION_TYPE_DEVICE)
+	|| add_completion (devname, ")", GRUB_COMPLETION_TYPE_DEVICE))
+      return 1;
 
   grub_errno = GRUB_ERR_NONE;
   return 0;
@@ -216,7 +207,7 @@ complete_device (void)
 
       if (dev)
 	{
-	  if (dev->disk && dev->disk->has_partitions)
+	  if (dev->disk)
 	    {
 	      if (grub_partition_iterate (dev->disk, iterate_partition))
 		{
diff --git a/normal/misc.c b/normal/misc.c
index 0a1a2f0..cddd1d3 100644
--- a/normal/misc.c
+++ b/normal/misc.c
@@ -94,10 +94,8 @@ grub_normal_print_device_info (const char *name)
 	      grub_errno = GRUB_ERR_NONE;
 	    }
 	}
-      else if (! dev->disk->has_partitions || dev->disk->partition)
-	grub_printf ("Unknown filesystem");
       else
-	grub_printf ("Partition table");
+	grub_printf ("Unknown filesystem");
 
       grub_device_close (dev);
     }
diff --git a/util/grub-fstest.c b/util/grub-fstest.c
index 4722269..ddb2024 100644
--- a/util/grub-fstest.c
+++ b/util/grub-fstest.c
@@ -278,7 +278,7 @@ fstest (char **images, int num_disks, int cmd, int n, char **args)
 {
   char host_file[128];
   char loop_name[8];
-  char *argv[3] = { "-p", loop_name, host_file};
+  char *argv[2] = { loop_name, host_file};
   int i;
 
   for (i = 0; i < num_disks; i++)
@@ -289,7 +289,7 @@ fstest (char **images, int num_disks, int cmd, int n, char **args)
       grub_sprintf (loop_name, "loop%d", i);
       grub_sprintf (host_file, "(host)%s", images[i]);
 
-      if (execute_command ("loopback", 3, argv))
+      if (execute_command ("loopback", 2, argv))
         grub_util_error ("loopback command fails.");
     }
 
diff --git a/util/hostdisk.c b/util/hostdisk.c
index 5842698..0740da1 100644
--- a/util/hostdisk.c
+++ b/util/hostdisk.c
@@ -170,7 +170,6 @@ grub_util_biosdisk_open (const char *name, grub_disk_t disk)
     return grub_error (GRUB_ERR_BAD_DEVICE,
 		       "no mapping exists for `%s'", name);
 
-  disk->has_partitions = 1;
   disk->id = drive;
 
   /* Get the size.  */
diff --git a/util/i386/pc/grub-setup.c b/util/i386/pc/grub-setup.c
index 852b498..9ec1faa 100644
--- a/util/i386/pc/grub-setup.c
+++ b/util/i386/pc/grub-setup.c
@@ -256,14 +256,6 @@ setup (const char *dir,
 	  tmp_img + GRUB_BOOT_MACHINE_BPB_START,
 	  GRUB_BOOT_MACHINE_BPB_END - GRUB_BOOT_MACHINE_BPB_START);
 
-  /* Copy the possible partition table.  */
-  if (dest_dev->disk->has_partitions)
-    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);
-
-  free (tmp_img);
-
   /* If DEST_DRIVE is a hard disk, enable the workaround, which is
      for buggy BIOSes which don't pass boot drive correctly. Instead,
      they pass 0x00 or 0x01 even when booted from 0x80.  */
@@ -307,12 +299,6 @@ setup (const char *dir,
   grub_util_info ("dos partition is %d, bsd partition is %d",
 		  dos_part, bsd_part);
 
-  if (! dest_dev->disk->has_partitions)
-    {
-      grub_util_warn ("Attempting to install GRUB to a partitionless disk.  This is a BAD idea.");
-      goto unable_to_embed;
-    }
-
   if (dest_dev->disk->partition)
     {
       grub_util_warn ("Attempting to install GRUB to a partition instead of the MBR.  This is a BAD idea.");
@@ -338,6 +324,14 @@ setup (const char *dir,
       goto unable_to_embed;
     }
 
+  /* 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);
+
+  free (tmp_img);
+
   grub_partition_iterate (dest_dev->disk, (strcmp (dest_partmap, "pc_partition_map") ?
 					   find_usable_region_gpt : find_usable_region_msdos));
 

Re: [PATCH] Fix when installing on pationless but partionable medium

[Robert Millan]

On Sun, Jul 26, 2009 at 03:54:41PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> +      for (i = 0; i < 4; i++)
> +	if (mbr.entries[i].flag & 0x7f)
> +	  break;
> +
> +      if (i != 4)
> +	return grub_error (GRUB_ERR_BAD_PART_TABLE, "bad boot flag");

Why not just return directly?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] Fix when installing on pationless but partionable medium

[Vladimir 'phcoder' Serbinenko]

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

On Tue, Jul 28, 2009 at 7:33 PM, Robert Millan<rmh@aybabtu.com> wrote:
> On Sun, Jul 26, 2009 at 03:54:41PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>> +      for (i = 0; i < 4; i++)
>> +     if (mbr.entries[i].flag & 0x7f)
>> +       break;
>> +
>> +      if (i != 4)
>> +     return grub_error (GRUB_ERR_BAD_PART_TABLE, "bad boot flag");
>
> Why not just return directly?
>
Force of canned code written by DMA from memory to keyboard bypassing brain ;)
> --
> Robert Millan
>
>  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
>  how) you may access your data; but nobody's threatening your freedom: we
>  still allow you to remove your data and not access it at all."
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

[-- Attachment #2: mbr.diff --]
[-- Type: text/plain, Size: 1005 bytes --]

diff --git a/ChangeLog b/ChangeLog
index 23c288e..74b9247 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 2009-07-28  Vladimir Serbinenko  <phcoder@gmail.com>
 
+	* partmap/pc.c (pc_partition_map_iterate): Check that boot flags are
+	valid.
+
+2009-07-28  Vladimir Serbinenko  <phcoder@gmail.com>
+
 	* loader/i386/multiboot_helper.S (grub_multiboot_backward_relocator):
 	Clear direction flag before jumping to OS.
 	(grub_multiboot2_real_boot): Likewise.
diff --git a/partmap/pc.c b/partmap/pc.c
index 6f68ecf..6a2efd2 100644
--- a/partmap/pc.c
+++ b/partmap/pc.c
@@ -121,6 +121,10 @@ pc_partition_map_iterate (grub_disk_t disk,
       if (mbr.signature != grub_cpu_to_le16 (GRUB_PC_PARTITION_SIGNATURE))
 	return grub_error (GRUB_ERR_BAD_PART_TABLE, "no signature");
 
+      for (i = 0; i < 4; i++)
+	if (mbr.entries[i].flag & 0x7f)
+	  return grub_error (GRUB_ERR_BAD_PART_TABLE, "bad boot flag");
+
       /* Analyze DOS partitions.  */
       for (p.index = 0; p.index < 4; p.index++)
 	{

Re: [PATCH] Fix when installing on pationless but partionable medium

[Robert Millan]

On Tue, Jul 28, 2009 at 11:42:07PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> diff --git a/ChangeLog b/ChangeLog
> index 23c288e..74b9247 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,10 @@
>  2009-07-28  Vladimir Serbinenko  <phcoder@gmail.com>
>  
> +	* partmap/pc.c (pc_partition_map_iterate): Check that boot flags are
> +	valid.
> +
> +2009-07-28  Vladimir Serbinenko  <phcoder@gmail.com>
> +
>  	* loader/i386/multiboot_helper.S (grub_multiboot_backward_relocator):
>  	Clear direction flag before jumping to OS.
>  	(grub_multiboot2_real_boot): Likewise.
> diff --git a/partmap/pc.c b/partmap/pc.c
> index 6f68ecf..6a2efd2 100644
> --- a/partmap/pc.c
> +++ b/partmap/pc.c
> @@ -121,6 +121,10 @@ pc_partition_map_iterate (grub_disk_t disk,
>        if (mbr.signature != grub_cpu_to_le16 (GRUB_PC_PARTITION_SIGNATURE))
>  	return grub_error (GRUB_ERR_BAD_PART_TABLE, "no signature");
>  
> +      for (i = 0; i < 4; i++)
> +	if (mbr.entries[i].flag & 0x7f)
> +	  return grub_error (GRUB_ERR_BAD_PART_TABLE, "bad boot flag");
> +
>        /* Analyze DOS partitions.  */
>        for (p.index = 0; p.index < 4; p.index++)
>  	{

Looks fine to me.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] Fix when installing on pationless but partionable medium

[Colin Watson]

On Sat, Jul 18, 2009 at 09:22:11PM +0200, Robert Millan wrote:
> On Sat, Jul 18, 2009 at 09:00:36PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> > On Sat, Jul 18, 2009 at 8:42 PM, Robert Millan<rmh@aybabtu.com> wrote:
> > > I'm not sure there's much we can do about this.  Using heuristics sounds like
> > > it will make the solution worse than the problem.  I don't care much about
> > > Microsoft filesystems, but I'd hate to see GRUB fail on a completely sane
> > > ext3 inside msdos label because it happened to look like FAT in raw disk at
> > > the same time.
> > 
> > The approach proposed by Collin avoids such problems since correct
> > pc_partition_map is always detected as such.
> 
> I haven't looked at the source code, but what he said is we can determine if
> an MBR is valid by checking the bootable flag, and this is not always so.

If the bootable flag is neither 0 nor 0x80, then neither libparted nor
the Linux kernel will understand it as a DOS partition table. Is it
really all that helpful for GRUB to attempt to do so?

I've never heard of false positives with the libparted/Linux checks. Do
you have real-world examples of them failing?

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] Fix when installing on pationless but partionable medium

[Robert Millan]

On Sun, Jul 19, 2009 at 11:02:05AM +0100, Colin Watson wrote:
> On Sat, Jul 18, 2009 at 09:22:11PM +0200, Robert Millan wrote:
> > On Sat, Jul 18, 2009 at 09:00:36PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> > > On Sat, Jul 18, 2009 at 8:42 PM, Robert Millan<rmh@aybabtu.com> wrote:
> > > > I'm not sure there's much we can do about this.  Using heuristics sounds like
> > > > it will make the solution worse than the problem.  I don't care much about
> > > > Microsoft filesystems, but I'd hate to see GRUB fail on a completely sane
> > > > ext3 inside msdos label because it happened to look like FAT in raw disk at
> > > > the same time.
> > > 
> > > The approach proposed by Collin avoids such problems since correct
> > > pc_partition_map is always detected as such.
> > 
> > I haven't looked at the source code, but what he said is we can determine if
> > an MBR is valid by checking the bootable flag, and this is not always so.
> 
> If the bootable flag is neither 0 nor 0x80, then neither libparted nor
> the Linux kernel will understand it as a DOS partition table. Is it
> really all that helpful for GRUB to attempt to do so?

No.  Sorry, I miss-read what you said before.  See my other mail.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."