by Andy Shevchenko

[permalink] [raw]

Subject: Re: [PATCH v2 0/7] efi: Enhance capsule loader to support signed Quark images

On Fri, Mar 24, 2017 at 7:34 PM, Jan Kiszka <[email protected]> wrote:
> This addresses the review feedback provided on round 1, specifically
> - refactored queue to keep the Quark mess in
> - only check for CSH on Quark CPUs
> - added some smaller cleanups of the capsule loader
> - documented capsule header / linked to original code
>
> See last patch for the background of the series.
>
> The series has been tested on the Galileo Gen2, to exclude regressions,
> with a firmware.cap with AND without security header and the SIMATIC
> IOT2040 which requires the header because of its mandatory secure boot.

The series looks good to me from code prospective. It's clean and
understandable, though I have couple of comments (see related
patches).
Please, address them.

I wasn't able to test it, thus FWIW:
Reviewed-by: Andy Shevchenko <[email protected]>

>
> Jan
>
> Jan Kiszka (7):
> efi/capsule: Fix return code on failing kmap/vmap
> efi/capsule: Remove pr_debug on ENOMEM or EFAULT
> efi/capsule: Clean up pr_err/info messages
> efi/capsule: Adjust return type of efi_capsule_setup_info
> efi/capsule: Prepare for loading images with security header
> efi/capsule: Factor out overloadable efi_capsule_identify_image
> efi/capsule: Add support for Quark security header
>
> arch/x86/platform/efi/quirks.c | 95 ++++++++++++++++++++++++
> drivers/firmware/efi/capsule-loader.c | 136 +++++++++++++++++++---------------
> drivers/firmware/efi/capsule.c | 21 +++++-
> include/linux/efi.h | 19 +++++
> 4 files changed, 208 insertions(+), 63 deletions(-)
>
> --
> 2.10.2
>

--
With Best Regards,
Andy Shevchenko

2017-03-25 23:35:06

On 2017-03-28 19:23, Ard Biesheuvel wrote:
> On 28 March 2017 at 18:17, Ard Biesheuvel <[email protected]> wrote:
>> On 28 March 2017 at 17:18, Jan Kiszka <[email protected]> wrote:
>>> On 2017-03-28 17:52, Ard Biesheuvel wrote:
>>>> On 28 March 2017 at 16:43, Jan Kiszka <[email protected]> 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