Re: [Buildroot] [PATCH 1/1] boot/grub2: add '--memdisk' option to grub-makeimage

["Yann E. MORIN" <yann.morin.1998@free.fr>]

Alexey, All,

Thank you for your contribution. I am so sorry that we did not look at
your patch earlier. Recent evolutions (by Köry, added in Cc) in grub
had me dig out the old patches we still had pending about grub. So if
you are still interested, here is a review of your patch.

On 2020-03-09 12:55 +0300, Alexey Lukyanchuk spake thusly:
> The memdisk is a virtual disk device viewed by grub.
> Memdisk can store some files(pictures, gpg key for verify module,etc), grub modules, into a core.img.

Commit log should be wrapped at 72 chars, as is standard practice with
git. I can fix that when applying, though.

> In UEFI platform, grub-mkimage does not pose a limit on the size of memdisk.

Does that mean there is a limit when booting in legacy BIOS? What is
that limit? Should we enforce/validate it when building the grub image?

> It's good solution for secure-boot or embedded systems

So, I was a bit curious about memdisk, and how one was supposed to use
it. But as [0] puts it, this features sorely lacks documentation. I was
only able to find a quick reference to memdisk in the manual [1] when
dealing about the naming of devices. And as usual, StackOverflow to the
rescue [2]. And then another example to setup LUKS on Debian [3].

It might be a good idea to add those 4 references to the commit log (or
the help text too, maybe):

[0] https://phip1611.de/blog/adding-and-using-files-to-from-the-grub-memdisk-grub-standalone/
[1] https://www.gnu.org/software/grub/manual/grub/grub.html#Device-syntax
[2] https://unix.stackexchange.com/questions/253657/actual-usage-of-grub-mkimage-config/273334#273334
[3] https://cryptsetup-team.pages.debian.net/cryptsetup/encrypted-boot.html

> Signed-off-by: Alexey Lukyanchuk <skif@skif-web.ru>
> ---
>  boot/grub2/Config.in |  6 ++++++
>  boot/grub2/grub2.mk  | 11 +++++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/boot/grub2/Config.in b/boot/grub2/Config.in
> index e45133999e..6ee2f351d9 100644
> --- a/boot/grub2/Config.in
> +++ b/boot/grub2/Config.in
> @@ -107,6 +107,12 @@ config BR2_TARGET_GRUB2_BUILTIN_CONFIG
>  	  device and other configuration parameters, but however menu
>  	  entries cannot be described in this embedded configuration.
>  
> +config BR2_TARGET_GRUB2_MEMDISK_DIR
> +	string "memdisk"
> +	help
> +	  Path to directory that will be a memdisk (-m option in 
> +	  grub2-mkimage)

It would be nice to have just a little blurb on how to use files from a
memdisk; maybe something like (if I understood all the above references
correctly):

    For a file BR2_TARGET_GRUB2_MEMDISK_DIR/boot/grub/foo, it can
    be accessed from grub with: (memdisk)/foo

So, to avoid any confusion or runtime issue, maybe we should enforce
that all files are in BR2_TARGET_GRUB2_MEMDISK_DIR/boot/grub/ (see
below [4]).

>  config BR2_TARGET_GRUB2_INSTALL_TOOLS
>  	bool "install tools"
>  	help
> diff --git a/boot/grub2/grub2.mk b/boot/grub2/grub2.mk
> index a202525d71..0627c6857f 100644
> --- a/boot/grub2/grub2.mk
> +++ b/boot/grub2/grub2.mk
> @@ -22,6 +22,7 @@ endif
>  GRUB2_BUILTIN_MODULES = $(call qstrip,$(BR2_TARGET_GRUB2_BUILTIN_MODULES))
>  GRUB2_BUILTIN_CONFIG = $(call qstrip,$(BR2_TARGET_GRUB2_BUILTIN_CONFIG))
>  GRUB2_BOOT_PARTITION = $(call qstrip,$(BR2_TARGET_GRUB2_BOOT_PARTITION))
> +GRUB2_MEMDISK_DIR = $(call qstrip,$(BR2_TARGET_GRUB2_MEMDISK_DIR))
>  
>  ifeq ($(BR2_TARGET_GRUB2_I386_PC),y)
>  GRUB2_IMAGE = $(BINARIES_DIR)/grub.img
> @@ -87,6 +88,15 @@ GRUB2_CONF_ENV = \
>  	TARGET_OBJCOPY="$(TARGET_OBJCOPY)" \
>  	TARGET_STRIP="$(TARGET_CROSS)strip"
>  
> +ifneq ($(GRUB2_MEMDISK_DIR),)
> +GRUB2_POST_BUILD_HOOKS += GRUB2_MAKE_MEMDISK
> +define GRUB2_MAKE_MEMDISK
> +	mkdir -p $(@D)/grub2_memdisk
> +	cp -a $(GRUB2_MEMDISK_DIR)/* $(@D)/grub2_memdisk/
> +	tar -cvf $(@D)/memdisk.tar -C $(@D)/grub2_memdisk/ .

[4] here we would enforce the prefix:

    mkdir -p $(@D)/grub2_memdisk/boot/grub
    cp -a $(GRUB2_MEMDISK_DIR)/* $(@D)/grub2_memdisk/boot/grub/
    tar -cvf $(@D)/memdisk.tar -C $(@D)/grub2_memdisk/ .

(if we do that, then we must also propagate that top the help text).

Additionally, generating a tarball like that is not reproducible; we do
have a tar helper that generates reproducible archives, but it is a
shell function [5]... Maybe we should make that an actual tar wrapper...
But the memdisk undocumentation speaks about "GNU tar format", so not
sure our reproducible archives would be usable...

Also, this requires that both the 'memdisk' and 'tarfs' modules be built
into the grub image. Maybe we should validate that they are in the list?
Or that we should just append them if not?

[5] https://git.buildroot.org/buildroot/tree/support/download/helpers

> +endef
> +endif
> +
>  GRUB2_CONF_OPTS = \
>  	--target=$(GRUB2_TARGET) \
>  	--with-platform=$(GRUB2_PLATFORM) \
> @@ -121,6 +131,7 @@ define GRUB2_INSTALL_IMAGES_CMDS
>  		-O $(GRUB2_TUPLE) \
>  		-o $(GRUB2_IMAGE) \
>  		-p "$(GRUB2_PREFIX)" \
> +		$(if $(BR2_TARGET_GRUB2_MEMDISK_DIR),-m $(@D)/memdisk.tar) \

BR2_TARGET_GRUB2_MEMDISK_DIR is a Kconfig string, so it is
double-quoted, so it is never empty for make. So the test should be
against GRUB2_MEMDISK_DIR, like you did a few lines before.

However, as per grub-mkimage manpage, this overrides any previous -p
option (but later -p options can again override it). Not sure how that
would work with our passing of -p, just before...

Anyway, this looks like a very interesting feature which we would like
to support in Buildroot (I know of some people that would like to have
it to display a splashscreen, for example).

Again, sorry for the delay it took us to look at your patch, but as you
can see, there is quite some food on the table:
    https://patchwork.ozlabs.org/project/buildroot/list/

Regards,
Yann E. MORIN.

>  		$(if $(GRUB2_BUILTIN_CONFIG),-c $(GRUB2_BUILTIN_CONFIG)) \
>  		$(GRUB2_BUILTIN_MODULES)
>  	mkdir -p $(dir $(GRUB2_CFG))

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot