Re: [PATCH v2 5/7] efi/capsule: Prepare for loading images with security header
From: Ard Biesheuvel
Date: Tue Mar 28 2017 - 09:50:19 EST
On 24 March 2017 at 17:34, Jan Kiszka <jan.kiszka@xxxxxxxxxxx> wrote:
> The Quark security header is nicely located in front of the capsule
> image, but we still need to pass the image to the update service as if
> there was none. Prepare efi_capsule_update and its user for this by
> defining and evaluating a EFI header displacement in the image located
> in memory. For standard-conforming capsules, this displacement is 0.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
Hello Jan,
Thanks for taking the time to respin this.
I played around with these patches a bit (I can't really test them
since I don't have the hardware), and I am not really happy with the
non-trivial changes to the generic code, only to allow a header
displacement.
So instead, I attempted to come up with an alternative which does not
use a displacement field, but makes the core capsule routines work
with a copy of the capsule header rather than mandating that it exists
at the start of the buffer. This way, we can override the code that
performs the copy, and make it originate from somewhere else.
Could you please have a look at
https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=quark-capsule
and tell me if that would work for you? I will send them out for
proper review in any case, but to avoid confusion (if I missed
something obvious), I don't want to send them out just yet.
Thanks,
Ard.
> ---
> drivers/firmware/efi/capsule-loader.c | 19 +++++++++++++------
> drivers/firmware/efi/capsule.c | 21 +++++++++++++++++----
> include/linux/efi.h | 1 +
> 3 files changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
> index 37d3f6e..59e2694 100644
> --- a/drivers/firmware/efi/capsule-loader.c
> +++ b/drivers/firmware/efi/capsule-loader.c
> @@ -26,6 +26,7 @@ struct capsule_info {
> long index;
> size_t count;
> size_t total_size;
> + unsigned int efi_hdr_displacement;
> struct page **pages;
> size_t page_bytes_remain;
> };
> @@ -83,6 +84,8 @@ static int efi_capsule_setup_info(struct capsule_info *cap_info,
> return ret;
> }
>
> + cap_info->efi_hdr_displacement = 0;
> +
> cap_info->total_size = cap_hdr->imagesize;
> temp_page = krealloc(cap_info->pages,
> pages_needed * sizeof(void *),
> @@ -103,16 +106,20 @@ static int efi_capsule_setup_info(struct capsule_info *cap_info,
> **/
> static ssize_t efi_capsule_submit_update(struct capsule_info *cap_info)
> {
> + efi_capsule_header_t *cap_hdr;
> + void *mapped_pages;
> int ret;
> - void *cap_hdr_temp;
>
> - cap_hdr_temp = vmap(cap_info->pages, cap_info->index,
> - VM_MAP, PAGE_KERNEL);
> - if (!cap_hdr_temp)
> + mapped_pages = vmap(cap_info->pages, cap_info->index,
> + VM_MAP, PAGE_KERNEL);
> + if (!mapped_pages)
> return -ENOMEM;
>
> - ret = efi_capsule_update(cap_hdr_temp, cap_info->pages);
> - vunmap(cap_hdr_temp);
> + cap_hdr = mapped_pages + cap_info->efi_hdr_displacement;
> +
> + ret = efi_capsule_update(cap_hdr, cap_info->efi_hdr_displacement,
> + cap_info->pages);
> + vunmap(mapped_pages);
> if (ret) {
> pr_err("capsule update failed\n");
> return ret;
> diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
> index 6eedff4..a60c4c4 100644
> --- a/drivers/firmware/efi/capsule.c
> +++ b/drivers/firmware/efi/capsule.c
> @@ -184,6 +184,8 @@ efi_capsule_update_locked(efi_capsule_header_t *capsule,
> /**
> * efi_capsule_update - send a capsule to the firmware
> * @capsule: capsule to send to firmware
> + * @efi_hdr_displacement: EFI header offset on first data page (only needed for
> + * non-conforming CSH capsules)
> * @pages: an array of capsule data pages
> *
> * Build a scatter gather list with EFI capsule block descriptors to
> @@ -214,9 +216,12 @@ efi_capsule_update_locked(efi_capsule_header_t *capsule,
> *
> * Return 0 on success, a converted EFI status code on failure.
> */
> -int efi_capsule_update(efi_capsule_header_t *capsule, struct page **pages)
> +int efi_capsule_update(efi_capsule_header_t *capsule,
> + unsigned int efi_hdr_displacement,
> + struct page **pages)
> {
> u32 imagesize = capsule->imagesize;
> + u32 total_size = imagesize + efi_hdr_displacement;
> efi_guid_t guid = capsule->guid;
> unsigned int count, sg_count;
> u32 flags = capsule->flags;
> @@ -224,11 +229,14 @@ int efi_capsule_update(efi_capsule_header_t *capsule, struct page **pages)
> int rv, reset_type;
> int i, j;
>
> - rv = efi_capsule_supported(guid, flags, imagesize, &reset_type);
> + if (efi_hdr_displacement > PAGE_SIZE - sizeof(efi_capsule_header_t))
> + return -EINVAL;
> +
> + rv = efi_capsule_supported(guid, flags, total_size, &reset_type);
> if (rv)
> return rv;
>
> - count = DIV_ROUND_UP(imagesize, PAGE_SIZE);
> + count = DIV_ROUND_UP(total_size, PAGE_SIZE);
> sg_count = sg_pages_num(count);
>
> sg_pages = kzalloc(sg_count * sizeof(*sg_pages), GFP_KERNEL);
> @@ -255,8 +263,13 @@ int efi_capsule_update(efi_capsule_header_t *capsule, struct page **pages)
> for (j = 0; j < SGLIST_PER_PAGE && count > 0; j++) {
> u64 sz = min_t(u64, imagesize, PAGE_SIZE);
>
> - sglist[j].length = sz;
> sglist[j].data = page_to_phys(*pages++);
> + if (efi_hdr_displacement > 0) {
> + sglist[j].data += efi_hdr_displacement;
> + sz -= efi_hdr_displacement;
> + efi_hdr_displacement = 0;
> + }
> + sglist[j].length = sz;
>
> imagesize -= sz;
> count--;
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 94d34e0..d83095c6 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1403,6 +1403,7 @@ extern int efi_capsule_supported(efi_guid_t guid, u32 flags,
> size_t size, int *reset);
>
> extern int efi_capsule_update(efi_capsule_header_t *capsule,
> + unsigned int efi_hdr_displacement,
> struct page **pages);
>
> #ifdef CONFIG_EFI_RUNTIME_MAP
> --
> 2.10.2
>