Re: Add TPM measured boot support

[Matthew Garrett]

On Tue, Jan 23, 2018 at 12:45:14PM +0100, Daniel Kiper wrote:

> Sadly yes. Sorry about that. However, this is still on my radar. I hope that
> I come back to work on this in a few weeks.

Hi Daniel,

Any news on this front? Thanks!

-- 
Matthew Garrett | mj...@srcf.ucam.org

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

Re: Checksummed environments

[Kristian Amlie]

On 06/04/18 14:35, Daniel Kiper wrote:
> On Fri, Apr 06, 2018 at 11:25:22AM +0200, Kristian Amlie wrote:
>> Hey, I work for Northern.tech, developing update software for embedded
>> Linux devices.
>>
>> I have a question about GRUB's environment block: This block is not
> 
> I am not sure what exactly you mean by "GRUB's environment block".
> Could you send me some references to the code?

Of course, sorry if the context wasn't clear. I'm talking about the
environment block mentioned here:
https://www.gnu.org/software/grub/manual/grub/grub.html#Environment-block,
which is used to store information from one boot to the next, using
save_env and load_env.

>> checksummed, and hence I reckon it can become corrupt if power is lost
>> in the middle of a write.
> 
> What about the other blocks?

In theory, there is only one block, because it is written in-place,
directly on disk, without changing any other filesystem blocks. Is that
what you meant by "other blocks"?

>> This is an important safety criterion for us, so we've been thinking of
>> developing environment block checksumming as an extension to the
>> existing save_env and load_env commands. The most likely approach will
>> be to grab X amount of bytes at the end of the block and use these for
>> the checksum.
> 
> Could you tell us more about that?

The idea comes from U-Boot [1], which uses a dedicated block on a
storage device to store data, and uses a CRC32 checksum to make sure it
is consistent. The motivation is to be able to detect that the block is
corrupt, rather than accepting a block of data that may have incorrect
data in if a write was interrupted midway by a powerloss. You can read
more about it in the link.

[2] https://www.denx.de/wiki/DULG/UBootEnvVariables

-- 
Kristian

> 
>> This would also allow us to fall back to an earlier environment file if
>> the current one is corrupt, hence implementing redundancy.
>>
>> Is this something that the GRUB project would be interested in? We want
>> to upstream this if possible, since we think many people may benefit
>> from this.
> 
> Great! However, first of all we need some more info about that.
> 
> Daniel
> 


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

Re: Checksummed environments

[Daniel Kiper]

On Fri, Apr 06, 2018 at 11:25:22AM +0200, Kristian Amlie wrote:
> Hey, I work for Northern.tech, developing update software for embedded
> Linux devices.
>
> I have a question about GRUB's environment block: This block is not

I am not sure what exactly you mean by "GRUB's environment block".
Could you send me some references to the code?

> checksummed, and hence I reckon it can become corrupt if power is lost
> in the middle of a write.

What about the other blocks?

> This is an important safety criterion for us, so we've been thinking of
> developing environment block checksumming as an extension to the
> existing save_env and load_env commands. The most likely approach will
> be to grab X amount of bytes at the end of the block and use these for
> the checksum.

Could you tell us more about that?

> This would also allow us to fall back to an earlier environment file if
> the current one is corrupt, hence implementing redundancy.
>
> Is this something that the GRUB project would be interested in? We want
> to upstream this if possible, since we think many people may benefit
> from this.

Great! However, first of all we need some more info about that.

Daniel

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

Re: Multiboot ELF segment handling patch

[Daniel Kiper]

On Thu, Mar 29, 2018 at 11:20:37AM +0200, Alexander Boettcher wrote:
> Hello,
>
> our project uses various (micro)kernels booted via GRUB2 using the
> Multiboot & Multiboot2 protocol. Some of the kernels have several ELF
> segments which are not necessarily placed close together.
>
> When booting such kernels with GRUB2 in BIOS legacy mode we observed
> that the screen shows colored blinking scrambled output, until the
> graphic drivers takes over. (Looks very fancy, but our customers don't
> like it ;-) ). We could track down the issue and have a fix for GRUB2,

That is sad... ;-)))

> which is attached.
>
> Can you please have a look and check regarding what should/could be
> changed to get it upstream? We did not test the dynamic relocation part,

Sure thing, please take a look below...

> since we have no such (kernel) setup. Thanks in advance.

You can use Xen (git://xenbits.xen.org/xen.git) for tests.
Just compile hypervisor without any tools and use xen.gz.
It produces nice output and you can see it is relocated or not.
Of course you have to use Multiboot2 protocol.

> Regards,
>
> Alex.
>
> --
> Alexander Boettcher
> Genode Labs
>
> http://www.genode-labs.com - http://www.genode.org -
> https://github.com/genodelabs/genode
>
> Genode Labs GmbH - Amtsgericht Dresden - HRB 28424 - Sitz Dresden
> Gesch??ftsf??hrer: Dr.-Ing. Norman Feske, Christian Helmuth


> From 605ca41045f97f92fb698ea49d7267e1cb29f40c Mon Sep 17 00:00:00 2001
> From: Alexander Boettcher 
> Date: Tue, 20 Mar 2018 09:21:06 +0100
> Subject: [PATCH] multiboot: use per segment a separate relocator chunk
>
> If the segments are not dense packed, the original code set up a huge
> relocator chunk comprising all segments.
>
> During the final relocation in grub_relocator_prepare_relocs, the chunk
> is moved to its desired place and overrides memory which are actually not
> covered/touched by the specified segments.
>
> The overridden memory may contain reserved memory
> (vga text mode, acpi tables e.g.), which leads to strange boot behaviour.

Well, this should not happen. I think that this is GRUB2 memory
allocator and/or relocator issue. Both should not touch anything
which is not real RAM. If you know how to trigger this could you
dive in it a bit deeper and check why this happens? Otherwise we
will be hit by the issue sooner or later again. Even if this patch
in any form is accepted.

However, this does not mean that I will not take this patch. Though
it requires some tweaking.

First of all, lack of SOB.

> ---
>  grub-core/loader/multiboot_elfxx.c | 59 
> +++---
>  1 file changed, 29 insertions(+), 30 deletions(-)
>
> diff --git a/grub-core/loader/multiboot_elfxx.c 
> b/grub-core/loader/multiboot_elfxx.c
> index 67daf59..0d10c64 100644
> --- a/grub-core/loader/multiboot_elfxx.c
> +++ b/grub-core/loader/multiboot_elfxx.c
> @@ -57,7 +57,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
>char *phdr_base;
>grub_err_t err;
>grub_relocator_chunk_t ch;
> -  grub_uint32_t load_offset, load_size;
> +  grub_uint32_t load_size;
>int i;
>void *source;
>
> @@ -99,29 +99,6 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
>
>load_size = highest_load - mld->link_base_addr;
>
> -  if (mld->relocatable)
> -{
> -  if (load_size > mld->max_addr || mld->min_addr > mld->max_addr - 
> load_size)
> - return grub_error (GRUB_ERR_BAD_OS, "invalid min/max address and/or 
> load size");
> -
> -  err = grub_relocator_alloc_chunk_align (GRUB_MULTIBOOT (relocator), 
> ,
> -   mld->min_addr, mld->max_addr - 
> load_size,
> -   load_size, mld->align ? 
> mld->align : 1,
> -   mld->preference, 
> mld->avoid_efi_boot_services);
> -}
> -  else
> -err = grub_relocator_alloc_chunk_addr (GRUB_MULTIBOOT (relocator), ,
> -mld->link_base_addr, load_size);
> -
> -  if (err)
> -{
> -  grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS 
> image\n");
> -  return err;
> -}
> -
> -  mld->load_base_addr = get_physical_target_address (ch);
> -  source = get_virtual_current_address (ch);
> -
>grub_dprintf ("multiboot_loader", "link_base_addr=0x%x, 
> load_base_addr=0x%x, "
>   "load_size=0x%x, relocatable=%d\n", mld->link_base_addr,
>   mld->load_base_addr, load_size, mld->relocatable);
> @@ -133,21 +110,44 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t 
> *mld)
>/* Load every loadable segment in memory.  */
>for (i = 0; i < ehdr->e_phnum; i++)
>  {
> -  if (phdr(i)->p_type == PT_LOAD)
> +  if (phdr(i)->p_type != PT_LOAD)
> +continue;
> +
> +  if (mld->relocatable)
>  {
> +  if (load_size > mld->max_addr || mld->min_addr > mld->max_addr - 
> load_size)
> +

Re: [PATCH 4/4] EFI: Do not set text-mode until we actually need it

[Daniel Kiper]

On Thu, Apr 05, 2018 at 08:05:47PM +0200, Hans de Goede wrote:
> Hi,
>
> On 05-04-18 14:34, Daniel Kiper wrote:
> >On Wed, Mar 28, 2018 at 04:50:28PM +0200, Hans de Goede wrote:
> >>If we're running with a hidden menu we may never need text mode, so do not
> >>change the video-mode to text until we actually need it.
> >>
> >>Signed-off-by: Hans de Goede 
> >>---
> >>  grub-core/term/efi/console.c | 72 +++-
> >>  1 file changed, 47 insertions(+), 25 deletions(-)
> >>
> >>diff --git a/grub-core/term/efi/console.c b/grub-core/term/efi/console.c
> >>index 02f64ea74..d9fd7cf48 100644
> >>--- a/grub-core/term/efi/console.c
> >>+++ b/grub-core/term/efi/console.c
> >>@@ -24,6 +24,11 @@
> >>  #include 
> >>  #include 
> >>
> >>+static grub_err_t grub_prepare_for_text_output(struct grub_term_output 
> >>*term);
> >
> >Please drop this forward declaration and put the function definition before 
> >the callers.
>
> I did not do that initially because that requires also moving
> grub_console_setcolorstate() and grub_console_setcursor() to
> higher in the file (or do a forward declaration for those).
>
> I'll add a preparation patch to v2 of the series moving just
> those 2 up to higher in the file.

Great!

> >>+static int text_mode_available = -1;
> >
> >Could you use bool type here? If yes please define grub_bool_t as stdbool.h
> >does (grub/bool.h?) and replace its usage in grub-core/term/tparm.c as 
> >separate
> >patch. If not bool please use enum here.
>
> There are 3 states, so a bool is not going to work I will
> switch to an enum for v2.

Perfect!

> >>+static int text_colorstate = -1;
>
> There already is a grub_term_color_state enum for this, which
> defines values 0-2, I want to know if setcolorstate has been
> called and text_colorstate contains a valid value, so I made
> this an int and use -1 to mean not set / invalid.
>
> I can see a number of solutions here:
>
> 1) Leave as is
> 2) Use:
>
> static grub_term_color_state text_colorstate = -1;
>
> Assuming the compiler will not warn about putting -1 in there.
>
> 3) Extend the grub_term_color_state like this:
>
> typedef enum
>   {
> /* Used for uninitialized grub_term_color_state variables */
> GRUB_TERM_COLOR_UNDEFINED = -1,
> /* The color used to display all text that does not use the
>user defined colors below.  */
> GRUB_TERM_COLOR_STANDARD = 0,
> /* The user defined colors for normal text.  */
> GRUB_TERM_COLOR_NORMAL,
> /* The user defined colors for highlighted text.  */
> GRUB_TERM_COLOR_HIGHLIGHT
>   }
> grub_term_color_state;
>
> Which solution would you prefer?

3rd, enum please.

Thanks,

Daniel

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

Re: [PATCH 3/4] Optionally print less messages at boot

[Daniel Kiper]

On Thu, Apr 05, 2018 at 07:53:23PM +0200, Hans de Goede wrote:
> Hi,
>
> On 05-04-18 14:12, Daniel Kiper wrote:
> >On Wed, Mar 28, 2018 at 04:50:27PM +0200, Hans de Goede wrote:
> >>The patch optionally makes grub not show any text (be fully quiet) when
> >>timeout_style=hidden is set and the user does not interrupt the boot.
> >>
> >>Combined with a later patch in this series which makes grub not touch
> >>the EFI console unless it actually has some text to print, this will keep
> >>the vendor logo which EFI put on the display in place until the kernel
> >>touches the display. Leading to a more smooth / seamless boot experience.
> >>
> >>At least Fedora/RHEL/CentOS and Ubuntu have been carrying patches for this
> >>for a long time now (since 2013). There have been several attempts to
> >>upstream these patches in the past already, which have been rejected
> >>because not everyone likes the quiet behavior.
> >
> >May I ask you to provide links to the relevant conversations?
>
> Sure you may ask, but I cannot really help with that because I was not
> involved in grub development back then.
>
> I've done a quick search of the mailinglist archives, but
> I'm afraid I could not find anything that way.

OK, let's leave it.

> >>This patch makes the quiet behavior optional and defaults to off, so
> >>unless grub is compiled with the new --enable-quiet-boot configure option
> >>this patch changes nothing.
> >
> >I am not sure why this should be build time option. I would see this as
> >a runtime option, e.g. boot_quiet shell variable or even timeout_style=quiet.
> >Hmmm... Latter is probably preferred.
>
> The problem is that grub already prints various things before reading
> its environment or config file.
>
> What I've understood from the history of this patches, removing those
> earlier prints (see e.g. the grub-core/boot/i386/pc/boot.S change in
> this patch) was objected against because that provides info that at
> least the first stage (for a classic PC BIOS boot setup) has successfully
> loaded.
>
> The menu code itself is already quiet when using timeout_style=hidden,
> the problem is the messages printed before (and after) the menu code.
>
> If you're happy with killing the messages shown before the config file
> is loaded (as most distros already do), then the rest could be made runtime
> configurable.

If they are printed deliberately I am not happy with killing them blindly.
However, I think that it can be done in other way. Could we add an option
to grub-mkimage (and other tools if it is needed) which disables printing
of these messages?

Daniel

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

Checksummed environments

[Kristian Amlie]

Hey, I work for Northern.tech, developing update software for embedded
Linux devices.

I have a question about GRUB's environment block: This block is not
checksummed, and hence I reckon it can become corrupt if power is lost
in the middle of a write.

This is an important safety criterion for us, so we've been thinking of
developing environment block checksumming as an extension to the
existing save_env and load_env commands. The most likely approach will
be to grab X amount of bytes at the end of the block and use these for
the checksum.

This would also allow us to fall back to an earlier environment file if
the current one is corrupt, hence implementing redundancy.

Is this something that the GRUB project would be interested in? We want
to upstream this if possible, since we think many people may benefit
from this.

-- 
Kristian

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