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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKv+Gu8FqhyhdpOa3=EOAG8bBgyAJbFzkVSgQbrVdUT0y1wHsw@mail.gmail.com>
Date:   Tue, 28 Mar 2017 16:52:18 +0100
From:   Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:     Jan Kiszka <jan.kiszka@...mens.com>
Cc:     Matt Fleming <matt@...eblueprint.co.uk>,
        "linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        "Bryan O'Donoghue" <pure.logic@...us-software.ie>,
        Hock Leong Kweh <hock.leong.kweh@...el.com>,
        Borislav Petkov <bp@...en8.de>,
        Sascha Weisenberger <sascha.weisenberger@...mens.com>
Subject: Re: [PATCH v2 5/7] efi/capsule: Prepare for loading images with
 security header

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