Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753815AbdC1Ntx (ORCPT ); Tue, 28 Mar 2017 09:49:53 -0400 Received: from mail-it0-f51.google.com ([209.85.214.51]:37894 "EHLO mail-it0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751908AbdC1Ntt (ORCPT ); Tue, 28 Mar 2017 09:49:49 -0400 MIME-Version: 1.0 In-Reply-To: <47e87c8eccc0ee5c4033ece037c0bffc32782876.1490376860.git.jan.kiszka@siemens.com> References: <47e87c8eccc0ee5c4033ece037c0bffc32782876.1490376860.git.jan.kiszka@siemens.com> From: Ard Biesheuvel Date: Tue, 28 Mar 2017 14:49:42 +0100 Message-ID: Subject: Re: [PATCH v2 5/7] efi/capsule: Prepare for loading images with security header To: Jan Kiszka , Matt Fleming Cc: "linux-efi@vger.kernel.org" , Linux Kernel Mailing List , Andy Shevchenko , "Bryan O'Donoghue" , Hock Leong Kweh , Borislav Petkov , Sascha Weisenberger Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6796 Lines: 164 On 24 March 2017 at 17:34, Jan Kiszka 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 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 >