* [PATCH v2] Support to disable reed-solomon codes
@ 2013-11-25 22:37 Jon McCune
2013-11-26 0:47 ` Vladimir 'φ-coder/phcoder' Serbinenko
0 siblings, 1 reply; 4+ messages in thread
From: Jon McCune @ 2013-11-25 22:37 UTC (permalink / raw)
To: grub-devel; +Cc: Jon McCune
[v0] new grub-*-setup flag to disable insertion of reed solomon codes
[v0] grub-install support for option --no-rs-codes
[v1] bugfix: also change the maxsec value in util/setup.c
[v2] Modified to support new C-language install utilities, added documentation
Signed-off-by: Jon McCune <jonmccune@google.com>
---
docs/grub.texi | 12 +++++++++-
include/grub/util/install.h | 4 ++--
util/grub-install.c | 20 ++++++++++++-----
util/grub-setup.c | 11 ++++++++--
util/setup.c | 53 +++++++++++++++++++++++++--------------------
5 files changed, 66 insertions(+), 34 deletions(-)
diff --git a/docs/grub.texi b/docs/grub.texi
index 6aee292..29547cd 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -5937,8 +5937,18 @@ mounted on
Recheck the device map, even if @file{/boot/grub/device.map} already
exists. You should use this option whenever you add/remove a disk
into/from your computer.
-@end table
+@item --no-rs-codes
+By default, @command{grub-install} will use some extra space in the
+bootloader embedding area for Reed-Solomon error-correcting
+codes. This enables GRUB to still boot successfully if one single
+block is corrupted. This redundancy may be cumbersome if attempting
+to cryptographically validate the contents of the bootloader embedding
+area, or in more modern systems with GPT-style partition tables
+(@pxref{BIOS installation}) where GRUB does not reside in any
+unpartitioned space outside of the MBR. Disable the Reed-Solomon
+codes with this option.
+@end table
@node Invoking grub-mkconfig
@chapter Invoking grub-mkconfig
diff --git a/include/grub/util/install.h b/include/grub/util/install.h
index 5cb33fc..2bfd2ec 100644
--- a/include/grub/util/install.h
+++ b/include/grub/util/install.h
@@ -176,12 +176,12 @@ void
grub_util_bios_setup (const char *dir,
const char *boot_file, const char *core_file,
const char *dest, int force,
- int fs_probe, int allow_floppy);
+ int fs_probe, int allow_floppy, int no_rs_codes);
void
grub_util_sparc_setup (const char *dir,
const char *boot_file, const char *core_file,
const char *dest, int force,
- int fs_probe, int allow_floppy);
+ int fs_probe, int allow_floppy, int no_rs_codes);
char *
grub_install_get_image_targets_string (void);
diff --git a/util/grub-install.c b/util/grub-install.c
index 5354a6d..12d0bc2 100644
--- a/util/grub-install.c
+++ b/util/grub-install.c
@@ -68,6 +68,7 @@ static int have_load_cfg = 0;
static FILE * load_cfg_f = NULL;
static char *load_cfg;
static int install_bootsector = 1;
+static int no_rs_codes = 0;
enum
{
@@ -93,7 +94,8 @@ enum
OPTION_DEBUG_IMAGE,
OPTION_NO_FLOPPY,
OPTION_DISK_MODULE,
- OPTION_NO_BOOTSECTOR
+ OPTION_NO_BOOTSECTOR,
+ OPTION_NO_RS_CODES,
};
static int fs_probe = 1;
@@ -180,6 +182,10 @@ argp_parser (int key, char *arg, struct argp_state *state)
install_bootsector = 0;
return 0;
+ case OPTION_NO_RS_CODES:
+ no_rs_codes = 1;
+ return 0;
+
case OPTION_DEBUG:
verbosity++;
return 0;
@@ -238,6 +244,8 @@ static struct argp_option options[] = {
N_("do not probe for filesystems in DEVICE"), 0},
{"no-bootsector", OPTION_NO_BOOTSECTOR, 0, 0,
N_("do not install bootsector"), 0},
+ {"no-rs-codes", OPTION_NO_RS_CODES, 0, 0,
+ N_("Do not apply any reed-solomon codes when embedding core.img, even if there is enough space."), 0},
{"debug", OPTION_DEBUG, 0, OPTION_HIDDEN, 0, 2},
{"no-floppy", OPTION_NO_FLOPPY, 0, OPTION_HIDDEN, 0, 2},
@@ -1412,12 +1420,13 @@ main (int argc, char *argv[])
"boot.img");
grub_install_copy_file (boot_img_src, boot_img, 1);
- grub_util_info ("%sgrub-bios-setup %s %s %s %s --directory='%s' --device-map='%s' '%s'",
+ grub_util_info ("%sgrub-bios-setup %s %s %s %s %s --directory='%s' --device-map='%s' '%s'",
install_bootsector ? "" : "NOT RUNNING: ",
allow_floppy ? "--allow-floppy " : "",
verbosity ? "--verbose " : "",
force ? "--force " : "",
!fs_probe ? "--skip-fs-probe" : "",
+ no_rs_codes ? "--no-rs-codes" : "",
platdir,
device_map,
install_device);
@@ -1426,7 +1435,7 @@ main (int argc, char *argv[])
if (install_bootsector)
grub_util_bios_setup (platdir, "boot.img", "core.img",
install_drive, force,
- fs_probe, allow_floppy);
+ fs_probe, allow_floppy, no_rs_codes);
break;
}
case GRUB_INSTALL_PLATFORM_SPARC64_IEEE1275:
@@ -1438,12 +1447,13 @@ main (int argc, char *argv[])
"boot.img");
grub_install_copy_file (boot_img_src, boot_img, 1);
- grub_util_info ("%sgrub-sparc64-setup %s %s %s %s --directory='%s' --device-map='%s' '%s'",
+ grub_util_info ("%sgrub-sparc64-setup %s %s %s %s %s --directory='%s' --device-map='%s' '%s'",
install_bootsector ? "" : "NOT RUNNING: ",
allow_floppy ? "--allow-floppy " : "",
verbosity ? "--verbose " : "",
force ? "--force " : "",
!fs_probe ? "--skip-fs-probe" : "",
+ no_rs_codes ? "--no-rs-codes" : "",
platdir,
device_map,
install_drive);
@@ -1452,7 +1462,7 @@ main (int argc, char *argv[])
if (install_bootsector)
grub_util_sparc_setup (platdir, "boot.img", "core.img",
install_device, force,
- fs_probe, allow_floppy);
+ fs_probe, allow_floppy, no_rs_codes);
break;
}
diff --git a/util/grub-setup.c b/util/grub-setup.c
index 90b9de0..41344d2 100644
--- a/util/grub-setup.c
+++ b/util/grub-setup.c
@@ -82,7 +82,8 @@ static struct argp_option options[] = {
/* TRANSLATORS: The potential breakage isn't limited to floppies but it's
likely to make the install unbootable from HDD. */
N_("make the drive also bootable as floppy (default for fdX devices). May break on some BIOSes."), 0},
-
+ {"no-rs-codes", 'n', 0, 0,
+ N_("Do not apply any reed-solomon codes when embedding core.img, even if there is enough space."), 0},
{ 0, 0, 0, 0, 0, 0 }
};
@@ -118,6 +119,7 @@ struct arguments
int fs_probe;
int allow_floppy;
char *device;
+ int no_rs_codes;
};
static error_t
@@ -173,6 +175,10 @@ argp_parser (int key, char *arg, struct argp_state *state)
verbosity++;
break;
+ case 'n':
+ arguments->no_rs_codes = 1;
+ break;
+
case ARGP_KEY_ARG:
if (state->arg_num == 0)
arguments->device = xstrdup(arg);
@@ -292,7 +298,8 @@ main (int argc, char *argv[])
arguments.boot_file ? : DEFAULT_BOOT_FILE,
arguments.core_file ? : DEFAULT_CORE_FILE,
dest_dev, arguments.force,
- arguments.fs_probe, arguments.allow_floppy);
+ arguments.fs_probe, arguments.allow_floppy,
+ arguments.no_rs_codes);
/* Free resources. */
grub_fini_all ();
diff --git a/util/setup.c b/util/setup.c
index 337c304..b60a109 100644
--- a/util/setup.c
+++ b/util/setup.c
@@ -248,7 +248,7 @@ void
SETUP (const char *dir,
const char *boot_file, const char *core_file,
const char *dest, int force,
- int fs_probe, int allow_floppy)
+ int fs_probe, int allow_floppy, int no_rs_codes)
{
char *core_path;
char *boot_img, *core_img, *boot_path;
@@ -486,7 +486,11 @@ SETUP (const char *dir,
nsec = core_sectors;
- maxsec = 2 * core_sectors;
+ if (!no_rs_codes)
+ maxsec = 2 * core_sectors;
+ else
+ maxsec = core_sectors;
+
if (maxsec > ((0x78000 - GRUB_KERNEL_I386_PC_LINK_ADDR)
>> GRUB_DISK_SECTOR_BITS))
maxsec = ((0x78000 - GRUB_KERNEL_I386_PC_LINK_ADDR)
@@ -547,27 +551,29 @@ SETUP (const char *dir,
bl.first_block = (struct grub_boot_blocklist *) (core_img
+ GRUB_DISK_SECTOR_SIZE
- sizeof (*bl.block));
-
- grub_size_t no_rs_length;
- grub_set_unaligned32 ((core_img + GRUB_DISK_SECTOR_SIZE
- + GRUB_KERNEL_I386_PC_REED_SOLOMON_REDUNDANCY),
- grub_host_to_target32 (nsec * GRUB_DISK_SECTOR_SIZE - core_size));
- no_rs_length = grub_target_to_host16
- (grub_get_unaligned16 (core_img
- + GRUB_DISK_SECTOR_SIZE
- + GRUB_KERNEL_I386_PC_NO_REED_SOLOMON_LENGTH));
-
- if (no_rs_length == 0xffff)
- grub_util_error ("%s", _("core.img version mismatch"));
-
- void *tmp = xmalloc (core_size);
- grub_memcpy (tmp, core_img, core_size);
- grub_reed_solomon_add_redundancy (core_img + no_rs_length + GRUB_DISK_SECTOR_SIZE,
- core_size - no_rs_length - GRUB_DISK_SECTOR_SIZE,
- nsec * GRUB_DISK_SECTOR_SIZE
- - core_size);
- assert (grub_memcmp (tmp, core_img, core_size) == 0);
- free (tmp);
+ if (!no_rs_codes)
+ {
+ grub_size_t no_rs_length;
+ grub_set_unaligned32 ((core_img + GRUB_DISK_SECTOR_SIZE
+ + GRUB_KERNEL_I386_PC_REED_SOLOMON_REDUNDANCY),
+ grub_host_to_target32 (nsec * GRUB_DISK_SECTOR_SIZE - core_size));
+ no_rs_length = grub_target_to_host16
+ (grub_get_unaligned16 (core_img
+ + GRUB_DISK_SECTOR_SIZE
+ + GRUB_KERNEL_I386_PC_NO_REED_SOLOMON_LENGTH));
+
+ if (no_rs_length == 0xffff)
+ grub_util_error ("%s", _("core.img version mismatch"));
+
+ void *tmp = xmalloc (core_size);
+ grub_memcpy (tmp, core_img, core_size);
+ grub_reed_solomon_add_redundancy (core_img + no_rs_length + GRUB_DISK_SECTOR_SIZE,
+ core_size - no_rs_length - GRUB_DISK_SECTOR_SIZE,
+ nsec * GRUB_DISK_SECTOR_SIZE
+ - core_size);
+ assert (grub_memcmp (tmp, core_img, core_size) == 0);
+ free (tmp);
+ }
/* Write the core image onto the disk. */
for (i = 0; i < nsec; i++)
@@ -762,4 +768,3 @@ unable_to_embed:
grub_device_close (dest_dev);
grub_device_close (root_dev);
}
-
--
1.8.4.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v2] Support to disable reed-solomon codes
2013-11-25 22:37 [PATCH v2] Support to disable reed-solomon codes Jon McCune
@ 2013-11-26 0:47 ` Vladimir 'φ-coder/phcoder' Serbinenko
2013-11-26 15:48 ` Jonathan McCune
0 siblings, 1 reply; 4+ messages in thread
From: Vladimir 'φ-coder/phcoder' Serbinenko @ 2013-11-26 0:47 UTC (permalink / raw)
To: The development of GNU GRUB
[-- Attachment #1: Type: text/plain, Size: 11301 bytes --]
On 25.11.2013 23:37, Jon McCune wrote:
> [v0] new grub-*-setup flag to disable insertion of reed solomon codes
> [v0] grub-install support for option --no-rs-codes
> [v1] bugfix: also change the maxsec value in util/setup.c
> [v2] Modified to support new C-language install utilities, added documentation
>
> Signed-off-by: Jon McCune <jonmccune@google.com>
> ---
> docs/grub.texi | 12 +++++++++-
> include/grub/util/install.h | 4 ++--
> util/grub-install.c | 20 ++++++++++++-----
> util/grub-setup.c | 11 ++++++++--
> util/setup.c | 53 +++++++++++++++++++++++++--------------------
> 5 files changed, 66 insertions(+), 34 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 6aee292..29547cd 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -5937,8 +5937,18 @@ mounted on
> Recheck the device map, even if @file{/boot/grub/device.map} already
> exists. You should use this option whenever you add/remove a disk
> into/from your computer.
> -@end table
>
> +@item --no-rs-codes
> +By default, @command{grub-install} will use some extra space in the
It must be clear that you speak about BIOS only.
> +bootloader embedding area for Reed-Solomon error-correcting
> +codes. This enables GRUB to still boot successfully if one single
> +block is corrupted.
We can survive more than one defective sector. If we have r sectors of
redundancy we can survive up to r/2 corrupted ones
> This redundancy may be cumbersome if attempting
> +to cryptographically validate the contents of the bootloader embedding
> +area, or in more modern systems with GPT-style partition tables
> +(@pxref{BIOS installation}) where GRUB does not reside in any
> +unpartitioned space outside of the MBR. Disable the Reed-Solomon
What's the reasonning behind GPT part?
> +codes with this option.
> +@end table
>
> @node Invoking grub-mkconfig
> @chapter Invoking grub-mkconfig
> diff --git a/include/grub/util/install.h b/include/grub/util/install.h
> index 5cb33fc..2bfd2ec 100644
> --- a/include/grub/util/install.h
> +++ b/include/grub/util/install.h
> @@ -176,12 +176,12 @@ void
> grub_util_bios_setup (const char *dir,
> const char *boot_file, const char *core_file,
> const char *dest, int force,
> - int fs_probe, int allow_floppy);
> + int fs_probe, int allow_floppy, int no_rs_codes);
> void
> grub_util_sparc_setup (const char *dir,
> const char *boot_file, const char *core_file,
> const char *dest, int force,
> - int fs_probe, int allow_floppy);
> + int fs_probe, int allow_floppy, int no_rs_codes);
>
> char *
> grub_install_get_image_targets_string (void);
> diff --git a/util/grub-install.c b/util/grub-install.c
> index 5354a6d..12d0bc2 100644
> --- a/util/grub-install.c
> +++ b/util/grub-install.c
> @@ -68,6 +68,7 @@ static int have_load_cfg = 0;
> static FILE * load_cfg_f = NULL;
> static char *load_cfg;
> static int install_bootsector = 1;
> +static int no_rs_codes = 0;
>
Negative logic variables are unreadable and prone to errors. Could you
use positive logic?
> enum
> {
> @@ -93,7 +94,8 @@ enum
> OPTION_DEBUG_IMAGE,
> OPTION_NO_FLOPPY,
> OPTION_DISK_MODULE,
> - OPTION_NO_BOOTSECTOR
> + OPTION_NO_BOOTSECTOR,
> + OPTION_NO_RS_CODES,
> };
>
> static int fs_probe = 1;
> @@ -180,6 +182,10 @@ argp_parser (int key, char *arg, struct argp_state *state)
> install_bootsector = 0;
> return 0;
>
> + case OPTION_NO_RS_CODES:
> + no_rs_codes = 1;
> + return 0;
> +
> case OPTION_DEBUG:
> verbosity++;
> return 0;
> @@ -238,6 +244,8 @@ static struct argp_option options[] = {
> N_("do not probe for filesystems in DEVICE"), 0},
> {"no-bootsector", OPTION_NO_BOOTSECTOR, 0, 0,
> N_("do not install bootsector"), 0},
> + {"no-rs-codes", OPTION_NO_RS_CODES, 0, 0,
> + N_("Do not apply any reed-solomon codes when embedding core.img, even if there is enough space."), 0},
>
> {"debug", OPTION_DEBUG, 0, OPTION_HIDDEN, 0, 2},
> {"no-floppy", OPTION_NO_FLOPPY, 0, OPTION_HIDDEN, 0, 2},
> @@ -1412,12 +1420,13 @@ main (int argc, char *argv[])
> "boot.img");
> grub_install_copy_file (boot_img_src, boot_img, 1);
>
> - grub_util_info ("%sgrub-bios-setup %s %s %s %s --directory='%s' --device-map='%s' '%s'",
> + grub_util_info ("%sgrub-bios-setup %s %s %s %s %s --directory='%s' --device-map='%s' '%s'",
> install_bootsector ? "" : "NOT RUNNING: ",
> allow_floppy ? "--allow-floppy " : "",
> verbosity ? "--verbose " : "",
> force ? "--force " : "",
> !fs_probe ? "--skip-fs-probe" : "",
> + no_rs_codes ? "--no-rs-codes" : "",
> platdir,
> device_map,
> install_device);
> @@ -1426,7 +1435,7 @@ main (int argc, char *argv[])
> if (install_bootsector)
> grub_util_bios_setup (platdir, "boot.img", "core.img",
> install_drive, force,
> - fs_probe, allow_floppy);
> + fs_probe, allow_floppy, no_rs_codes);
> break;
> }
> case GRUB_INSTALL_PLATFORM_SPARC64_IEEE1275:
> @@ -1438,12 +1447,13 @@ main (int argc, char *argv[])
> "boot.img");
> grub_install_copy_file (boot_img_src, boot_img, 1);
>
> - grub_util_info ("%sgrub-sparc64-setup %s %s %s %s --directory='%s' --device-map='%s' '%s'",
> + grub_util_info ("%sgrub-sparc64-setup %s %s %s %s %s --directory='%s' --device-map='%s' '%s'",
> install_bootsector ? "" : "NOT RUNNING: ",
> allow_floppy ? "--allow-floppy " : "",
> verbosity ? "--verbose " : "",
> force ? "--force " : "",
> !fs_probe ? "--skip-fs-probe" : "",
> + no_rs_codes ? "--no-rs-codes" : "",
On sparc it has no effect.
> platdir,
> device_map,
> install_drive);
> @@ -1452,7 +1462,7 @@ main (int argc, char *argv[])
> if (install_bootsector)
> grub_util_sparc_setup (platdir, "boot.img", "core.img",
> install_device, force,
> - fs_probe, allow_floppy);
> + fs_probe, allow_floppy, no_rs_codes);
> break;
> }
>
> diff --git a/util/grub-setup.c b/util/grub-setup.c
> index 90b9de0..41344d2 100644
> --- a/util/grub-setup.c
> +++ b/util/grub-setup.c
> @@ -82,7 +82,8 @@ static struct argp_option options[] = {
> /* TRANSLATORS: The potential breakage isn't limited to floppies but it's
> likely to make the install unbootable from HDD. */
> N_("make the drive also bootable as floppy (default for fdX devices). May break on some BIOSes."), 0},
> -
> + {"no-rs-codes", 'n', 0, 0,
> + N_("Do not apply any reed-solomon codes when embedding core.img, even if there is enough space."), 0},
It doesn't seem to be important enough to give it a letter of its own.
> { 0, 0, 0, 0, 0, 0 }
> };
>
Don't add it on sparc, it doesn't have any effect there.
> @@ -118,6 +119,7 @@ struct arguments
> int fs_probe;
> int allow_floppy;
> char *device;
> + int no_rs_codes;
> };
>
> static error_t
> @@ -173,6 +175,10 @@ argp_parser (int key, char *arg, struct argp_state *state)
> verbosity++;
> break;
>
> + case 'n':
> + arguments->no_rs_codes = 1;
> + break;
> +
> case ARGP_KEY_ARG:
> if (state->arg_num == 0)
> arguments->device = xstrdup(arg);
> @@ -292,7 +298,8 @@ main (int argc, char *argv[])
> arguments.boot_file ? : DEFAULT_BOOT_FILE,
> arguments.core_file ? : DEFAULT_CORE_FILE,
> dest_dev, arguments.force,
> - arguments.fs_probe, arguments.allow_floppy);
> + arguments.fs_probe, arguments.allow_floppy,
> + arguments.no_rs_codes);
>
> /* Free resources. */
> grub_fini_all ();
> diff --git a/util/setup.c b/util/setup.c
> index 337c304..b60a109 100644
> --- a/util/setup.c
> +++ b/util/setup.c
> @@ -248,7 +248,7 @@ void
> SETUP (const char *dir,
> const char *boot_file, const char *core_file,
> const char *dest, int force,
> - int fs_probe, int allow_floppy)
> + int fs_probe, int allow_floppy, int no_rs_codes)
> {
> char *core_path;
> char *boot_img, *core_img, *boot_path;
> @@ -486,7 +486,11 @@ SETUP (const char *dir,
>
> nsec = core_sectors;
>
> - maxsec = 2 * core_sectors;
> + if (!no_rs_codes)
> + maxsec = 2 * core_sectors;
> + else
> + maxsec = core_sectors;
> +
> if (maxsec > ((0x78000 - GRUB_KERNEL_I386_PC_LINK_ADDR)
> >> GRUB_DISK_SECTOR_BITS))
> maxsec = ((0x78000 - GRUB_KERNEL_I386_PC_LINK_ADDR)
> @@ -547,27 +551,29 @@ SETUP (const char *dir,
> bl.first_block = (struct grub_boot_blocklist *) (core_img
> + GRUB_DISK_SECTOR_SIZE
> - sizeof (*bl.block));
> -
> - grub_size_t no_rs_length;
> - grub_set_unaligned32 ((core_img + GRUB_DISK_SECTOR_SIZE
> - + GRUB_KERNEL_I386_PC_REED_SOLOMON_REDUNDANCY),
> - grub_host_to_target32 (nsec * GRUB_DISK_SECTOR_SIZE - core_size));
> - no_rs_length = grub_target_to_host16
> - (grub_get_unaligned16 (core_img
> - + GRUB_DISK_SECTOR_SIZE
> - + GRUB_KERNEL_I386_PC_NO_REED_SOLOMON_LENGTH));
> -
> - if (no_rs_length == 0xffff)
> - grub_util_error ("%s", _("core.img version mismatch"));
> -
> - void *tmp = xmalloc (core_size);
> - grub_memcpy (tmp, core_img, core_size);
> - grub_reed_solomon_add_redundancy (core_img + no_rs_length + GRUB_DISK_SECTOR_SIZE,
> - core_size - no_rs_length - GRUB_DISK_SECTOR_SIZE,
> - nsec * GRUB_DISK_SECTOR_SIZE
> - - core_size);
> - assert (grub_memcmp (tmp, core_img, core_size) == 0);
> - free (tmp);
> + if (!no_rs_codes)
> + {
> + grub_size_t no_rs_length;
> + grub_set_unaligned32 ((core_img + GRUB_DISK_SECTOR_SIZE
> + + GRUB_KERNEL_I386_PC_REED_SOLOMON_REDUNDANCY),
> + grub_host_to_target32 (nsec * GRUB_DISK_SECTOR_SIZE - core_size));
> + no_rs_length = grub_target_to_host16
> + (grub_get_unaligned16 (core_img
> + + GRUB_DISK_SECTOR_SIZE
> + + GRUB_KERNEL_I386_PC_NO_REED_SOLOMON_LENGTH));
> +
> + if (no_rs_length == 0xffff)
> + grub_util_error ("%s", _("core.img version mismatch"));
> +
I'd keep this check
> + void *tmp = xmalloc (core_size);
> + grub_memcpy (tmp, core_img, core_size);
> + grub_reed_solomon_add_redundancy (core_img + no_rs_length + GRUB_DISK_SECTOR_SIZE,
> + core_size - no_rs_length - GRUB_DISK_SECTOR_SIZE,
> + nsec * GRUB_DISK_SECTOR_SIZE
> + - core_size);
> + assert (grub_memcmp (tmp, core_img, core_size) == 0);
> + free (tmp);
> + }
>
> /* Write the core image onto the disk. */
> for (i = 0; i < nsec; i++)
> @@ -762,4 +768,3 @@ unable_to_embed:
> grub_device_close (dest_dev);
> grub_device_close (root_dev);
> }
> -
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] Support to disable reed-solomon codes
2013-11-26 0:47 ` Vladimir 'φ-coder/phcoder' Serbinenko
@ 2013-11-26 15:48 ` Jonathan McCune
2013-11-26 16:09 ` Vladimir 'φ-coder/phcoder' Serbinenko
0 siblings, 1 reply; 4+ messages in thread
From: Jonathan McCune @ 2013-11-26 15:48 UTC (permalink / raw)
To: The development of GNU GRUB
[-- Attachment #1: Type: text/plain, Size: 14150 bytes --]
On Mon, Nov 25, 2013 at 4:47 PM, Vladimir 'φ-coder/phcoder' Serbinenko <
phcoder@gmail.com> wrote:
> On 25.11.2013 23:37, Jon McCune wrote:
> > [v0] new grub-*-setup flag to disable insertion of reed solomon codes
> > [v0] grub-install support for option --no-rs-codes
> > [v1] bugfix: also change the maxsec value in util/setup.c
> > [v2] Modified to support new C-language install utilities, added
> documentation
> >
> > Signed-off-by: Jon McCune <jonmccune@google.com>
> > ---
> > docs/grub.texi | 12 +++++++++-
> > include/grub/util/install.h | 4 ++--
> > util/grub-install.c | 20 ++++++++++++-----
> > util/grub-setup.c | 11 ++++++++--
> > util/setup.c | 53
> +++++++++++++++++++++++++--------------------
> > 5 files changed, 66 insertions(+), 34 deletions(-)
> >
> > diff --git a/docs/grub.texi b/docs/grub.texi
> > index 6aee292..29547cd 100644
> > --- a/docs/grub.texi
> > +++ b/docs/grub.texi
> > @@ -5937,8 +5937,18 @@ mounted on
> > Recheck the device map, even if @file{/boot/grub/device.map} already
> > exists. You should use this option whenever you add/remove a disk
> > into/from your computer.
> > -@end table
> >
> > +@item --no-rs-codes
> > +By default, @command{grub-install} will use some extra space in the
> It must be clear that you speak about BIOS only.
>
Noted in v3.
> > +bootloader embedding area for Reed-Solomon error-correcting
> > +codes. This enables GRUB to still boot successfully if one single
> > +block is corrupted.
> We can survive more than one defective sector. If we have r sectors of
> redundancy we can survive up to r/2 corrupted ones
>
Noted in v3.
> > This redundancy may be cumbersome if attempting
> > +to cryptographically validate the contents of the bootloader embedding
> > +area, or in more modern systems with GPT-style partition tables
> > +(@pxref{BIOS installation}) where GRUB does not reside in any
> > +unpartitioned space outside of the MBR. Disable the Reed-Solomon
> What's the reasonning behind GPT part?
>
I looked at these archived threads discussing the reasoning behind
including RS codes in the first place:
http://lists.gnu.org/archive/html/grub-devel/2010-09/msg00218.html
http://lists.gnu.org/archive/html/grub-devel/2010-09/msg00205.html
... and the motivation appeared to prioritize tolerating bad behavior from
proprietary software over actual disk errors. I'm not aware of weird
proprietary software stealing blocks from a GPT BIOS boot partition.
> > +codes with this option.
> > +@end table
> >
> > @node Invoking grub-mkconfig
> > @chapter Invoking grub-mkconfig
> > diff --git a/include/grub/util/install.h b/include/grub/util/install.h
> > index 5cb33fc..2bfd2ec 100644
> > --- a/include/grub/util/install.h
> > +++ b/include/grub/util/install.h
> > @@ -176,12 +176,12 @@ void
> > grub_util_bios_setup (const char *dir,
> > const char *boot_file, const char *core_file,
> > const char *dest, int force,
> > - int fs_probe, int allow_floppy);
> > + int fs_probe, int allow_floppy, int no_rs_codes);
> > void
> > grub_util_sparc_setup (const char *dir,
> > const char *boot_file, const char *core_file,
> > const char *dest, int force,
> > - int fs_probe, int allow_floppy);
> > + int fs_probe, int allow_floppy, int no_rs_codes);
> >
> > char *
> > grub_install_get_image_targets_string (void);
> > diff --git a/util/grub-install.c b/util/grub-install.c
> > index 5354a6d..12d0bc2 100644
> > --- a/util/grub-install.c
> > +++ b/util/grub-install.c
> > @@ -68,6 +68,7 @@ static int have_load_cfg = 0;
> > static FILE * load_cfg_f = NULL;
> > static char *load_cfg;
> > static int install_bootsector = 1;
> > +static int no_rs_codes = 0;
> >
>
> Negative logic variables are unreadable and prone to errors. Could you
> use positive logic?
>
Sure, changed in v3. I left the actual option as --no-rs-codes, and it
changes an option variable from its default of 1 to 0.
> > enum
> > {
> > @@ -93,7 +94,8 @@ enum
> > OPTION_DEBUG_IMAGE,
> > OPTION_NO_FLOPPY,
> > OPTION_DISK_MODULE,
> > - OPTION_NO_BOOTSECTOR
> > + OPTION_NO_BOOTSECTOR,
> > + OPTION_NO_RS_CODES,
> > };
> >
> > static int fs_probe = 1;
> > @@ -180,6 +182,10 @@ argp_parser (int key, char *arg, struct argp_state
> *state)
> > install_bootsector = 0;
> > return 0;
> >
> > + case OPTION_NO_RS_CODES:
> > + no_rs_codes = 1;
> > + return 0;
> > +
> > case OPTION_DEBUG:
> > verbosity++;
> > return 0;
> > @@ -238,6 +244,8 @@ static struct argp_option options[] = {
> > N_("do not probe for filesystems in DEVICE"), 0},
> > {"no-bootsector", OPTION_NO_BOOTSECTOR, 0, 0,
> > N_("do not install bootsector"), 0},
> > + {"no-rs-codes", OPTION_NO_RS_CODES, 0, 0,
> > + N_("Do not apply any reed-solomon codes when embedding core.img,
> even if there is enough space."), 0},
> >
> > {"debug", OPTION_DEBUG, 0, OPTION_HIDDEN, 0, 2},
> > {"no-floppy", OPTION_NO_FLOPPY, 0, OPTION_HIDDEN, 0, 2},
> > @@ -1412,12 +1420,13 @@ main (int argc, char *argv[])
> > "boot.img");
> > grub_install_copy_file (boot_img_src, boot_img, 1);
> >
> > - grub_util_info ("%sgrub-bios-setup %s %s %s %s --directory='%s'
> --device-map='%s' '%s'",
> > + grub_util_info ("%sgrub-bios-setup %s %s %s %s %s --directory='%s'
> --device-map='%s' '%s'",
> > install_bootsector ? "" : "NOT RUNNING: ",
> > allow_floppy ? "--allow-floppy " : "",
> > verbosity ? "--verbose " : "",
> > force ? "--force " : "",
> > !fs_probe ? "--skip-fs-probe" : "",
> > + no_rs_codes ? "--no-rs-codes" : "",
> > platdir,
> > device_map,
> > install_device);
> > @@ -1426,7 +1435,7 @@ main (int argc, char *argv[])
> > if (install_bootsector)
> > grub_util_bios_setup (platdir, "boot.img", "core.img",
> > install_drive, force,
> > - fs_probe, allow_floppy);
> > + fs_probe, allow_floppy, no_rs_codes);
> > break;
> > }
> > case GRUB_INSTALL_PLATFORM_SPARC64_IEEE1275:
> > @@ -1438,12 +1447,13 @@ main (int argc, char *argv[])
> > "boot.img");
> > grub_install_copy_file (boot_img_src, boot_img, 1);
> >
> > - grub_util_info ("%sgrub-sparc64-setup %s %s %s %s --directory='%s'
> --device-map='%s' '%s'",
> > + grub_util_info ("%sgrub-sparc64-setup %s %s %s %s %s
> --directory='%s' --device-map='%s' '%s'",
> > install_bootsector ? "" : "NOT RUNNING: ",
> > allow_floppy ? "--allow-floppy " : "",
> > verbosity ? "--verbose " : "",
> > force ? "--force " : "",
> > !fs_probe ? "--skip-fs-probe" : "",
> > + no_rs_codes ? "--no-rs-codes" : "",
> On sparc it has no effect.
>
Okay, added __attribute__ ((unused)) and a comment where it gets passed a 0
on sparc64. The way setup.c is written it would be more invasive to
actually drop the parameter.
> > platdir,
> > device_map,
> > install_drive);
> > @@ -1452,7 +1462,7 @@ main (int argc, char *argv[])
> > if (install_bootsector)
> > grub_util_sparc_setup (platdir, "boot.img", "core.img",
> > install_device, force,
> > - fs_probe, allow_floppy);
> > + fs_probe, allow_floppy, no_rs_codes);
> > break;
> > }
> >
> > diff --git a/util/grub-setup.c b/util/grub-setup.c
> > index 90b9de0..41344d2 100644
> > --- a/util/grub-setup.c
> > +++ b/util/grub-setup.c
> > @@ -82,7 +82,8 @@ static struct argp_option options[] = {
> > /* TRANSLATORS: The potential breakage isn't limited to floppies but
> it's
> > likely to make the install unbootable from HDD. */
> > N_("make the drive also bootable as floppy (default for fdX
> devices). May break on some BIOSes."), 0},
> > -
> > + {"no-rs-codes", 'n', 0, 0,
> > + N_("Do not apply any reed-solomon codes when embedding core.img,
> even if there is enough space."), 0},
> It doesn't seem to be important enough to give it a letter of its own.
>
Okay, dropped. Not sure if the way I #defined a NO_RS_CODES_KEY -1 is the
right way to do an option with no short form.
> > { 0, 0, 0, 0, 0, 0 }
> > };
> >
> Don't add it on sparc, it doesn't have any effect there.
> > @@ -118,6 +119,7 @@ struct arguments
> > int fs_probe;
> > int allow_floppy;
> > char *device;
> > + int no_rs_codes;
> > };
> >
> > static error_t
> > @@ -173,6 +175,10 @@ argp_parser (int key, char *arg, struct argp_state
> *state)
> > verbosity++;
> > break;
> >
> > + case 'n':
> > + arguments->no_rs_codes = 1;
> > + break;
> > +
> > case ARGP_KEY_ARG:
> > if (state->arg_num == 0)
> > arguments->device = xstrdup(arg);
> > @@ -292,7 +298,8 @@ main (int argc, char *argv[])
> > arguments.boot_file ? : DEFAULT_BOOT_FILE,
> > arguments.core_file ? : DEFAULT_CORE_FILE,
> > dest_dev, arguments.force,
> > - arguments.fs_probe, arguments.allow_floppy);
> > + arguments.fs_probe, arguments.allow_floppy,
> > + arguments.no_rs_codes);
> >
> > /* Free resources. */
> > grub_fini_all ();
> > diff --git a/util/setup.c b/util/setup.c
> > index 337c304..b60a109 100644
> > --- a/util/setup.c
> > +++ b/util/setup.c
> > @@ -248,7 +248,7 @@ void
> > SETUP (const char *dir,
> > const char *boot_file, const char *core_file,
> > const char *dest, int force,
> > - int fs_probe, int allow_floppy)
> > + int fs_probe, int allow_floppy, int no_rs_codes)
> > {
> > char *core_path;
> > char *boot_img, *core_img, *boot_path;
> > @@ -486,7 +486,11 @@ SETUP (const char *dir,
> >
> > nsec = core_sectors;
> >
> > - maxsec = 2 * core_sectors;
> > + if (!no_rs_codes)
> > + maxsec = 2 * core_sectors;
> > + else
> > + maxsec = core_sectors;
> > +
> > if (maxsec > ((0x78000 - GRUB_KERNEL_I386_PC_LINK_ADDR)
> > >> GRUB_DISK_SECTOR_BITS))
> > maxsec = ((0x78000 - GRUB_KERNEL_I386_PC_LINK_ADDR)
> > @@ -547,27 +551,29 @@ SETUP (const char *dir,
> > bl.first_block = (struct grub_boot_blocklist *) (core_img
> > +
> GRUB_DISK_SECTOR_SIZE
> > - sizeof (*bl.block));
> > -
> > - grub_size_t no_rs_length;
> > - grub_set_unaligned32 ((core_img + GRUB_DISK_SECTOR_SIZE
> > - + GRUB_KERNEL_I386_PC_REED_SOLOMON_REDUNDANCY),
> > - grub_host_to_target32 (nsec *
> GRUB_DISK_SECTOR_SIZE - core_size));
> > - no_rs_length = grub_target_to_host16
> > - (grub_get_unaligned16 (core_img
> > - + GRUB_DISK_SECTOR_SIZE
> > - +
> GRUB_KERNEL_I386_PC_NO_REED_SOLOMON_LENGTH));
> > -
> > - if (no_rs_length == 0xffff)
> > - grub_util_error ("%s", _("core.img version mismatch"));
> > -
> > - void *tmp = xmalloc (core_size);
> > - grub_memcpy (tmp, core_img, core_size);
> > - grub_reed_solomon_add_redundancy (core_img + no_rs_length +
> GRUB_DISK_SECTOR_SIZE,
> > - core_size - no_rs_length -
> GRUB_DISK_SECTOR_SIZE,
> > - nsec * GRUB_DISK_SECTOR_SIZE
> > - - core_size);
> > - assert (grub_memcmp (tmp, core_img, core_size) == 0);
> > - free (tmp);
> > + if (!no_rs_codes)
> > + {
> > + grub_size_t no_rs_length;
> > + grub_set_unaligned32 ((core_img + GRUB_DISK_SECTOR_SIZE
> > + +
> GRUB_KERNEL_I386_PC_REED_SOLOMON_REDUNDANCY),
> > + grub_host_to_target32 (nsec *
> GRUB_DISK_SECTOR_SIZE - core_size));
> > + no_rs_length = grub_target_to_host16
> > + (grub_get_unaligned16 (core_img
> > + + GRUB_DISK_SECTOR_SIZE
> > + +
> GRUB_KERNEL_I386_PC_NO_REED_SOLOMON_LENGTH));
> > +
> > + if (no_rs_length == 0xffff)
> > + grub_util_error ("%s", _("core.img version mismatch"));
> > +
> I'd keep this check
>
Okay, moved it outside.
> > + void *tmp = xmalloc (core_size);
> > + grub_memcpy (tmp, core_img, core_size);
> > + grub_reed_solomon_add_redundancy (core_img + no_rs_length +
> GRUB_DISK_SECTOR_SIZE,
> > + core_size - no_rs_length -
> GRUB_DISK_SECTOR_SIZE,
> > + nsec * GRUB_DISK_SECTOR_SIZE
> > + - core_size);
> > + assert (grub_memcmp (tmp, core_img, core_size) == 0);
> > + free (tmp);
> > + }
> >
> > /* Write the core image onto the disk. */
> > for (i = 0; i < nsec; i++)
> > @@ -762,4 +768,3 @@ unable_to_embed:
> > grub_device_close (dest_dev);
> > grub_device_close (root_dev);
> > }
> > -
> >
>
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>
>
[-- Attachment #2: Type: text/html, Size: 18514 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] Support to disable reed-solomon codes
2013-11-26 15:48 ` Jonathan McCune
@ 2013-11-26 16:09 ` Vladimir 'φ-coder/phcoder' Serbinenko
0 siblings, 0 replies; 4+ messages in thread
From: Vladimir 'φ-coder/phcoder' Serbinenko @ 2013-11-26 16:09 UTC (permalink / raw)
To: The development of GNU GRUB
[-- Attachment #1: Type: text/plain, Size: 1547 bytes --]
On 26.11.2013 16:48, Jonathan McCune wrote:
> > This redundancy may be cumbersome if attempting
> > +to cryptographically validate the contents of the bootloader
> embedding
> > +area, or in more modern systems with GPT-style partition tables
> > +(@pxref{BIOS installation}) where GRUB does not reside in any
> > +unpartitioned space outside of the MBR. Disable the Reed-Solomon
> What's the reasonning behind GPT part?
>
>
> I looked at these archived threads discussing the reasoning behind
> including RS codes in the first place:
> http://lists.gnu.org/archive/html/grub-devel/2010-09/msg00218.html
> http://lists.gnu.org/archive/html/grub-devel/2010-09/msg00205.html
> ... and the motivation appeared to prioritize tolerating bad behavior
> from proprietary software over actual disk errors. I'm not aware of
> weird proprietary software stealing blocks from a GPT BIOS boot partition.
>
Perhaps we should contact Adobe to ask for an upgrade...
> Sure, changed in v3. I left the actual option as --no-rs-codes, and it
> changes an option variable from its default of 1 to 0.
>
Yes, that's what I meant.
> Okay, added __attribute__ ((unused)) and a comment where it gets passed
> a 0 on sparc64. The way setup.c is written it would be more invasive to
> actually drop the parameter.
>
Ok.
>
> Okay, dropped. Not sure if the way I #defined a NO_RS_CODES_KEY -1 is
> the right way to do an option with no short form.
>
No, it should be enum and start at 0x100
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-11-26 16:09 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-25 22:37 [PATCH v2] Support to disable reed-solomon codes Jon McCune
2013-11-26 0:47 ` Vladimir 'φ-coder/phcoder' Serbinenko
2013-11-26 15:48 ` Jonathan McCune
2013-11-26 16:09 ` Vladimir 'φ-coder/phcoder' Serbinenko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.