Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1610966 > unrolled thread

Re: [PATCH v2 5/7] efi/capsule: Prepare for loading images with security header

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 5/7] efi/capsule: Prepare for loading images with  security header Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-03-28 16:00 +0200
    Re: [PATCH v2 5/7] efi/capsule: Prepare for loading images with  security header Jan Kiszka <jan.kiszka@siemens.com> - 2017-03-28 17:20 +0200
      Re: [PATCH v2 5/7] efi/capsule: Prepare for loading images with  security header Jan Kiszka <jan.kiszka@siemens.com> - 2017-03-28 17:50 +0200
        Re: [PATCH v2 5/7] efi/capsule: Prepare for loading images with  security header Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-03-28 18:00 +0200
          Re: [PATCH v2 5/7] efi/capsule: Prepare for loading images with  security header Jan Kiszka <jan.kiszka@siemens.com> - 2017-03-28 18:30 +0200
            Re: [PATCH v2 5/7] efi/capsule: Prepare for loading images with  security header Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-03-28 19:30 +0200
              Re: [PATCH v2 5/7] efi/capsule: Prepare for loading images with  security header Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-03-28 19:30 +0200
                Re: [PATCH v2 5/7] efi/capsule: Prepare for loading images with  security header Jan Kiszka <jan.kiszka@siemens.com> - 2017-03-30 11:10 +0200
                Re: [PATCH v2 5/7] efi/capsule: Prepare for loading images with  security header Jan Kiszka <jan.kiszka@siemens.com> - 2017-04-04 19:50 +0200

#1610966 — Re: [PATCH v2 5/7] efi/capsule: Prepare for loading images with security header

On 24 March 2017 at 17:34, Jan Kiszka <jan.kiszka@siemens.com> 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@siemens.com>

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
>

[toc] | [next] | [standalone]

#1611084

On 2017-03-28 15:49, Ard Biesheuvel wrote:
> On 24 March 2017 at 17:34, Jan Kiszka <jan.kiszka@siemens.com> 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@siemens.com>
> 
> 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.

There is more needed to make things work again, maybe around passing the
right image size. I'm looking into this.

Another observation: Making EFI_CAPSULE_QUIRK_QUARK_CSH select
EFI_CAPSULE_LOADER technically resolves the problem that the platform
code would otherwise need something from a capsule loader module.
However, a logical configuration would rather make the quirk depend on
the loader, wouldn't it? But I'm fine with both.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

[toc] | [prev] | [next] | [standalone]

#1611116

On 2017-03-28 17:13, Jan Kiszka wrote:
> On 2017-03-28 15:49, Ard Biesheuvel wrote:
>> On 24 March 2017 at 17:34, Jan Kiszka <jan.kiszka@siemens.com> 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@siemens.com>
>>
>> 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.
> 
> There is more needed to make things work again, maybe around passing the
> right image size. I'm looking into this.
> 

This makes CSH images being accepted again:

diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 4b6f93f..a4e2311 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -562,6 +562,8 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
 {
 	struct quark_security_header *csh = kbuff;
 
+	cap_info->total_size = 0;
+
 	if (!x86_match_cpu(quark_ids))
 		goto fallback;
 
@@ -587,12 +589,16 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
 
 	kbuff += csh->headersize;
 
+	cap_info->total_size = csh->headersize;
+
 fallback:
 	if (hdr_bytes < sizeof(efi_capsule_header_t))
 		return 0;
 
 	memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
 
+	cap_info->total_size += cap_info->header.imagesize;
+
 	return __efi_capsule_setup_info(cap_info);
 }
 
diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
index e851951..40dc354 100644
--- a/drivers/firmware/efi/capsule-loader.c
+++ b/drivers/firmware/efi/capsule-loader.c
@@ -42,7 +42,7 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
 	int ret;
 	void *temp_page;
 
-	pages_needed = ALIGN(cap_info->header.imagesize, PAGE_SIZE) / PAGE_SIZE;
+	pages_needed = ALIGN(cap_info->total_size, PAGE_SIZE) / PAGE_SIZE;
 
 	if (pages_needed == 0) {
 		pr_err("invalid capsule size");
@@ -59,7 +59,6 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
 		return ret;
 	}
 
-	cap_info->total_size = cap_info->header.imagesize;
 	temp_page = krealloc(cap_info->pages,
 			     pages_needed * sizeof(void *),
 			     GFP_KERNEL | __GFP_ZERO);
@@ -87,6 +86,8 @@ int __weak efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
 
 	memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
 
+	cap_info->total_size = cap_info->header.imagesize;
+
 	return __efi_capsule_setup_info(cap_info);
 }
 
But then my changes to efi_capsule_update are missing the present the
right format to the loader. As efi_capsule_update needs to lay out the
sg-list in as special way, excluding the CSH on the first page, it needs
to know about the displacement.

Any suggestions how to address that without rolling back to my aproach?

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

[toc] | [prev] | [next] | [standalone]

#1611138

On 28 March 2017 at 16:43, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-03-28 17:13, Jan Kiszka wrote:
>> On 2017-03-28 15:49, Ard Biesheuvel wrote:
[..]
>>> 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.
>>
>> There is more needed to make things work again, maybe around passing the
>> right image size. I'm looking into this.
>>
>
> This makes CSH images being accepted again:
>
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index 4b6f93f..a4e2311 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -562,6 +562,8 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>  {
>         struct quark_security_header *csh = kbuff;
>
> +       cap_info->total_size = 0;
> +
>         if (!x86_match_cpu(quark_ids))
>                 goto fallback;
>
> @@ -587,12 +589,16 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>
>         kbuff += csh->headersize;
>
> +       cap_info->total_size = csh->headersize;
> +
>  fallback:
>         if (hdr_bytes < sizeof(efi_capsule_header_t))
>                 return 0;
>
>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>
> +       cap_info->total_size += cap_info->header.imagesize;
> +
>         return __efi_capsule_setup_info(cap_info);
>  }
>
> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
> index e851951..40dc354 100644
> --- a/drivers/firmware/efi/capsule-loader.c
> +++ b/drivers/firmware/efi/capsule-loader.c
> @@ -42,7 +42,7 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>         int ret;
>         void *temp_page;
>
> -       pages_needed = ALIGN(cap_info->header.imagesize, PAGE_SIZE) / PAGE_SIZE;
> +       pages_needed = ALIGN(cap_info->total_size, PAGE_SIZE) / PAGE_SIZE;
>
>         if (pages_needed == 0) {
>                 pr_err("invalid capsule size");
> @@ -59,7 +59,6 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>                 return ret;
>         }
>
> -       cap_info->total_size = cap_info->header.imagesize;
>         temp_page = krealloc(cap_info->pages,
>                              pages_needed * sizeof(void *),
>                              GFP_KERNEL | __GFP_ZERO);
> @@ -87,6 +86,8 @@ int __weak efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>
>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>
> +       cap_info->total_size = cap_info->header.imagesize;
> +
>         return __efi_capsule_setup_info(cap_info);
>  }
>

OK, thanks for debugging that.

> But then my changes to efi_capsule_update are missing the present the
> right format to the loader. As efi_capsule_update needs to lay out the
> sg-list in as special way, excluding the CSH on the first page, it needs
> to know about the displacement.
>
> Any suggestions how to address that without rolling back to my aproach?
>

OK, I'm a bit lost now: for my understanding, could you please
reiterate how the CSH image deviates from the ordinary one? Or more
specifically, what exactly is preventing us from simply chopping off
the CSH header and pushing the capsule header + payload into
/dev/capsule_loader?

[toc] | [prev] | [next] | [standalone]

#1611185

On 2017-03-28 17:52, Ard Biesheuvel wrote:
> On 28 March 2017 at 16:43, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2017-03-28 17:13, Jan Kiszka wrote:
>>> On 2017-03-28 15:49, Ard Biesheuvel wrote:
> [..]
>>>> 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.
>>>
>>> There is more needed to make things work again, maybe around passing the
>>> right image size. I'm looking into this.
>>>
>>
>> This makes CSH images being accepted again:
>>
>> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
>> index 4b6f93f..a4e2311 100644
>> --- a/arch/x86/platform/efi/quirks.c
>> +++ b/arch/x86/platform/efi/quirks.c
>> @@ -562,6 +562,8 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>  {
>>         struct quark_security_header *csh = kbuff;
>>
>> +       cap_info->total_size = 0;
>> +
>>         if (!x86_match_cpu(quark_ids))
>>                 goto fallback;
>>
>> @@ -587,12 +589,16 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>
>>         kbuff += csh->headersize;
>>
>> +       cap_info->total_size = csh->headersize;
>> +
>>  fallback:
>>         if (hdr_bytes < sizeof(efi_capsule_header_t))
>>                 return 0;
>>
>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>
>> +       cap_info->total_size += cap_info->header.imagesize;
>> +
>>         return __efi_capsule_setup_info(cap_info);
>>  }
>>
>> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
>> index e851951..40dc354 100644
>> --- a/drivers/firmware/efi/capsule-loader.c
>> +++ b/drivers/firmware/efi/capsule-loader.c
>> @@ -42,7 +42,7 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>         int ret;
>>         void *temp_page;
>>
>> -       pages_needed = ALIGN(cap_info->header.imagesize, PAGE_SIZE) / PAGE_SIZE;
>> +       pages_needed = ALIGN(cap_info->total_size, PAGE_SIZE) / PAGE_SIZE;
>>
>>         if (pages_needed == 0) {
>>                 pr_err("invalid capsule size");
>> @@ -59,7 +59,6 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>                 return ret;
>>         }
>>
>> -       cap_info->total_size = cap_info->header.imagesize;
>>         temp_page = krealloc(cap_info->pages,
>>                              pages_needed * sizeof(void *),
>>                              GFP_KERNEL | __GFP_ZERO);
>> @@ -87,6 +86,8 @@ int __weak efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>
>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>
>> +       cap_info->total_size = cap_info->header.imagesize;
>> +
>>         return __efi_capsule_setup_info(cap_info);
>>  }
>>
> 
> OK, thanks for debugging that.
> 
>> But then my changes to efi_capsule_update are missing the present the
>> right format to the loader. As efi_capsule_update needs to lay out the
>> sg-list in as special way, excluding the CSH on the first page, it needs
>> to know about the displacement.
>>
>> Any suggestions how to address that without rolling back to my aproach?
>>
> 
> OK, I'm a bit lost now: for my understanding, could you please
> reiterate how the CSH image deviates from the ordinary one? Or more
> specifically, what exactly is preventing us from simply chopping off
> the CSH header and pushing the capsule header + payload into
> /dev/capsule_loader?
> 

Devices that mandate a signed capsule for updates expect the CSH in
front of the regular capsule image. The interface to UEFI remains
unchanged, i.e. you pass the virtually chopped off capsule, but you have
to leave the CSH in RAM right in front of that very same image. It's a
"nice" side-channel API.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

[toc] | [prev] | [next] | [standalone]

#1611236

On 28 March 2017 at 17:18, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-03-28 17:52, Ard Biesheuvel wrote:
>> On 28 March 2017 at 16:43, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> On 2017-03-28 17:13, Jan Kiszka wrote:
>>>> On 2017-03-28 15:49, Ard Biesheuvel wrote:
>> [..]
>>>>> 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.
>>>>
>>>> There is more needed to make things work again, maybe around passing the
>>>> right image size. I'm looking into this.
>>>>
>>>
>>> This makes CSH images being accepted again:
>>>
>>> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
>>> index 4b6f93f..a4e2311 100644
>>> --- a/arch/x86/platform/efi/quirks.c
>>> +++ b/arch/x86/platform/efi/quirks.c
>>> @@ -562,6 +562,8 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>  {
>>>         struct quark_security_header *csh = kbuff;
>>>
>>> +       cap_info->total_size = 0;
>>> +
>>>         if (!x86_match_cpu(quark_ids))
>>>                 goto fallback;
>>>
>>> @@ -587,12 +589,16 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>
>>>         kbuff += csh->headersize;
>>>
>>> +       cap_info->total_size = csh->headersize;
>>> +
>>>  fallback:
>>>         if (hdr_bytes < sizeof(efi_capsule_header_t))
>>>                 return 0;
>>>
>>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>>
>>> +       cap_info->total_size += cap_info->header.imagesize;
>>> +
>>>         return __efi_capsule_setup_info(cap_info);
>>>  }
>>>
>>> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
>>> index e851951..40dc354 100644
>>> --- a/drivers/firmware/efi/capsule-loader.c
>>> +++ b/drivers/firmware/efi/capsule-loader.c
>>> @@ -42,7 +42,7 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>>         int ret;
>>>         void *temp_page;
>>>
>>> -       pages_needed = ALIGN(cap_info->header.imagesize, PAGE_SIZE) / PAGE_SIZE;
>>> +       pages_needed = ALIGN(cap_info->total_size, PAGE_SIZE) / PAGE_SIZE;
>>>
>>>         if (pages_needed == 0) {
>>>                 pr_err("invalid capsule size");
>>> @@ -59,7 +59,6 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>>                 return ret;
>>>         }
>>>
>>> -       cap_info->total_size = cap_info->header.imagesize;
>>>         temp_page = krealloc(cap_info->pages,
>>>                              pages_needed * sizeof(void *),
>>>                              GFP_KERNEL | __GFP_ZERO);
>>> @@ -87,6 +86,8 @@ int __weak efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>
>>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>>
>>> +       cap_info->total_size = cap_info->header.imagesize;
>>> +
>>>         return __efi_capsule_setup_info(cap_info);
>>>  }
>>>
>>
>> OK, thanks for debugging that.
>>
>>> But then my changes to efi_capsule_update are missing the present the
>>> right format to the loader. As efi_capsule_update needs to lay out the
>>> sg-list in as special way, excluding the CSH on the first page, it needs
>>> to know about the displacement.
>>>
>>> Any suggestions how to address that without rolling back to my aproach?
>>>
>>
>> OK, I'm a bit lost now: for my understanding, could you please
>> reiterate how the CSH image deviates from the ordinary one? Or more
>> specifically, what exactly is preventing us from simply chopping off
>> the CSH header and pushing the capsule header + payload into
>> /dev/capsule_loader?
>>
>
> Devices that mandate a signed capsule for updates expect the CSH in
> front of the regular capsule image. The interface to UEFI remains
> unchanged, i.e. you pass the virtually chopped off capsule, but you have
> to leave the CSH in RAM right in front of that very same image. It's a
> "nice" side-channel API.
>

Wow, that is worse than I thought.

So my suggestion (which I coded up, please pull again), is to replace
the array of struct page pointers with an array of physical addresses
in the capsule_info struct. This way, your special version of
efi_capsule_setup_info() can advance the first one by the size of the
header.

I hope this works for you. I am not sure whether imagesize needs to be
modified, so I left it alone for now (but I suspect it should be).

[toc] | [prev] | [next] | [standalone]

#1611238

On 28 March 2017 at 18:17, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 28 March 2017 at 17:18, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2017-03-28 17:52, Ard Biesheuvel wrote:
>>> On 28 March 2017 at 16:43, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>> On 2017-03-28 17:13, Jan Kiszka wrote:
>>>>> On 2017-03-28 15:49, Ard Biesheuvel wrote:
>>> [..]
>>>>>> 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.
>>>>>
>>>>> There is more needed to make things work again, maybe around passing the
>>>>> right image size. I'm looking into this.
>>>>>
>>>>
>>>> This makes CSH images being accepted again:
>>>>
>>>> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
>>>> index 4b6f93f..a4e2311 100644
>>>> --- a/arch/x86/platform/efi/quirks.c
>>>> +++ b/arch/x86/platform/efi/quirks.c
>>>> @@ -562,6 +562,8 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>>  {
>>>>         struct quark_security_header *csh = kbuff;
>>>>
>>>> +       cap_info->total_size = 0;
>>>> +
>>>>         if (!x86_match_cpu(quark_ids))
>>>>                 goto fallback;
>>>>
>>>> @@ -587,12 +589,16 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>>
>>>>         kbuff += csh->headersize;
>>>>
>>>> +       cap_info->total_size = csh->headersize;
>>>> +
>>>>  fallback:
>>>>         if (hdr_bytes < sizeof(efi_capsule_header_t))
>>>>                 return 0;
>>>>
>>>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>>>
>>>> +       cap_info->total_size += cap_info->header.imagesize;
>>>> +
>>>>         return __efi_capsule_setup_info(cap_info);
>>>>  }
>>>>
>>>> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
>>>> index e851951..40dc354 100644
>>>> --- a/drivers/firmware/efi/capsule-loader.c
>>>> +++ b/drivers/firmware/efi/capsule-loader.c
>>>> @@ -42,7 +42,7 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>>>         int ret;
>>>>         void *temp_page;
>>>>
>>>> -       pages_needed = ALIGN(cap_info->header.imagesize, PAGE_SIZE) / PAGE_SIZE;
>>>> +       pages_needed = ALIGN(cap_info->total_size, PAGE_SIZE) / PAGE_SIZE;
>>>>
>>>>         if (pages_needed == 0) {
>>>>                 pr_err("invalid capsule size");
>>>> @@ -59,7 +59,6 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>>>                 return ret;
>>>>         }
>>>>
>>>> -       cap_info->total_size = cap_info->header.imagesize;
>>>>         temp_page = krealloc(cap_info->pages,
>>>>                              pages_needed * sizeof(void *),
>>>>                              GFP_KERNEL | __GFP_ZERO);
>>>> @@ -87,6 +86,8 @@ int __weak efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>>
>>>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>>>
>>>> +       cap_info->total_size = cap_info->header.imagesize;
>>>> +
>>>>         return __efi_capsule_setup_info(cap_info);
>>>>  }
>>>>
>>>
>>> OK, thanks for debugging that.
>>>
>>>> But then my changes to efi_capsule_update are missing the present the
>>>> right format to the loader. As efi_capsule_update needs to lay out the
>>>> sg-list in as special way, excluding the CSH on the first page, it needs
>>>> to know about the displacement.
>>>>
>>>> Any suggestions how to address that without rolling back to my aproach?
>>>>
>>>
>>> OK, I'm a bit lost now: for my understanding, could you please
>>> reiterate how the CSH image deviates from the ordinary one? Or more
>>> specifically, what exactly is preventing us from simply chopping off
>>> the CSH header and pushing the capsule header + payload into
>>> /dev/capsule_loader?
>>>
>>
>> Devices that mandate a signed capsule for updates expect the CSH in
>> front of the regular capsule image. The interface to UEFI remains
>> unchanged, i.e. you pass the virtually chopped off capsule, but you have
>> to leave the CSH in RAM right in front of that very same image. It's a
>> "nice" side-channel API.
>>
>
> Wow, that is worse than I thought.
>
> So my suggestion (which I coded up, please pull again),

Hmm, it does not build on x86 atm. Let me fix that up first (~15 min)

> is to replace
> the array of struct page pointers with an array of physical addresses
> in the capsule_info struct. This way, your special version of
> efi_capsule_setup_info() can advance the first one by the size of the
> header.
>
> I hope this works for you. I am not sure whether imagesize needs to be
> modified, so I left it alone for now (but I suspect it should be).

[toc] | [prev] | [next] | [standalone]

#1612754

On 2017-03-28 19:23, Ard Biesheuvel wrote:
> On 28 March 2017 at 18:17, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 28 March 2017 at 17:18, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> On 2017-03-28 17:52, Ard Biesheuvel wrote:
>>>> On 28 March 2017 at 16:43, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>> On 2017-03-28 17:13, Jan Kiszka wrote:
>>>>>> On 2017-03-28 15:49, Ard Biesheuvel wrote:
>>>> [..]
>>>>>>> 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.
>>>>>>
>>>>>> There is more needed to make things work again, maybe around passing the
>>>>>> right image size. I'm looking into this.
>>>>>>
>>>>>
>>>>> This makes CSH images being accepted again:
>>>>>
>>>>> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
>>>>> index 4b6f93f..a4e2311 100644
>>>>> --- a/arch/x86/platform/efi/quirks.c
>>>>> +++ b/arch/x86/platform/efi/quirks.c
>>>>> @@ -562,6 +562,8 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>>>  {
>>>>>         struct quark_security_header *csh = kbuff;
>>>>>
>>>>> +       cap_info->total_size = 0;
>>>>> +
>>>>>         if (!x86_match_cpu(quark_ids))
>>>>>                 goto fallback;
>>>>>
>>>>> @@ -587,12 +589,16 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>>>
>>>>>         kbuff += csh->headersize;
>>>>>
>>>>> +       cap_info->total_size = csh->headersize;
>>>>> +
>>>>>  fallback:
>>>>>         if (hdr_bytes < sizeof(efi_capsule_header_t))
>>>>>                 return 0;
>>>>>
>>>>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>>>>
>>>>> +       cap_info->total_size += cap_info->header.imagesize;
>>>>> +
>>>>>         return __efi_capsule_setup_info(cap_info);
>>>>>  }
>>>>>
>>>>> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
>>>>> index e851951..40dc354 100644
>>>>> --- a/drivers/firmware/efi/capsule-loader.c
>>>>> +++ b/drivers/firmware/efi/capsule-loader.c
>>>>> @@ -42,7 +42,7 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>>>>         int ret;
>>>>>         void *temp_page;
>>>>>
>>>>> -       pages_needed = ALIGN(cap_info->header.imagesize, PAGE_SIZE) / PAGE_SIZE;
>>>>> +       pages_needed = ALIGN(cap_info->total_size, PAGE_SIZE) / PAGE_SIZE;
>>>>>
>>>>>         if (pages_needed == 0) {
>>>>>                 pr_err("invalid capsule size");
>>>>> @@ -59,7 +59,6 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>>>>                 return ret;
>>>>>         }
>>>>>
>>>>> -       cap_info->total_size = cap_info->header.imagesize;
>>>>>         temp_page = krealloc(cap_info->pages,
>>>>>                              pages_needed * sizeof(void *),
>>>>>                              GFP_KERNEL | __GFP_ZERO);
>>>>> @@ -87,6 +86,8 @@ int __weak efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>>>
>>>>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>>>>
>>>>> +       cap_info->total_size = cap_info->header.imagesize;
>>>>> +
>>>>>         return __efi_capsule_setup_info(cap_info);
>>>>>  }
>>>>>
>>>>
>>>> OK, thanks for debugging that.
>>>>
>>>>> But then my changes to efi_capsule_update are missing the present the
>>>>> right format to the loader. As efi_capsule_update needs to lay out the
>>>>> sg-list in as special way, excluding the CSH on the first page, it needs
>>>>> to know about the displacement.
>>>>>
>>>>> Any suggestions how to address that without rolling back to my aproach?
>>>>>
>>>>
>>>> OK, I'm a bit lost now: for my understanding, could you please
>>>> reiterate how the CSH image deviates from the ordinary one? Or more
>>>> specifically, what exactly is preventing us from simply chopping off
>>>> the CSH header and pushing the capsule header + payload into
>>>> /dev/capsule_loader?
>>>>
>>>
>>> Devices that mandate a signed capsule for updates expect the CSH in
>>> front of the regular capsule image. The interface to UEFI remains
>>> unchanged, i.e. you pass the virtually chopped off capsule, but you have
>>> to leave the CSH in RAM right in front of that very same image. It's a
>>> "nice" side-channel API.
>>>
>>
>> Wow, that is worse than I thought.
>>
>> So my suggestion (which I coded up, please pull again),
> 
> Hmm, it does not build on x86 atm. Let me fix that up first (~15 min)
> 

I'm without access to the test device till next Tuesday. Will come back
to you then.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

[toc] | [prev] | [next] | [standalone]

#1616257

On 2017-03-28 19:23, Ard Biesheuvel wrote:
> On 28 March 2017 at 18:17, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 28 March 2017 at 17:18, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> On 2017-03-28 17:52, Ard Biesheuvel wrote:
>>>> On 28 March 2017 at 16:43, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>> On 2017-03-28 17:13, Jan Kiszka wrote:
>>>>>> On 2017-03-28 15:49, Ard Biesheuvel wrote:
>>>> [..]
>>>>>>> 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.
>>>>>>
>>>>>> There is more needed to make things work again, maybe around passing the
>>>>>> right image size. I'm looking into this.
>>>>>>
>>>>>
>>>>> This makes CSH images being accepted again:
>>>>>
>>>>> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
>>>>> index 4b6f93f..a4e2311 100644
>>>>> --- a/arch/x86/platform/efi/quirks.c
>>>>> +++ b/arch/x86/platform/efi/quirks.c
>>>>> @@ -562,6 +562,8 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>>>  {
>>>>>         struct quark_security_header *csh = kbuff;
>>>>>
>>>>> +       cap_info->total_size = 0;
>>>>> +
>>>>>         if (!x86_match_cpu(quark_ids))
>>>>>                 goto fallback;
>>>>>
>>>>> @@ -587,12 +589,16 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>>>
>>>>>         kbuff += csh->headersize;
>>>>>
>>>>> +       cap_info->total_size = csh->headersize;
>>>>> +
>>>>>  fallback:
>>>>>         if (hdr_bytes < sizeof(efi_capsule_header_t))
>>>>>                 return 0;
>>>>>
>>>>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>>>>
>>>>> +       cap_info->total_size += cap_info->header.imagesize;
>>>>> +
>>>>>         return __efi_capsule_setup_info(cap_info);
>>>>>  }
>>>>>
>>>>> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
>>>>> index e851951..40dc354 100644
>>>>> --- a/drivers/firmware/efi/capsule-loader.c
>>>>> +++ b/drivers/firmware/efi/capsule-loader.c
>>>>> @@ -42,7 +42,7 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>>>>         int ret;
>>>>>         void *temp_page;
>>>>>
>>>>> -       pages_needed = ALIGN(cap_info->header.imagesize, PAGE_SIZE) / PAGE_SIZE;
>>>>> +       pages_needed = ALIGN(cap_info->total_size, PAGE_SIZE) / PAGE_SIZE;
>>>>>
>>>>>         if (pages_needed == 0) {
>>>>>                 pr_err("invalid capsule size");
>>>>> @@ -59,7 +59,6 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>>>>                 return ret;
>>>>>         }
>>>>>
>>>>> -       cap_info->total_size = cap_info->header.imagesize;
>>>>>         temp_page = krealloc(cap_info->pages,
>>>>>                              pages_needed * sizeof(void *),
>>>>>                              GFP_KERNEL | __GFP_ZERO);
>>>>> @@ -87,6 +86,8 @@ int __weak efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>>>
>>>>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>>>>
>>>>> +       cap_info->total_size = cap_info->header.imagesize;
>>>>> +
>>>>>         return __efi_capsule_setup_info(cap_info);
>>>>>  }
>>>>>
>>>>
>>>> OK, thanks for debugging that.
>>>>
>>>>> But then my changes to efi_capsule_update are missing the present the
>>>>> right format to the loader. As efi_capsule_update needs to lay out the
>>>>> sg-list in as special way, excluding the CSH on the first page, it needs
>>>>> to know about the displacement.
>>>>>
>>>>> Any suggestions how to address that without rolling back to my aproach?
>>>>>
>>>>
>>>> OK, I'm a bit lost now: for my understanding, could you please
>>>> reiterate how the CSH image deviates from the ordinary one? Or more
>>>> specifically, what exactly is preventing us from simply chopping off
>>>> the CSH header and pushing the capsule header + payload into
>>>> /dev/capsule_loader?
>>>>
>>>
>>> Devices that mandate a signed capsule for updates expect the CSH in
>>> front of the regular capsule image. The interface to UEFI remains
>>> unchanged, i.e. you pass the virtually chopped off capsule, but you have
>>> to leave the CSH in RAM right in front of that very same image. It's a
>>> "nice" side-channel API.
>>>
>>
>> Wow, that is worse than I thought.
>>
>> So my suggestion (which I coded up, please pull again),
> 
> Hmm, it does not build on x86 atm. Let me fix that up first (~15 min)
> 

Just tested the patches from b51f20c780 on top of our queue, and this
time the flashing worked fine!

Thanks,
Jan

>> is to replace
>> the array of struct page pointers with an array of physical addresses
>> in the capsule_info struct. This way, your special version of
>> efi_capsule_setup_info() can advance the first one by the size of the
>> header.
>>
>> I hope this works for you. I am not sure whether imagesize needs to be
>> modified, so I left it alone for now (but I suspect it should be).

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web