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

[Ard Biesheuvel]

On 28 March 2017 at 16:43, Jan Kiszka <jan.kiszka@xxxxxxxxxxx> 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?