[PATCH v2] i386-pc: build verifiers API as module

[PATCH v2] i386-pc: build verifiers API as module

[Michael Chang]

Given no core functions on i386-pc would require verifiers to work and
the only consumer of the verifier API is the pgp module, it looks good
to me that we can move the verifiers out of the kernel image and let
moddep.lst to auto-load it when pgp is loaded on i386-pc platform.

This helps to reduce the size of core image and thus can relax the
tension of exploding on some i386-pc system with very short MBR gap
size. See also a very comprehensive summary from Colin [1] about the
details.

[1] https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00240.html

V2:
Drop COND_NOT_i386_pc and use !COND_i386_pc.
Add comment in kern/verifiers.c to help understanding what's going on
without digging into the commit history.

Reported-by: Colin Watson <cjwatson@debian.org>
Reviewed-by: Colin Watson <cjwatson@debian.org>
Signed-off-by: Michael Chang <mchang@suse.com>
---
 grub-core/Makefile.am       |  2 ++
 grub-core/Makefile.core.def |  8 +++++++-
 grub-core/kern/main.c       |  4 ++++
 grub-core/kern/verifiers.c  | 17 +++++++++++++++++
 include/grub/verify.h       |  9 +++++++++
 5 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
index ee88e44e9..b6872d20f 100644
--- a/grub-core/Makefile.am
+++ b/grub-core/Makefile.am
@@ -93,7 +93,9 @@ KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/partition.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/stack_protector.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/term.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/time.h
+if !COND_i386_pc
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/verify.h
+endif
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/mm_private.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/net.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/memory.h
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 8022e1c0a..77fdccdb1 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -141,7 +141,7 @@ kernel = {
   common = kern/rescue_parser.c;
   common = kern/rescue_reader.c;
   common = kern/term.c;
-  common = kern/verifiers.c;
+  nopc = kern/verifiers.c;
 
   noemu = kern/compiler-rt.c;
   noemu = kern/mm.c;
@@ -946,6 +946,12 @@ module = {
   cppflags = '-I$(srcdir)/lib/posix_wrap';
 };
 
+module = {
+  name = verifiers;
+  common = kern/verifiers.c;
+  enable = i386_pc;
+};
+
 module = {
   name = hdparm;
   common = commands/hdparm.c;
diff --git a/grub-core/kern/main.c b/grub-core/kern/main.c
index 73967e2f5..c7c6d2d0b 100644
--- a/grub-core/kern/main.c
+++ b/grub-core/kern/main.c
@@ -29,7 +29,9 @@
 #include <grub/command.h>
 #include <grub/reader.h>
 #include <grub/parser.h>
+#ifndef GRUB_MACHINE_PCBIOS
 #include <grub/verify.h>
+#endif
 
 #ifdef GRUB_MACHINE_PCBIOS
 #include <grub/machine/memory.h>
@@ -275,8 +277,10 @@ grub_main (void)
   grub_printf ("Welcome to GRUB!\n\n");
   grub_setcolorstate (GRUB_TERM_COLOR_STANDARD);
 
+#ifndef GRUB_MACHINE_PCBIOS
   /* Init verifiers API. */
   grub_verifiers_init ();
+#endif
 
   grub_load_config ();
 
diff --git a/grub-core/kern/verifiers.c b/grub-core/kern/verifiers.c
index 75d7994cf..1245d0d9e 100644
--- a/grub-core/kern/verifiers.c
+++ b/grub-core/kern/verifiers.c
@@ -221,8 +221,25 @@ grub_verify_string (char *str, enum grub_verify_string_type type)
   return GRUB_ERR_NONE;
 }
 
+/*
+ * It is intended to build verifiers as module on i386-pc platform to minimize
+ * the impact of growing core image size could blow up the 63 sectors limit of
+ * some MBR gap one day. It is also adequate to do so, given no core function
+ * on i386-pc would require the verifiers API to work.
+ */
+#ifdef GRUB_MACHINE_PCBIOS
+GRUB_MOD_INIT(verifiers)
+#else
 void
 grub_verifiers_init (void)
+#endif
 {
   grub_file_filter_register (GRUB_FILE_FILTER_VERIFY, grub_verifiers_open);
 }
+
+#ifdef GRUB_MACHINE_PCBIOS
+GRUB_MOD_FINI(verifiers)
+{
+  grub_file_filter_unregister (GRUB_FILE_FILTER_VERIFY);
+}
+#endif
diff --git a/include/grub/verify.h b/include/grub/verify.h
index cd129c398..6fde244fc 100644
--- a/include/grub/verify.h
+++ b/include/grub/verify.h
@@ -64,10 +64,14 @@ struct grub_file_verifier
   grub_err_t (*verify_string) (char *str, enum grub_verify_string_type type);
 };
 
+#ifdef GRUB_MACHINE_PCBIOS
+extern struct grub_file_verifier *grub_file_verifiers;
+#else
 extern struct grub_file_verifier *EXPORT_VAR (grub_file_verifiers);
 
 extern void
 grub_verifiers_init (void);
+#endif
 
 static inline void
 grub_verifier_register (struct grub_file_verifier *ver)
@@ -81,7 +85,12 @@ grub_verifier_unregister (struct grub_file_verifier *ver)
   grub_list_remove (GRUB_AS_LIST (ver));
 }
 
+#ifdef GRUB_MACHINE_PCBIOS
+grub_err_t
+grub_verify_string (char *str, enum grub_verify_string_type type);
+#else
 extern grub_err_t
 EXPORT_FUNC (grub_verify_string) (char *str, enum grub_verify_string_type type);
+#endif
 
 #endif /* ! GRUB_VERIFY_HEADER */
-- 
2.26.2

Re: [PATCH v2] i386-pc: build verifiers API as module

[Daniel Kiper]

On Thu, Mar 18, 2021 at 07:30:26PM +0800, Michael Chang via Grub-devel wrote:
> Given no core functions on i386-pc would require verifiers to work and
> the only consumer of the verifier API is the pgp module, it looks good
> to me that we can move the verifiers out of the kernel image and let
> moddep.lst to auto-load it when pgp is loaded on i386-pc platform.
>
> This helps to reduce the size of core image and thus can relax the
> tension of exploding on some i386-pc system with very short MBR gap
> size. See also a very comprehensive summary from Colin [1] about the
> details.
>
> [1] https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00240.html
>
> V2:
> Drop COND_NOT_i386_pc and use !COND_i386_pc.
> Add comment in kern/verifiers.c to help understanding what's going on
> without digging into the commit history.
>
> Reported-by: Colin Watson <cjwatson@debian.org>
> Reviewed-by: Colin Watson <cjwatson@debian.org>
> Signed-off-by: Michael Chang <mchang@suse.com>

NAK for this patch and others "fixing" small MBR gaps. I am not going to
deal with this kind of issues any longer because a few folks in the
world cannot/do not want/... reinstall their systems. Sorry guys.

Additionally, the commit 5fd18f77e (mbr: Warn if MBR gap is small and
user uses advanced modules) and 505d92f5e (mbr: Document new limitations
on MBR gap support) are pretty clear we do not support advanced configs
with small MBR gaps any longer.

Daniel

PS FYI, I am not sure anybody cares but I think patch is not fully correct
   because lockdown part of kernel calls into verifiers module on i386-pc.

Re: [PATCH v2] i386-pc: build verifiers API as module

[Colin Watson]

On Mon, Mar 22, 2021 at 04:20:00PM +0100, Daniel Kiper wrote:
> NAK for this patch and others "fixing" small MBR gaps. I am not going to
> deal with this kind of issues any longer because a few folks in the
> world cannot/do not want/... reinstall their systems. Sorry guys.

I'd just like to say that I think this is an unfortunate mistake, and
puts distributions in an invidious position.

> Additionally, the commit 5fd18f77e (mbr: Warn if MBR gap is small and
> user uses advanced modules) and 505d92f5e (mbr: Document new limitations
> on MBR gap support) are pretty clear we do not support advanced configs
> with small MBR gaps any longer.

Yes.  As I recently posted to the list, I do not think this should be
used as an excuse not to fix problems where it is relatively
straightforward to do so.

-- 
Colin Watson (he/him)                              [cjwatson@debian.org]

Re: [PATCH v2] i386-pc: build verifiers API as module

[Colin Watson]

On Mon, Mar 22, 2021 at 04:16:26PM +0000, Colin Watson wrote:
> On Mon, Mar 22, 2021 at 04:20:00PM +0100, Daniel Kiper wrote:
> > NAK for this patch and others "fixing" small MBR gaps. I am not going to
> > deal with this kind of issues any longer because a few folks in the
> > world cannot/do not want/... reinstall their systems. Sorry guys.
> 
> I'd just like to say that I think this is an unfortunate mistake, and
> puts distributions in an invidious position.

Additionally, it doesn't make sense to use this argument to nack
https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00271.html,
which merely makes error messages consistent across the codebase; I
might well have done that sort of cleanup on general principles.
Similarly, are you really going to refuse
https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00278.html
merely because applying it would reduce the image size on i386-pc?  (I
could understand if the argument were "let's remove that code entirely
rather than ifdeffing it out ...)

A blanket nack policy for this sort of thing just doesn't seem to make
any logical sense.

-- 
Colin Watson (he/him)                              [cjwatson@debian.org]

Re: [PATCH v2] i386-pc: build verifiers API as module

[Glenn Washburn]

On Mon, 22 Mar 2021 16:16:26 +0000
Colin Watson <cjwatson@debian.org> wrote:

> On Mon, Mar 22, 2021 at 04:20:00PM +0100, Daniel Kiper wrote:
> > NAK for this patch and others "fixing" small MBR gaps. I am not
> > going to deal with this kind of issues any longer because a few
> > folks in the world cannot/do not want/... reinstall their systems.
> > Sorry guys.
> 
> I'd just like to say that I think this is an unfortunate mistake, and
> puts distributions in an invidious position.

Forgive my ignorance, this seems like a fairly simple patch. While I
personally do not like maintaining patches just solely for myself, my
understanding is that distros are quite accustomed to carrying patches
for very long periods of time (indefinitely?). Is part of the push back
because its onerous for distro/package maintainers? Or is this more a
coming from a matter of principal?

Glenn

Re: [PATCH v2] i386-pc: build verifiers API as module

[Colin Watson]

On Mon, Mar 22, 2021 at 03:19:06PM -0500, Glenn Washburn wrote:
> On Mon, 22 Mar 2021 16:16:26 +0000
> Colin Watson <cjwatson@debian.org> wrote:
> > On Mon, Mar 22, 2021 at 04:20:00PM +0100, Daniel Kiper wrote:
> > > NAK for this patch and others "fixing" small MBR gaps. I am not
> > > going to deal with this kind of issues any longer because a few
> > > folks in the world cannot/do not want/... reinstall their systems.
> > > Sorry guys.
> > 
> > I'd just like to say that I think this is an unfortunate mistake, and
> > puts distributions in an invidious position.
> 
> Forgive my ignorance, this seems like a fairly simple patch. While I
> personally do not like maintaining patches just solely for myself, my
> understanding is that distros are quite accustomed to carrying patches
> for very long periods of time (indefinitely?). Is part of the push back
> because its onerous for distro/package maintainers? Or is this more a
> coming from a matter of principal?

We certainly can and do carry our own patches, but it's pretty
unsatisfying when the reasons for rejecting them upstream don't actually
make sense (as for "buffer: Sync up out-of-range error message" and
"kern/dl: Disable grub_dl_unload_unneeded").  In the last couple of
rounds of security megapatches we've also seen that the amount of
divergence between upstream and various distributions in
security-critical code is in fact a serious problem that needs to be
addressed, and so I'm not happy about adding more to it for things that
touch e.g. the verifiers framework - obviously a security-critical
component.

However, we probably won't have any choice.  Bugs of the form "I
couldn't upgrade without reinstalling my entire system" are quite likely
to be considered critical by any distribution worth its salt, regardless
of whether upstream cares about them, and so this is likely to be just
another way in which in practice distributions end up diverging from
upstream.  I think that's worth at least a bit of pushback.

-- 
Colin Watson (he/him)                              [cjwatson@debian.org]

Re: [PATCH v2] i386-pc: build verifiers API as module

[Daniel Kiper]

On Mon, Mar 22, 2021 at 08:45:27PM +0000, Colin Watson wrote:
> On Mon, Mar 22, 2021 at 03:19:06PM -0500, Glenn Washburn wrote:
> > On Mon, 22 Mar 2021 16:16:26 +0000
> > Colin Watson <cjwatson@debian.org> wrote:
> > > On Mon, Mar 22, 2021 at 04:20:00PM +0100, Daniel Kiper wrote:
> > > > NAK for this patch and others "fixing" small MBR gaps. I am not
> > > > going to deal with this kind of issues any longer because a few
> > > > folks in the world cannot/do not want/... reinstall their systems.
> > > > Sorry guys.
> > >
> > > I'd just like to say that I think this is an unfortunate mistake, and
> > > puts distributions in an invidious position.
> >
> > Forgive my ignorance, this seems like a fairly simple patch. While I
> > personally do not like maintaining patches just solely for myself, my
> > understanding is that distros are quite accustomed to carrying patches
> > for very long periods of time (indefinitely?). Is part of the push back
> > because its onerous for distro/package maintainers? Or is this more a
> > coming from a matter of principal?

First of all, I am not going to make anybody lives more difficult just
for the sake of making it more difficult. However, I want to make my
life as a maintainer easier.

> We certainly can and do carry our own patches, but it's pretty
> unsatisfying when the reasons for rejecting them upstream don't actually
> make sense (as for "buffer: Sync up out-of-range error message" and
> "kern/dl: Disable grub_dl_unload_unneeded").  In the last couple of

OK, I was imprecise. Sorry about that. In general I am OK with the
patches which are beneficial not only for the i386-pc (I will take
closer look at above mentioned patches shortly). Though I am against
introducing piles of ifdefery, especially into security critical stuff,
to make GRUB i386-pc work with small MBR gaps. Or move code around and
make it ugly just for the same thing.

> rounds of security megapatches we've also seen that the amount of
> divergence between upstream and various distributions in
> security-critical code is in fact a serious problem that needs to be
> addressed, and so I'm not happy about adding more to it for things that
> touch e.g. the verifiers framework - obviously a security-critical
> component.
>
> However, we probably won't have any choice.  Bugs of the form "I
> couldn't upgrade without reinstalling my entire system" are quite likely
> to be considered critical by any distribution worth its salt, regardless

How long are you going to support such systems? 1, 5 or 10 years? This
approach makes GRUB upstream as a hostage of small MBR gaps users.
Anyway, I think we have to make users aware that small MBR gaps are not
supported any longer. Otherwise we will be playing whack-a-mole game
which we will loose sooner or later.

> of whether upstream cares about them, and so this is likely to be just
> another way in which in practice distributions end up diverging from
> upstream.  I think that's worth at least a bit of pushback.

Again, believe me, this is not my goal...

Daniel

Re: [PATCH v2] i386-pc: build verifiers API as module

[Lennart Sorensen]

On Tue, Mar 23, 2021 at 05:33:12PM +0100, Daniel Kiper wrote:
> How long are you going to support such systems? 1, 5 or 10 years? This
> approach makes GRUB upstream as a hostage of small MBR gaps users.
> Anyway, I think we have to make users aware that small MBR gaps are not
> supported any longer. Otherwise we will be playing whack-a-mole game
> which we will loose sooner or later.

I was surprised to find a 63 sector gap on one of my systems recently
when upgrading the root disk to a larger drive (by cloning the old disk
and then resizing with gparted).  I decided being that this was an SSD
it was in my interest to change it to 1MB alignment while doing the
resizing anyhow.  I guess this install has been around for a while from
before Debian changed to using 1MB alignment by default.  So some such
systems apparently are still around.

Perhaps you need to actually have grub generate a warning when installed
in a 63 sector gap for a while (where a while preferaby is more than one
release cycle for common distributions, so hence more than 2 or 3 years)
so that people get told there will be a problem with upgrades at some
point and they should look into fixing their partitioning now.

Ideally to make people know about it, put the warning on the boot screen,
not just when doing grub install.  Make people aware so they can start
to look into what they need to do to change their system.  Finding out
after they can't boot is too late.

-- 
Len Sorensen

Re: [PATCH v2] i386-pc: build verifiers API as module

[Michael Chang]

On Tue, Mar 23, 2021 at 05:33:12PM +0100, Daniel Kiper wrote:
> On Mon, Mar 22, 2021 at 08:45:27PM +0000, Colin Watson wrote:

[snip]

> > rounds of security megapatches we've also seen that the amount of
> > divergence between upstream and various distributions in
> > security-critical code is in fact a serious problem that needs to be
> > addressed, and so I'm not happy about adding more to it for things that
> > touch e.g. the verifiers framework - obviously a security-critical
> > component.
> >
> > However, we probably won't have any choice.  Bugs of the form "I
> > couldn't upgrade without reinstalling my entire system" are quite likely
> > to be considered critical by any distribution worth its salt, regardless
> 
> How long are you going to support such systems? 1, 5 or 10 years? This
> approach makes GRUB upstream as a hostage of small MBR gaps users.
> Anyway, I think we have to make users aware that small MBR gaps are not
> supported any longer. Otherwise we will be playing whack-a-mole game
> which we will loose sooner or later.

IMHO It is doing the right thing to declare MBR gap is not supported, it
is also doing the right thing to not breaking updates. We are yet to
seek out or arrive at right time to have short MBR gap completely out of
the game. Maybe a few years later nobody would care as the legacy pc
bios is diminishing, or at some point of time everyone here would agree
that we really have to blow up the limit in order to move on and convey
a clear message that people who is running short mbr gap won't receive
grub updates any longer unless they change it - given we have give
acceptable grace period for them to do the migration ...

Thanks,
Michael

Re: [PATCH v2] i386-pc: build verifiers API as module

[Daniel Kiper]

On Wed, Mar 24, 2021 at 12:44:52PM +0800, Michael Chang via Grub-devel wrote:
> On Tue, Mar 23, 2021 at 05:33:12PM +0100, Daniel Kiper wrote:
> > On Mon, Mar 22, 2021 at 08:45:27PM +0000, Colin Watson wrote:
>
> [snip]
>
> > > rounds of security megapatches we've also seen that the amount of
> > > divergence between upstream and various distributions in
> > > security-critical code is in fact a serious problem that needs to be
> > > addressed, and so I'm not happy about adding more to it for things that
> > > touch e.g. the verifiers framework - obviously a security-critical
> > > component.
> > >
> > > However, we probably won't have any choice.  Bugs of the form "I
> > > couldn't upgrade without reinstalling my entire system" are quite likely
> > > to be considered critical by any distribution worth its salt, regardless
> >
> > How long are you going to support such systems? 1, 5 or 10 years? This
> > approach makes GRUB upstream as a hostage of small MBR gaps users.
> > Anyway, I think we have to make users aware that small MBR gaps are not
> > supported any longer. Otherwise we will be playing whack-a-mole game
> > which we will loose sooner or later.
>
> IMHO It is doing the right thing to declare MBR gap is not supported, it
> is also doing the right thing to not breaking updates. We are yet to
> seek out or arrive at right time to have short MBR gap completely out of
> the game. Maybe a few years later nobody would care as the legacy pc
> bios is diminishing, or at some point of time everyone here would agree
> that we really have to blow up the limit in order to move on and convey
> a clear message that people who is running short mbr gap won't receive
> grub updates any longer unless they change it - given we have give
> acceptable grace period for them to do the migration ...

After some thinking it seems to me we can do this. I can take "i386-pc:
build verifiers API as module", "kern/misc: Move grub_printf_fmt_check
to gfxmenu" and similar patches into 2.06. I will revert after the
release all the patches which adds ifdefery or make code ugly and do not
benefit other platforms than i386-pc. This way you will have support for
small MBR gaps in 2.06 and I will have clean code after 2.06 release.

Does it work for you guys?

Daniel

Re: [PATCH v2] i386-pc: build verifiers API as module

[Daniel Kiper]

On Fri, Mar 26, 2021 at 06:01:01PM +0100, Daniel Kiper wrote:
> On Wed, Mar 24, 2021 at 12:44:52PM +0800, Michael Chang via Grub-devel wrote:
> > On Tue, Mar 23, 2021 at 05:33:12PM +0100, Daniel Kiper wrote:
> > > On Mon, Mar 22, 2021 at 08:45:27PM +0000, Colin Watson wrote:
> >
> > [snip]
> >
> > > > rounds of security megapatches we've also seen that the amount of
> > > > divergence between upstream and various distributions in
> > > > security-critical code is in fact a serious problem that needs to be
> > > > addressed, and so I'm not happy about adding more to it for things that
> > > > touch e.g. the verifiers framework - obviously a security-critical
> > > > component.
> > > >
> > > > However, we probably won't have any choice.  Bugs of the form "I
> > > > couldn't upgrade without reinstalling my entire system" are quite likely
> > > > to be considered critical by any distribution worth its salt, regardless
> > >
> > > How long are you going to support such systems? 1, 5 or 10 years? This
> > > approach makes GRUB upstream as a hostage of small MBR gaps users.
> > > Anyway, I think we have to make users aware that small MBR gaps are not
> > > supported any longer. Otherwise we will be playing whack-a-mole game
> > > which we will loose sooner or later.
> >
> > IMHO It is doing the right thing to declare MBR gap is not supported, it
> > is also doing the right thing to not breaking updates. We are yet to
> > seek out or arrive at right time to have short MBR gap completely out of
> > the game. Maybe a few years later nobody would care as the legacy pc
> > bios is diminishing, or at some point of time everyone here would agree
> > that we really have to blow up the limit in order to move on and convey
> > a clear message that people who is running short mbr gap won't receive
> > grub updates any longer unless they change it - given we have give
> > acceptable grace period for them to do the migration ...
>
> After some thinking it seems to me we can do this. I can take "i386-pc:
> build verifiers API as module", "kern/misc: Move grub_printf_fmt_check
> to gfxmenu" and similar patches into 2.06. I will revert after the
> release all the patches which adds ifdefery or make code ugly and do not
> benefit other platforms than i386-pc. This way you will have support for
> small MBR gaps in 2.06 and I will have clean code after 2.06 release.
>
> Does it work for you guys?

Does anybody care?

Daniel

Re: [PATCH v2] i386-pc: build verifiers API as module

[Michael Chang]

On Mon, Apr 12, 2021 at 03:15:53PM +0200, Daniel Kiper wrote:
> On Fri, Mar 26, 2021 at 06:01:01PM +0100, Daniel Kiper wrote:
> > On Wed, Mar 24, 2021 at 12:44:52PM +0800, Michael Chang via Grub-devel wrote:
> > > On Tue, Mar 23, 2021 at 05:33:12PM +0100, Daniel Kiper wrote:
> > > > On Mon, Mar 22, 2021 at 08:45:27PM +0000, Colin Watson wrote:

[snip]

> > After some thinking it seems to me we can do this. I can take "i386-pc:
> > build verifiers API as module", "kern/misc: Move grub_printf_fmt_check
> > to gfxmenu" and similar patches into 2.06. I will revert after the
> > release all the patches which adds ifdefery or make code ugly and do not
> > benefit other platforms than i386-pc. This way you will have support for
> > small MBR gaps in 2.06 and I will have clean code after 2.06 release.
> >
> > Does it work for you guys?
> 
> Does anybody care?

Could you please consider not reverting them ? For me it is worse than
keeping them as distribution specific patch, as we will have a hard time
to explain what's going on when we have to reintroduce them in rebasing
to commits later to 2.06.

Thanks,
Michael

> 
> Daniel
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2] i386-pc: build verifiers API as module

[Daniel Kiper]

On Tue, Apr 13, 2021 at 12:13:02PM +0800, Michael Chang via Grub-devel wrote:
> On Mon, Apr 12, 2021 at 03:15:53PM +0200, Daniel Kiper wrote:
> > On Fri, Mar 26, 2021 at 06:01:01PM +0100, Daniel Kiper wrote:
> > > On Wed, Mar 24, 2021 at 12:44:52PM +0800, Michael Chang via Grub-devel wrote:
> > > > On Tue, Mar 23, 2021 at 05:33:12PM +0100, Daniel Kiper wrote:
> > > > > On Mon, Mar 22, 2021 at 08:45:27PM +0000, Colin Watson wrote:
>
> [snip]
>
> > > After some thinking it seems to me we can do this. I can take "i386-pc:
> > > build verifiers API as module", "kern/misc: Move grub_printf_fmt_check
> > > to gfxmenu" and similar patches into 2.06. I will revert after the
> > > release all the patches which adds ifdefery or make code ugly and do not
> > > benefit other platforms than i386-pc. This way you will have support for
> > > small MBR gaps in 2.06 and I will have clean code after 2.06 release.
> > >
> > > Does it work for you guys?
> >
> > Does anybody care?
>
> Could you please consider not reverting them ? For me it is worse than
> keeping them as distribution specific patch, as we will have a hard time
> to explain what's going on when we have to reintroduce them in rebasing
> to commits later to 2.06.

Colin, Michael, how long are you going to support small MBR gaps?

Daniel

Re: [PATCH v2] i386-pc: build verifiers API as module

[Colin Watson]

On Wed, Apr 14, 2021 at 03:22:35PM +0200, Daniel Kiper wrote:
> On Tue, Apr 13, 2021 at 12:13:02PM +0800, Michael Chang via Grub-devel wrote:
> > On Mon, Apr 12, 2021 at 03:15:53PM +0200, Daniel Kiper wrote:
> > > On Fri, Mar 26, 2021 at 06:01:01PM +0100, Daniel Kiper wrote:
> > > > After some thinking it seems to me we can do this. I can take "i386-pc:
> > > > build verifiers API as module", "kern/misc: Move grub_printf_fmt_check
> > > > to gfxmenu" and similar patches into 2.06. I will revert after the
> > > > release all the patches which adds ifdefery or make code ugly and do not
> > > > benefit other platforms than i386-pc. This way you will have support for
> > > > small MBR gaps in 2.06 and I will have clean code after 2.06 release.
> > > >
> > > > Does it work for you guys?
> > >
> > > Does anybody care?
> >
> > Could you please consider not reverting them ? For me it is worse than
> > keeping them as distribution specific patch, as we will have a hard time
> > to explain what's going on when we have to reintroduce them in rebasing
> > to commits later to 2.06.
> 
> Colin, Michael, how long are you going to support small MBR gaps?

Quite honestly?  My intention is to continue supporting them until
there's no viable technical alternative, or until somebody figures out
how to effectively (and ethically!) collect data indicating that the
number of affected users is negligible.  Given the complexity of fixing
affected systems, and Debian's long-standing commitment to incremental
upgrades, I find myself unable to justify breaking systems in this state
when it's something I can avoid.  Clean code is a noble enough goal, but
I rank it lower than maintaining the ability for real systems in the
wild to upgrade.

I certainly expect that the number of affected systems will decrease
organically over time, though I have little intuition for the absolute
numbers involved.  The only real (anec)data point I have is that any
time I release packages that unintentionally break this I get several
bug reports about it in quick succession, and I generally work on the
assumption that only a smallish percentage of users are engaged enough
to file bug reports and so each report probably represents many more
affected users; but I know this is very fuzzy.

I have on my to-do list an item to add something to the Debian release
notes about this, since that's a way to reach less-engaged users who
won't read the GRUB manual or mailing lists.  That will likely help to
some extent, although I can't say how much.

(That said: for various reasons my available time to contribute to GRUB
has been pretty low for some years anyway, particularly since it stopped
being something I did on work time, and Debian would probably be served
better by a more active maintainer, so I expect to be looking around for
somebody to gradually replace me over the coming months and years.  So
maybe it won't be me whom you need to persuade anyway ...)

Cheers,

-- 
Colin Watson (he/him)                              [cjwatson@debian.org]

Re: [PATCH v2] i386-pc: build verifiers API as module

[Michael Chang]

On Wed, Apr 14, 2021 at 03:22:35PM +0200, Daniel Kiper wrote:
> On Tue, Apr 13, 2021 at 12:13:02PM +0800, Michael Chang via Grub-devel wrote:
> > On Mon, Apr 12, 2021 at 03:15:53PM +0200, Daniel Kiper wrote:
> > > On Fri, Mar 26, 2021 at 06:01:01PM +0100, Daniel Kiper wrote:
> > > > On Wed, Mar 24, 2021 at 12:44:52PM +0800, Michael Chang via Grub-devel wrote:
> > > > > On Tue, Mar 23, 2021 at 05:33:12PM +0100, Daniel Kiper wrote:
> > > > > > On Mon, Mar 22, 2021 at 08:45:27PM +0000, Colin Watson wrote:
> >
> > [snip]
> >
> > > > After some thinking it seems to me we can do this. I can take "i386-pc:
> > > > build verifiers API as module", "kern/misc: Move grub_printf_fmt_check
> > > > to gfxmenu" and similar patches into 2.06. I will revert after the
> > > > release all the patches which adds ifdefery or make code ugly and do not
> > > > benefit other platforms than i386-pc. This way you will have support for
> > > > small MBR gaps in 2.06 and I will have clean code after 2.06 release.
> > > >
> > > > Does it work for you guys?
> > >
> > > Does anybody care?
> >
> > Could you please consider not reverting them ? For me it is worse than
> > keeping them as distribution specific patch, as we will have a hard time
> > to explain what's going on when we have to reintroduce them in rebasing
> > to commits later to 2.06.
> 
> Colin, Michael, how long are you going to support small MBR gaps?

Sorry I may not be able to provide right answer. I think the problem has
two folds.

1. For the development code stream, especially upstream git. We wanted
to stop short mbr gap support for good. There are good reasons for doing
so. (clean code, free to add new features and so on).

2. For the maintenace code stream, especially downstream branch with
long term service support for years. We cherry-picked patch only for bug
and security fixes. The long life span may have building up quite some
number of setup running on short mbr gap. Ideally we didn't have to
worry about that, as long as no new feature would be allowed to add up
the core size. But things changed after this very high priority boothole
security fixes that contributed too much size growth and may have very
bad consequence. We have no choice but to manage that or the result is
out of our control.

If we are seeking for the common ground. For me it would be that the
size still matters for bug and security fixes as that would be the
material interested by LTSS backport. For new features, it is nice to
have. But that still depends on the feedback as different distrubution
may see different needs.

Finally, thanks your patience for this issue.

Regards,
Michael

> 
> Daniel
> 

Re: [PATCH v2] i386-pc: build verifiers API as module

[Daniel Kiper]

Hi all,

On Tue, Apr 20, 2021 at 11:49:09AM +0800, Michael Chang via Grub-devel wrote:
> On Wed, Apr 14, 2021 at 03:22:35PM +0200, Daniel Kiper wrote:
> > On Tue, Apr 13, 2021 at 12:13:02PM +0800, Michael Chang via Grub-devel wrote:
> > > On Mon, Apr 12, 2021 at 03:15:53PM +0200, Daniel Kiper wrote:
> > > > On Fri, Mar 26, 2021 at 06:01:01PM +0100, Daniel Kiper wrote:
> > > > > On Wed, Mar 24, 2021 at 12:44:52PM +0800, Michael Chang via Grub-devel wrote:
> > > > > > On Tue, Mar 23, 2021 at 05:33:12PM +0100, Daniel Kiper wrote:
> > > > > > > On Mon, Mar 22, 2021 at 08:45:27PM +0000, Colin Watson wrote:
> > >
> > > [snip]
> > >
> > > > > After some thinking it seems to me we can do this. I can take "i386-pc:
> > > > > build verifiers API as module", "kern/misc: Move grub_printf_fmt_check
> > > > > to gfxmenu" and similar patches into 2.06. I will revert after the
> > > > > release all the patches which adds ifdefery or make code ugly and do not
> > > > > benefit other platforms than i386-pc. This way you will have support for
> > > > > small MBR gaps in 2.06 and I will have clean code after 2.06 release.
> > > > >
> > > > > Does it work for you guys?
> > > >
> > > > Does anybody care?
> > >
> > > Could you please consider not reverting them ? For me it is worse than
> > > keeping them as distribution specific patch, as we will have a hard time
> > > to explain what's going on when we have to reintroduce them in rebasing
> > > to commits later to 2.06.
> >
> > Colin, Michael, how long are you going to support small MBR gaps?
>
> Sorry I may not be able to provide right answer. I think the problem has
> two folds.
>
> 1. For the development code stream, especially upstream git. We wanted
> to stop short mbr gap support for good. There are good reasons for doing
> so. (clean code, free to add new features and so on).

I thought about the issue quite long time and finally decided to drop
official small MBR gap support from the GRUB upstream starting from now.
This was not easy decision. However, as I can see less and less people
care about small MBR gaps setups. So, I think it makes less and less
sense to keep such strong size constraints on the core.img size for all
architectures and platforms. Especially if these constraints make the
GRUB development more complicated and weird.

> 2. For the maintenace code stream, especially downstream branch with
> long term service support for years. We cherry-picked patch only for bug
> and security fixes. The long life span may have building up quite some
> number of setup running on short mbr gap. Ideally we didn't have to
> worry about that, as long as no new feature would be allowed to add up
> the core size. But things changed after this very high priority boothole
> security fixes that contributed too much size growth and may have very
> bad consequence. We have no choice but to manage that or the result is
> out of our control.
>
> If we are seeking for the common ground. For me it would be that the
> size still matters for bug and security fixes as that would be the
> material interested by LTSS backport. For new features, it is nice to
> have. But that still depends on the feedback as different distrubution
> may see different needs.

I think distros which care about small MBR gaps setups may build and
share common set of patches making that support possible for them. I am
also happy to make their life a bit easier by taking the patches which
reduce core.img size and still are valuable optimizations for other
architectures and platforms. Additionally, I am OK with adding to the
GRUB documentation common instructions for migrating from the small MBR
gaps to different partitioning schemes which does not impose so strong
limitations on the core.img size.

One take away for me from this situation is that I should do better job
with communicating my plans in the future.

> Finally, thanks your patience for this issue.

You are welcome!

Sorry for the inconvenience...

Daniel

Re: [PATCH v2] i386-pc: build verifiers API as module

[James Bottomley]

On Mon, 2021-03-22 at 15:19 -0500, Glenn Washburn wrote:
> On Mon, 22 Mar 2021 16:16:26 +0000
> Colin Watson <cjwatson@debian.org> wrote:
> 
> > On Mon, Mar 22, 2021 at 04:20:00PM +0100, Daniel Kiper wrote:
> > > NAK for this patch and others "fixing" small MBR gaps. I am not
> > > going to deal with this kind of issues any longer because a few
> > > folks in the world cannot/do not want/... reinstall their
> > > systems. Sorry guys.
> > 
> > I'd just like to say that I think this is an unfortunate mistake,
> > and puts distributions in an invidious position.
> 
> Forgive my ignorance, this seems like a fairly simple patch. While I
> personally do not like maintaining patches just solely for myself, my
> understanding is that distros are quite accustomed to carrying
> patches for very long periods of time (indefinitely?). Is part of the
> push back because its onerous for distro/package maintainers? Or is
> this more a coming from a matter of principal?

The point of an open source upstream is to provide a code base from
which users can build things.  That's why most upstreams aren't simply
code dumps, but are buildable projects.  Since most users don't build
things any more, distributions serve as the build and delivery
mechanism for the majority.  This means that in an ideal world the
upstream and the distributions that package them have a synergistic
relationship.  Ideally any patch which is useful to more than one
distribution should be in the upstream,  so in the ideal model the
upstream serves as a communication and synchronization point for
distributions even if they serve greatly differing markets and open
discussion of these differences helps the upstream determine the best
form of any proposed change which often provides great synergy when a
common and agreed upon resolution is achieved because the distributions
are reflecting the needs of their users back into the upstream and the
upstream is, in turn, responding and refining the inputs.

In the real world, of course, there are a variety of dysfunctional
upstream to distro relationships for reasons as varied as philosophical
differences to personality clashes.  However, when the relationship
becomes dysfunctional it's usually the everyone (including the
upstream) who suffers.

James

Re: [PATCH v2] i386-pc: build verifiers API as module

[Michael Chang]

On Mon, Mar 22, 2021 at 04:20:00PM +0100, Daniel Kiper wrote:
> On Thu, Mar 18, 2021 at 07:30:26PM +0800, Michael Chang via Grub-devel wrote:

[snip]

> NAK for this patch and others "fixing" small MBR gaps. I am not going to
> deal with this kind of issues any longer because a few folks in the
> world cannot/do not want/... reinstall their systems. Sorry guys.
> 
> Additionally, the commit 5fd18f77e (mbr: Warn if MBR gap is small and
> user uses advanced modules) and 505d92f5e (mbr: Document new limitations
> on MBR gap support) are pretty clear we do not support advanced configs
> with small MBR gaps any longer.

If I remember correctly, the short mbr gap warning is only visible to
the user who really runs into the problem at the moment that is too late
for them to take any prevention measure. In other words, this is not
useful as a reminder to the user that they have to start to change their
setup for good. I raised the concern but it seemed nobody cared [1] ..

[1] https://lists.gnu.org/archive/html/grub-devel/2020-11/msg00077.html

The "too late" literally means the system is unbootable afterwards,
becase incompleted installs would end up with inconsistent state between
image and module, resulting in symbol looking up failure. Therefore it
is also something we have to look after although it is also rather
complicated [2] ...

[2] https://www.mail-archive.com/grub-devel@gnu.org/msg30663.html

Afterall, keeping existing running system to survive update (NOT new
install) is really an important thing as many can't afford that to
happen. If we can make it any better to reduce the cost please consider
to do it. It doesn't conflict with the purpose to stop the short mbr gap
support, given we all know the broken system can be avoided in the first
place ...

> PS FYI, I am not sure anybody cares but I think patch is not fully correct
>    because lockdown part of kernel calls into verifiers module on i386-pc.

It is covered by this [3] hunk ...

As long as the mandatory "efi" verifiers lockdown is not used by
platform other than efi, we could defer it's initialization to module
loading in pc platform which is auto-loaded by dependent verifier
modules as to what is used to be ... 

[3]
--- a/grub-core/kern/main.c
+++ b/grub-core/kern/main.c
@@ -29,7 +29,9 @@
 #include <grub/command.h>
 #include <grub/reader.h>
 #include <grub/parser.h>
+#ifndef GRUB_MACHINE_PCBIOS
 #include <grub/verify.h>
+#endif

 #ifdef GRUB_MACHINE_PCBIOS
 #include <grub/machine/memory.h>
@@ -275,8 +277,10 @@ grub_main (void)
   grub_printf ("Welcome to GRUB!\n\n");
   grub_setcolorstate (GRUB_TERM_COLOR_STANDARD);

+#ifndef GRUB_MACHINE_PCBIOS
   /* Init verifiers API. */
   grub_verifiers_init ();
+#endif

   grub_load_config ();

Thanks,
Michael

> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2] i386-pc: build verifiers API as module

[Javier Martinez Canillas]

On 3/23/21 5:16 AM, Michael Chang wrote

[snip]

> 
> Afterall, keeping existing running system to survive update (NOT new
> install) is really an important thing as many can't afford that to
> happen. If we can make it any better to reduce the cost please consider
> to do it. It doesn't conflict with the purpose to stop the short mbr gap
> support, given we all know the broken system can be avoided in the first
> place ...
>

My take on this is that we are conflating the need for distros to prevent an
update to break their users' existing installs, with a new GRUB release that
ships a set of CVE fixes (among other things).

I do agree that distros should avoid breaking users' installs, but don't think
that upstream should keep supporting the 63 sectors embedding are size forever
just to facilitate that. Otherwise it is a race to the bottom, since GRUB will
have to pile up workarounds and massage the code just to keep this constraint.

In the past, we posted other patches that we had been carrying for a long time
downstream (i.e: "kern: Make grub_error() more verbose" [0]) and were told it
couldn't be accepted due increasing the core.img size. It's really hard to add
new stuff by keeping this constraint, without having a lot of ifdefery in GRUB.

Also, most partitions tools have been aligning to a MiB boundary for quite some
time by now. So I don't believe is the correct trade-off for upstream to support
the 31 KiB embedding gap, in detriment of the 1 MiB case that's much more common.

Yes, it's a bummer to have to carry downstream patches (and we have a bunch in
our distro) just to support existing users. But it's up to each distro to decide
what's the proper action to take for their supported OS releases (e.g: rebase to
the latest 2.06 version with some patches, only backport security fixes, etc).

For this particular case, it might be better for distros to just revert commit
9e95f45ceee ("verifiers: Move verifiers API to kernel image") instead of making
it conditional for i386-pc, adding complexity to the GRUB upstream code IMO.

As mentioned, not every upstream change may work for all distros. For example,
we had to revert e346414725a ("templates: Disable the os-prober by default")
since otherwise users that don't have anything set in their /etc/default/grub
wouldn't have entries for other OS in their GRUB config file anymore.

[0]: https://lists.endsoftwarepatents.org/archive/html/grub-devel/2020-03/msg00053.html

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH v2] i386-pc: build verifiers API as module

[Colin Watson]

On Tue, Mar 23, 2021 at 12:37:20PM +0100, Javier Martinez Canillas wrote:
> On 3/23/21 5:16 AM, Michael Chang wrote
> > Afterall, keeping existing running system to survive update (NOT new
> > install) is really an important thing as many can't afford that to
> > happen. If we can make it any better to reduce the cost please consider
> > to do it. It doesn't conflict with the purpose to stop the short mbr gap
> > support, given we all know the broken system can be avoided in the first
> > place ...
> 
> My take on this is that we are conflating the need for distros to prevent an
> update to break their users' existing installs, with a new GRUB release that
> ships a set of CVE fixes (among other things).
> 
> I do agree that distros should avoid breaking users' installs, but don't think
> that upstream should keep supporting the 63 sectors embedding are size forever
> just to facilitate that. Otherwise it is a race to the bottom, since GRUB will
> have to pile up workarounds and massage the code just to keep this constraint.

We don't need to take this out of proportion, though: the actual
workarounds being discussed here are not that complicated, and certainly
*far* less complicated than some of the existing workarounds that exist
only for i386-pc.

> In the past, we posted other patches that we had been carrying for a long time
> downstream (i.e: "kern: Make grub_error() more verbose" [0]) and were told it
> couldn't be accepted due increasing the core.img size. It's really hard to add
> new stuff by keeping this constraint, without having a lot of ifdefery in GRUB.

This has been an issue in some form or another for as long as I've been
working on GRUB.  It's not clear to me why it's particularly urgent to
break things now, when patches to make things work again exist and
aren't of unreasonable complexity compared to the rest of GRUB.  (To be
clear, I would be perfectly happy to be told that particular details of
some particular patch are problematic; it's the blanket refusal even for
simple and innocuous patches that don't add complexity or in some cases
might even remove complexity that I find unreasonable.)

I agree that the size constraints can be difficult at times, but well,
that's boot loaders for you; they're a constrained environment.  After
all, as I understand it, the entire system of a core GRUB image with
loadable modules originally came into existence due to this kind of size
constraint.

For those pointing to the documentation change as justification of
unilaterally ignoring certain constraints, I'd say that the number of
users who'll actually have seen that in advance of their systems being
broken is likely to be a pretty good approximation to zero.  I support
the basic idea of the documentation change, but it needs a *much* longer
deprecation cycle.  Yes, that may be inconvenient for us as GRUB
developers.

Zooming out a bit, it's in the interest of the whole free software
ecosystem for security updates to be reliable and not break existing use
cases.  I don't think any of us want users to be any more hesitant about
installing security updates than they already are: the world is
generally better off if we can deploy security updates as quickly as
possible.

The argument I'm making here is, I think, the same sort of argument by
which Torvalds famously has a very low tolerance for userspace
regressions in the Linux kernel.  The argument there runs something like
this: perhaps userspace is being objectively unreasonable, but
nevertheless, it exists in the real world and nobody is helped by having
to solve n-dimensional simultaneous equations in order to work out
whether they can install a given new kernel version.  Maintaining an
intolerance of regressions reduces the friction for installing updates.

> Also, most partitions tools have been aligning to a MiB boundary for quite some
> time by now. So I don't believe is the correct trade-off for upstream to support
> the 31 KiB embedding gap, in detriment of the 1 MiB case that's much more common.

Right, I remember working on changes to improve alignment handling as
far back as 2010.  That's a good thing, but unfortunately the nature of
the beast here is that old mistakes stick around for a long time.

> Yes, it's a bummer to have to carry downstream patches (and we have a bunch in
> our distro) just to support existing users. But it's up to each distro to decide
> what's the proper action to take for their supported OS releases (e.g: rebase to
> the latest 2.06 version with some patches, only backport security fixes, etc).
> 
> For this particular case, it might be better for distros to just revert commit
> 9e95f45ceee ("verifiers: Move verifiers API to kernel image") instead of making
> it conditional for i386-pc, adding complexity to the GRUB upstream code IMO.

That would also mean skipping or substantially modifying your lockdown
patch that followed it, which requires great care.  I did something like
this in various forms for our security updates because there wasn't much
choice there, but I'm not keen on it as a long-term solution.

In the long term, we do seem to want to have the verifiers API in the
kernel image at least for EFI platforms, don't we?  So reverting that
patch entirely seems like a bad move, and Michael's approach seems a
reasonable compromise.

> As mentioned, not every upstream change may work for all distros. For example,
> we had to revert e346414725a ("templates: Disable the os-prober by default")
> since otherwise users that don't have anything set in their /etc/default/grub
> wouldn't have entries for other OS in their GRUB config file anymore.

I'm very substantially less concerned about distro divergence in things
like grub-mkconfig.  While it can introduce confusion, it's typically
much less security-critical.


Let me be clear here: I'm no newcomer to the business of maintaining
large distro patch stacks for GRUB, far from it.  I've been doing it for
over a decade and I'm well aware of the trade-offs.  Often it's either
the right thing to do or at least the path of least resistance.  Some of
the patches I shepherd make it upstream, some never really do, some need
significant reworking; that's life.  I'm not expecting Debian to get to
zero upstream patches against GRUB before I hit retirement age, and I
don't think that's the point here.

Rather, I don't believe that the existence of distros as intermediaries
means that upstream can simply declare a whole class of regressions to
be a distro problem, with a mere handful of months of notice in the form
of a documentation change in an unreleased version before (no doubt
unintentionally) landing patches that break some of the systems in
question, and not expect at least some pushback about that.

Finally, I also notice that the person who sent the patch at the head of
this thread is also the same person who sent
https://lists.gnu.org/archive/html/grub-devel/2020-03/msg00059.html, so
it seems quite ironic to reject the patch on that basis ...

Thanks,

-- 
Colin Watson (he/him)                              [cjwatson@debian.org]

Re: [PATCH v2] i386-pc: build verifiers API as module

[Javier Martinez Canillas]

On 3/23/21 2:27 PM, Colin Watson wrote:
> On Tue, Mar 23, 2021 at 12:37:20PM +0100, Javier Martinez Canillas wrote:

[snip]

>>
>> For this particular case, it might be better for distros to just revert commit
>> 9e95f45ceee ("verifiers: Move verifiers API to kernel image") instead of making
>> it conditional for i386-pc, adding complexity to the GRUB upstream code IMO.
> 
> That would also mean skipping or substantially modifying your lockdown
> patch that followed it, which requires great care.  I did something like
> this in various forms for our security updates because there wasn't much
> choice there, but I'm not keen on it as a long-term solution.
> 
> In the long term, we do seem to want to have the verifiers API in the
> kernel image at least for EFI platforms, don't we?  So reverting that
> patch entirely seems like a bad move, and Michael's approach seems a
> reasonable compromise.
> 

Yes, that's a good point. Accepting Michael's patch to fix the issue for
i386-pc but start pushing back other patches whose goal is to keep the
GRUB core image minimal seems to be a good middle ground for this topic.

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH v2] i386-pc: build verifiers API as module

[Daniel Kiper]

On Tue, Mar 23, 2021 at 01:27:15PM +0000, Colin Watson wrote:
> On Tue, Mar 23, 2021 at 12:37:20PM +0100, Javier Martinez Canillas wrote:
> > On 3/23/21 5:16 AM, Michael Chang wrote
> > > Afterall, keeping existing running system to survive update (NOT new
> > > install) is really an important thing as many can't afford that to
> > > happen. If we can make it any better to reduce the cost please consider
> > > to do it. It doesn't conflict with the purpose to stop the short mbr gap
> > > support, given we all know the broken system can be avoided in the first
> > > place ...
> >
> > My take on this is that we are conflating the need for distros to prevent an
> > update to break their users' existing installs, with a new GRUB release that
> > ships a set of CVE fixes (among other things).
> >
> > I do agree that distros should avoid breaking users' installs, but don't think
> > that upstream should keep supporting the 63 sectors embedding are size forever
> > just to facilitate that. Otherwise it is a race to the bottom, since GRUB will
> > have to pile up workarounds and massage the code just to keep this constraint.
>
> We don't need to take this out of proportion, though: the actual
> workarounds being discussed here are not that complicated, and certainly
> *far* less complicated than some of the existing workarounds that exist
> only for i386-pc.
>
> > In the past, we posted other patches that we had been carrying for a long time
> > downstream (i.e: "kern: Make grub_error() more verbose" [0]) and were told it
> > couldn't be accepted due increasing the core.img size. It's really hard to add
> > new stuff by keeping this constraint, without having a lot of ifdefery in GRUB.
>
> This has been an issue in some form or another for as long as I've been
> working on GRUB.  It's not clear to me why it's particularly urgent to
> break things now, when patches to make things work again exist and

This is not new thing. I was saying about dropping support for small MBR
gaps on various conferences for at least a year. Nobody complained up
until now...

> aren't of unreasonable complexity compared to the rest of GRUB.  (To be
> clear, I would be perfectly happy to be told that particular details of
> some particular patch are problematic; it's the blanket refusal even for
> simple and innocuous patches that don't add complexity or in some cases
> might even remove complexity that I find unreasonable.)

I think I explained it in the other email...

> I agree that the size constraints can be difficult at times, but well,
> that's boot loaders for you; they're a constrained environment.  After
> all, as I understand it, the entire system of a core GRUB image with
> loadable modules originally came into existence due to this kind of size
> constraint.

OK but I do not agree that ancient limitations have to hinder further
development...

> For those pointing to the documentation change as justification of
> unilaterally ignoring certain constraints, I'd say that the number of
> users who'll actually have seen that in advance of their systems being
> broken is likely to be a pretty good approximation to zero.  I support
> the basic idea of the documentation change, but it needs a *much* longer
> deprecation cycle.  Yes, that may be inconvenient for us as GRUB
> developers.

We are all aware about the issue for years. I was saying about dropping
support for small MBR gaps from upstream for a year. Do you think it is
short "deprecation cycle"?

> Zooming out a bit, it's in the interest of the whole free software
> ecosystem for security updates to be reliable and not break existing use
> cases.  I don't think any of us want users to be any more hesitant about
> installing security updates than they already are: the world is
> generally better off if we can deploy security updates as quickly as
> possible.
>
> The argument I'm making here is, I think, the same sort of argument by
> which Torvalds famously has a very low tolerance for userspace
> regressions in the Linux kernel.  The argument there runs something like
> this: perhaps userspace is being objectively unreasonable, but
> nevertheless, it exists in the real world and nobody is helped by having
> to solve n-dimensional simultaneous equations in order to work out
> whether they can install a given new kernel version.  Maintaining an
> intolerance of regressions reduces the friction for installing updates.

Yes, but I think we have to hammer out better story than keeping small MBR
gaps alive for the end of the world...

Daniel

Re: [PATCH v2] i386-pc: build verifiers API as module

[Daniel Kiper]

On Tue, Mar 23, 2021 at 12:16:21PM +0800, Michael Chang via Grub-devel wrote:
> On Mon, Mar 22, 2021 at 04:20:00PM +0100, Daniel Kiper wrote:
> > On Thu, Mar 18, 2021 at 07:30:26PM +0800, Michael Chang via Grub-devel wrote:
>
> [snip]
>
> > NAK for this patch and others "fixing" small MBR gaps. I am not going to
> > deal with this kind of issues any longer because a few folks in the
> > world cannot/do not want/... reinstall their systems. Sorry guys.
> >
> > Additionally, the commit 5fd18f77e (mbr: Warn if MBR gap is small and
> > user uses advanced modules) and 505d92f5e (mbr: Document new limitations
> > on MBR gap support) are pretty clear we do not support advanced configs
> > with small MBR gaps any longer.
>
> If I remember correctly, the short mbr gap warning is only visible to
> the user who really runs into the problem at the moment that is too late
> for them to take any prevention measure. In other words, this is not
> useful as a reminder to the user that they have to start to change their
> setup for good. I raised the concern but it seemed nobody cared [1] ..
>
> [1] https://lists.gnu.org/archive/html/grub-devel/2020-11/msg00077.html
>
> The "too late" literally means the system is unbootable afterwards,
> becase incompleted installs would end up with inconsistent state between
> image and module, resulting in symbol looking up failure. Therefore it
> is also something we have to look after although it is also rather
> complicated [2] ...
>
> [2] https://www.mail-archive.com/grub-devel@gnu.org/msg30663.html

IIRC I was looking at this patch a few weeks ago but decided to not take
it because the changes are too intrusive for freeze stage. Though I can
reconsider it once again if you think it is worth of it...

> Afterall, keeping existing running system to survive update (NOT new
> install) is really an important thing as many can't afford that to
> happen. If we can make it any better to reduce the cost please consider
> to do it. It doesn't conflict with the purpose to stop the short mbr gap
> support, given we all know the broken system can be avoided in the first
> place ...

This makes sense for me and I am OK with hardening the upgrade path.
However, I think it is post release work...

Daniel

Re: [PATCH v2] i386-pc: build verifiers API as module

[Michael Chang]

On Tue, Mar 23, 2021 at 05:48:01PM +0100, Daniel Kiper wrote:
> On Tue, Mar 23, 2021 at 12:16:21PM +0800, Michael Chang via Grub-devel wrote:
> > On Mon, Mar 22, 2021 at 04:20:00PM +0100, Daniel Kiper wrote:
> > > On Thu, Mar 18, 2021 at 07:30:26PM +0800, Michael Chang via Grub-devel wrote:

[snip]

> IIRC I was looking at this patch a few weeks ago but decided to not take
> it because the changes are too intrusive for freeze stage. Though I can
> reconsider it once again if you think it is worth of it...

Yes please ... It is indeed a bit instrusive, but neverthelast it is
also worth the effort to integrate a method that will help to improve
integrity of the grub installation.

At present the procedure of module and image install is not atomic, so
the system may suffer from booting in unspecified state if the process
aborted prematurely in the halfway. A promising solution to revert the
unspecified state to the original one is therefore very much desired and
will benefit us in the log run ... 

> > Afterall, keeping existing running system to survive update (NOT new
> > install) is really an important thing as many can't afford that to
> > happen. If we can make it any better to reduce the cost please consider
> > to do it. It doesn't conflict with the purpose to stop the short mbr gap
> > support, given we all know the broken system can be avoided in the first
> > place ...
> 
> This makes sense for me and I am OK with hardening the upgrade path.
> However, I think it is post release work...

Thanks for taking the patch into consideration. Your plan also looks
good to me.

Regards,
Michael

> 
> Daniel
> 

Re: [PATCH v2] i386-pc: build verifiers API as module

[Daniel Kiper]

On Wed, Mar 24, 2021 at 11:50:56AM +0800, Michael Chang wrote:
> On Tue, Mar 23, 2021 at 05:48:01PM +0100, Daniel Kiper wrote:
> > On Tue, Mar 23, 2021 at 12:16:21PM +0800, Michael Chang via Grub-devel wrote:
> > > On Mon, Mar 22, 2021 at 04:20:00PM +0100, Daniel Kiper wrote:
> > > > On Thu, Mar 18, 2021 at 07:30:26PM +0800, Michael Chang via Grub-devel wrote:
>
> [snip]
>
> > IIRC I was looking at this patch a few weeks ago but decided to not take
> > it because the changes are too intrusive for freeze stage. Though I can
> > reconsider it once again if you think it is worth of it...
>
> Yes please ... It is indeed a bit instrusive, but neverthelast it is
> also worth the effort to integrate a method that will help to improve
> integrity of the grub installation.

OK, I will take a look at it once again.

> At present the procedure of module and image install is not atomic, so
> the system may suffer from booting in unspecified state if the process
> aborted prematurely in the halfway. A promising solution to revert the
> unspecified state to the original one is therefore very much desired and
> will benefit us in the log run ...
>
> > > Afterall, keeping existing running system to survive update (NOT new
> > > install) is really an important thing as many can't afford that to
> > > happen. If we can make it any better to reduce the cost please consider
> > > to do it. It doesn't conflict with the purpose to stop the short mbr gap
> > > support, given we all know the broken system can be avoided in the first
> > > place ...
> >
> > This makes sense for me and I am OK with hardening the upgrade path.
> > However, I think it is post release work...
>
> Thanks for taking the patch into consideration. Your plan also looks
> good to me.

Great!

Thanks,

Daniel

Re: [PATCH v2] i386-pc: build verifiers API as module

[Michael Schierl]

Hello,


On Fri, 16 Apr 2021 22:23:39 +0100, Colin Watson wrote:
> I have on my to-do list an item to add something to the Debian release
> notes about this, since that's a way to reach less-engaged users who
> won't read the GRUB manual or mailing lists.  That will likely help to
> some extent, although I can't say how much.

Did anything like this happen for the Debian Bullseye release? I found
many interesting things in the release notes, but nothing about GRUB.

Also, updating some existing systems created emails about breaking
changes in some packages to the root user (I believe it is
apt-listchanges which is doing this) but nothing about GRUB either.
(Those systems either used a large MBR gap or a simple setup with only
ext4 and no LVM etc).

I also "just for fun" took the official Debian Installer ISO
(debian-11.0.0-i386-netinst.iso) and used it to install Debian Bullseye
into a new volume on an aleady existing LVM volume group (that is on a
part_msdos style partition that still uses a small MBR gap). The
installer did not complain, and proceeded to the point where it wanted
to install GRUB, but then stopped with a fatal error while executing
"grub install /dev/sda". Did not even show me the real error message.
Seems that core.img was 33 KB large and could therefore not be embedded
into the small MBR gap. To be honest, I am not sure whether this was
still possible with the GRUB version included in Buster.


Regards,


Michael

Re: [PATCH v2] i386-pc: build verifiers API as module

[Didier Spaier]

Le 22/08/2021 à 21:50, Michael Schierl a écrit :
> On Fri, 16 Apr 2021 22:23:39 +0100, Colin Watson wrote:
>> I have on my to-do list an item to add something to the Debian release
>> notes about this, since that's a way to reach less-engaged users who
>> won't read the GRUB manual or mailing lists.  That will likely help to
>> some extent, although I can't say how much.
> 
> Did anything like this happen for the Debian Bullseye release? I found
> many interesting things in the release notes, but nothing about GRUB.
> 
> Also, updating some existing systems created emails about breaking
> changes in some packages to the root user (I believe it is
> apt-listchanges which is doing this) but nothing about GRUB either.
> (Those systems either used a large MBR gap or a simple setup with only
> ext4 and no LVM etc).
> 
> I also "just for fun" took the official Debian Installer ISO
> (debian-11.0.0-i386-netinst.iso) and used it to install Debian Bullseye
> into a new volume on an aleady existing LVM volume group (that is on a
> part_msdos style partition that still uses a small MBR gap). The
> installer did not complain, and proceeded to the point where it wanted
> to install GRUB, but then stopped with a fatal error while executing
> "grub install /dev/sda". Did not even show me the real error message.
> Seems that core.img was 33 KB large and could therefore not be embedded
> into the small MBR gap. To be honest, I am not sure whether this was
> still possible with the GRUB version included in Buster.

According to the packages browser grub-2.06 didn't make its way in any
Debian version at time of writing. That's all I know.

Didier