Re: Linux DRTM on UEFI platforms
On Wed, Jul 06, 2022 at 09:33:23AM +0930, Brendan Trotter wrote: > 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. Surely the entire point of DRTM is that we *don't* have to trust the bootloader? ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: Linux DRTM on UEFI platforms
Hi, 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. 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 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: Linux DRTM on UEFI platforms
On 6/10/22 12:40, Ard Biesheuvel wrote:> On Thu, 19 May 2022 at 22:59, Daniel P. Smith > wrote: >> >> >> Greetings, >> >> While Matthew's original proposal was around having a location in the >> efi-stub for the callback to be registered, it is felt that it would be >> better suited as part of the Secure Launch specification. What is >> proposed is for the address of the DL Handler to be stored in the SLRT, >> details for the SLRT are below. Then the bootloader that is responsible >> for loading the DL Handler will register the SLRT in the EFI >> configuration table. Checking for the SLRT GUID in the EFI configuration >> table will enable the EFI bootoader responsible for calling EBS, in this >> case efi-stub, to know that a Dynamic Launch has been requested. It also >> seems this would be more in line with how EFI tends to work, versus a >> Linux-specific callback boot param or something similar. >> > > This all looks reasonable but it is not clear to me what a 'Linux > setup kernel' is, and what the boot protocol is for that handover. I used the term 'Linux setup kernel' just to encapsulate the entrypoints, compressed kernel, and pre-"kernel proper" code. As for boot protocol, I believe the SLRT is what you are looking for, and we are in the process of drafting and verifying with a prototype. > The EFI stub to core kernel handover ABI is private to Linux, > architecture specific, and subject to change. This is basically the > point I made before: if you want to boot Linux in EFI mode, you have > to go via the EFI stub, and the EFI stub is in charge of booting the > kernel proper. This is why I (we?) suggested some kind of callback > mechanism, where the EFI stub makes a D-RTM specific call after EBS(), > but does regain control over the boot flow. Literally, this is not how the hardware works nor in line with the architecture developed by TrenchBoot. By doing the Dynamic Launch Event (DLE) the system, meaning the CPU, has effectively been reset. The CPU/Dynamic Configuration Environment (DCE) is expecting to be provided a kernel to measures, an entrypoint within that kernel to jump to, and that kernel understands the reset state of the system. As agreed, the callback approach is the most satisfactory approach to allow the efi-stub to do its private protocol. Once the flow has switched to a dynamic launch, the system is now starting the kernel via a dynamic launch sequence. The dynamic launch entrypoint, sl-stub, is and will be used as the entrypoint regardless of the firmware, UEFI/coreboot/oreboot/slimboot/etc., or CPU vendor, Intel/AMD, in use. Once efi-stub finishes and invokes the callback, its job is complete and the new dl-handler along with sl-stub from the current patch set will handle the system and security setup needed before entering into the kernel proper. 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 Effectively, all this is doing is after efi-stub is done, instead of jumping into head_64.S, it will call the dynamic launch handler to do the dynamic launch, let sl_stub bring the system back into an expected state, and then enter head_64.S just as efi-stub would have done. > If we add another entry point into the kernel proper for the Secure > Launch Entry component to use, we are again exposing an internal ABI > in a way that limits our future ability to make changes to the EFI <-> > kernel handover. I am not sure how you make that jump, but as I stated above, when incorporating dynamic launch it is no longer a straight pass from EFI to kernel proper. The sl-stub is there to provide a common entry point from the CPU as the result of a dynamic launch, regardless of firmware or vendor. Once sl-stub is done with its start-of-day setup, it will jump to the same location efi-stub would have jumped. This means, efi-stub does everything it always has and is free to change what it does. The only addition is that it will now enable a call-out to allow secure launch to do what it needs to do, and then execution returns back to head_64.S. To be concise, it specifically returns where efi-stub would have jumped to without removing or circumventing anything from the existing flow. It should also be noted that sl-stub will not be looking to reconfigure the kernel. It will use the kernel as it was set up by efi-stub. The only job of sl-stub is to make the world right, measure what efi-stub provided for measurement, and then enter the kernel proper. The design of the SLRT structure below is specifically not to bind to anything from the efi-stub ABI/API. The only information needed to take the measurements is the location of any config items, their size, and an identifier for each item. The efi-stub is free to add to and/or remove from the list of items, along with changing where it is stored, or even change the format of
Re: [PATCH] efi: Add missing header from efi/console_control.h
On Wed, May 11, 2022 at 10:02:27PM -0500, Glenn Washburn wrote: > Signed-off-by: Glenn Washburn Reviewed-by: Daniel Kiper Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 2/2] docs: Document efitextmode command
On Fri, May 13, 2022 at 12:54:12PM -0500, Glenn Washburn wrote:
> Signed-off-by: Glenn Washburn
I think this patch should be merged with patch #1.
> ---
> docs/grub.texi | 26 ++
> 1 file changed, 26 insertions(+)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 5de94d062..178957096 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -4049,6 +4049,7 @@ you forget a command, you can run the command
> @command{help}
> * distrust::Remove a pubkey from trusted keys
> * drivemap::Map a drive to another
> * echo::Display a line of text
> +* efitextmode:: Set/Get text output mode resolution
> * eval::Evaluate agruments as GRUB commands
> * export:: Export an environment variable
> * false:: Do nothing, unsuccessfully
> @@ -4505,6 +4506,31 @@ character will print that character.
> @end deffn
>
>
> +@node efitextmode
> +@subsection efitextmode
> +
> +@deffn Command efitextmode [min | max | mode_num]
s/mode_num//?
After some thinking it seems to me this interface is not very convenient.
My guess is that the same mode may have different on
different implementations/platforms. I think EFI shell "mode" command
"col" "row" approach is more universal/reliable. Though I think it makes
sense to leave min and max as is...
> +When used with no arguments displays all available text output modes. The
> +set mode determines the columns and rows of the text display when in
> +text mode. An asterisk, @samp{*}, will be at the end of the line of the
> +currently set mode.
> +
> +Otherwise the command only takes a single parameter, which can be
> +@samp{min}, @samp{max}, or a mode number given by the listing when run
> +with no arguments. These arguments set the mode to the minimum, maximum,
> +and particular mode respectively.
> +
> +By default GRUB will start in whatever mode the EFI firmware defaults to.
> +There are firmwares known to set up the default mode such that output
> +behaves strangely, for example the cursor in the grub shell never reaches
> +the bottom of the screen or, when typing characters at the prompt,
> +characters from previous command output are overwritten. Setting the mode
> +may fix this.
> +
> +Note: This command is only available on EFI platforms.
s/./ and is similar to EFI shell "mode" command./?
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 1/2] efi: Add efitextmode command for getting/setting the text mode resolution
On Fri, May 13, 2022 at 12:54:11PM -0500, Glenn Washburn wrote:
> This command is meant to behave similarly to the 'mode' command of the EFI
> Shell application. One difference is that to set the mode the mode number
> is given, not the rows and columns of the desired mode. Also supported are
> the arguments "min" and "max", which set the mode to the minimum and
> maximum mode respectively as calculated by the columns * rows of that mode.
>
> Signed-off-by: Glenn Washburn
> Reviewed-by: Gerd Hoffmann
> Tested-by: Gerd Hoffmann
> ---
> grub-core/Makefile.core.def | 6 ++
> grub-core/commands/efi/efitextmode.c | 118 +++
> 2 files changed, 124 insertions(+)
> create mode 100644 grub-core/commands/efi/efitextmode.c
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 726f51be7..b22e48f0f 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -813,6 +813,12 @@ module = {
>enable = efi;
> };
>
> +module = {
> + name = efitextmode;
> + efi = commands/efi/efitextmode.c;
> + enable = efi;
> +};
> +
> module = {
>name = blocklist;
>common = commands/blocklist.c;
> diff --git a/grub-core/commands/efi/efitextmode.c
> b/grub-core/commands/efi/efitextmode.c
> new file mode 100644
> index 0..fb72aa6f3
> --- /dev/null
> +++ b/grub-core/commands/efi/efitextmode.c
> @@ -0,0 +1,118 @@
> +/* efitextmode.c - command to get/set text mode resolution */
> +/*
> + * GRUB -- GRand Unified Bootloader
> + * Copyright (C) 2003,2005,2006,2007,2009 Free Software Foundation, Inc.
s/2003,2005,2006,2007,2009/2022/
> + * GRUB is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * GRUB is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with GRUB. If not, see
Re: [PATCH v2 00/15] GDB script fixes and improvements
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. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v5 2/2] devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM parameters
On Wed, Jun 15, 2022 at 12:02:29PM +0200, Josselin Poiret via Grub-devel wrote:
> This lets a LUKS2 cryptodisk have its cipher and hash filled out,
> otherwise they wouldn't be initialized if cheat mounted.
Please add your Signed-off-by here. Same applies to first patch too.
> ---
> grub-core/osdep/devmapper/getroot.c | 87 -
> 1 file changed, 86 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/osdep/devmapper/getroot.c
> b/grub-core/osdep/devmapper/getroot.c
> index 2bf4264cf..80c7e54da 100644
> --- a/grub-core/osdep/devmapper/getroot.c
> +++ b/grub-core/osdep/devmapper/getroot.c
> @@ -51,6 +51,8 @@
> #include
> #include
>
> +#include
> +
> static int
> grub_util_open_dm (const char *os_dev, struct dm_tree **tree,
> struct dm_tree_node **node)
> @@ -186,7 +188,6 @@ grub_util_pull_devmapper (const char *os_dev)
>&& lastsubdev)
> {
>char *grdev = grub_util_get_grub_dev (lastsubdev);
> - dm_tree_free (tree);
>if (grdev)
> {
> grub_err_t err;
> @@ -194,7 +195,91 @@ grub_util_pull_devmapper (const char *os_dev)
> if (err)
> grub_util_error (_("can't mount encrypted volume `%s': %s"),
>lastsubdev, grub_errmsg);
> + if (strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) ==
> 0)
> +{
> + /* set LUKS2 cipher from dm parameters, since it is not
> + * possible to determine the correct one without
> + * unlocking, as there might be multiple segments.
> + */
Please format comments, here and below, properly [1].
> + grub_disk_t source;
> + grub_cryptodisk_t cryptodisk;
> + grub_uint64_t start, length;
> + char *target_type;
> + char *params;
> + const char *name;
> + char *cipher, *cipher_mode;
> + struct dm_task *dmt;
> + char *seek_head, *c;
> + unsigned int remaining;
> +
> + source = grub_disk_open (grdev);
This function may fail. Please check the returned value and fail if needed.
> + cryptodisk = grub_cryptodisk_get_by_source_disk (source);
Ditto.
> + grub_disk_close (source);
> +
> + name = dm_tree_node_get_name (node);
I think same applies here...
> + grub_util_info ("populating parameters of cryptomount `%s'
> from DM device `%s'",
> + uuid, name);
> +
> + dmt = dm_task_create (DM_DEVICE_TABLE);
> + if (dmt == 0)
s/0/NULL/ Please use NULL instead of 0 for pointers. Same below...
> +grub_util_error (_("can't create dm task DM_DEVICE_TABLE"));
Yes, fail properly like here...
> + if (dm_task_set_name (dmt, name) == 0)
> +grub_util_error (_("can't set dm task name to `%s'"), name);
> + if (dm_task_run (dmt) == 0)
> +grub_util_error (_("can't run dm task for `%s'"), name);
> + /* dm_get_next_target doesn't have any error modes, everything
> has
> + * been handled by dm_task_run.
> + */
> + dm_get_next_target (dmt, NULL, , ,
> + _type, );
> + if (strncmp (target_type, "crypt", sizeof ("crypt")) != 0)
> +grub_util_error (_("dm target of type `%s' is not `crypt'"),
> + target_type);
I am OK with lines a bit longer than 80. So, you can put this in one line.
> + /* dm target parameters for dm-crypt is
> + *
> [<#opt_params> ...]
> + */
> + c = params;
> + remaining = grub_strlen (c);
> +
> + /* first, get the cipher name from the cipher */
> + if (!(seek_head = grub_memchr (c, '-', remaining)))
> +grub_util_error (_("can't get cipher from dm-crypt
> parameters `%s'"),
> + params);
> + cipher = grub_strndup (c, seek_head - c);
grub_strndup() may fail. Please check if cipher != NULL.
> + remaining -= seek_head - c + 1;
> + c = seek_head + 1;
> +
> + /* now, the cipher mode */
> + if (!(seek_head = grub_memchr (c, ' ', remaining)))
seek_head = grub_memchr (c, ' ', remaining);
if (seek_head == NULL)
> +grub_util_error (_("can't get cipher mode from dm-crypt
> parameters `%s'"),
> + params);
> + cipher_mode = grub_strndup (c, seek_head - c);
Again, grub_strndup() may fail. In general please do not ignore errors
from functions which may fail.
> + remaining -= seek_head - c + 1;
> + c = seek_head + 1;
> +
> + err = grub_cryptodisk_setcipher (cryptodisk, cipher,
> cipher_mode);
> +
