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

Groups > linux.kernel > #1581530 > unrolled thread

[PATCH 0/2] efi: Enhance capsule loader to support signed Quark images

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images Jan Kiszka <jan.kiszka@siemens.com> - 2017-02-15 19:20 +0100
    Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-02-15 19:20 +0100
      Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark  images Jan Kiszka <jan.kiszka@siemens.com> - 2017-02-15 19:50 +0100
    [PATCH 1/2] efi/capsule: Prepare for loading images with security header Jan Kiszka <jan.kiszka@siemens.com> - 2017-02-15 19:20 +0100
    [PATCH 2/2] efi/capsule: Add support for Quark security header Jan Kiszka <jan.kiszka@siemens.com> - 2017-02-15 19:20 +0100
      Re: [PATCH 2/2] efi/capsule: Add support for Quark security header Bryan O'Donoghue <pure.logic@nexus-software.ie> - 2017-02-17 02:40 +0100
    Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-15 19:50 +0100
    Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-15 19:50 +0100
      Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark  images Jan Kiszka <jan.kiszka@siemens.com> - 2017-02-15 20:00 +0100
        Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark  images Jan Kiszka <jan.kiszka@siemens.com> - 2017-02-15 20:10 +0100
          RE: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark  images "Kweh, Hock Leong" <hock.leong.kweh@intel.com> - 2017-02-16 04:10 +0100
            Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark  images Jan Kiszka <jan.kiszka@siemens.com> - 2017-02-16 08:40 +0100
              Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-02-18 23:00 +0100
                Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark  images Jan Kiszka <jan.kiszka@siemens.com> - 2017-02-19 14:40 +0100
                  Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark  images Bryan O'Donoghue <pure.logic@nexus-software.ie> - 2017-02-20 02:40 +0100
                    Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark  images Jan Kiszka <jan.kiszka@siemens.com> - 2017-02-20 03:00 +0100
            Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark  images Bryan O'Donoghue <pure.logic@nexus-software.ie> - 2017-02-17 02:00 +0100
              RE: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark  images "Kweh, Hock Leong" <hock.leong.kweh@intel.com> - 2017-02-17 09:30 +0100
                Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark  images Jan Kiszka <jan.kiszka@siemens.com> - 2017-02-17 10:30 +0100
                  Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark  images Matt Fleming <matt@codeblueprint.co.uk> - 2017-02-28 13:20 +0100
                    Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark  images Jan Kiszka <jan.kiszka@siemens.com> - 2017-02-28 13:30 +0100
                      Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark  images Matt Fleming <matt@codeblueprint.co.uk> - 2017-02-28 13:40 +0100
                        Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-28 14:40 +0100
                          Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark  images Bryan O'Donoghue <pure.logic@nexus-software.ie> - 2017-02-28 16:10 +0100
                          Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark  images Bryan O'Donoghue <pure.logic@nexus-software.ie> - 2017-02-28 16:20 +0100
                            Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-28 17:30 +0100
                              Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-28 18:20 +0100
                                Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark  images Bryan O'Donoghue <pure.logic@nexus-software.ie> - 2017-02-28 19:50 +0100
                              Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark  images Bryan O'Donoghue <pure.logic@nexus-software.ie> - 2017-02-28 18:30 +0100
                        Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-02-28 14:40 +0100
                          Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-28 14:40 +0100
                Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark  images Bryan O'Donoghue <pure.logic@nexus-software.ie> - 2017-02-17 11:00 +0100
                  Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark  images Jan Kiszka <jan.kiszka@siemens.com> - 2017-02-17 11:20 +0100
                    Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark  images Bryan O'Donoghue <pure.logic@nexus-software.ie> - 2017-02-17 12:50 +0100

Page 1 of 2  [1] 2  Next page →

#1581530 — [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images

See patch 2 for the background.

Series has been tested on the Galileo Gen2, to exclude regressions, with
a firmware.cap without security header and the SIMATIC IOT2040 which
requires the header because of its mandatory secure boot.

Jan

Jan Kiszka (2):
  efi/capsule: Prepare for loading images with security header
  efi/capsule: Add support for Quark security header

 drivers/firmware/efi/capsule-loader.c | 73 ++++++++++++++++++++++++++++++-----
 drivers/firmware/efi/capsule.c        | 19 +++++++--
 include/linux/efi.h                   |  2 +-
 3 files changed, 79 insertions(+), 15 deletions(-)

-- 
2.1.4

[toc] | [next] | [standalone]

#1581534

On 15 February 2017 at 18:14, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> See patch 2 for the background.
>
> Series has been tested on the Galileo Gen2, to exclude regressions, with
> a firmware.cap without security header and the SIMATIC IOT2040 which
> requires the header because of its mandatory secure boot.
>

Hello Jan,

What is a Quark? Is it in the UEFI spec?

> Jan Kiszka (2):
>   efi/capsule: Prepare for loading images with security header
>   efi/capsule: Add support for Quark security header
>
>  drivers/firmware/efi/capsule-loader.c | 73 ++++++++++++++++++++++++++++++-----
>  drivers/firmware/efi/capsule.c        | 19 +++++++--
>  include/linux/efi.h                   |  2 +-
>  3 files changed, 79 insertions(+), 15 deletions(-)
>
> --
> 2.1.4
>

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

#1581568 — Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images

On 2017-02-15 19:17, Ard Biesheuvel wrote:
> On 15 February 2017 at 18:14, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> See patch 2 for the background.
>>
>> Series has been tested on the Galileo Gen2, to exclude regressions, with
>> a firmware.cap without security header and the SIMATIC IOT2040 which
>> requires the header because of its mandatory secure boot.
>>
> 
> Hello Jan,
> 
> What is a Quark? Is it in the UEFI spec?

http://ark.intel.com/products/79084/Intel-Quark-SoC-X1000-16K-Cache-400-MHz

I didn't find any obvious reference to this format in the UEFI spec.
This might be specific to the Quark UEFI EDK2 that Intel ships (it's not
in upstream edk2) and that was used as foundation for the IOT2000
series. The capsule driver that Intel includes in their Galileo BSP does
something similar (I don't have a browsable reference to that at hand,
sorry, must be in this nice package
https://downloadcenter.intel.com/download/24702/Intel-Galileo-Board-GPL-Compliance-files-1-0-4?product=83137).

Jan

> 
>> Jan Kiszka (2):
>>   efi/capsule: Prepare for loading images with security header
>>   efi/capsule: Add support for Quark security header
>>
>>  drivers/firmware/efi/capsule-loader.c | 73 ++++++++++++++++++++++++++++++-----
>>  drivers/firmware/efi/capsule.c        | 19 +++++++--
>>  include/linux/efi.h                   |  2 +-
>>  3 files changed, 79 insertions(+), 15 deletions(-)
>>
>> --
>> 2.1.4
>>


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

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

#1581538 — [PATCH 1/2] efi/capsule: Prepare for loading images with security header

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 for this by taking an image
offset that encodes the start of the EFI standard capsule.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/firmware/efi/capsule-loader.c |  2 +-
 drivers/firmware/efi/capsule.c        | 19 +++++++++++++++----
 include/linux/efi.h                   |  2 +-
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
index 9ae6c11..63ceca9 100644
--- a/drivers/firmware/efi/capsule-loader.c
+++ b/drivers/firmware/efi/capsule-loader.c
@@ -116,7 +116,7 @@ static ssize_t efi_capsule_submit_update(struct capsule_info *cap_info)
 		return -EFAULT;
 	}
 
-	ret = efi_capsule_update(cap_hdr_temp, cap_info->pages);
+	ret = efi_capsule_update(cap_hdr_temp, 0, cap_info->pages);
 	vunmap(cap_hdr_temp);
 	if (ret) {
 		pr_err("%s: efi_capsule_update() failed\n", __func__);
diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
index 6eedff4..f025ccf 100644
--- a/drivers/firmware/efi/capsule.c
+++ b/drivers/firmware/efi/capsule.c
@@ -184,6 +184,7 @@ efi_capsule_update_locked(efi_capsule_header_t *capsule,
 /**
  * efi_capsule_update - send a capsule to the firmware
  * @capsule: capsule to send to firmware
+ * @image_offs: image offset on first data page
  * @pages: an array of capsule data pages
  *
  * Build a scatter gather list with EFI capsule block descriptors to
@@ -214,9 +215,11 @@ 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 image_offs,
+		       struct page **pages)
 {
 	u32 imagesize = capsule->imagesize;
+	u32 total_size = imagesize + image_offs;
 	efi_guid_t guid = capsule->guid;
 	unsigned int count, sg_count;
 	u32 flags = capsule->flags;
@@ -224,11 +227,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 (image_offs >= PAGE_SIZE)
+		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 +261,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 (image_offs > 0) {
+				sglist[j].data += image_offs;
+				sz -= image_offs;
+				image_offs = 0;
+			}
+			sglist[j].length = sz;
 
 			imagesize -= sz;
 			count--;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 5b1af30..57e27a1 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1399,7 +1399,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,
-			      struct page **pages);
+			      unsigned int image_offs, struct page **pages);
 
 #ifdef CONFIG_EFI_RUNTIME_MAP
 int efi_runtime_map_init(struct kobject *);
-- 
2.1.4

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

#1581539 — [PATCH 2/2] efi/capsule: Add support for Quark security header

The firmware for Quark X102x prepends a security header to the capsule
which is needed to support the mandatory secure boot on this processor.
The header can be detected by checking for the "_CSH" signature and -
to avoid any GUID conflict - validating its size field to contain the
expected value. Then we need to look for the EFI header right after the
security header and pass the image offset to efi_capsule_update while
keeping the whole image in RAM - the firmware will look for the header
on its own.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/firmware/efi/capsule-loader.c | 73 ++++++++++++++++++++++++++++++-----
 1 file changed, 63 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
index 63ceca9..571d931 100644
--- a/drivers/firmware/efi/capsule-loader.c
+++ b/drivers/firmware/efi/capsule-loader.c
@@ -26,10 +26,32 @@ struct capsule_info {
 	long		index;
 	size_t		count;
 	size_t		total_size;
+	unsigned int	efi_hdr_offset;
 	struct page	**pages;
 	size_t		page_bytes_remain;
 };
 
+#define QUARK_CSH_SIGNATURE		0x5f435348	/* _CSH */
+#define QUARK_SECURITY_HEADER_SIZE	0x400
+
+struct efi_quark_security_header {
+	u32 csh_signature;
+	u32 version;
+	u32 modulesize;
+	u32 security_version_number_index;
+	u32 security_version_number;
+	u32 rsvd_module_id;
+	u32 rsvd_module_vendor;
+	u32 rsvd_date;
+	u32 headersize;
+	u32 hash_algo;
+	u32 cryp_algo;
+	u32 keysize;
+	u32 signaturesize;
+	u32 rsvd_next_header;
+	u32 rsvd[2];
+};
+
 /**
  * efi_free_all_buff_pages - free all previous allocated buffer pages
  * @cap_info: pointer to current instance of capsule_info structure
@@ -56,18 +78,46 @@ static void efi_free_all_buff_pages(struct capsule_info *cap_info)
 static ssize_t efi_capsule_setup_info(struct capsule_info *cap_info,
 				      void *kbuff, size_t hdr_bytes)
 {
+	struct efi_quark_security_header *quark_hdr;
 	efi_capsule_header_t *cap_hdr;
 	size_t pages_needed;
 	int ret;
 	void *temp_page;
 
-	/* Only process data block that is larger than efi header size */
-	if (hdr_bytes < sizeof(efi_capsule_header_t))
+	/* Only process data block that is larger than the security header
+	 * (which is larger than the EFI header) */
+	if (hdr_bytes < sizeof(struct efi_quark_security_header))
 		return 0;
 
 	/* Reset back to the correct offset of header */
 	cap_hdr = kbuff - cap_info->count;
-	pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >> PAGE_SHIFT;
+
+	quark_hdr = (struct efi_quark_security_header *)cap_hdr;
+
+	if (quark_hdr->csh_signature == QUARK_CSH_SIGNATURE &&
+	    quark_hdr->headersize == QUARK_SECURITY_HEADER_SIZE) {
+		/* Only process data block if EFI header is included */
+		if (hdr_bytes < QUARK_SECURITY_HEADER_SIZE +
+				sizeof(efi_capsule_header_t))
+			return 0;
+
+		pr_debug("%s: Quark security header detected\n", __func__);
+
+		if (quark_hdr->rsvd_next_header != 0) {
+			pr_err("%s: multiple security headers not supported\n",
+			       __func__);
+			return -EINVAL;
+		}
+
+		cap_hdr = (void *)cap_hdr + quark_hdr->headersize;
+		cap_info->total_size = quark_hdr->modulesize;
+		cap_info->efi_hdr_offset = quark_hdr->headersize;
+	} else {
+		cap_info->total_size = cap_hdr->imagesize;
+		cap_info->efi_hdr_offset = 0;
+	}
+
+	pages_needed = ALIGN(cap_info->total_size, PAGE_SIZE) >> PAGE_SHIFT;
 
 	if (pages_needed == 0) {
 		pr_err("%s: pages count invalid\n", __func__);
@@ -76,7 +126,7 @@ static ssize_t efi_capsule_setup_info(struct capsule_info *cap_info,
 
 	/* Check if the capsule binary supported */
 	ret = efi_capsule_supported(cap_hdr->guid, cap_hdr->flags,
-				    cap_hdr->imagesize,
+				    cap_info->total_size,
 				    &cap_info->reset_type);
 	if (ret) {
 		pr_err("%s: efi_capsule_supported() failed\n",
@@ -84,7 +134,6 @@ static ssize_t efi_capsule_setup_info(struct capsule_info *cap_info,
 		return ret;
 	}
 
-	cap_info->total_size = cap_hdr->imagesize;
 	temp_page = krealloc(cap_info->pages,
 			     pages_needed * sizeof(void *),
 			     GFP_KERNEL | __GFP_ZERO);
@@ -106,18 +155,22 @@ static ssize_t 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,
+	mapped_pages = vmap(cap_info->pages, cap_info->index,
 			VM_MAP, PAGE_KERNEL);
-	if (!cap_hdr_temp) {
+	if (!mapped_pages) {
 		pr_debug("%s: vmap() failed\n", __func__);
 		return -EFAULT;
 	}
 
-	ret = efi_capsule_update(cap_hdr_temp, 0, cap_info->pages);
-	vunmap(cap_hdr_temp);
+	cap_hdr = mapped_pages + cap_info->efi_hdr_offset;
+
+	ret = efi_capsule_update(cap_hdr, cap_info->efi_hdr_offset,
+				 cap_info->pages);
+	vunmap(mapped_pages);
 	if (ret) {
 		pr_err("%s: efi_capsule_update() failed\n", __func__);
 		return ret;
-- 
2.1.4

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

#1583018 — Re: [PATCH 2/2] efi/capsule: Add support for Quark security header

On 15/02/17 18:14, Jan Kiszka wrote:
> The firmware for Quark X102x prepends a security header to the capsule
> which is needed to support the mandatory secure boot on this processor.
> The header can be detected by checking for the "_CSH" signature and -
> to avoid any GUID conflict - validating its size field to contain the
> expected value. Then we need to look for the EFI header right after the
> security header and pass the image offset to efi_capsule_update while
> keeping the whole image in RAM - the firmware will look for the header
> on its own.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  drivers/firmware/efi/capsule-loader.c | 73 ++++++++++++++++++++++++++++++-----
>  1 file changed, 63 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
> index 63ceca9..571d931 100644
> --- a/drivers/firmware/efi/capsule-loader.c
> +++ b/drivers/firmware/efi/capsule-loader.c
> @@ -26,10 +26,32 @@ struct capsule_info {
>  	long		index;
>  	size_t		count;
>  	size_t		total_size;
> +	unsigned int	efi_hdr_offset;
>  	struct page	**pages;
>  	size_t		page_bytes_remain;
>  };
>
> +#define QUARK_CSH_SIGNATURE		0x5f435348	/* _CSH */
> +#define QUARK_SECURITY_HEADER_SIZE	0x400
> +
> +struct efi_quark_security_header {
> +	u32 csh_signature;
> +	u32 version;
> +	u32 modulesize;
> +	u32 security_version_number_index;
> +	u32 security_version_number;
> +	u32 rsvd_module_id;
> +	u32 rsvd_module_vendor;
> +	u32 rsvd_date;
> +	u32 headersize;
> +	u32 hash_algo;
> +	u32 cryp_algo;
> +	u32 keysize;
> +	u32 signaturesize;
> +	u32 rsvd_next_header;
> +	u32 rsvd[2];
> +};

This is a real nitpick (sorry) - but it'd be nice to have a document 
reference or a link to describe this header i.e. it is officially 
documented - outside of the UEFI specification. Make life easy for 
someone reading this header and make an document reference.

Also it'd be appreciated if you could describe the format of the 
structure with

@member	member-attribute description

> +
>  /**
>   * efi_free_all_buff_pages - free all previous allocated buffer pages
>   * @cap_info: pointer to current instance of capsule_info structure
> @@ -56,18 +78,46 @@ static void efi_free_all_buff_pages(struct capsule_info *cap_info)
>  static ssize_t efi_capsule_setup_info(struct capsule_info *cap_info,
>  				      void *kbuff, size_t hdr_bytes)
>  {
> +	struct efi_quark_security_header *quark_hdr;
>  	efi_capsule_header_t *cap_hdr;
>  	size_t pages_needed;
>  	int ret;
>  	void *temp_page;
>
> -	/* Only process data block that is larger than efi header size */
> -	if (hdr_bytes < sizeof(efi_capsule_header_t))
> +	/* Only process data block that is larger than the security header
> +	 * (which is larger than the EFI header) */
> +	if (hdr_bytes < sizeof(struct efi_quark_security_header))
>  		return 0;
>
>  	/* Reset back to the correct offset of header */
>  	cap_hdr = kbuff - cap_info->count;
> -	pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >> PAGE_SHIFT;
> +
> +	quark_hdr = (struct efi_quark_security_header *)cap_hdr;
> +
> +	if (quark_hdr->csh_signature == QUARK_CSH_SIGNATURE &&
> +	    quark_hdr->headersize == QUARK_SECURITY_HEADER_SIZE) {
> +		/* Only process data block if EFI header is included */
> +		if (hdr_bytes < QUARK_SECURITY_HEADER_SIZE +
> +				sizeof(efi_capsule_header_t))
> +			return 0;

At this point if cap_info->header_obtained == false then this is an 
error - you should be barfing on this - not literally barfing - at least 
not on your keyboard :)

return -ETHISHEADERSUCKS or some other sensible value -EINVAL like you 
have below.

Point being you've validated the signature, the header size and 
cap_info->header_obtained is false then you definitely have a bogus 
capsule..


> +
> +		pr_debug("%s: Quark security header detected\n", __func__);

... and %s __func__ is verboten don't do it. actually there's a bunch of 
those pairs all over this code - if you have the time in a supplementary 
patch please kill them - there must be a a dev pointer we can get at 
somewhere that makes sense to use dev_dbg- if not those __func__ 
parameters still need to go away - please kill them.

> +
> +		if (quark_hdr->rsvd_next_header != 0) {
> +			pr_err("%s: multiple security headers not supported\n",
> +			       __func__);
> +			return -EINVAL;
> +		}


> +
> +		cap_hdr = (void *)cap_hdr + quark_hdr->headersize;

You could have a separate void * variable and not have the cast.


---
bod

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

#1581565

On Wed, Feb 15, 2017 at 8:14 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> See patch 2 for the background.
>
> Series has been tested on the Galileo Gen2, to exclude regressions, with
> a firmware.cap without security header and the SIMATIC IOT2040 which
> requires the header because of its mandatory secure boot.
>
> Jan

Based on discussions in [1], please, Cc your further messages
regarding this topic to Borislav, Bryan, Hock Leong.

[1] http://lists-archives.com/linux-kernel/28494369-enable-capsule-loader-interface-for-efi-firmware-updating.html

>
> Jan Kiszka (2):
>   efi/capsule: Prepare for loading images with security header
>   efi/capsule: Add support for Quark security header
>
>  drivers/firmware/efi/capsule-loader.c | 73 ++++++++++++++++++++++++++++++-----
>  drivers/firmware/efi/capsule.c        | 19 +++++++--
>  include/linux/efi.h                   |  2 +-
>  3 files changed, 79 insertions(+), 15 deletions(-)
>
> --
> 2.1.4
>



-- 
With Best Regards,
Andy Shevchenko

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

#1581566

On Wed, Feb 15, 2017 at 8:14 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> See patch 2 for the background.
>
> Series has been tested on the Galileo Gen2, to exclude regressions, with
> a firmware.cap without security header and the SIMATIC IOT2040 which
> requires the header because of its mandatory secure boot.

Briefly looking to the code it looks like a real hack.
Sorry, but it would be carefully (re-)designed.

Just my 2 cents.

-- 
With Best Regards,
Andy Shevchenko

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

#1581573 — Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images

On 2017-02-15 19:46, Andy Shevchenko wrote:
> On Wed, Feb 15, 2017 at 8:14 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> See patch 2 for the background.
>>
>> Series has been tested on the Galileo Gen2, to exclude regressions, with
>> a firmware.cap without security header and the SIMATIC IOT2040 which
>> requires the header because of its mandatory secure boot.
> 
> Briefly looking to the code it looks like a real hack.
> Sorry, but it would be carefully (re-)designed.

The interface that the firmware provides us? That should have been done
differently, I agree, but I'm not too much into those firmware details,
specifically when it comes to signatures.

The Linux code was designed around that suboptimal situation. If there
are better ideas, I'm all ears.

Jan

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

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

#1581585 — Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images

On 2017-02-15 19:50, Jan Kiszka wrote:
> On 2017-02-15 19:46, Andy Shevchenko wrote:
>> On Wed, Feb 15, 2017 at 8:14 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> See patch 2 for the background.
>>>
>>> Series has been tested on the Galileo Gen2, to exclude regressions, with
>>> a firmware.cap without security header and the SIMATIC IOT2040 which
>>> requires the header because of its mandatory secure boot.
>>
>> Briefly looking to the code it looks like a real hack.
>> Sorry, but it would be carefully (re-)designed.
> 
> The interface that the firmware provides us? That should have been done
> differently, I agree, but I'm not too much into those firmware details,
> specifically when it comes to signatures.
> 
> The Linux code was designed around that suboptimal situation. If there
> are better ideas, I'm all ears.
> 

Expanding CC's as requested by Andy.

Jan

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

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

#1582291 — RE: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images

> -----Original Message-----
> From: Jan Kiszka [mailto:jan.kiszka@siemens.com]
> Sent: Thursday, February 16, 2017 3:00 AM
> To: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Matt Fleming <matt@codeblueprint.co.uk>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; linux-efi@vger.kernel.org; Linux Kernel Mailing
> List <linux-kernel@vger.kernel.org>; Borislav Petkov <bp@alien8.de>; Kweh,
> Hock Leong <hock.leong.kweh@intel.com>; Bryan O'Donoghue
> <pure.logic@nexus-software.ie>
> Subject: Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark
> images
> 
> On 2017-02-15 19:50, Jan Kiszka wrote:
> > On 2017-02-15 19:46, Andy Shevchenko wrote:
> >> On Wed, Feb 15, 2017 at 8:14 PM, Jan Kiszka <jan.kiszka@siemens.com>
> wrote:
> >>> See patch 2 for the background.
> >>>
> >>> Series has been tested on the Galileo Gen2, to exclude regressions,
> >>> with a firmware.cap without security header and the SIMATIC IOT2040
> >>> which requires the header because of its mandatory secure boot.
> >>
> >> Briefly looking to the code it looks like a real hack.
> >> Sorry, but it would be carefully (re-)designed.
> >
> > The interface that the firmware provides us? That should have been
> > done differently, I agree, but I'm not too much into those firmware
> > details, specifically when it comes to signatures.
> >
> > The Linux code was designed around that suboptimal situation. If there
> > are better ideas, I'm all ears.
> >
> 
> Expanding CC's as requested by Andy.
> 
> Jan
> 

Hi Jan,

While I upstreaming the capsule loader patches, I did work with maintainer
Matt and look into this security header created for Quark. Eventually both
of us agreed that this will not be upstream to mainline as it is really a Quark
specific implementation.

The proper implementation may require to work with UEFI community
to expand its capsule spec to support signed binary. 


Regards,
Wilson

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

#1582367 — Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images

On 2017-02-16 04:00, Kweh, Hock Leong wrote:
>> -----Original Message-----
>> From: Jan Kiszka [mailto:jan.kiszka@siemens.com]
>> Sent: Thursday, February 16, 2017 3:00 AM
>> To: Andy Shevchenko <andy.shevchenko@gmail.com>
>> Cc: Matt Fleming <matt@codeblueprint.co.uk>; Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>; linux-efi@vger.kernel.org; Linux Kernel Mailing
>> List <linux-kernel@vger.kernel.org>; Borislav Petkov <bp@alien8.de>; Kweh,
>> Hock Leong <hock.leong.kweh@intel.com>; Bryan O'Donoghue
>> <pure.logic@nexus-software.ie>
>> Subject: Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark
>> images
>>
>> On 2017-02-15 19:50, Jan Kiszka wrote:
>>> On 2017-02-15 19:46, Andy Shevchenko wrote:
>>>> On Wed, Feb 15, 2017 at 8:14 PM, Jan Kiszka <jan.kiszka@siemens.com>
>> wrote:
>>>>> See patch 2 for the background.
>>>>>
>>>>> Series has been tested on the Galileo Gen2, to exclude regressions,
>>>>> with a firmware.cap without security header and the SIMATIC IOT2040
>>>>> which requires the header because of its mandatory secure boot.
>>>>
>>>> Briefly looking to the code it looks like a real hack.
>>>> Sorry, but it would be carefully (re-)designed.
>>>
>>> The interface that the firmware provides us? That should have been
>>> done differently, I agree, but I'm not too much into those firmware
>>> details, specifically when it comes to signatures.
>>>
>>> The Linux code was designed around that suboptimal situation. If there
>>> are better ideas, I'm all ears.
>>>
>>
>> Expanding CC's as requested by Andy.
>>
>> Jan
>>
> 
> Hi Jan,
> 
> While I upstreaming the capsule loader patches, I did work with maintainer
> Matt and look into this security header created for Quark. Eventually both
> of us agreed that this will not be upstream to mainline as it is really a Quark
> specific implementation.

This is ... [swallowing down a lengthy rant about Quark upstreaming]
unfortunate given that Intel hands out firmware and BSPs to their
customers without further explanations on this "minor detail".

I have no idea what other integrators of the X102x did with that, but my
customer has now thousands and thousands of devices in the field with
firmware that works exactly this way. Only for that feature, we will now
have to provide a non-upstream kernel in order to keep the installed
devices updatable. Or create and maintain a different mechanism. Beautiful.

> 
> The proper implementation may require to work with UEFI community
> to expand its capsule spec to support signed binary. 
> 

Are you working on this? How is this solved on other platforms that
require signatures? No one tried that yet? In any case, this sounds like
a lengthy, way too late considered process that will not solve our issue
in the foreseeable future.

Don't get me wrong, I'm not intending to push this into the kernel
because "What the heck?!" was my first reaction as well once I found out
how this update interface is actually working. But maybe you can bring
this topic up on your side as well so that we come to an upstreamable
solution in all affected projects.

Thanks,
Jan

PS: @Daniel, another example for your presentation. ;)

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

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

#1584013

On 16 February 2017 at 07:29, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-02-16 04:00, Kweh, Hock Leong wrote:
>>> -----Original Message-----
>>> From: Jan Kiszka [mailto:jan.kiszka@siemens.com]
>>> Sent: Thursday, February 16, 2017 3:00 AM
>>> To: Andy Shevchenko <andy.shevchenko@gmail.com>
>>> Cc: Matt Fleming <matt@codeblueprint.co.uk>; Ard Biesheuvel
>>> <ard.biesheuvel@linaro.org>; linux-efi@vger.kernel.org; Linux Kernel Mailing
>>> List <linux-kernel@vger.kernel.org>; Borislav Petkov <bp@alien8.de>; Kweh,
>>> Hock Leong <hock.leong.kweh@intel.com>; Bryan O'Donoghue
>>> <pure.logic@nexus-software.ie>
>>> Subject: Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark
>>> images
>>>
>>> On 2017-02-15 19:50, Jan Kiszka wrote:
>>>> On 2017-02-15 19:46, Andy Shevchenko wrote:
>>>>> On Wed, Feb 15, 2017 at 8:14 PM, Jan Kiszka <jan.kiszka@siemens.com>
>>> wrote:
>>>>>> See patch 2 for the background.
>>>>>>
>>>>>> Series has been tested on the Galileo Gen2, to exclude regressions,
>>>>>> with a firmware.cap without security header and the SIMATIC IOT2040
>>>>>> which requires the header because of its mandatory secure boot.
>>>>>
>>>>> Briefly looking to the code it looks like a real hack.
>>>>> Sorry, but it would be carefully (re-)designed.
>>>>
>>>> The interface that the firmware provides us? That should have been
>>>> done differently, I agree, but I'm not too much into those firmware
>>>> details, specifically when it comes to signatures.
>>>>
>>>> The Linux code was designed around that suboptimal situation. If there
>>>> are better ideas, I'm all ears.
>>>>
>>>
>>> Expanding CC's as requested by Andy.
>>>
>>> Jan
>>>
>>
>> Hi Jan,
>>
>> While I upstreaming the capsule loader patches, I did work with maintainer
>> Matt and look into this security header created for Quark. Eventually both
>> of us agreed that this will not be upstream to mainline as it is really a Quark
>> specific implementation.
>
> This is ... [swallowing down a lengthy rant about Quark upstreaming]
> unfortunate given that Intel hands out firmware and BSPs to their
> customers without further explanations on this "minor detail".
>
> I have no idea what other integrators of the X102x did with that, but my
> customer has now thousands and thousands of devices in the field with
> firmware that works exactly this way. Only for that feature, we will now
> have to provide a non-upstream kernel in order to keep the installed
> devices updatable. Or create and maintain a different mechanism. Beautiful.
>

OK, so you shipped thousands and thousands of devices with mainline
kernels and never tested the capsule update feature, which now turns
out to require modifications to support the non-UEFI compliant
firmware on these devices.

I'm sorry, but that puts it firmly in the 'not our problem' category,
simply because I refuse to believe that you would seriously consider
performing this kind of firmware update on that many devices in the
field if you never tested it in development.

So while I fully agree that
a) it is quite unfortunate that Intel, which has such a dominant
presence in all aspects of UEFI and PI standardization, ships a
non-compliant BSP, and
b) it is useful to be able to sign capsules,
I think we should push back on random, unstandardized signature headers.

The argument that Quark is the only working implementation of capsule
updates, and so we should support it, does not hold. First of all,
arm64 servers are shipping with working capsule update based on the
current kernel implementation, but what is currently shipping is not
really the point for mainline imo, but what is intended to be shipped
with the next kernel release.

I would not object strongly to having conditionally compiled code in
mainline that adds support for this, but bodging the default code path
like this for a Quark quirk is out of the question imo.

Thanks,
Ard.



>>
>> The proper implementation may require to work with UEFI community
>> to expand its capsule spec to support signed binary.
>>
>
> Are you working on this? How is this solved on other platforms that
> require signatures? No one tried that yet? In any case, this sounds like
> a lengthy, way too late considered process that will not solve our issue
> in the foreseeable future.
>
> Don't get me wrong, I'm not intending to push this into the kernel
> because "What the heck?!" was my first reaction as well once I found out
> how this update interface is actually working. But maybe you can bring
> this topic up on your side as well so that we come to an upstreamable
> solution in all affected projects.
>
> Thanks,
> Jan
>
> PS: @Daniel, another example for your presentation. ;)
>
> --
> Siemens AG, Corporate Technology, CT RDA ITP SES-DE
> Corporate Competence Center Embedded Linux

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

#1584142 — Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images

On 2017-02-18 13:48, Ard Biesheuvel wrote:
> On 16 February 2017 at 07:29, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2017-02-16 04:00, Kweh, Hock Leong wrote:
>>>> -----Original Message-----
>>>> From: Jan Kiszka [mailto:jan.kiszka@siemens.com]
>>>> Sent: Thursday, February 16, 2017 3:00 AM
>>>> To: Andy Shevchenko <andy.shevchenko@gmail.com>
>>>> Cc: Matt Fleming <matt@codeblueprint.co.uk>; Ard Biesheuvel
>>>> <ard.biesheuvel@linaro.org>; linux-efi@vger.kernel.org; Linux Kernel Mailing
>>>> List <linux-kernel@vger.kernel.org>; Borislav Petkov <bp@alien8.de>; Kweh,
>>>> Hock Leong <hock.leong.kweh@intel.com>; Bryan O'Donoghue
>>>> <pure.logic@nexus-software.ie>
>>>> Subject: Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark
>>>> images
>>>>
>>>> On 2017-02-15 19:50, Jan Kiszka wrote:
>>>>> On 2017-02-15 19:46, Andy Shevchenko wrote:
>>>>>> On Wed, Feb 15, 2017 at 8:14 PM, Jan Kiszka <jan.kiszka@siemens.com>
>>>> wrote:
>>>>>>> See patch 2 for the background.
>>>>>>>
>>>>>>> Series has been tested on the Galileo Gen2, to exclude regressions,
>>>>>>> with a firmware.cap without security header and the SIMATIC IOT2040
>>>>>>> which requires the header because of its mandatory secure boot.
>>>>>>
>>>>>> Briefly looking to the code it looks like a real hack.
>>>>>> Sorry, but it would be carefully (re-)designed.
>>>>>
>>>>> The interface that the firmware provides us? That should have been
>>>>> done differently, I agree, but I'm not too much into those firmware
>>>>> details, specifically when it comes to signatures.
>>>>>
>>>>> The Linux code was designed around that suboptimal situation. If there
>>>>> are better ideas, I'm all ears.
>>>>>
>>>>
>>>> Expanding CC's as requested by Andy.
>>>>
>>>> Jan
>>>>
>>>
>>> Hi Jan,
>>>
>>> While I upstreaming the capsule loader patches, I did work with maintainer
>>> Matt and look into this security header created for Quark. Eventually both
>>> of us agreed that this will not be upstream to mainline as it is really a Quark
>>> specific implementation.
>>
>> This is ... [swallowing down a lengthy rant about Quark upstreaming]
>> unfortunate given that Intel hands out firmware and BSPs to their
>> customers without further explanations on this "minor detail".
>>
>> I have no idea what other integrators of the X102x did with that, but my
>> customer has now thousands and thousands of devices in the field with
>> firmware that works exactly this way. Only for that feature, we will now
>> have to provide a non-upstream kernel in order to keep the installed
>> devices updatable. Or create and maintain a different mechanism. Beautiful.
>>
> 
> OK, so you shipped thousands and thousands of devices with mainline
> kernels and never tested the capsule update feature, which now turns
> out to require modifications to support the non-UEFI compliant
> firmware on these devices.

We are shipping an open platform. The users can download a reference
image with Yocto-Linux that comes with a Yocto kernel plus some enabling
patches. One of them is currently a forward-port of the original Intel
capsule loader driver that does a similar thing like these patches and
therefore works fine (of course firmware update has been tested before
the release).

But in order to overcome the dependencies on Yocto kernels as well our
own patch queue, we are in the process of upstreaming necessary changes
(and upstream cleanups as well, some are already merged). In the end,
our users should have the possibility to chose mainline or Yocto or some
other kernel flavour without having the need for additional BSP patches.
That will ensure long-term support for the hardware, software-wise.
Users already asked us if they will eventually be stuck with a patch
queue and, thus, an outdated kernel like it is a sad standard in this
domain. But that is not our plan.

Yes, in an ideal world, this discussion had happened earlier and
prevented at least our deployment of the non-standard firmware. But the
world is not always working ideally.

> 
> I'm sorry, but that puts it firmly in the 'not our problem' category,
> simply because I refuse to believe that you would seriously consider
> performing this kind of firmware update on that many devices in the
> field if you never tested it in development.

I would recommend reading my email completely (see below) to understand
who I was targeting.

> 
> So while I fully agree that
> a) it is quite unfortunate that Intel, which has such a dominant
> presence in all aspects of UEFI and PI standardization, ships a
> non-compliant BSP, and
> b) it is useful to be able to sign capsules,
> I think we should push back on random, unstandardized signature headers.
> 
> The argument that Quark is the only working implementation of capsule
> updates, and so we should support it, does not hold. First of all,
> arm64 servers are shipping with working capsule update based on the
> current kernel implementation, but what is currently shipping is not
> really the point for mainline imo, but what is intended to be shipped
> with the next kernel release.
> 
> I would not object strongly to having conditionally compiled code in
> mainline that adds support for this, but bodging the default code path
> like this for a Quark quirk is out of the question imo.

I'm open for any consensus that avoids bending mainline too much and
still helps us (and maybe also other Quark X1020 integrators) getting
rid of additional patches.

Thanks,
Jan

> 
> Thanks,
> Ard.
> 
> 
> 
>>>
>>> The proper implementation may require to work with UEFI community
>>> to expand its capsule spec to support signed binary.
>>>
>>
>> Are you working on this? How is this solved on other platforms that
>> require signatures? No one tried that yet? In any case, this sounds like
>> a lengthy, way too late considered process that will not solve our issue
>> in the foreseeable future.
>>
>> Don't get me wrong, I'm not intending to push this into the kernel
>> because "What the heck?!" was my first reaction as well once I found out
>> how this update interface is actually working. But maybe you can bring
>> this topic up on your side as well so that we come to an upstreamable
>> solution in all affected projects.
>>
>> Thanks,
>> Jan
>>
>> PS: @Daniel, another example for your presentation. ;)
>>
>> --
>> Siemens AG, Corporate Technology, CT RDA ITP SES-DE
>> Corporate Competence Center Embedded Linux

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

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

#1584303 — Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images

On 19/02/17 13:33, Jan Kiszka wrote:
>> I would not object strongly to having conditionally compiled code in
>> mainline that adds support for this, but bodging the default code path
>> like this for a Quark quirk is out of the question imo.
> I'm open for any consensus that avoids bending mainline too much and
> still helps us (and maybe also other Quark X1020 integrators) getting
> rid of additional patches.

We could make efi_capsule_setup_info() a weak symbol just like

drivers/firmware/efi/reboot.c:
bool __weak efi_poweroff_required(void)

that way Arm is none the wiser and we can bury the Quark Quirk in 
x86/platform/efi/quirks.c - where you're right Ard it arguably belongs - 
not in the core code.

diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 30031d5..950663da 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -495,3 +495,19 @@ bool efi_poweroff_required(void)
  {
         return acpi_gbl_reduced_hardware || acpi_no_s5;
  }
+
+ssize_t csh_efi_capsule_setup_info(struct capsule_info *cap_info,
+                                  void *kbuff, size_t hdr_bytes)
+{
+       /* Code to deal with the CSH goes here */
+       return 0;
+}
+
+ssize_t efi_capsule_setup_info(struct capsule_info *cap_info,
+                              void *kbuff, size_t hdr_bytes)
+{
+       if (quark)
+               return csh_efi_capsule_setup_info(cap_info, kbuff, 
hdr_bytes);
+       else
+               return __efi_capsule_setup_info(cap_info, kbuff, hdr_bytes);
+}

diff --git a/drivers/firmware/efi/capsule-loader.c 
b/drivers/firmware/efi/capsule-loader.c
index 9ae6c11..d8bdc6f 100644
--- a/drivers/firmware/efi/capsule-loader.c
+++ b/drivers/firmware/efi/capsule-loader.c
@@ -53,7 +53,7 @@ static void efi_free_all_buff_pages(struct 
capsule_info *cap_info)
   * @kbuff: a mapped first page buffer pointer
   * @hdr_bytes: the total received number of bytes for efi header
   **/
-static ssize_t efi_capsule_setup_info(struct capsule_info *cap_info,
+ssize_t __efi_capsule_setup_info(struct capsule_info *cap_info,
                                       void *kbuff, size_t hdr_bytes)
  {
         efi_capsule_header_t *cap_hdr;
@@ -98,6 +98,13 @@ static ssize_t efi_capsule_setup_info(struct 
capsule_info *cap_info,

         return 0;
  }
+EXPORT_SYMBOL_GPL(__efi_capsule_setup_info);
+
+ssize_t __weak efi_capsule_setup_info(struct capsule_info *cap_info,
+                                            void *kbuff, size_t hdr_bytes)
+{
+       return __efi_capsule_setup_info(cap_info, kbuff, hdr_bytes);
+}

One thing we want is to continue to have Quark work on ia32 builds 
without having to compile a Quark specific kernel just to get this 
feature working.

Jan I haven't had time to look at what you said about the BSP code not 
working with capsules on Gen2 (I will during the week though). If you 
currently have to strip the CSH to make this work then we're missing a 
trick on tip-of-tree and need to sort that out for the final version of 
this.

---
bod

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

#1584306 — Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images

On 2017-02-19 17:33, Bryan O'Donoghue wrote:
> 
> 
> On 19/02/17 13:33, Jan Kiszka wrote:
>>> I would not object strongly to having conditionally compiled code in
>>> mainline that adds support for this, but bodging the default code path
>>> like this for a Quark quirk is out of the question imo.
>> I'm open for any consensus that avoids bending mainline too much and
>> still helps us (and maybe also other Quark X1020 integrators) getting
>> rid of additional patches.
> 
> We could make efi_capsule_setup_info() a weak symbol just like
> 
> drivers/firmware/efi/reboot.c:
> bool __weak efi_poweroff_required(void)
> 
> that way Arm is none the wiser and we can bury the Quark Quirk in
> x86/platform/efi/quirks.c - where you're right Ard it arguably belongs -
> not in the core code.
> 
> diff --git a/arch/x86/platform/efi/quirks.c
> b/arch/x86/platform/efi/quirks.c
> index 30031d5..950663da 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -495,3 +495,19 @@ bool efi_poweroff_required(void)
>  {
>         return acpi_gbl_reduced_hardware || acpi_no_s5;
>  }
> +
> +ssize_t csh_efi_capsule_setup_info(struct capsule_info *cap_info,
> +                                  void *kbuff, size_t hdr_bytes)
> +{
> +       /* Code to deal with the CSH goes here */
> +       return 0;
> +}
> +
> +ssize_t efi_capsule_setup_info(struct capsule_info *cap_info,
> +                              void *kbuff, size_t hdr_bytes)
> +{
> +       if (quark)
> +               return csh_efi_capsule_setup_info(cap_info, kbuff,
> hdr_bytes);
> +       else
> +               return __efi_capsule_setup_info(cap_info, kbuff,
> hdr_bytes);
> +}
> 
> diff --git a/drivers/firmware/efi/capsule-loader.c
> b/drivers/firmware/efi/capsule-loader.c
> index 9ae6c11..d8bdc6f 100644
> --- a/drivers/firmware/efi/capsule-loader.c
> +++ b/drivers/firmware/efi/capsule-loader.c
> @@ -53,7 +53,7 @@ static void efi_free_all_buff_pages(struct
> capsule_info *cap_info)
>   * @kbuff: a mapped first page buffer pointer
>   * @hdr_bytes: the total received number of bytes for efi header
>   **/
> -static ssize_t efi_capsule_setup_info(struct capsule_info *cap_info,
> +ssize_t __efi_capsule_setup_info(struct capsule_info *cap_info,
>                                       void *kbuff, size_t hdr_bytes)
>  {
>         efi_capsule_header_t *cap_hdr;
> @@ -98,6 +98,13 @@ static ssize_t efi_capsule_setup_info(struct
> capsule_info *cap_info,
> 
>         return 0;
>  }
> +EXPORT_SYMBOL_GPL(__efi_capsule_setup_info);
> +
> +ssize_t __weak efi_capsule_setup_info(struct capsule_info *cap_info,
> +                                            void *kbuff, size_t hdr_bytes)
> +{
> +       return __efi_capsule_setup_info(cap_info, kbuff, hdr_bytes);
> +}
>

Good idea.

> One thing we want is to continue to have Quark work on ia32 builds
> without having to compile a Quark specific kernel just to get this
> feature working.
> 
> Jan I haven't had time to look at what you said about the BSP code not
> working with capsules on Gen2 (I will during the week though). If you
> currently have to strip the CSH to make this work then we're missing a
> trick on tip-of-tree and need to sort that out for the final version of
> this.

Yes, I agree. I will look into this when I'm back from ELC next week (no
related hardware with me).

Jan

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

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

#1583006 — Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images

On 16/02/17 03:00, Kweh, Hock Leong wrote:
>> -----Original Message-----
>> From: Jan Kiszka [mailto:jan.kiszka@siemens.com]
>> Sent: Thursday, February 16, 2017 3:00 AM
>> To: Andy Shevchenko <andy.shevchenko@gmail.com>
>> Cc: Matt Fleming <matt@codeblueprint.co.uk>; Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>; linux-efi@vger.kernel.org; Linux Kernel Mailing
>> List <linux-kernel@vger.kernel.org>; Borislav Petkov <bp@alien8.de>; Kweh,
>> Hock Leong <hock.leong.kweh@intel.com>; Bryan O'Donoghue
>> <pure.logic@nexus-software.ie>
>> Subject: Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark
>> images
>>
>> On 2017-02-15 19:50, Jan Kiszka wrote:
>>> On 2017-02-15 19:46, Andy Shevchenko wrote:
>>>> On Wed, Feb 15, 2017 at 8:14 PM, Jan Kiszka <jan.kiszka@siemens.com>
>> wrote:
>>>>> See patch 2 for the background.
>>>>>
>>>>> Series has been tested on the Galileo Gen2, to exclude regressions,
>>>>> with a firmware.cap without security header and the SIMATIC IOT2040
>>>>> which requires the header because of its mandatory secure boot.
>>>>
>>>> Briefly looking to the code it looks like a real hack.
>>>> Sorry, but it would be carefully (re-)designed.
>>>
>>> The interface that the firmware provides us? That should have been
>>> done differently, I agree, but I'm not too much into those firmware
>>> details, specifically when it comes to signatures.
>>>
>>> The Linux code was designed around that suboptimal situation. If there
>>> are better ideas, I'm all ears.
>>>
>>
>> Expanding CC's as requested by Andy.
>>
>> Jan
>>
>
> Hi Jan,
>
> While I upstreaming the capsule loader patches, I did work with maintainer
> Matt and look into this security header created for Quark. Eventually both
> of us agreed that this will not be upstream to mainline as it is really a Quark
> specific implementation.

What's the logic of that ?

It should be possible to provide a hook (or a custom function).

>
> The proper implementation may require to work with UEFI community
> to expand its capsule spec to support signed binary.

Are you volunteering to do that with - getting the CSH into the UEFI spec ?

If not then we should have a method to load/ignore a capsule including 
the CSH, if so then we should have a realistic timeline laid out for 
getting that spec work done.

Hint: I don't believe integrating the CSH into the UEFI standard will 
happen...

>
>
> Regards,
> Wilson
>

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

#1583197 — RE: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images

> -----Original Message-----
> From: Bryan O'Donoghue [mailto:pure.logic@nexus-software.ie]
> Sent: Friday, February 17, 2017 8:54 AM
> To: Kweh, Hock Leong <hock.leong.kweh@intel.com>; Jan Kiszka
> <jan.kiszka@siemens.com>; Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Matt Fleming <matt@codeblueprint.co.uk>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; linux-efi@vger.kernel.org; Linux Kernel Mailing
> List <linux-kernel@vger.kernel.org>; Borislav Petkov <bp@alien8.de>
> Subject: Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark
> images
> 
> 
> 
> On 16/02/17 03:00, Kweh, Hock Leong wrote:
> >> -----Original Message-----
> >> From: Jan Kiszka [mailto:jan.kiszka@siemens.com]
> >> Sent: Thursday, February 16, 2017 3:00 AM
> >> To: Andy Shevchenko <andy.shevchenko@gmail.com>
> >> Cc: Matt Fleming <matt@codeblueprint.co.uk>; Ard Biesheuvel
> >> <ard.biesheuvel@linaro.org>; linux-efi@vger.kernel.org; Linux Kernel
> >> Mailing List <linux-kernel@vger.kernel.org>; Borislav Petkov
> >> <bp@alien8.de>; Kweh, Hock Leong <hock.leong.kweh@intel.com>; Bryan
> >> O'Donoghue <pure.logic@nexus-software.ie>
> >> Subject: Re: [PATCH 0/2] efi: Enhance capsule loader to support
> >> signed Quark images
> >>
> >> On 2017-02-15 19:50, Jan Kiszka wrote:
> >>> On 2017-02-15 19:46, Andy Shevchenko wrote:
> >>>> On Wed, Feb 15, 2017 at 8:14 PM, Jan Kiszka
> >>>> <jan.kiszka@siemens.com>
> >> wrote:
> >>>>> See patch 2 for the background.
> >>>>>
> >>>>> Series has been tested on the Galileo Gen2, to exclude
> >>>>> regressions, with a firmware.cap without security header and the
> >>>>> SIMATIC IOT2040 which requires the header because of its mandatory
> secure boot.
> >>>>
> >>>> Briefly looking to the code it looks like a real hack.
> >>>> Sorry, but it would be carefully (re-)designed.
> >>>
> >>> The interface that the firmware provides us? That should have been
> >>> done differently, I agree, but I'm not too much into those firmware
> >>> details, specifically when it comes to signatures.
> >>>
> >>> The Linux code was designed around that suboptimal situation. If
> >>> there are better ideas, I'm all ears.
> >>>
> >>
> >> Expanding CC's as requested by Andy.
> >>
> >> Jan
> >>
> >
> > Hi Jan,
> >
> > While I upstreaming the capsule loader patches, I did work with
> > maintainer Matt and look into this security header created for Quark.
> > Eventually both of us agreed that this will not be upstream to
> > mainline as it is really a Quark specific implementation.
> 
> What's the logic of that ?
> 
> It should be possible to provide a hook (or a custom function).
> 
> >
> > The proper implementation may require to work with UEFI community to
> > expand its capsule spec to support signed binary.
> 
> Are you volunteering to do that with - getting the CSH into the UEFI spec ?
> 
> If not then we should have a method to load/ignore a capsule including the CSH,
> if so then we should have a realistic timeline laid out for getting that spec work
> done.
> 
> Hint: I don't believe integrating the CSH into the UEFI standard will happen...
> 

Hi Jan & Bryan,

Please don't get me wrong. I am not rejecting the patch submission.
I am just sharing a summary for the discussion that I had had it a year back
with maintainer Matt for this topic. From the discussion, we did mention
what would be the proper handling to this case. And to have UEFI expand
it capsule support and take in signed binary would be a more secured way.
So, influencing UEFI community to have such support would be the right
move throughout the discussion. That is my summary.

Of course, influencing the UEFI community would be the longer path.
I think it is worth for try to bring the topic up here again to see if Matt
would reconsider it. 


Regards,
Wilson

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

#1583232 — Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images

On 2017-02-17 09:23, Kweh, Hock Leong wrote:
>> -----Original Message-----
>> From: Bryan O'Donoghue [mailto:pure.logic@nexus-software.ie]
>> Sent: Friday, February 17, 2017 8:54 AM
>> To: Kweh, Hock Leong <hock.leong.kweh@intel.com>; Jan Kiszka
>> <jan.kiszka@siemens.com>; Andy Shevchenko <andy.shevchenko@gmail.com>
>> Cc: Matt Fleming <matt@codeblueprint.co.uk>; Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>; linux-efi@vger.kernel.org; Linux Kernel Mailing
>> List <linux-kernel@vger.kernel.org>; Borislav Petkov <bp@alien8.de>
>> Subject: Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark
>> images
>>
>>
>>
>> On 16/02/17 03:00, Kweh, Hock Leong wrote:
>>>> -----Original Message-----
>>>> From: Jan Kiszka [mailto:jan.kiszka@siemens.com]
>>>> Sent: Thursday, February 16, 2017 3:00 AM
>>>> To: Andy Shevchenko <andy.shevchenko@gmail.com>
>>>> Cc: Matt Fleming <matt@codeblueprint.co.uk>; Ard Biesheuvel
>>>> <ard.biesheuvel@linaro.org>; linux-efi@vger.kernel.org; Linux Kernel
>>>> Mailing List <linux-kernel@vger.kernel.org>; Borislav Petkov
>>>> <bp@alien8.de>; Kweh, Hock Leong <hock.leong.kweh@intel.com>; Bryan
>>>> O'Donoghue <pure.logic@nexus-software.ie>
>>>> Subject: Re: [PATCH 0/2] efi: Enhance capsule loader to support
>>>> signed Quark images
>>>>
>>>> On 2017-02-15 19:50, Jan Kiszka wrote:
>>>>> On 2017-02-15 19:46, Andy Shevchenko wrote:
>>>>>> On Wed, Feb 15, 2017 at 8:14 PM, Jan Kiszka
>>>>>> <jan.kiszka@siemens.com>
>>>> wrote:
>>>>>>> See patch 2 for the background.
>>>>>>>
>>>>>>> Series has been tested on the Galileo Gen2, to exclude
>>>>>>> regressions, with a firmware.cap without security header and the
>>>>>>> SIMATIC IOT2040 which requires the header because of its mandatory
>> secure boot.
>>>>>>
>>>>>> Briefly looking to the code it looks like a real hack.
>>>>>> Sorry, but it would be carefully (re-)designed.
>>>>>
>>>>> The interface that the firmware provides us? That should have been
>>>>> done differently, I agree, but I'm not too much into those firmware
>>>>> details, specifically when it comes to signatures.
>>>>>
>>>>> The Linux code was designed around that suboptimal situation. If
>>>>> there are better ideas, I'm all ears.
>>>>>
>>>>
>>>> Expanding CC's as requested by Andy.
>>>>
>>>> Jan
>>>>
>>>
>>> Hi Jan,
>>>
>>> While I upstreaming the capsule loader patches, I did work with
>>> maintainer Matt and look into this security header created for Quark.
>>> Eventually both of us agreed that this will not be upstream to
>>> mainline as it is really a Quark specific implementation.
>>
>> What's the logic of that ?
>>
>> It should be possible to provide a hook (or a custom function).
>>
>>>
>>> The proper implementation may require to work with UEFI community to
>>> expand its capsule spec to support signed binary.
>>
>> Are you volunteering to do that with - getting the CSH into the UEFI spec ?
>>
>> If not then we should have a method to load/ignore a capsule including the CSH,
>> if so then we should have a realistic timeline laid out for getting that spec work
>> done.
>>
>> Hint: I don't believe integrating the CSH into the UEFI standard will happen...
>>
> 
> Hi Jan & Bryan,
> 
> Please don't get me wrong. I am not rejecting the patch submission.
> I am just sharing a summary for the discussion that I had had it a year back
> with maintainer Matt for this topic. From the discussion, we did mention
> what would be the proper handling to this case. And to have UEFI expand

Do you happen to have a reference to the part of the discussions that
deal with the CSH topic?

> it capsule support and take in signed binary would be a more secured way.
> So, influencing UEFI community to have such support would be the right
> move throughout the discussion. That is my summary.
> 
> Of course, influencing the UEFI community would be the longer path.
> I think it is worth for try to bring the topic up here again to see if Matt
> would reconsider it. 

I just can re-express my frustration that this essential step hasn't
been started years ago by whoever designed the extension. Then I bet
there would have been constructive feedback on the interface BEFORE its
ugliness spread to broader use.

Or is there a technical need, in general or on Quark, to have the
signature header right before the standard capsule *for the handover* to
the firmware? I mean, I would naively put it into another capsule and
prepend that to the core so that the existing UEFI API can palate it
transparently and cleanly.

Jan

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

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

#1589398 — Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images

On Fri, 17 Feb, at 10:24:41AM, Jan Kiszka wrote:
> 
> I just can re-express my frustration that this essential step hasn't
> been started years ago by whoever designed the extension. Then I bet
> there would have been constructive feedback on the interface BEFORE its
> ugliness spread to broader use.
> 
> Or is there a technical need, in general or on Quark, to have the
> signature header right before the standard capsule *for the handover* to
> the firmware? I mean, I would naively put it into another capsule and
> prepend that to the core so that the existing UEFI API can palate it
> transparently and cleanly.

I'm fairly sure this was my first thought when we discussed this
originally, some years ago now.

The whole CSH concept is, frankly, stupid. It makes a mockery of
everything the capsule interface was designed to be.

I have long been holding out in hope that someone would patch the
firmware to work around this CSH requirement, something along the
lines of the double wrapping Jan mentions above. It's not like the
Quark is the only platform that wants to verify capsules.

But to my knowledge, that hasn't happened.

Nevertheless my answer is still the same - someone needs to go and
update the Quark firmware source to work with the generic capsule
mechanism.

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web