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

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

[Jan Kiszka]

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

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

[Jan Kiszka]

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

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

[Jan Kiszka]

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

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

[Ard Biesheuvel]

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
>

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

[Ard Biesheuvel]

On 15 February 2017 at 18:14, Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org> 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
>

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

[Andy Shevchenko]

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

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

[Andy Shevchenko]

On Wed, Feb 15, 2017 at 8:14 PM, Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org> 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

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

[Andy Shevchenko]

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

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

[Andy Shevchenko]

On Wed, Feb 15, 2017 at 8:14 PM, Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org> 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

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

[Jan Kiszka]

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

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

[Jan Kiszka]

On 2017-02-15 19:17, Ard Biesheuvel wrote:
> On 15 February 2017 at 18:14, Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org> 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

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

[Jan Kiszka]

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

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

[Jan Kiszka]

On 2017-02-15 19:46, Andy Shevchenko wrote:
> On Wed, Feb 15, 2017 at 8:14 PM, Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org> 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

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

[Jan Kiszka]

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

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

[Jan Kiszka]

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-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org> 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

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

[Kweh, Hock Leong]

> -----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

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

[Kweh, Hock Leong]

> -----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

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

[Jan Kiszka]

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

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

[Jan Kiszka]

On 2017-02-16 04:00, Kweh, Hock Leong wrote:
>> -----Original Message-----
>> From: Jan Kiszka [mailto:jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org]
>> Sent: Thursday, February 16, 2017 3:00 AM
>> To: Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>; Ard Biesheuvel
>> <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>; linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Linux Kernel Mailing
>> List <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>; Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org>; Kweh,
>> Hock Leong <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>; Bryan O'Donoghue
>> <pure.logic-SyKdqv6vbfZdzvEItQ6vdLNAH6kLmebB@public.gmane.org>
>> 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-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
>> 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

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

[Bryan O'Donoghue]

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
>

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

[Bryan O'Donoghue]

On 16/02/17 03:00, Kweh, Hock Leong wrote:
>> -----Original Message-----
>> From: Jan Kiszka [mailto:jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org]
>> Sent: Thursday, February 16, 2017 3:00 AM
>> To: Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>; Ard Biesheuvel
>> <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>; linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Linux Kernel Mailing
>> List <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>; Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org>; Kweh,
>> Hock Leong <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>; Bryan O'Donoghue
>> <pure.logic-SyKdqv6vbfZdzvEItQ6vdLNAH6kLmebB@public.gmane.org>
>> 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-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
>> 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
>

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

[Bryan O'Donoghue]

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

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

[Bryan O'Donoghue]

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-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
> ---
>  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

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

[Kweh, Hock Leong]

> -----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

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

[Kweh, Hock Leong]

> -----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

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

[Jan Kiszka]

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

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

[Jan Kiszka]

On 2017-02-17 09:23, Kweh, Hock Leong wrote:
>> -----Original Message-----
>> From: Bryan O'Donoghue [mailto:pure.logic-SyKdqv6vbfZdzvEItQ6vdLNAH6kLmebB@public.gmane.org]
>> Sent: Friday, February 17, 2017 8:54 AM
>> To: Kweh, Hock Leong <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>; Jan Kiszka
>> <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>; Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>; Ard Biesheuvel
>> <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>; linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Linux Kernel Mailing
>> List <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>; Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org>
>> 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-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org]
>>>> Sent: Thursday, February 16, 2017 3:00 AM
>>>> To: Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>; Ard Biesheuvel
>>>> <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>; linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Linux Kernel
>>>> Mailing List <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>; Borislav Petkov
>>>> <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org>; Kweh, Hock Leong <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>; Bryan
>>>> O'Donoghue <pure.logic-SyKdqv6vbfZdzvEItQ6vdLNAH6kLmebB@public.gmane.org>
>>>> 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-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
>>>> 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

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

[Bryan O'Donoghue]

On 17/02/17 08:23, Kweh, Hock Leong wrote:
> 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.

CSH stands for "Clanton Secure Header" - Clanton being the internal
code-name for Quark X1000 prior to release.

There is no chance the UEFI standard (which can be used on ARM and
potentially other architectures) will accept a SoC specific
route-of-trust prepended header.

Sure some kind of binary signed headers might become part of the
standard eventually but, definitely _not_ a CSH.

The fact is CSH exists in the real-world and a UEFI firmware supports
accepting the CSH/UEFI-capsule pair for updating itself.

I think a far more practical solution is to accommodate the defacto
implementation (the only ? current implementation). To me it defies
reason to have Quark X1000 be the only system (that I know of) capable
of doing a capsule update - have capsule code in the kernel - but _not_
support the header prepended to that capsule that the Quark
firmware/bootrom require.

Right now the capsule code is dead code on Quark x1000. Let's do the
right thing and make it usable. I fully support having a
separate/parallel conversation with the UEFI body but, I'd be amazed if
the "Clanton Secure Header" made it into the standard...

-- 
bod

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

[Bryan O'Donoghue]

On 17/02/17 08:23, Kweh, Hock Leong wrote:
> 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.

CSH stands for "Clanton Secure Header" - Clanton being the internal
code-name for Quark X1000 prior to release.

There is no chance the UEFI standard (which can be used on ARM and
potentially other architectures) will accept a SoC specific
route-of-trust prepended header.

Sure some kind of binary signed headers might become part of the
standard eventually but, definitely _not_ a CSH.

The fact is CSH exists in the real-world and a UEFI firmware supports
accepting the CSH/UEFI-capsule pair for updating itself.

I think a far more practical solution is to accommodate the defacto
implementation (the only ? current implementation). To me it defies
reason to have Quark X1000 be the only system (that I know of) capable
of doing a capsule update - have capsule code in the kernel - but _not_
support the header prepended to that capsule that the Quark
firmware/bootrom require.

Right now the capsule code is dead code on Quark x1000. Let's do the
right thing and make it usable. I fully support having a
separate/parallel conversation with the UEFI body but, I'd be amazed if
the "Clanton Secure Header" made it into the standard...

-- 
bod

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

[Jan Kiszka]

On 2017-02-17 10:51, Bryan O'Donoghue wrote:
> On 17/02/17 08:23, Kweh, Hock Leong wrote:
>> 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.
> 
> CSH stands for "Clanton Secure Header" - Clanton being the internal
> code-name for Quark X1000 prior to release.
> 
> There is no chance the UEFI standard (which can be used on ARM and
> potentially other architectures) will accept a SoC specific
> route-of-trust prepended header.
> 
> Sure some kind of binary signed headers might become part of the
> standard eventually but, definitely _not_ a CSH.
> 
> The fact is CSH exists in the real-world and a UEFI firmware supports
> accepting the CSH/UEFI-capsule pair for updating itself.
> 
> I think a far more practical solution is to accommodate the defacto
> implementation (the only ? current implementation). To me it defies
> reason to have Quark X1000 be the only system (that I know of) capable
> of doing a capsule update - have capsule code in the kernel - but _not_
> support the header prepended to that capsule that the Quark
> firmware/bootrom require.
> 
> Right now the capsule code is dead code on Quark x1000. Let's do the
> right thing and make it usable. I fully support having a
> separate/parallel conversation with the UEFI body but, I'd be amazed if
> the "Clanton Secure Header" made it into the standard...
> 

To be precise, CSH is only required on X102x. The X100x SoCs, those are
also found on the Galileo Gen2 maker board, do not support secure boot
and do not use the header. IIRC, there used to be an eval system with
the X1020 as well, but I think it's no longer available.

Interestingly, the capsule file found in Intel's Galileo firmware update
package [1] contains the CSH header. But I only succeeded flashing it on
a Gen2 by removing the header first.

Jan

[1]
https://downloadcenter.intel.com/download/26417/Intel-Galileo-Firmware-Updater-and-Drivers?product=83137

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

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

[Jan Kiszka]

On 2017-02-17 10:51, Bryan O'Donoghue wrote:
> On 17/02/17 08:23, Kweh, Hock Leong wrote:
>> 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.
> 
> CSH stands for "Clanton Secure Header" - Clanton being the internal
> code-name for Quark X1000 prior to release.
> 
> There is no chance the UEFI standard (which can be used on ARM and
> potentially other architectures) will accept a SoC specific
> route-of-trust prepended header.
> 
> Sure some kind of binary signed headers might become part of the
> standard eventually but, definitely _not_ a CSH.
> 
> The fact is CSH exists in the real-world and a UEFI firmware supports
> accepting the CSH/UEFI-capsule pair for updating itself.
> 
> I think a far more practical solution is to accommodate the defacto
> implementation (the only ? current implementation). To me it defies
> reason to have Quark X1000 be the only system (that I know of) capable
> of doing a capsule update - have capsule code in the kernel - but _not_
> support the header prepended to that capsule that the Quark
> firmware/bootrom require.
> 
> Right now the capsule code is dead code on Quark x1000. Let's do the
> right thing and make it usable. I fully support having a
> separate/parallel conversation with the UEFI body but, I'd be amazed if
> the "Clanton Secure Header" made it into the standard...
> 

To be precise, CSH is only required on X102x. The X100x SoCs, those are
also found on the Galileo Gen2 maker board, do not support secure boot
and do not use the header. IIRC, there used to be an eval system with
the X1020 as well, but I think it's no longer available.

Interestingly, the capsule file found in Intel's Galileo firmware update
package [1] contains the CSH header. But I only succeeded flashing it on
a Gen2 by removing the header first.

Jan

[1]
https://downloadcenter.intel.com/download/26417/Intel-Galileo-Firmware-Updater-and-Drivers?product=83137

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

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

[Bryan O'Donoghue]

On 17/02/17 10:14, Jan Kiszka wrote:
> On 2017-02-17 10:51, Bryan O'Donoghue wrote:
>> On 17/02/17 08:23, Kweh, Hock Leong wrote:
>>> 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.
>>
>> CSH stands for "Clanton Secure Header" - Clanton being the internal
>> code-name for Quark X1000 prior to release.
>>
>> There is no chance the UEFI standard (which can be used on ARM and
>> potentially other architectures) will accept a SoC specific
>> route-of-trust prepended header.
>>
>> Sure some kind of binary signed headers might become part of the
>> standard eventually but, definitely _not_ a CSH.
>>
>> The fact is CSH exists in the real-world and a UEFI firmware supports
>> accepting the CSH/UEFI-capsule pair for updating itself.
>>
>> I think a far more practical solution is to accommodate the defacto
>> implementation (the only ? current implementation). To me it defies
>> reason to have Quark X1000 be the only system (that I know of) capable
>> of doing a capsule update - have capsule code in the kernel - but _not_
>> support the header prepended to that capsule that the Quark
>> firmware/bootrom require.
>>
>> Right now the capsule code is dead code on Quark x1000. Let's do the
>> right thing and make it usable. I fully support having a
>> separate/parallel conversation with the UEFI body but, I'd be amazed if
>> the "Clanton Secure Header" made it into the standard...
>>
> 
> To be precise, CSH is only required on X102x. The X100x SoCs, those are
> also found on the Galileo Gen2 maker board, do not support secure boot
> and do not use the header.

The CSH is supported on the non-secure SoCs - the BSP on Gen1 certainly did.

https://downloadcenter.intel.com/download/23197/Intel-Quark-SoC-X1000-Board-Support-Package-BSP-

- > QuarkPlatformPkg/Platform/Dxe/PlatformInit/DxeCapsuleSecurity.c

CreateCapsuleBufferForWriting()

Looks like it will tolerate a lack of CSH but, obviously that's no
solution for the secure boot parts.

> IIRC, there used to be an eval system with
> the X1020 as well, but I think it's no longer available.
> 
> Interestingly, the capsule file found in Intel's Galileo firmware update
> package [1] contains the CSH header. But I only succeeded flashing it on
> a Gen2 by removing the header first.

Hmm - the out of the box firmware will accept capsules from the website.
Gen1 and Gen2 should be the same in that respect - if you use the BSP
kernel.

You should validate the out-of-the-box capsule update - with the kernel
on SPI flash works with the CSH in place.

Next step then is to get it working on tip-of-tree.

I have one Gen1 left (which I won't be experimenting with) - and I think
one Gen2 (which I don't especially mind blowing up).

Let's take the time to validate (or repudiate)

1. Out of the box BSP works with CSH capsules in place
2. New tip-of-tree addition supports CSH capsules

and review.

Stripping the CSH shouldn't be necessary (and breaks the secure boot
parts anyway).

-- 
bod

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

[Ard Biesheuvel]

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

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

[Jan Kiszka]

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

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

[Jan Kiszka]

On 2017-02-18 13:48, Ard Biesheuvel wrote:
> On 16 February 2017 at 07:29, Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org> wrote:
>> On 2017-02-16 04:00, Kweh, Hock Leong wrote:
>>>> -----Original Message-----
>>>> From: Jan Kiszka [mailto:jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org]
>>>> Sent: Thursday, February 16, 2017 3:00 AM
>>>> To: Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>; Ard Biesheuvel
>>>> <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>; linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Linux Kernel Mailing
>>>> List <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>; Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org>; Kweh,
>>>> Hock Leong <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>; Bryan O'Donoghue
>>>> <pure.logic-SyKdqv6vbfZdzvEItQ6vdLNAH6kLmebB@public.gmane.org>
>>>> 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-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
>>>> 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

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

[Bryan O'Donoghue]

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

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

[Jan Kiszka]

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

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

[Jan Kiszka]

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

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

[Matt Fleming]

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.

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

[Matt Fleming]

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.

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

[Jan Kiszka]

On 2017-02-28 13:12, Matt Fleming wrote:
> 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.
> 

>From you POV, does this exclude upstream quirk support for already
shipped devices?

Jan

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

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

[Jan Kiszka]

On 2017-02-28 13:12, Matt Fleming wrote:
> 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.
> 

>From you POV, does this exclude upstream quirk support for already
shipped devices?

Jan

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

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

[Matt Fleming]

On Tue, 28 Feb, at 01:20:25PM, Jan Kiszka wrote:
> 
> From you POV, does this exclude upstream quirk support for already
> shipped devices?

It would need to be an extremely small, well-contained change, that
had no chance of disrupting other users of the capsule interface and
where I had a good feeling that supporting it wouldn't turn into a
maintenance nightmare (mountains of DMI strings or new platforms
coming to market that used it).

That's a tall order, and I'm pretty skeptical. Still, I'll never say
never. Plus Ard would need convincing to give his ACK too.

P.S. Has anyone actually investigated what would be required to fix
the firmware to be able to extract the CSH if it was contained inside
a capsule?

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

[Ard Biesheuvel]

On 28 February 2017 at 12:29, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Tue, 28 Feb, at 01:20:25PM, Jan Kiszka wrote:
>>
>> From you POV, does this exclude upstream quirk support for already
>> shipped devices?
>
> It would need to be an extremely small, well-contained change, that
> had no chance of disrupting other users of the capsule interface and
> where I had a good feeling that supporting it wouldn't turn into a
> maintenance nightmare (mountains of DMI strings or new platforms
> coming to market that used it).
>
> That's a tall order, and I'm pretty skeptical. Still, I'll never say
> never. Plus Ard would need convincing to give his ACK too.
>
> P.S. Has anyone actually investigated what would be required to fix
> the firmware to be able to extract the CSH if it was contained inside
> a capsule?

As I said before, I'd be ok with it if we select it compile time,
i.e., no runtime logic that infers whether we are running on such a
system or not, and no carrying both implementations in all kernels
that have capsule loading built in.

But I do realise that it increases the validation space for Matt,
given that he does the testing on the x86 side. For the ARM side of
things, the Kconfig option would simply not be settable. So I am going
to let Matt have the final word on this.

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

[Ard Biesheuvel]

On 28 February 2017 at 12:29, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> On Tue, 28 Feb, at 01:20:25PM, Jan Kiszka wrote:
>>
>> From you POV, does this exclude upstream quirk support for already
>> shipped devices?
>
> It would need to be an extremely small, well-contained change, that
> had no chance of disrupting other users of the capsule interface and
> where I had a good feeling that supporting it wouldn't turn into a
> maintenance nightmare (mountains of DMI strings or new platforms
> coming to market that used it).
>
> That's a tall order, and I'm pretty skeptical. Still, I'll never say
> never. Plus Ard would need convincing to give his ACK too.
>
> P.S. Has anyone actually investigated what would be required to fix
> the firmware to be able to extract the CSH if it was contained inside
> a capsule?

As I said before, I'd be ok with it if we select it compile time,
i.e., no runtime logic that infers whether we are running on such a
system or not, and no carrying both implementations in all kernels
that have capsule loading built in.

But I do realise that it increases the validation space for Matt,
given that he does the testing on the x86 side. For the ARM side of
things, the Kconfig option would simply not be settable. So I am going
to let Matt have the final word on this.

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

[Andy Shevchenko]

On Tue, Feb 28, 2017 at 3:25 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 28 February 2017 at 12:29, Matt Fleming <matt@codeblueprint.co.uk> wrote:
>> On Tue, 28 Feb, at 01:20:25PM, Jan Kiszka wrote:

> As I said before, I'd be ok with it if we select it compile time,
> i.e., no runtime logic that infers whether we are running on such a
> system or not, and no carrying both implementations in all kernels
> that have capsule loading built in.

Actually it most likely that Quark kernel (kernel compiled to be run
on Quark) will be ever used on the rest of (modern) x86 since it's
486+ architecture (kernel has specific option for it, 586TSC).
So, we might just be dependent or chosen by Quark.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Feb 28, 2017 at 3:25 PM, Ard Biesheuvel
<ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 28 February 2017 at 12:29, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
>> On Tue, 28 Feb, at 01:20:25PM, Jan Kiszka wrote:

> As I said before, I'd be ok with it if we select it compile time,
> i.e., no runtime logic that infers whether we are running on such a
> system or not, and no carrying both implementations in all kernels
> that have capsule loading built in.

Actually it most likely that Quark kernel (kernel compiled to be run
on Quark) will be ever used on the rest of (modern) x86 since it's
486+ architecture (kernel has specific option for it, 586TSC).
So, we might just be dependent or chosen by Quark.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Feb 28, 2017 at 3:35 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Feb 28, 2017 at 3:25 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> On 28 February 2017 at 12:29, Matt Fleming <matt@codeblueprint.co.uk> wrote:
>>> On Tue, 28 Feb, at 01:20:25PM, Jan Kiszka wrote:
>
>> As I said before, I'd be ok with it if we select it compile time,
>> i.e., no runtime logic that infers whether we are running on such a
>> system or not, and no carrying both implementations in all kernels
>> that have capsule loading built in.
>
> Actually it most likely that Quark kernel (kernel compiled to be run
> on Quark) will be ever used on the rest of (modern) x86 since it's
> 486+ architecture (kernel has specific option for it, 586TSC).

+ it's UP only!

> So, we might just be dependent or chosen by Quark.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Feb 28, 2017 at 3:35 PM, Andy Shevchenko
<andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Feb 28, 2017 at 3:25 PM, Ard Biesheuvel
> <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> On 28 February 2017 at 12:29, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
>>> On Tue, 28 Feb, at 01:20:25PM, Jan Kiszka wrote:
>
>> As I said before, I'd be ok with it if we select it compile time,
>> i.e., no runtime logic that infers whether we are running on such a
>> system or not, and no carrying both implementations in all kernels
>> that have capsule loading built in.
>
> Actually it most likely that Quark kernel (kernel compiled to be run
> on Quark) will be ever used on the rest of (modern) x86 since it's
> 486+ architecture (kernel has specific option for it, 586TSC).

+ it's UP only!

> So, we might just be dependent or chosen by Quark.

-- 
With Best Regards,
Andy Shevchenko

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

[Bryan O'Donoghue]

On 28/02/17 13:36, Andy Shevchenko wrote:
> On Tue, Feb 28, 2017 at 3:35 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Tue, Feb 28, 2017 at 3:25 PM, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org> wrote:
>>> On 28 February 2017 at 12:29, Matt Fleming <matt@codeblueprint.co.uk> wrote:
>>>> On Tue, 28 Feb, at 01:20:25PM, Jan Kiszka wrote:
>>
>>> As I said before, I'd be ok with it if we select it compile time,
>>> i.e., no runtime logic that infers whether we are running on such a
>>> system or not, and no carrying both implementations in all kernels
>>> that have capsule loading built in.
>>
>> Actually it most likely that Quark kernel (kernel compiled to be run
>> on Quark) will be ever used on the rest of (modern) x86 since it's
>> 486+ architecture (kernel has specific option for it, 586TSC).
> 
> + it's UP only!
> 
>> So, we might just be dependent or chosen by Quark.
> 

Still though the current ia32 kernel runs on Quark and all other ia32
systems. It would be a pity/shame to make this feature dependent on
compiling a Quark specific kernel, after all its only a header on a
capsule as opposed to a large hardware-level architectural divergence.

I'd still like us to try for a low-fat hook that would a big fat ia32
kernel just work without having to force a user compile up a
Quark-specific kernel.

-- 
bod

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

[Bryan O'Donoghue]

On 28/02/17 13:36, Andy Shevchenko wrote:
> On Tue, Feb 28, 2017 at 3:35 PM, Andy Shevchenko
> <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Tue, Feb 28, 2017 at 3:25 PM, Ard Biesheuvel
>> <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>> On 28 February 2017 at 12:29, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
>>>> On Tue, 28 Feb, at 01:20:25PM, Jan Kiszka wrote:
>>
>>> As I said before, I'd be ok with it if we select it compile time,
>>> i.e., no runtime logic that infers whether we are running on such a
>>> system or not, and no carrying both implementations in all kernels
>>> that have capsule loading built in.
>>
>> Actually it most likely that Quark kernel (kernel compiled to be run
>> on Quark) will be ever used on the rest of (modern) x86 since it's
>> 486+ architecture (kernel has specific option for it, 586TSC).
> 
> + it's UP only!
> 
>> So, we might just be dependent or chosen by Quark.
> 

Still though the current ia32 kernel runs on Quark and all other ia32
systems. It would be a pity/shame to make this feature dependent on
compiling a Quark specific kernel, after all its only a header on a
capsule as opposed to a large hardware-level architectural divergence.

I'd still like us to try for a low-fat hook that would a big fat ia32
kernel just work without having to force a user compile up a
Quark-specific kernel.

-- 
bod

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

[Bryan O'Donoghue]

On 28/02/17 15:07, Bryan O'Donoghue wrote:
> a big fat ia32

*allow a full fat..

-- 
bod

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

[Andy Shevchenko]

On Tue, Feb 28, 2017 at 5:07 PM, Bryan O'Donoghue
<pure.logic@nexus-software.ie> wrote:
> On 28/02/17 13:36, Andy Shevchenko wrote:
>> On Tue, Feb 28, 2017 at 3:35 PM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Tue, Feb 28, 2017 at 3:25 PM, Ard Biesheuvel
>>> <ard.biesheuvel@linaro.org> wrote:
>>>> On 28 February 2017 at 12:29, Matt Fleming <matt@codeblueprint.co.uk> wrote:
>>>>> On Tue, 28 Feb, at 01:20:25PM, Jan Kiszka wrote:
>>>
>>>> As I said before, I'd be ok with it if we select it compile time,
>>>> i.e., no runtime logic that infers whether we are running on such a
>>>> system or not, and no carrying both implementations in all kernels
>>>> that have capsule loading built in.
>>>
>>> Actually it most likely that Quark kernel (kernel compiled to be run
>>> on Quark) will be ever used on the rest of (modern) x86 since it's
>>> 486+ architecture (kernel has specific option for it, 586TSC).
>>
>> + it's UP only!
>>
>>> So, we might just be dependent or chosen by Quark.
>>
>
> Still though the current ia32 kernel runs on Quark and all other ia32
> systems.

How come? Quark has a silicon bug (SMP kernel might oops) and it is
not even i586!

> It would be a pity/shame to make this feature dependent on
> compiling a Quark specific kernel, after all its only a header on a
> capsule as opposed to a large hardware-level architectural divergence.
>

> I'd still like us to try for a low-fat hook that would a big fat ia32
> kernel just work without having to force a user compile up a
> Quark-specific kernel.

Can you elaborate how to run i686 kernel (which is default for x86
32-bit AFAIK) on Quark?

-- 
With Best Regards,
Andy Shevchenko

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

[Bryan O'Donoghue]

On 28/02/17 15:27, Andy Shevchenko wrote:
> On Tue, Feb 28, 2017 at 5:07 PM, Bryan O'Donoghue
> <pure.logic@nexus-software.ie> wrote:
>> On 28/02/17 13:36, Andy Shevchenko wrote:
>>> On Tue, Feb 28, 2017 at 3:35 PM, Andy Shevchenko
>>> <andy.shevchenko@gmail.com> wrote:
>>>> On Tue, Feb 28, 2017 at 3:25 PM, Ard Biesheuvel
>>>> <ard.biesheuvel@linaro.org> wrote:
>>>>> On 28 February 2017 at 12:29, Matt Fleming <matt@codeblueprint.co.uk> wrote:
>>>>>> On Tue, 28 Feb, at 01:20:25PM, Jan Kiszka wrote:
>>>>
>>>>> As I said before, I'd be ok with it if we select it compile time,
>>>>> i.e., no runtime logic that infers whether we are running on such a
>>>>> system or not, and no carrying both implementations in all kernels
>>>>> that have capsule loading built in.
>>>>
>>>> Actually it most likely that Quark kernel (kernel compiled to be run
>>>> on Quark) will be ever used on the rest of (modern) x86 since it's
>>>> 486+ architecture (kernel has specific option for it, 586TSC).
>>>
>>> + it's UP only!
>>>
>>>> So, we might just be dependent or chosen by Quark.
>>>
>>
>> Still though the current ia32 kernel runs on Quark and all other ia32
>> systems.
> 
> How come? Quark has a silicon bug (SMP kernel might oops) and it is
> not even i586!

sorry if this is a bit long in advance...

You mean a lock prefixed pagefault.

For reference Quark should be considered CONFIG_M586TSC=y (or better)
i.e. it's 586 ISA with a TSC added on.

I've been meaning to do a write up about this, since I've spent some
time with a debugger doing an analysis of the fault. Basically any
operation that is lock-prefixed that also page-faults pushes the address
of the _previous_ instruction (not the instruction that faulted) onto
the PF# stack. Which sucks.

Obviously then when you IRET you will execute the previous instruction
again - on return to user-space - as opposed to the instruction you
faulted on.

So it's nothing to do with SMP per se, except that the lock prefix was
added to drive the #lock signal on future SMP versions of the part (that
never happened). We discussed this @ the time Dave Jones did

commit d4e1a0af1d3a88cdfc8c954d3005eb8745ec518d
Author: Dave Jones <davej@redhat.com>
Date:   Tue Oct 28 13:57:53 2014 -0400

    x86: Don't enable F00F workaround on Intel Quark processors

... and we agreed to have a good look at lock prefixed instructions in
the kernel. On !SMP builds there is (or was at the time anyway) alot of
LOCK_PREFIX looking like this

#ifdef CONFIG_SMP
#define LOCK_PREFIX_HERE \
                ".pushsection .smp_locks,\"a\"\n"       \
                ".balign 4\n"                           \
                ".long 671f - .\n" /* offset */         \
                ".popsection\n"                         \
                "671:"

#define LOCK_PREFIX LOCK_PREFIX_HERE "\n\tlock; "

#else /* ! CONFIG_SMP */
#define LOCK_PREFIX_HERE ""
#define LOCK_PREFIX ""
#endif

Which meant that !SMP was safe. It was probably overkill though because
kernel code shouldn't PF anyway.

User-space is another story.

This image faults reliably in rpc-stad and sshd - because like I say -
lock-prefixed page faults push the previous (not the current)
instruction onto the pagefault stack.

https://sourceforge.net/projects/galileodebian/

anyway...

!SMP ia32 builds should be fine and we have never actually seen _kernel_
code die on SMP builds ... presumably (demonstrably) because lock
prefixed operations in the kernel don't PF#.

>> It would be a pity/shame to make this feature dependent on
>> compiling a Quark specific kernel, after all its only a header on a
>> capsule as opposed to a large hardware-level architectural divergence.
>>
> 
>> I'd still like us to try for a low-fat hook that would a big fat ia32
>> kernel just work without having to force a user compile up a
>> Quark-specific kernel.
> 
> Can you elaborate how to run i686 kernel (which is default for x86
> 32-bit AFAIK) on Quark?

A kernel compiled like this

make menuconfig ARCH=i386
make bzImage -j 8

will run just fine on Quark x1000 I do it regularly. CPUID ought to (and
does) inform the runtime kernel of what to do re: MSRs etc.

We won't execute xmm/mmx, we won't touch 686 specific MSRs etc, etc.

-- 
bod

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

[Andy Shevchenko]

On Tue, Feb 28, 2017 at 6:52 PM, Bryan O'Donoghue
<pure.logic@nexus-software.ie> wrote:
> On 28/02/17 15:27, Andy Shevchenko wrote:
>> On Tue, Feb 28, 2017 at 5:07 PM, Bryan O'Donoghue
>> <pure.logic@nexus-software.ie> wrote:
>>> On 28/02/17 13:36, Andy Shevchenko wrote:
>>>> On Tue, Feb 28, 2017 at 3:35 PM, Andy Shevchenko
>>>> <andy.shevchenko@gmail.com> wrote:
>>>>> On Tue, Feb 28, 2017 at 3:25 PM, Ard Biesheuvel
>>>>> <ard.biesheuvel@linaro.org> wrote:
>>>>>> On 28 February 2017 at 12:29, Matt Fleming <matt@codeblueprint.co.uk> wrote:
>>>>>>> On Tue, 28 Feb, at 01:20:25PM, Jan Kiszka wrote:
>>>>>
>>>>>> As I said before, I'd be ok with it if we select it compile time,
>>>>>> i.e., no runtime logic that infers whether we are running on such a
>>>>>> system or not, and no carrying both implementations in all kernels
>>>>>> that have capsule loading built in.
>>>>>
>>>>> Actually it most likely that Quark kernel (kernel compiled to be run
>>>>> on Quark) will be ever used on the rest of (modern) x86 since it's
>>>>> 486+ architecture (kernel has specific option for it, 586TSC).
>>>>
>>>> + it's UP only!
>>>>
>>>>> So, we might just be dependent or chosen by Quark.
>>>>
>>>
>>> Still though the current ia32 kernel runs on Quark and all other ia32
>>> systems.
>>
>> How come? Quark has a silicon bug (SMP kernel might oops) and it is
>> not even i586!
>
> sorry if this is a bit long in advance...
>
> You mean a lock prefixed pagefault.
>
> For reference Quark should be considered CONFIG_M586TSC=y (or better)
> i.e. it's 586 ISA with a TSC added on.

So, if it would be CONFIG_M686=y then?
This is default for x86 32-bit kernels.

>
> I've been meaning to do a write up about this, since I've spent some
> time with a debugger doing an analysis of the fault. Basically any
> operation that is lock-prefixed that also page-faults pushes the address
> of the _previous_ instruction (not the instruction that faulted) onto
> the PF# stack. Which sucks.
>
> Obviously then when you IRET you will execute the previous instruction
> again - on return to user-space - as opposed to the instruction you
> faulted on.
>
> So it's nothing to do with SMP per se, except that the lock prefix was
> added to drive the #lock signal on future SMP versions of the part (that
> never happened). We discussed this @ the time Dave Jones did
>
> commit d4e1a0af1d3a88cdfc8c954d3005eb8745ec518d
> Author: Dave Jones <davej@redhat.com>
> Date:   Tue Oct 28 13:57:53 2014 -0400
>
>     x86: Don't enable F00F workaround on Intel Quark processors
>
> ... and we agreed to have a good look at lock prefixed instructions in
> the kernel. On !SMP builds there is (or was at the time anyway) alot of
> LOCK_PREFIX looking like this
>
> #ifdef CONFIG_SMP
> #define LOCK_PREFIX_HERE \
>                 ".pushsection .smp_locks,\"a\"\n"       \
>                 ".balign 4\n"                           \
>                 ".long 671f - .\n" /* offset */         \
>                 ".popsection\n"                         \
>                 "671:"
>
> #define LOCK_PREFIX LOCK_PREFIX_HERE "\n\tlock; "
>
> #else /* ! CONFIG_SMP */
> #define LOCK_PREFIX_HERE ""
> #define LOCK_PREFIX ""
> #endif
>
> Which meant that !SMP was safe. It was probably overkill though because
> kernel code shouldn't PF anyway.

So, would it work if CONFIG_SMP=y ?
(This is default for x86 32-bit kernels)

> !SMP ia32 builds should be fine and we have never actually seen _kernel_
> code die on SMP builds ... presumably (demonstrably) because lock
> prefixed operations in the kernel don't PF#.

Which doesn't guarantee that it will not oops at some circumstances.

>>> It would be a pity/shame to make this feature dependent on
>>> compiling a Quark specific kernel, after all its only a header on a
>>> capsule as opposed to a large hardware-level architectural divergence.
>>>
>>
>>> I'd still like us to try for a low-fat hook that would a big fat ia32
>>> kernel just work without having to force a user compile up a
>>> Quark-specific kernel.
>>
>> Can you elaborate how to run i686 kernel (which is default for x86
>> 32-bit AFAIK) on Quark?
>
> A kernel compiled like this
>
> make menuconfig ARCH=i386

I hope you care that it is equivalent to

make menuconfig ARCH=i686

> make bzImage -j 8
>
> will run just fine on Quark x1000 I do it regularly. CPUID ought to (and
> does) inform the runtime kernel of what to do re: MSRs etc.
>
> We won't execute xmm/mmx, we won't touch 686 specific MSRs etc, etc.

It is i*6*86 code still.

So, summarize, you state that
1. CONFIG_SMP=y and
2. CONFIG_M686=y and
3. Kernel works on Quark

Is it correct?

If 1 or 2 is not correct it means we can *not* use the same kernel on
Quark. Unfortunately.

-- 
With Best Regards,
Andy Shevchenko

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

[Bryan O'Donoghue]

On 28/02/17 17:18, Andy Shevchenko wrote:
> On Tue, Feb 28, 2017 at 6:52 PM, Bryan O'Donoghue
>> A kernel compiled like this
>>
>> make menuconfig ARCH=i386
> 
> I hope you care that it is equivalent to
> 
> make menuconfig ARCH=i686
> 
>> make bzImage -j 8
>>
>> will run just fine on Quark x1000 I do it regularly. CPUID ought to (and
>> does) inform the runtime kernel of what to do re: MSRs etc.
>>
>> We won't execute xmm/mmx, we won't touch 686 specific MSRs etc, etc.
> 
> It is i*6*86 code still.
> 
> So, summarize, you state that
> 1. CONFIG_SMP=y and
> 2. CONFIG_M686=y and
> 3. Kernel works on Quark
> 
> Is it correct?

Logically yes. It's a very long time since I looked in detail. No harm
in checking it out though.

I'll compile up the above kernel this evening (GMT) and verify.


-- 
bod

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

[Bryan O'Donoghue]

On 28/02/17 17:18, Andy Shevchenko wrote:
> On Tue, Feb 28, 2017 at 6:52 PM, Bryan O'Donoghue
>> A kernel compiled like this
>>
>> make menuconfig ARCH=i386
> 
> I hope you care that it is equivalent to
> 
> make menuconfig ARCH=i686
> 
>> make bzImage -j 8
>>
>> will run just fine on Quark x1000 I do it regularly. CPUID ought to (and
>> does) inform the runtime kernel of what to do re: MSRs etc.
>>
>> We won't execute xmm/mmx, we won't touch 686 specific MSRs etc, etc.
> 
> It is i*6*86 code still.
> 
> So, summarize, you state that
> 1. CONFIG_SMP=y and
> 2. CONFIG_M686=y and
> 3. Kernel works on Quark
> 
> Is it correct?

Logically yes. It's a very long time since I looked in detail. No harm
in checking it out though.

I'll compile up the above kernel this evening (GMT) and verify.


-- 
bod

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

[Bryan O'Donoghue]

On 28/02/17 17:42, Bryan O'Donoghue wrote:
> On 28/02/17 17:18, Andy Shevchenko wrote:
>> On Tue, Feb 28, 2017 at 6:52 PM, Bryan O'Donoghue
>>> A kernel compiled like this
>>>
>>> make menuconfig ARCH=i386
>>
>> I hope you care that it is equivalent to
>>
>> make menuconfig ARCH=i686
>>
>>> make bzImage -j 8
>>>
>>> will run just fine on Quark x1000 I do it regularly. CPUID ought to (and
>>> does) inform the runtime kernel of what to do re: MSRs etc.
>>>
>>> We won't execute xmm/mmx, we won't touch 686 specific MSRs etc, etc.
>>
>> It is i*6*86 code still.
>>
>> So, summarize, you state that
>> 1. CONFIG_SMP=y and
>> 2. CONFIG_M686=y and
>> 3. Kernel works on Quark
>>
>> Is it correct?
>
> Logically yes. It's a very long time since I looked in detail. No harm
> in checking it out though.
>
> I'll compile up the above kernel this evening (GMT) and verify.
>
>

CONFIG_SMP=y - no difference - like I say it's PF# on lock prefix
                instructions, not SMP=y that's the problem here
CONFIG_M686=y (doesnt' boot)
CONFIG_M586TSC=y does boot

So yes M686 is not bootable on this part. My point to you about having a 
custom kernel though still stands, you shouldn't have to compile a quark 
specific kernel - just a 586TSC kernel with Quark support.

For example CentOS is bootable on Quark.


---
bod

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

[Bryan O'Donoghue]

On 28/02/17 17:42, Bryan O'Donoghue wrote:
> On 28/02/17 17:18, Andy Shevchenko wrote:
>> On Tue, Feb 28, 2017 at 6:52 PM, Bryan O'Donoghue
>>> A kernel compiled like this
>>>
>>> make menuconfig ARCH=i386
>>
>> I hope you care that it is equivalent to
>>
>> make menuconfig ARCH=i686
>>
>>> make bzImage -j 8
>>>
>>> will run just fine on Quark x1000 I do it regularly. CPUID ought to (and
>>> does) inform the runtime kernel of what to do re: MSRs etc.
>>>
>>> We won't execute xmm/mmx, we won't touch 686 specific MSRs etc, etc.
>>
>> It is i*6*86 code still.
>>
>> So, summarize, you state that
>> 1. CONFIG_SMP=y and
>> 2. CONFIG_M686=y and
>> 3. Kernel works on Quark
>>
>> Is it correct?
>
> Logically yes. It's a very long time since I looked in detail. No harm
> in checking it out though.
>
> I'll compile up the above kernel this evening (GMT) and verify.
>
>

CONFIG_SMP=y - no difference - like I say it's PF# on lock prefix
                instructions, not SMP=y that's the problem here
CONFIG_M686=y (doesnt' boot)
CONFIG_M586TSC=y does boot

So yes M686 is not bootable on this part. My point to you about having a 
custom kernel though still stands, you shouldn't have to compile a quark 
specific kernel - just a 586TSC kernel with Quark support.

For example CentOS is bootable on Quark.


---
bod

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

[Andy Shevchenko]

On Wed, Mar 1, 2017 at 4:02 PM, Bryan O'Donoghue
<pure.logic@nexus-software.ie> wrote:

>>> So, summarize, you state that
>>> 1. CONFIG_SMP=y and
>>> 2. CONFIG_M686=y and
>>> 3. Kernel works on Quark
>>>
>>> Is it correct?

>> Logically yes. It's a very long time since I looked in detail. No harm
>> in checking it out though.
>>
>> I'll compile up the above kernel this evening (GMT) and verify.

> CONFIG_SMP=y - no difference - like I say it's PF# on lock prefix
>                instructions, not SMP=y that's the problem here
> CONFIG_M686=y (doesnt' boot)
> CONFIG_M586TSC=y does boot

> So yes M686 is not bootable on this part. My point to you about having a
> custom kernel though still stands, you shouldn't have to compile a quark
> specific kernel - just a 586TSC kernel with Quark support.

... which is not default. That's my point.

> For example CentOS is bootable on Quark.

Because someone there took care of it. (I think being i586 compatible
binaries as well)

-- 
With Best Regards,
Andy Shevchenko

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

[Jan Kiszka]

On 2017-02-20 02: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);
> +}
> 
> 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.

Done now in my newer patches. Some refactoring in the core is still
required, but the Quark logic will be active only on x86 and fully
executed only on Quark CPUs.

> 
> 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.

No idea what went wrong back then, but I was able to reflash a Gen2
multiple times these days with the original 1.1.0 and 1.0.4 capsules
Intel ships for that board - without removing the CSH. IOW, we can
finally use capsule support on the Galileo.

Patches to come soon.

Jan

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

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

[Jan Kiszka]

(restarting this topic, new patches are in the make)

On 2017-02-17 02:30, Bryan O'Donoghue wrote:
> 
> 
> 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

Done.

> 
>> +
>>  /**
>>   * 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..

Nope: efi_capsule_setup_info is called whenever the user wrote some bits
to the device, not necessarily enough to evaluate all the headers. We
need to return 0 here (and not set header_obtained) until we find enough
data to declare the capsule valid or broken.

> 
> 
>> +
>> +        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.

I've written some cleanup patches for this module and refrain from
adding my own mess.

> 
>> +
>> +        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.
> 

Done.

Thanks,
Jan

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

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

[Jan Kiszka]

(restarting this topic, new patches are in the make)

On 2017-02-17 02:30, Bryan O'Donoghue wrote:
> 
> 
> 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-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
>> ---
>>  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

Done.

> 
>> +
>>  /**
>>   * 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..

Nope: efi_capsule_setup_info is called whenever the user wrote some bits
to the device, not necessarily enough to evaluate all the headers. We
need to return 0 here (and not set header_obtained) until we find enough
data to declare the capsule valid or broken.

> 
> 
>> +
>> +        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.

I've written some cleanup patches for this module and refrain from
adding my own mess.

> 
>> +
>> +        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.
> 

Done.

Thanks,
Jan

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