[meta-intel] [PATCH RFC/RFT] systemd-boot: Add patch to systemd boot stub to fix kernel command line

[Minchev, Todor]

Just a couple of small nitpicks. Please see below.

On Wed, 2017-04-12 at 17:35 -0700, California Sullivan wrote:
> As is, invoking the executable through the EFI shell causes the kernel
> command line to be replaced. Typing "bootx64" as we often do, causes the
> kernel command line to be replaced with just "bootx64", for example.
> This causes a boot failure since no root is defined. In addition, some
> boards, such as the Galileo, have garbage data in LoadOptions, which
> will also replace the command line and cause a boot failure.
> 
> There are two options to solve the issue. We could either completely
> disable the block that checks for LoadOptions data to replace the
> command line with, or we can append the data from LoadOptions to the
> kernel command line. The former would be a lot cleaner, especially in
> cases like the Galileo which have garbage data in LoadOptions, but it
> would also disable to ability to modify the command line on the fly,
> making it much less useful for debugging or bringing up a new board. The
> latter results in some useless information ending up on the kernel
> command line, but it does keep some flexibility in the system.
> 
> This patch implements the latter option.
> 
> Signed-off-by: California Sullivan <california.l.sullivan at intel.com>
> ---
> I'd appreciate some review on this since the last C code patch I wrote
> ended up being very buggy.
> 
>  ...ub-append-LoadOptions-to-command-line-ins.patch | 55 ++++++++++++++++++++++
>  .../systemd-boot/systemd-boot_%.bbappend           |  1 +
>  2 files changed, 56 insertions(+)
>  create mode 100644 common/recipes-bsp/systemd-boot/systemd-boot/0001-boot-efi-stub-append-LoadOptions-to-command-line-ins.patch
> 
> diff --git a/common/recipes-bsp/systemd-boot/systemd-boot/0001-boot-efi-stub-append-LoadOptions-to-command-line-ins.patch b/common/recipes-bsp/systemd-boot/systemd-boot/0001-boot-efi-stub-append-LoadOptions-to-command-line-ins.patch
> new file mode 100644
> index 0000000..94a4f60
> --- /dev/null
> +++ b/common/recipes-bsp/systemd-boot/systemd-boot/0001-boot-efi-stub-append-LoadOptions-to-command-line-ins.patch
> @@ -0,0 +1,55 @@
> +From 726a1722d30376d2e30561854809edfb05e34047 Mon Sep 17 00:00:00 2001
> +From: California Sullivan <california.l.sullivan at intel.com>
> +Date: Wed, 12 Apr 2017 11:10:52 -0700
> +Subject: [PATCH] boot: efi: stub: append LoadOptions to command line instead
> + of replace
> +
> +Upstream-status: Inappropriate
> +
> +This patch changes systemd-boot's stub program to append LoadOptions to
> +the command line instead of replace. This way, calling the binary from
> +the shell does not get rid of any kernel command line built in to the
> +binary.
> +
> +Signed-off-by: California Sullivan <california.l.sullivan at intel.com>
> +---
> + src/boot/efi/stub.c | 18 +++++++++++++++---
> + 1 file changed, 15 insertions(+), 3 deletions(-)
> +
> +diff --git a/src/boot/efi/stub.c b/src/boot/efi/stub.c
> +index 74e95fd..f8f3e95 100644
> +--- a/src/boot/efi/stub.c
> ++++ b/src/boot/efi/stub.c
> +@@ -94,14 +94,26 @@ EFI_STATUS efi_main(EFI_HANDLE image, EFI_SYSTEM_TABLE *sys_table) {
> +         if (!secure && loaded_image->LoadOptionsSize > 0 && *(CHAR16 *)loaded_image->LoadOptions != 0) {
> +                 CHAR16 *options;
> +                 CHAR8 *line;
> ++                UINTN options_len;
> ++                UINTN total_len;
> +                 UINTN i;
> ++                UINTN j;

UINTN options_len, total_len, i, j;

> +                 options = (CHAR16 *)loaded_image->LoadOptions;
> +-                cmdline_len = (loaded_image->LoadOptionsSize / sizeof(CHAR16)) * sizeof(CHAR8);
> +-                line = AllocatePool(cmdline_len);
> ++                options_len = (loaded_image->LoadOptionsSize / sizeof(CHAR16)) * sizeof(CHAR8);
> ++                total_len = cmdline_len + options_len + 1;
> ++                line = AllocatePool(total_len);
> ++
> +                 for (i = 0; i < cmdline_len; i++)
> +-                        line[i] = options[i];
> ++                        line[i] = cmdline[i];

I'd do something like this instead of looping over the string.
CopyMem(line, options, number_of_bytes);

> ++                line[i++] = ' ';
> ++
> ++                for (j = 0; j < options_len && i + j < total_len; j++)
> ++                        line[i + j] = options[j];

Same as above. CopyMem will be easier to understand

> +                 cmdline = line;
> ++                cmdline_len = total_len;
> + 
> + #ifdef SD_BOOT_LOG_TPM
> +                 /* Try to log any options to the TPM, escpecially manually edited options */
> +-- 
> +2.5.5
> +
> diff --git a/common/recipes-bsp/systemd-boot/systemd-boot_%.bbappend b/common/recipes-bsp/systemd-boot/systemd-boot_%.bbappend
> index 98eaf45..f13763b 100644
> --- a/common/recipes-bsp/systemd-boot/systemd-boot_%.bbappend
> +++ b/common/recipes-bsp/systemd-boot/systemd-boot_%.bbappend
> @@ -21,6 +21,7 @@ SRC_URI_append_intel-x86-common = " \
>              file://0004-sd-boot-Support-global-kernel-command-line-fragment-in-EFI-stub.patch \
>              file://0001-efi-boot.c-workaround-for-Joule-BIOS-hang.patch \
>              file://0001-sd-boot-stub-check-LoadOptions-contains-data.patch \
> +            file://0001-boot-efi-stub-append-LoadOptions-to-command-line-ins.patch \
>              "
>  
>  PACKAGE_ARCH_intel-x86-common = "${INTEL_COMMON_PACKAGE_ARCH}"
> -- 
> 2.5.5
>