Re: Linux DRTM on UEFI platforms

2022-07-07 Thread Daniel P. Smith

On 7/5/22 20:03, Brendan Trotter wrote:

Hi,


Greetings!

Not sure why I got dropped from distro, but no worries.


On Wed, Jul 6, 2022 at 4:52 AM Daniel P. Smith
 wrote:

On 6/10/22 12:40, Ard Biesheuvel wrote:> On Thu, 19 May 2022 at 22:59,
To help provide clarity, consider the following flows for comparison,

Normal/existing efi-stub:
   EFI -> efi-stub -> head_64.S

Proposed secure launch:
   EFI -> efi-stub -> dl-handler -> [cpu] -> sl_stub ->head_64.S


For more clarity; the entire point is to ensure that the kernel only
has to trust itself and the CPU/TPM hardware (and does not have to
trust a potentially malicious boot loader)..Any attempt to avoid a
one-off solution for Linux is an attempt to weaken security.


Please elaborate so I might understand how this entrypoint allows for 
the kernel to only trust itself and the CPU/TPM.



The only correct approach is "efi-stub -> head_64.S -> kernel's own
secure init"; where (on UEFI systems) neither GRUB nor Trenchboot has
a valid reason to exist and should never be installed.


Cheers,

Brendan


v/r,
dps

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


Re: [PATCH v2] configure.ac: warn if stack-protector not allowed

2022-07-07 Thread Daniel Kiper
On Wed, Jul 06, 2022 at 03:25:58AM -0400, Nicholas Vinson wrote:
> Introduce ERROR_PLATFORM_NOT_SUPPORT_SSP environment variable to treat
> the '--enable-stack-protector is only supported on EFI platforms'
> message as a warning instead of an error. If
> ERROR_PLATFORM_NOT_SUPPORT_SSP is set to 'no' (case-insensitive), then
> the message will be printed as a warning.  Otherwise, it prints as an
> error. The default behavior is to print the message as an error.
>
> For any wrapper build script that has some variation of:
>
> for p in SELECTED_GRUB_PLATFORMS; do\
> configure --enable-stack-protector  \
> --with-platform${P} ... || die; \
> done
> make
>
> GRUB will fail to build if SELECTED_GRUB_PLATFORMS contains a platform
> that does not support SSP.
>
> Such wrapper scripts need to work-around this issue by modifying the
> above for-loop, so it conditionally passes '--enable-stack-protector' to
> configure for the proper GRUB platform(s).
>
> However, if the above example is modified to have to conditionally pass
> in --enable-stack-protector, its behavior is effectively the same as the
> proposed change. Additionally, The list of SSP supported platforms is
> now in 2 places. One in the configure script and one in the build
> wrapper script. If the second list is not properly maintained it could
> mistakenly disable SSP for a platform that later gained support for it.
>
> Signed-off-by: Nicholas Vinson 

Reviewed-by: Daniel Kiper 

But one nit below...

> ---
>  configure.ac | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/configure.ac b/configure.ac
> index 57fb70945..a08f3285e 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -36,6 +36,10 @@ dnl description of the relationships between them.
>
>  AC_INIT([GRUB],[2.11],[bug-g...@gnu.org])
>
> +AS_CASE(["$ERROR_PLATFORM_NOT_SUPPORT_SSP"],
> +  [n | no | nO | N | No | NO], [ERROR_PLATFORM_NOT_SUPPORT_SSP=no],
> +  [ERROR_PLATFORM_NOT_SUPPORT_SSP=yes])
> +
>  # We don't want -g -O2 by default in CFLAGS
>  : ${CFLAGS=""}
>
> @@ -1342,6 +1346,7 @@ AC_ARG_ENABLE([stack-protector],
>[enable the stack protector]),
> [],
> [enable_stack_protector=no])
> +

I think this change can be dropped...

>  if test "x$enable_stack_protector" = xno; then
>if test "x$ssp_possible" = xyes; then
>  # Need that, because some distributions ship compilers that include
> @@ -1349,7 +1354,12 @@ if test "x$enable_stack_protector" = xno; then
>  TARGET_CFLAGS="$TARGET_CFLAGS -fno-stack-protector"
>fi
>  elif test "x$platform" != xefi; then
> -  AC_MSG_ERROR([--enable-stack-protector is only supported on EFI platforms])
> +  if test "$ERROR_PLATFORM_NOT_SUPPORT_SSP" = "yes"; then
> +AC_MSG_ERROR([--enable-stack-protector is only supported on EFI 
> platforms])
> +  else
> +AC_MSG_WARN([--enable-stack-protector is only supported on EFI 
> platforms])
> +  fi
> +  enable_stack_protector=no
>  elif test "x$ssp_global_possible" != xyes; then
>AC_MSG_ERROR([--enable-stack-protector is not supported (compiler doesn't 
> support -mstack-protector-guard=global)])
>  else

Daniel

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


Re: [PATCH] Relax memory constraints on required_pages for EFI boot

2022-07-07 Thread Daniel Kiper
Hi,

On Sun, Jan 17, 2021 at 02:43:50PM -0800, Hanson Char via Grub-devel wrote:
> (Finally figured out how to git send-email on this patch properly.)
>
> As reported earlier, when booted in UEFI mode, Grub would fail to load a
> ramdisk of size larger than "(total_pages >> 2)" with
>
> "error: out of memory"
>
> (https://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/kern/efi/mm.c#n616)
>
> Further investigation into the EFI memory map indicates the current limit of
> MAX_HEAP_MEMORY and the use of a quarter of the total_pages returned from EFI
> available memory (type 7) as the default seems arbitrary and unnecessary.
>
> Therefore this proposed patch removes the aribrary limit, and lets Grub make
> full use of the EFI available memory reported by the BIOS.
>
> The patch has been successfully tested to load large ramdisk with size that
> would otherwise fail.

I am not sure you still care about the issue but I want to give you
heads up we have just merged alternative solution into the GRUB upstream
which should solve your problem. It would be nice if you could test it
and tell us how it works for you.

Daniel

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


Re: [PATCH RESEND v3 1/2] font: Add font scaling feature to grub_font_draw_glyph()

2022-07-07 Thread Daniel Kiper
On Mon, Jun 27, 2022 at 05:35:24PM +0800, Zhang Boyang wrote:
> This patch adds an argument 'scale' to grub_font_draw_glyph(). If
> scale > 1, then the function will create a new scaled bitmap of the
> drawing glyph, and draws the scaled glyph. The scaled bitmap is cached
> in the glyph itself, so it can be reused if same glyph is used many
> times.
>
> To avoid interger overflow during scaling, this patch intruduces
> dimension limits to font files, described by GRUB_FONT_MAX_DIMENSION.
> The scaled glyph should also obey this limit. In addition, scale factor
> is also limited by GRUB_FONT_MAX_SCALE.

If you use overflow aware math operators defined as macros, e.g.
grub_mul(), defined in include/grub/safemath.h probably you
can drop these artificial limits.

> Signed-off-by: Zhang Boyang 
> ---
>  grub-core/commands/videotest.c |   4 +-
>  grub-core/font/font.c  | 120 ++---
>  grub-core/gfxmenu/font.c   |   2 +-
>  grub-core/term/gfxterm.c   |   2 +-
>  include/grub/font.h|  16 -
>  5 files changed, 131 insertions(+), 13 deletions(-)
>
> diff --git a/grub-core/commands/videotest.c b/grub-core/commands/videotest.c
> index ac145afc2..d95ee411d 100644
> --- a/grub-core/commands/videotest.c
> +++ b/grub-core/commands/videotest.c
> @@ -87,7 +87,7 @@ grub_cmd_videotest (grub_command_t cmd __attribute__ 
> ((unused)),
>return grub_error (GRUB_ERR_BAD_FONT, "no font loaded");
>
>  glyph = grub_font_get_glyph (fixed, '*');
> -grub_font_draw_glyph (glyph, color, 200 ,0);
> +grub_font_draw_glyph (glyph, color, 200, 0, 1);
>
>  color = grub_video_map_rgb (255, 255, 255);
>
> @@ -148,7 +148,7 @@ grub_cmd_videotest (grub_command_t cmd __attribute__ 
> ((unused)),
>{
>   color = grub_video_map_color (i);
>   palette[i] = color;
> - grub_font_draw_glyph (glyph, color, 16 + i * 16, 220);
> + grub_font_draw_glyph (glyph, color, 16 + i * 16, 220, 1);
>}
>}
>
> diff --git a/grub-core/font/font.c b/grub-core/font/font.c
> index 42189c325..46ffec84c 100644
> --- a/grub-core/font/font.c
> +++ b/grub-core/font/font.c
> @@ -137,6 +137,8 @@ ascii_glyph_lookup (grub_uint32_t code)
> ascii_font_glyph[current]->offset_x = 0;
> ascii_font_glyph[current]->offset_y = -2;
> ascii_font_glyph[current]->device_width = 8;
> +   ascii_font_glyph[current]->scaled_bitmap = NULL;
> +   ascii_font_glyph[current]->scale = 0;
> ascii_font_glyph[current]->font = NULL;
>
> grub_memcpy (ascii_font_glyph[current]->bitmap,
> @@ -172,6 +174,8 @@ grub_font_loader_init (void)
>unknown_glyph->offset_x = 0;
>unknown_glyph->offset_y = -3;
>unknown_glyph->device_width = 8;
> +  unknown_glyph->scaled_bitmap = NULL;
> +  unknown_glyph->scale = 0;
>grub_memcpy (unknown_glyph->bitmap,
>  unknown_glyph_bitmap, sizeof (unknown_glyph_bitmap));
>
> @@ -646,6 +650,20 @@ grub_font_load (const char *filename)
>goto fail;
>  }
>
> +  if (font->max_char_width <= 0
> +  || font->max_char_width > GRUB_FONT_MAX_DIMENSION
> +  || font->max_char_height <= 0
> +  || font->max_char_height > GRUB_FONT_MAX_DIMENSION
> +  || font->ascent <= 0
> +  || font->ascent > GRUB_FONT_MAX_DIMENSION
> +  || font->descent <= 0
> +  || font->descent > GRUB_FONT_MAX_DIMENSION)
> +{
> +  grub_error (GRUB_ERR_BAD_FONT,
> +   "invalid font file: bad font metrics");

grub_error (GRUB_ERR_BAD_FONT, N_("invalid font file: bad font metrics"));

Every grub_error() message has to be processed by N_() macro. And yes,
there is no space after N_ and before "(".

Additionally, I am OK with lines a bit longer than 80 chars. So, you can
put grub_error() call in one line here and below.

> +  goto fail;
> +}
> +
>/* Add the font to the global font registry.  */
>if (register_font (font) != 0)
>  goto fail;
> @@ -738,7 +756,7 @@ grub_font_get_glyph_internal (grub_font_t font, 
> grub_uint32_t code)
>grub_uint16_t height;
>grub_int16_t xoff;
>grub_int16_t yoff;
> -  grub_int16_t dwidth;
> +  grub_uint16_t dwidth;
>int len;
>
>if (index_entry->glyph)
> @@ -760,7 +778,18 @@ grub_font_get_glyph_internal (grub_font_t font, 
> grub_uint32_t code)
> || read_be_uint16 (font->file, &height) != 0
> || read_be_int16 (font->file, &xoff) != 0
> || read_be_int16 (font->file, &yoff) != 0
> -   || read_be_int16 (font->file, &dwidth) != 0)
> +   || read_be_uint16 (font->file, &dwidth) != 0)
> + {
> +   remove_font (font);
> +   return 0;
> + }
> +
> +  /* Reject illegal glyphs.  */
> +  if (width > font->max_char_width
> +   || height > font->max_char_height
> +   || xoff < -GRUB_FONT_MAX_DIMENSION || xoff > GRUB_FONT_MAX_DIMENSION
> +   || yoff < -GRUB_FONT_MAX_DIMENSION || yoff > GRUB_FONT_MAX_DIMENSION
> +   || dwidth > GRUB_FONT_MAX_DIME

Re: [PATCH RESEND v3 2/2] term/gfxterm: Preliminary HiDPI support

2022-07-07 Thread Daniel Kiper
On Mon, Jun 27, 2022 at 05:35:25PM +0800, Zhang Boyang wrote:
> Currently GRUB's default font is too small to see on a HiDPI monitor.
> This patch adds preliminary HiDPI support to gfxterm. It introduces a
> new environment variable 'gfxterm_scale'. If set to 0, and a high
> resolution monitor is detected, it will scale the font size
> automatically. If set to other number, that number will be the scale
> factor, overriding automatic scale factor calculation.
>
> Signed-off-by: Zhang Boyang 
> ---
>  docs/grub.texi   | 11 ++
>  grub-core/gfxmenu/view.c |  1 +
>  grub-core/term/gfxterm.c | 72 
>  include/grub/gfxterm.h   |  3 +-
>  4 files changed, 64 insertions(+), 23 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 9b902273c..8f82061f6 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -3274,6 +3274,7 @@ These variables have special meaning to GRUB.
>  * gfxmode::
>  * gfxpayload::
>  * gfxterm_font::
> +* gfxterm_scale::
>  * grub_cpu::
>  * grub_platform::
>  * icondir::
> @@ -3548,6 +3549,16 @@ If this variable is set, it names a font to use for 
> text on the
>  available font.
>
>
> +@node gfxterm_scale
> +@subsection gfxterm_scale
> +
> +If this variable is not set, or set to @samp{0}, the @samp{gfxterm}
> +graphical terminal will scale the font automatically when a high resolution
> +monitor is detected.  If set to other number, the font scale factor will be
> +forced to that number.  Set this to @samp{1} if user don't want
> +@samp{gfxterm} to scale the font on screen.
> +
> +
>  @node grub_cpu
>  @subsection grub_cpu
>
> diff --git a/grub-core/gfxmenu/view.c b/grub-core/gfxmenu/view.c
> index 6358004b2..94b9ef4db 100644
> --- a/grub-core/gfxmenu/view.c
> +++ b/grub-core/gfxmenu/view.c
> @@ -546,6 +546,7 @@ init_terminal (grub_gfxmenu_view_t view)
> view->terminal_rect.height,
> view->double_repaint,
> terminal_font,
> +   1,
> view->terminal_border);
>grub_gfxterm_decorator_hook = grub_gfxmenu_draw_terminal_box;
>  }
> diff --git a/grub-core/term/gfxterm.c b/grub-core/term/gfxterm.c
> index 4512dee6f..df2d3f86b 100644
> --- a/grub-core/term/gfxterm.c
> +++ b/grub-core/term/gfxterm.c
> @@ -82,6 +82,7 @@ struct grub_virtual_screen
>
>/* Font settings. */
>grub_font_t font;
> +  unsigned int scale;
>
>/* Terminal color settings.  */
>grub_uint8_t standard_color_setting;
> @@ -204,7 +205,7 @@ grub_virtual_screen_free (void)
>  static grub_err_t
>  grub_virtual_screen_setup (unsigned int x, unsigned int y,
> unsigned int width, unsigned int height,
> -grub_font_t font)
> +grub_font_t font, unsigned int scale)
>  {
>unsigned int i;
>
> @@ -213,6 +214,7 @@ grub_virtual_screen_setup (unsigned int x, unsigned int y,
>
>/* Initialize with default data.  */
>virtual_screen.font = font;
> +  virtual_screen.scale = scale;
>virtual_screen.width = width;
>virtual_screen.height = height;
>virtual_screen.offset_x = x;
> @@ -220,9 +222,9 @@ grub_virtual_screen_setup (unsigned int x, unsigned int y,
>virtual_screen.normal_char_width =
>  calculate_normal_character_width (virtual_screen.font);
>virtual_screen.normal_char_height =
> -grub_font_get_max_char_height (virtual_screen.font);
> +grub_font_get_max_char_height (virtual_screen.font) * 
> virtual_screen.scale;

Another good place for safe math macro...

>if (virtual_screen.normal_char_height == 0)
> -virtual_screen.normal_char_height = 16;
> +virtual_screen.normal_char_height = 16 * virtual_screen.scale;

... and here and below...

>virtual_screen.cursor_x = 0;
>virtual_screen.cursor_y = 0;
>virtual_screen.cursor_state = 1;
> @@ -297,7 +299,8 @@ grub_err_t
>  grub_gfxterm_set_window (struct grub_video_render_target *target,
>int x, int y, int width, int height,
>int double_repaint,
> -  grub_font_t font, int border_width)
> +  grub_font_t font, int scale,
> +  int border_width)
>  {
>/* Clean up any prior instance.  */
>destroy_window ();
> @@ -306,10 +309,10 @@ grub_gfxterm_set_window (struct 
> grub_video_render_target *target,
>render_target = target;
>
>/* Create virtual screen.  */
> -  if (grub_virtual_screen_setup (border_width, border_width,
> - width - 2 * border_width,
> - height - 2 * border_width,
> - font)
> +  if (grub_virtual_screen_setup (border_width * scale, border_width * scale,
> + width - 2 * border_width * scale,
> + height - 2 * border_width * scale,
> + f

[PATCH v3 1/1] mkfont: Fix tainted loop boundary issues with substitutions

2022-07-07 Thread Darren Kenny
With gsub substitutions the offsets should be validated against the
the number of glyphs in a font face and the memory allocated for the gsub
substitution data.

Both the number of glyphs and the last address in the allocated data are
passed in to process_cursive(), where the number of glyphs validates the end
of the range.

Enabling memory allocation validation uses two macros, one to simply check the
address against the allocated space, and the other to check that the number of
items of a given size doesn't extend outside of the allocated space.

Fixes: CID 73770
Fixes: CID 314040

Signed-off-by: Darren Kenny 
---
v2 -> v3:
  Updated to resolve style issues around the spacing when calling macros.

 util/grub-mkfont.c | 62 +++---
 1 file changed, 53 insertions(+), 9 deletions(-)

diff --git a/util/grub-mkfont.c b/util/grub-mkfont.c
index d0e5ec564f03..5f4130951eb1 100644
--- a/util/grub-mkfont.c
+++ b/util/grub-mkfont.c
@@ -67,6 +67,11 @@
 
 #define GRUB_FONT_RANGE_BLOCK  1024
 
+#define GSUB_PTR_VALID(x, end) assert((grub_uint8_t*)(x) <= (end))
+
+#define GSUB_ARRAY_SIZE_VALID(a, sz, end) \
+assert((sz) >= 0 && ((sz) <= ((end) - (grub_uint8_t*)(a)) / sizeof(*(a
+
 struct grub_glyph_info
 {
   struct grub_glyph_info *next;
@@ -487,16 +492,22 @@ subst (const struct gsub_substitution *sub, grub_uint32_t 
glyph,
 static void
 process_cursive (struct gsub_feature *feature,
 struct gsub_lookup_list *lookups,
-grub_uint32_t feattag)
+grub_uint32_t feattag,
+grub_uint32_t num_glyphs,
+grub_uint8_t *gsub_end)
 {
   int j, k;
   int i;
+  int lookup_count = grub_be_to_cpu16 (feature->lookupcount);
   struct glyph_replace **target = NULL;
   struct gsub_substitution *sub;
 
-  for (j = 0; j < grub_be_to_cpu16 (feature->lookupcount); j++)
+  GSUB_ARRAY_SIZE_VALID (feature->lookupindices, lookup_count, gsub_end);
+
+  for (j = 0; j < lookup_count; j++)
 {
   int lookup_index = grub_be_to_cpu16 (feature->lookupindices[j]);
+  int sub_count;
   struct gsub_lookup *lookup;
   if (lookup_index >= grub_be_to_cpu16 (lookups->count))
{
@@ -538,7 +549,12 @@ process_cursive (struct gsub_feature *feature,
  target = &subst_medijoin;
  break;
}
-  for (k = 0; k < grub_be_to_cpu16 (lookup->subtablecount); k++)
+
+  sub_count = grub_be_to_cpu16 (lookup->subtablecount);
+
+  GSUB_ARRAY_SIZE_VALID (lookup->subtables, sub_count, gsub_end);
+
+  for (k = 0; k < sub_count; k++)
{
  sub = (struct gsub_substitution *)
((grub_uint8_t *) lookup + grub_be_to_cpu16 (lookup->subtables[k]));
@@ -559,18 +575,33 @@ process_cursive (struct gsub_feature *feature,
  if (covertype == GSUB_COVERAGE_LIST)
{
  struct gsub_coverage_list *cover = coverage;
+ int count = grub_be_to_cpu16 (cover->count);
  int l;
- for (l = 0; l < grub_be_to_cpu16 (cover->count); l++)
+
+ GSUB_ARRAY_SIZE_VALID (cover->glyphs, count, gsub_end);
+
+ for (l = 0; l < count; l++)
subst (sub, grub_be_to_cpu16 (cover->glyphs[l]), target, &i);
}
  else if (covertype == GSUB_COVERAGE_RANGE)
{
  struct gsub_coverage_ranges *cover = coverage;
+ int count = grub_be_to_cpu16 (cover->count);
  int l, m;
- for (l = 0; l < grub_be_to_cpu16 (cover->count); l++)
-   for (m = grub_be_to_cpu16 (cover->ranges[l].start);
-m <= grub_be_to_cpu16 (cover->ranges[l].end); m++)
- subst (sub, m, target, &i);
+
+ GSUB_ARRAY_SIZE_VALID (cover->ranges, count, gsub_end);
+
+ for (l = 0; l < count; l++)
+   {
+ grub_uint16_t start = grub_be_to_cpu16 
(cover->ranges[l].start);
+ grub_uint16_t end = grub_be_to_cpu16 (cover->ranges[l].end);
+
+ if (start > end || end > num_glyphs) 
+   grub_util_error ("%s", _("invalid font range"));
+
+ for (m = start; m <= end; m++)
+   subst (sub, m, target, &i);
+   }
}
  else
/* TRANSLATORS: most font transformations apply only to
@@ -589,6 +620,7 @@ add_font (struct grub_font_info *font_info, FT_Face face, 
int nocut)
 {
   struct gsub_header *gsub = NULL;
   FT_ULong gsub_len = 0;
+  grub_uint8_t *gsub_end = NULL;
 
   if (!FT_Load_Sfnt_Table (face, TTAG_GSUB, 0, NULL, &gsub_len))
 {
@@ -600,6 +632,9 @@ add_font (struct grub_font_info *font_info, FT_Face face, 
int nocut)
  gsub_len = 0;
}
 }
+
+  gsub_end = (grub_uint8_t *) gsub + gsub_len;
+
   if (gsub)
 {
   struct gsub_features *features
@@ -610,6 +645,11 @@ add_font (struct grub_font_info *font_info, FT_Face face, 
int nocut)
   

Re: [PATCH v2] configure.ac: warn if stack-protector not allowed

2022-07-07 Thread Nicholas Vinson

On 7/7/22 09:17, Daniel Kiper wrote:

On Wed, Jul 06, 2022 at 03:25:58AM -0400, Nicholas Vinson wrote:

Introduce ERROR_PLATFORM_NOT_SUPPORT_SSP environment variable to treat
the '--enable-stack-protector is only supported on EFI platforms'
message as a warning instead of an error. If
ERROR_PLATFORM_NOT_SUPPORT_SSP is set to 'no' (case-insensitive), then
the message will be printed as a warning.  Otherwise, it prints as an
error. The default behavior is to print the message as an error.

For any wrapper build script that has some variation of:

 for p in SELECTED_GRUB_PLATFORMS; do\
 configure --enable-stack-protector  \
 --with-platform${P} ... || die; \
 done
 make

GRUB will fail to build if SELECTED_GRUB_PLATFORMS contains a platform
that does not support SSP.

Such wrapper scripts need to work-around this issue by modifying the
above for-loop, so it conditionally passes '--enable-stack-protector' to
configure for the proper GRUB platform(s).

However, if the above example is modified to have to conditionally pass
in --enable-stack-protector, its behavior is effectively the same as the
proposed change. Additionally, The list of SSP supported platforms is
now in 2 places. One in the configure script and one in the build
wrapper script. If the second list is not properly maintained it could
mistakenly disable SSP for a platform that later gained support for it.

Signed-off-by: Nicholas Vinson 


Reviewed-by: Daniel Kiper 

But one nit below...


---
  configure.ac | 12 +++-
  1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 57fb70945..a08f3285e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -36,6 +36,10 @@ dnl description of the relationships between them.

  AC_INIT([GRUB],[2.11],[bug-g...@gnu.org])

+AS_CASE(["$ERROR_PLATFORM_NOT_SUPPORT_SSP"],
+  [n | no | nO | N | No | NO], [ERROR_PLATFORM_NOT_SUPPORT_SSP=no],
+  [ERROR_PLATFORM_NOT_SUPPORT_SSP=yes])
+
  # We don't want -g -O2 by default in CFLAGS
  : ${CFLAGS=""}

@@ -1342,6 +1346,7 @@ AC_ARG_ENABLE([stack-protector],
 [enable the stack protector]),
  [],
  [enable_stack_protector=no])
+


I think this change can be dropped...


Agreed. It's an errant white space only change. Unless someone says 
otherwise, I'll regenerate the request with this change removed.


Thanks,
Nicholas Vinson




  if test "x$enable_stack_protector" = xno; then
if test "x$ssp_possible" = xyes; then
  # Need that, because some distributions ship compilers that include
@@ -1349,7 +1354,12 @@ if test "x$enable_stack_protector" = xno; then
  TARGET_CFLAGS="$TARGET_CFLAGS -fno-stack-protector"
fi
  elif test "x$platform" != xefi; then
-  AC_MSG_ERROR([--enable-stack-protector is only supported on EFI platforms])
+  if test "$ERROR_PLATFORM_NOT_SUPPORT_SSP" = "yes"; then
+AC_MSG_ERROR([--enable-stack-protector is only supported on EFI platforms])
+  else
+AC_MSG_WARN([--enable-stack-protector is only supported on EFI platforms])
+  fi
+  enable_stack_protector=no
  elif test "x$ssp_global_possible" != xyes; then
AC_MSG_ERROR([--enable-stack-protector is not supported (compiler doesn't 
support -mstack-protector-guard=global)])
  else


Daniel


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


Re: [PATCH v3 1/1] mkfont: Fix tainted loop boundary issues with substitutions

2022-07-07 Thread Daniel Kiper
On Thu, Jul 07, 2022 at 03:34:38PM +, Darren Kenny wrote:
> With gsub substitutions the offsets should be validated against the
> the number of glyphs in a font face and the memory allocated for the gsub
> substitution data.
>
> Both the number of glyphs and the last address in the allocated data are
> passed in to process_cursive(), where the number of glyphs validates the end
> of the range.
>
> Enabling memory allocation validation uses two macros, one to simply check the
> address against the allocated space, and the other to check that the number of
> items of a given size doesn't extend outside of the allocated space.
>
> Fixes: CID 73770
> Fixes: CID 314040
>
> Signed-off-by: Darren Kenny 

Reviewed-by: Daniel Kiper 

Daniel

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


[PATCH v3] configure.ac: warn if stack-protector not allowed

2022-07-07 Thread Nicholas Vinson
Introduce ERROR_PLATFORM_NOT_SUPPORT_SSP environment variable to treat
the '--enable-stack-protector is only supported on EFI platforms'
message as a warning instead of an error. If
ERROR_PLATFORM_NOT_SUPPORT_SSP is set to 'no' (case-insensitive), then
the message will be printed as a warning.  Otherwise, it prints as an
error. The default behavior is to print the message as an error.

For any wrapper build script that has some variation of:

for p in SELECTED_GRUB_PLATFORMS; do\
configure --enable-stack-protector  \
--with-platform${P} ... || die; \
done
make

GRUB will fail to build if SELECTED_GRUB_PLATFORMS contains a platform
that does not support SSP.

Such wrapper scripts need to work-around this issue by modifying the
above for-loop, so it conditionally passes '--enable-stack-protector' to
configure for the proper GRUB platform(s).

However, if the above example is modified to have to conditionally pass
in --enable-stack-protector, its behavior is effectively the same as the
proposed change. Additionally, The list of SSP supported platforms is
now in 2 places. One in the configure script and one in the build
wrapper script. If the second list is not properly maintained it could
mistakenly disable SSP for a platform that later gained support for it.

Signed-off-by: Nicholas Vinson 
---
 configure.ac | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 57fb70945..90f686f79 100644
--- a/configure.ac
+++ b/configure.ac
@@ -36,6 +36,10 @@ dnl description of the relationships between them.
 
 AC_INIT([GRUB],[2.11],[bug-g...@gnu.org])
 
+AS_CASE(["$ERROR_PLATFORM_NOT_SUPPORT_SSP"],
+  [n | no | nO | N | No | NO], [ERROR_PLATFORM_NOT_SUPPORT_SSP=no],
+  [ERROR_PLATFORM_NOT_SUPPORT_SSP=yes])
+
 # We don't want -g -O2 by default in CFLAGS
 : ${CFLAGS=""}
 
@@ -1349,7 +1353,12 @@ if test "x$enable_stack_protector" = xno; then
 TARGET_CFLAGS="$TARGET_CFLAGS -fno-stack-protector"
   fi
 elif test "x$platform" != xefi; then
-  AC_MSG_ERROR([--enable-stack-protector is only supported on EFI platforms])
+  if test "$ERROR_PLATFORM_NOT_SUPPORT_SSP" = "yes"; then
+AC_MSG_ERROR([--enable-stack-protector is only supported on EFI platforms])
+  else
+AC_MSG_WARN([--enable-stack-protector is only supported on EFI platforms])
+  fi
+  enable_stack_protector=no
 elif test "x$ssp_global_possible" != xyes; then
   AC_MSG_ERROR([--enable-stack-protector is not supported (compiler doesn't 
support -mstack-protector-guard=global)])
 else
-- 
2.35.1


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


Re: Linux DRTM on UEFI platforms

2022-07-07 Thread Brendan Trotter
Hi,

On Thu, Jul 7, 2022 at 7:18 PM Daniel P. Smith
 wrote:
> On 7/5/22 20:03, Brendan Trotter wrote:
> Greetings!
>
> Not sure why I got dropped from distro, but no worries.
>
> > On Wed, Jul 6, 2022 at 4:52 AM Daniel P. Smith
> >  wrote:
> >> On 6/10/22 12:40, Ard Biesheuvel wrote:> On Thu, 19 May 2022 at 22:59,
> >> To help provide clarity, consider the following flows for comparison,
> >>
> >> Normal/existing efi-stub:
> >>EFI -> efi-stub -> head_64.S
> >>
> >> Proposed secure launch:
> >>EFI -> efi-stub -> dl-handler -> [cpu] -> sl_stub ->head_64.S
> >
> > For more clarity; the entire point is to ensure that the kernel only
> > has to trust itself and the CPU/TPM hardware (and does not have to
> > trust a potentially malicious boot loader)..Any attempt to avoid a
> > one-off solution for Linux is an attempt to weaken security.
>
> Please elaborate so I might understand how this entrypoint allows for
> the kernel to only trust itself and the CPU/TPM.

Is this a serious request?

Kernel is started (via. firmware using the kernel's efi-stub, or via.
"kexec()", or..); and regardless of how the kernel was started the
kernel establishes its own dynamic root of trust.(e.g. AMD"s SKINIT or
Intel's TXT, followed by measuring the remainder of itself and
anything passed from firmware like APCI tables) without relying on a
call-back provided by "untrusted by kernel" third-parties that don't
exist in most cases. The dynamic root of trust that kernel creates
depends on the kernel, CPU, TPM, etc (and excludes untrusted and
unnecessary third parties)..

The only potential benefit that the callback solution provides is that
it, in theory, it could reduce duplication of work for other operating
systems (FreeBSD, Solaris, Haiku, Fuchsia, .. could use the same
callback instead of doing it themselves); but previous discussions
(talk of formalising the contract between the boot stub and the Linux
kernel) suggest that you aren't interested in any other OS.

This leaves me wondering what your true motivation is. Are you trying
to benefit GRUB/Trenchboot (at the expense of security, end-user
convenience, distro installer hassle, etc); or trying to manufacture
scope for future man-in-the middle attacks (by promoting a solution
that requires something between firmware and kernel)?

- Brendan

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


Re: [PATCH v2 00/15] GDB script fixes and improvements

2022-07-07 Thread Glenn Washburn
On Tue, 5 Jul 2022 13:16:27 +0200
Daniel Kiper  wrote:

> Hi Glenn,
> 
> On Fri, May 13, 2022 at 06:12:33PM -0500, Glenn Washburn wrote:
> > There's been a lot of changes since v1. There are more fixes and more
> > features. The majority of the shell code has been moved to an external
> > file named gdb_helper.sh, instead of being inline in the GDB script. The
> > one (direct) PERL dependency in GRUB has been removed and converted to
> > shell script. Also a section on debugging is added to the developer docs.
> >
> > Glenn
> >
> > Glenn Washburn (15):
> >   gdb: Fix redirection issue in dump_module_sections
> >   gdb: Prevent wrapping when writing to .segments.tmp
> >   gdb: If no modules have been loaded, do not try to load module symbols
> >   gdb: Move runtime module loading into runtime_load_module
> >   gdb: Get correct mod variable value
> >   gdb: Do not run load_module if module has already been loaded
> >   gdb: Add functions to make loading from dynamically positioned targets
> > easier
> >   gdb: Remove Perl dependency for GRUB GDB script
> >   gdb: If enabled, print line used to load EFI kernel symbols when using
> > gdb_grub script
> >   gdb: Conditionally run GDB script logic for dynamically or statically
> > positioned GRUB
> >   gdb: Only connect to remote target once when first sourced
> >   gdb: Allow user defined "onload_" command to be run when
> > module is loaded
> >   gdb: Allow running user-defined commands at GRUB start
> >   gdb: Add ability to turn on shell tracing for gdb helper script
> >   docs: Add debugging chapter to development documentation
> >
> >  config.h.in |   3 +
> >  docs/grub-dev.texi  | 191 ++
> >  grub-core/Makefile.core.def |   4 +-
> >  grub-core/gdb_grub.in   | 198 
> >  grub-core/gdb_helper.sh.in  | 108 
> >  grub-core/gmodule.pl.in |  30 --
> >  grub-core/kern/efi/efi.c|   4 +-
> >  grub-core/kern/efi/init.c   |  19 +++-
> >  include/grub/efi/efi.h  |   2 +-
> >  9 files changed, 501 insertions(+), 58 deletions(-)
> >  create mode 100644 grub-core/gdb_helper.sh.in
> >  delete mode 100644 grub-core/gmodule.pl.in
> 
> This is great improvement. However, after looking at the list of changed
> files it seems to me it should be rebased. May I ask you to do that?
> Sorry for the inconvenience.

A rangediff of this series against the rebased commits shows nothing
has changed. Confirming rangediff, only docs/grub-dev.texi has been
changed in master since this series was sent and those changes are all
after the changes in this series. So there's no reason to send an
updated series as it would just be the exact same patch files.

Glenn

> 
> Daniel

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


Re: Linux DRTM on UEFI platforms

2022-07-07 Thread Matthew Garrett
On Fri, Jul 08, 2022 at 01:06:19PM +0930, Brendan Trotter wrote:

> This leaves me wondering what your true motivation is. Are you trying
> to benefit GRUB/Trenchboot (at the expense of security, end-user
> convenience, distro installer hassle, etc); or trying to manufacture
> scope for future man-in-the middle attacks (by promoting a solution
> that requires something between firmware and kernel)?

The described mechanism doesn't require trusting the code that's in the 
middle - if the state is perturbed by this code, the measurements will 
be different, and the system will be untrusted. I agree that this 
implementation is more complicated than just leaving it all up to the 
kernel, but I'm having a *lot* of trouble seeing how this has any impact 
on its security. Jumping immediately to impugning the motivation of the 
people involved is entirely inappropriate.

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