Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220916081441.1993492-1-ilias.apalodimas@linaro.org> <20220916081441.1993492-2-ilias.apalodimas@linaro.org>
In-Reply-To: <20220916081441.1993492-2-ilias.apalodimas@linaro.org>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Fri, 16 Sep 2022 10:26:46 +0200
Message-ID: <CAMj1kXEtzCF-19MHNmBB45t3X343bd1G+PRNn=h8=PMfLVq+pA@mail.gmail.com>
Subject: Re: [PATCH 2/2] efi/libstub: measure EFI LoadOptions
To:     Ilias Apalodimas <ilias.apalodimas@linaro.org>
Cc:     pjones@redhat.com, daniel.kiper@oracle.com,
        James.Bottomley@hansenpartnership.com, leif@nuviainc.com,
        jroedel@suse.de, Sunil V L <sunilvl@ventanamicro.com>,
        Baskov Evgeniy <baskov@ispras.ru>,
        Palmer Dabbelt <palmer@rivosinc.com>,
        linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, 16 Sept 2022 at 10:15, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> The EFI TCG spec, in =C2=A710.2.6 Measuring UEFI Variables and UEFI GPT D=
ata,
> is  measuring the entire UEFI_LOAD_OPTION (in PCR5).  As a result boot
> variables that point to the same UEFI application but with different
> optional data,  will have distinct measurements.
>

That is not the main problem. The main problem is that
LoadImage()/StartImage() may be used to invoke things beyond Boot####
options, and at StartImage() time, the load options could be anything.
So not measuring the load options when the image is actually being
invoked is a huge oversight.


> However, PCR5 is used for more than that and there might be a need to use
> a PCR with a more limited scope which measures our initramfs and
> LoadOptions.
>
> So add a measurement in PCR9 (which we already use for our initrd) and
> extend it with the LoadOption measurements
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  .../firmware/efi/libstub/efi-stub-helper.c    | 21 +++++++++++++++++++
>  drivers/firmware/efi/libstub/efistub.h        |  1 +
>  2 files changed, 22 insertions(+)
>
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/fir=
mware/efi/libstub/efi-stub-helper.c
> index 3ef4867344b9..5b03248527c6 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -370,6 +370,27 @@ char *efi_convert_cmdline(efi_loaded_image_t *image,=
 int *cmd_line_len)
>         int options_bytes =3D 0, safe_options_bytes =3D 0;  /* UTF-8 byte=
s */
>         bool in_quote =3D false;
>         efi_status_t status;
> +       static const struct efi_measured_event load_options_tcg2_event =
=3D {
> +               {
> +                       sizeof(load_options_tcg2_event) + sizeof("Load Op=
tions"),
> +                       {
> +                               sizeof(load_options_tcg2_event.event_data=
.event_header),
> +                               EFI_TCG2_EVENT_HEADER_VERSION,
> +                               9,
> +                               EV_EVENT_TAG,
> +                       },
> +               },
> +               {
> +                       LOAD_OPTIONS_EVENT_TAG_ID,
> +                       sizeof("Load Options"),
> +               },
> +               { "Load Options" },
> +       };
> +
> +       if (options_chars > 0)
> +               efi_measure_tagged_event((unsigned long) options,
> +                                        (unsigned long) options_chars,
> +                                        &load_options_tcg2_event);
>

The name 'options_chars' is a bit misleading here, as it is actually
the size in bytes at this point.

>         efi_apply_loadoptions_quirk((const void **)&options, &options_cha=
rs);
>         options_chars /=3D sizeof(*options);
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/ef=
i/libstub/efistub.h
> index cb7eb5ed9f14..e3605b383964 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -741,6 +741,7 @@ union apple_properties_protocol {
>  typedef u32 efi_tcg2_event_log_format;
>
>  #define INITRD_EVENT_TAG_ID 0x8F3B22ECU
> +#define LOAD_OPTIONS_EVENT_TAG_ID 0x8F3B22EDU

Is this an arbitrarily chosen value?

>  #define EV_EVENT_TAG 0x00000006U
>  #define EFI_TCG2_EVENT_HEADER_VERSION  0x1
>
> --
> 2.34.1
>