Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 218599 - sys-boot/grub-0.97-r5: setup_boot_dir running grub.conf commands is dangerous
Summary: sys-boot/grub-0.97-r5: setup_boot_dir running grub.conf commands is dangerous
Status: RESOLVED FIXED
Alias: None
Product: Gentoo Linux
Classification: Unclassified
Component: [OLD] Core system (show other bugs)
Hardware: All Linux
: High normal (vote)
Assignee: Gentoo's Team for Core System packages
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-04-20 16:04 UTC by Martin von Gagern
Modified: 2008-06-04 20:44 UTC (History)
3 users (show)

See Also:
Package list:
Runtime testing required: ---


Attachments
Sane automatic installation (bug218599a.patch,4.13 KB, patch)
2008-04-21 14:41 UTC, Martin von Gagern
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin von Gagern 2008-04-20 16:04:49 UTC
In current grub ebuilds, there is a section in setup_boot_dir that greps most commands from grub.conf, the grub boot menu file, and unconditionally executes them with the grub shell. I consider this dangerous and harmful.

Suppose you have a menu entry to setup grub on a floppy disk, and while emerging grub you accidentially have a floppy with some valuable data inserted? Or maybe your device maps don't fit, you have a menu entry for installing grub, and consequently grub overwrites the boot sector of the wrong drive?

You might allow for automatic reinstallation of grub on update, but for this, you should introduce a new config file, and not reuse grub.conf.
Comment 1 Panagiotis Christopoulos (RETIRED) gentoo-dev 2008-04-20 16:42:38 UTC
You have a point but there is a DONT_MOUNT_BOOT variable. When it is non-zero, setup_boot_dir function will not be executed.

echo DONT_MOUNT_BOOT=\"yes\" >> /etc/make.conf

and you will be fine, i think

Panagiotis (pchrist)
Comment 2 Martin von Gagern 2008-04-20 17:35:28 UTC
(In reply to comment #1)
> You have a point but there is a DONT_MOUNT_BOOT variable. When it is non-zero,
> setup_boot_dir function will not be executed.

Interesting point. Generally, I'd think not running grub with these commands should probably be the default, to play it safe. Personally, I would want to have the grub files copied to the boot directory, which would be prevented as well by setting that variable.

But you could combine that variable and the auto-install commands file in an interesting way: introduce some "${ROOT}"/etc/grub-install.conf or "${ROOT}"/etc/grub.install or whatever file, and if it exists, mount boot and run that file thorugh grub shell, and if it doesn't, log some useful message instead, mentioning that possibility.
Comment 3 Robin Johnson archtester Gentoo Infrastructure gentoo-dev Security 2008-04-20 19:33:06 UTC
The danger of not running the setup_boot_dir stuff is that if grub is updated, and stagefiles change, the /boot copy won't get updated, which in some cases can lead to an unbootable box.

If we do change this, we need to make sure that the handbook docs match, and that every user is aware of it, so that they always run 'emerge --config sys-boot/grub' after an update.
Comment 4 Martin von Gagern 2008-04-20 19:50:41 UTC
(In reply to comment #3)
> The danger of not running the setup_boot_dir stuff is that if grub is updated,
> and stagefiles change, the /boot copy won't get updated, which in some cases
> can lead to an unbootable box.

If you don't run this whole setup_boot_dir stuff, then the stages won't be overridden, but that just amounts to DONT_MOUNT_BOOT=yes, I guess. So the problem is copying the stages (in setup_boot_dir) but not installing them. The current approach will not always help against this, as my own bug #98768 prooves. I have no setup or install command in my boot menu. By the way, that's a condition that should be pretty easy to check, and might be useful for migration.

> If we do change this, we need to make sure that the handbook docs match, and
> that every user is aware of it, so that they always run 'emerge --config
> sys-boot/grub' after an update.

How about this:
1. Provide a install config file as part of the ebuild
2. Have that file contain "setup (hd0)" and/or "setup (hd0,0)" as comments, along with description
3. Have the ebuild check that file contains either setup or install outside a comment, if so run them (stripping all comments), otherwise ewarn the user to update the file or install grub manually, including copying of the stage files.
4. Update the manual to mention this file

Migration path: file is default, admin gets ewarn, updates file, runs emerge --config sys-boot/grub as requested by ewarn, and everything should be fine.

New users path: users read manual, learn about file, edit it, run emerge --config sys-boot/grub as (then) described in manual, and everything should be fine.

Update path: ebuild checks file, notices it's been worked on, automatically copies stages and executes commands from config file, thus updating automatically.
Comment 5 nm (RETIRED) gentoo-dev 2008-04-20 20:24:26 UTC
I'd prefer to keep the behavior of grub and what's mentioned in the handbooks as-is. It's worked for all our users up to this point. This bug describes a theoretical edge case.

If you leave in a floppy you didn't want overwritten, well, tough luck. Same goes for rm -rf / -- changing documentation to force an additional step that's never been needed increases the chances of user error, not decreases.

One of the nice things about using grub is that you don't have to touch it after an update. Forcing --config after updating makes it little better than lilo.
Comment 6 Jan Kundrát (RETIRED) gentoo-dev 2008-04-20 21:17:16 UTC
At a quick glance, executing all lines concerning "root" or "install" in grub.conf doesn't look like an excellent idea, but forcing the user to perform some manuall steps wouldn't be really user-friendly, either.

I guess that executing all commands before first "title" line won't work in following case:

title kernel
root (hd6,0)
kernel /linux
Comment 7 Martin von Gagern 2008-04-20 22:06:00 UTC
(In reply to comment #5)
> I'd prefer to keep the behavior of grub and what's mentioned in the handbooks
> as-is. It's worked for all our users up to this point.

How so? http://www.gentoo.org/doc/en/handbook/handbook-x86.xml?part=1&chap=10#doc_chap2 doesn't give any indication that install commands should be included in the menu. It does, on the other hand, describe ways to install grub manually on first run. So I'd be surprised if automatic reinstalation would work for even a majority of users.

> This bug describes a theoretical edge case.
> If you leave in a floppy you didn't want overwritten, well, tough luck. Same
> goes for rm -rf /

That's an invalid comparison. If I type the rm command, I can expect it to do what it will do. If I run an emerge -u, I will only expect it to update my software, not to wipe my floppies. So if the floppy get's wiped, I'd not blame the users here, but the ebuild.

> One of the nice things about using grub is that you don't have to touch it
> after an update.

On the contrary, I have to make damn sure to configure it at least afer every second update, or I'll get bit by bug #98768.

> Forcing --config after updating makes it little better than lilo.

I wasn't suggesting that. If users create a file describing how to install grub, then the ebuild can and should do so automatically. That's waht I described as "Update path" in comment #4. I merely don't want these commands in the boot menu, as the install step doesn't belong in the menu, and the menu commands don't belong with the install process.

(In reply to comment #6)
> At a quick glance, executing all lines concerning "root" or "install" in
> grub.conf doesn't look like an excellent idea, but forcing the user to perform
> some manuall steps wouldn't be really user-friendly, either.

You definitely want manual steps at first install, at least show the users the defaults you would use and ask him to confirm. Otherwise Gentoo would be worse than Windows. If you remember these steps, you can automatically execute them on update.

> I guess that executing all commands before first "title" line won't work in
> following case:

Before the first title? The current ebuild would execute commands from al places, stripping title among others.

> title kernel
> root (hd6,0)
> kernel /linux

Would do exactly nothing, as there is neither setup nor install command. That's pretty much the most simple scenario where automatic update will fail even without a floppy in the drive. I'd consider it pretty common.
Comment 8 SpanKY gentoo-dev 2008-04-21 01:27:21 UTC
here's a generalization that i'm sure the docs team will agree with: people dont read.  the updating of the boot loader needs to be automatic and safe.  if the only unsafe condition you can propose is the highly uncommon (and unlikely) scenario where people have lines in their grub.conf to boot/setup a floppy disc, and they happen to have a floppy disc in their drive at the time of the emerge, and the mbr may get clobbered, then that's tough love for you.
Comment 9 nm (RETIRED) gentoo-dev 2008-04-21 04:36:31 UTC
(In reply to comment #8)
> here's a generalization that i'm sure the docs team will agree with: people
> dont read.  the updating of the boot loader needs to be automatic and safe.  if
> the only unsafe condition you can propose is the highly uncommon (and unlikely)
> scenario where people have lines in their grub.conf to boot/setup a floppy
> disc, and they happen to have a floppy disc in their drive at the time of the
> emerge, and the mbr may get clobbered, then that's tough love for you.

Exactly. Un-CCing the GDP, as there's nothing for us to do here.
Comment 10 Martin von Gagern 2008-04-21 14:41:08 UTC
Created attachment 150501 [details, diff]
Sane automatic installation

This is my first shot at a patch to illustrate my suggestions.
Let's see what's new in here.

1. introduced grub.install, with full path in $instcfg. For the time being, I placed that in /etc, but one might as well put it in $dir/grub. Up to you.

2. Demoted warning about moved menu.lst, because there are more important notes in the new ebuild, and calling every message important is kind of pointless.

3. Create grub.install if it does not exist. Right now, its contents is included in setup_boot_dir within the ebuild, but it might be moved to $FILESDIR, and it might be installed in the install phase and thus be merged with full config protection. This would make automatic cutsomization of that file difficult, though.

4. If grub.conf exists and contains setup or install commands, these are appended to grub.install. The sed script basically takes all menu entries with a setup or install in them, so we don't get all those commands used to actually boot some system. From the general commands before the first title, only the root command is included, I'd guess that should be enough.
If you are worried about this whole sed script, you may use the old egrep command instead, but I'd prefer the sed, as it is more restrictive in what it takes, and thus closer to my original intent, while it still should work.

5. If there are setup instructions, either freshly extracted from grub.conf, or installed from a previous build and probably edited by the admin, then execute these commands in order to automatically update grub, even its stage 1.

6. Do some error checking. The return calue from the grub shell seems to be no indication of errors. At least for me, a "setup (hd0,0)" without prior root caused an error and still resulted in a zero return value. Therefore I redirected the whole output from the grub shell to a temporary file, and grepped for error messages. The return value would still be useful in case of signals.

7. If there are no automatic update commands, tell the user to update manually. Also, if this is an update, warn him especially that failure to update might result in an unbootable system - my favorite bug #98768. I changed the wording somewhat, as you don't have to install into the MBR but can install into a partiton boot sector, and what stage2 gets executed depends on if you have a stage1.5, i.e. if you refer to the stage2 by block list or file name.

On the whole, this should give you
- No warnings or other log entries for successful automatic updates
- More accurate messages when there is something to be done
- More information when an error occurs
- An independent file for the install instructions
- Automatic migration, expected without lost functionality or extra steps
- A pretty sane subset of commands to be migrated

Did some testing, with and without grub.install present, with and without a setup command in my grub.conf. So far, everything looks good. Comments?
Comment 11 Robin Johnson archtester Gentoo Infrastructure gentoo-dev Security 2008-05-02 19:56:44 UTC
I'm going to consider this later for r6 or r7.
Just because I don't have time at the moment (going to Africa, back in 2 weeks).
Comment 12 Sven 2008-05-07 02:22:07 UTC
My comments about this:

- Updated my grub today
- Didn't have any problems, because i don't have a setup-command in my grub.conf (so grub actually does nothing, right?)
- So i have added a proper root- and setup-command to my grub.conf to see what happens.
- I was REALLY pissed to see, that i see nothing, because grub output is piped to /dev/null and nobody checks if grub has failed or not or if there was a setup-command or not.
- Is there any standard way to store somewhere, where grub should be installed? So there is a program called grub-install, but it doesn't have a config file and doesn't use grub.conf. Instead, it takes a device name as a parameter. Where is a program/script, that actually uses the configuration from grub.conf to (re)install grub? (If i configured it somewhere, i'd like to be abled to easily execute it)
- In which device's bootsector grub is installed, might be crucial for future support of hibernate, altering the boot-sector/MBR before going to suspend to disk. Any clue, what hibernate might expect in the future?
- What if a grub.conf/grub.install or whatever exists, but i'm actually currently using LILO? The ebuild should check, if grub is really installed. (read 512 from the device, grep for GRUB to test, whether GRUB is installed. Unfortunatly, i don't know a good tool, to analyse the boot sector/MBR and grub doesn't offer any, i think). Of course, the device's name is needed, but grub.conf only supplies something like (hd0,0) which is hard to map back to a device name while grub-install takes a good old plain device name like in "grub-install /dev/sda2"
- It's hard to have a good sane system for managing boot loaders. Don't do anything or do something really good. The current sollution is in between and therefor error prone.

Just my 50 cent.
Comment 13 Martin von Gagern 2008-05-07 07:49:28 UTC
(In reply to comment #12)
> - In which device's bootsector grub is installed, might be crucial for future
> support of hibernate, altering the boot-sector/MBR before going to suspend to
> disk. Any clue, what hibernate might expect in the future?

I would assume that this would use grub-set-default or maybe scripted manipulation of /boot/grub/default rather than any low-level modification of the boot sector. But as hibernation never worked for me (due to binary drivers), I might be wrong.

> - What if a grub.conf/grub.install or whatever exists, but i'm actually
> currently using LILO?

My suggested patch would log a message if grub.install was comments only and execute the setup/install comments if there were any, not caring about lilo. I would expect people to only edit grub.install when they are using grub. As there are some ugly guesses involved with checking for GRUB, just as you pointed out, I would rather not do this automatically.

> Of course, the device's name is needed, but
> grub.conf only supplies something like (hd0,0) which is hard to map back to a
> device name while grub-install takes a good old plain device name like in
> "grub-install /dev/sda2"

One might use /boot/grub/device.map for the drive, from where access to the partitions should be easier at least. I don't know how far we can trust the contents of that file, but as it is used by the grub shell as well, we'd at least be consistent with the executed setup commands. One can have a look at grub-install to see how this file can be (re)created when it's not present.

> Just my 50 cent.

Thanks!
Comment 14 Robin Johnson archtester Gentoo Infrastructure gentoo-dev Security 2008-06-04 20:44:14 UTC
grub.conf.install implemented.