[PATCH 0/8] efi: add support for non-standard capsule headers

[PATCH 0/8] efi: add support for non-standard capsule headers

[Ard Biesheuvel]

This is a followup to Jan's series [0] to add support for the non-standard
and awkward capsule header layout that is used by the Quark platform.

While we would prefer to adhere to the standard rigorously, the reality
(and common practice) in Linux is that we sometimes have to deal with
quirks. So while Jan's aim is to get Quark to work, the reason for my
involvement is to try and accommodate this in a flexible way without
putting any handling specific to this quirk in the common code.

Patches #1 to #4 are minor preparatory cleanups.

Patch #5 reworks the capsule loader code to use a cached copy of the
header rather than load it from memory multiple times (which may involve
a kmap/kunmap sequence if it is in highmem). This also allows some mangling
to be performed by quirks code.

Patch #6 splits up efi_capsule_setup_info() into a primary part called
__efi_capsule_setup_info(), and a __weak wrapper under the original name,
allowing it to be overridden externally.

Patch #7 changes the array of struct page pointers maintained by the capsule
loader into an array of physical addresses. This allows special versions of
efi_capsule_setup_info() to mangle the contents of the capsule (and skip
headers by moving pointers around) without putting any intimate knowledge
of the quirks handling into the common code.

Patch #8 is Jan's original patch to add the Quark specific quirk to arch/x86,
but reworked to take advantage of the facilities added in #6 and #7.

This has been tested by Jan on Quark, but this needs testing on other
platforms to ensure that the common code still works as expected on
conforming firmware implementations.

Ard Biesheuvel (3):
  efi/capsule-loader: use cached copy of capsule header
  efi/capsule-loader: indirect calls to efi_capsule_setup_info via weak
    alias
  efi/capsule-loader: use page addresses rather than struct page
    pointers

Jan Kiszka (5):
  efi/capsule: Fix return code on failing kmap/vmap
  efi/capsule: Remove pr_debug on ENOMEM or EFAULT
  efi/capsule: Clean up pr_err/info messages
  efi/capsule: Adjust return type of efi_capsule_setup_info
  efi/capsule: Add support for Quark security header

 arch/x86/platform/efi/quirks.c        | 112 ++++++++++++++++++++
 drivers/firmware/efi/Kconfig          |   9 ++
 drivers/firmware/efi/capsule-loader.c | 111 ++++++++-----------
 drivers/firmware/efi/capsule.c        |   7 +-
 include/linux/efi.h                   |  14 ++-
 5 files changed, 184 insertions(+), 69 deletions(-)

-- 
2.9.3

[PATCH 1/8] efi/capsule: Fix return code on failing kmap/vmap

[Ard Biesheuvel]

From: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>

If kmap or vmap fail, it means we ran out of memory. There are no
user-provided addressed involved that would justify EFAULT.

Signed-off-by: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/capsule-loader.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
index 9ae6c116c474..91e91f7a8807 100644
--- a/drivers/firmware/efi/capsule-loader.c
+++ b/drivers/firmware/efi/capsule-loader.c
@@ -113,7 +113,7 @@ static ssize_t efi_capsule_submit_update(struct capsule_info *cap_info)
 			VM_MAP, PAGE_KERNEL);
 	if (!cap_hdr_temp) {
 		pr_debug("%s: vmap() failed\n", __func__);
-		return -EFAULT;
+		return -ENOMEM;
 	}
 
 	ret = efi_capsule_update(cap_hdr_temp, cap_info->pages);
@@ -185,7 +185,7 @@ static ssize_t efi_capsule_write(struct file *file, const char __user *buff,
 	kbuff = kmap(page);
 	if (!kbuff) {
 		pr_debug("%s: kmap() failed\n", __func__);
-		ret = -EFAULT;
+		ret = -ENOMEM;
 		goto failed;
 	}
 	kbuff += PAGE_SIZE - cap_info->page_bytes_remain;
-- 
2.9.3

Re: [PATCH 1/8] efi/capsule: Fix return code on failing kmap/vmap

[Matt Fleming]

On Wed, 05 Apr, at 10:23:10AM, Ard Biesheuvel wrote:
> From: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
> 
> If kmap or vmap fail, it means we ran out of memory. There are no
> user-provided addressed involved that would justify EFAULT.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
> Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/firmware/efi/capsule-loader.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>

[PATCH 2/8] efi/capsule: Remove pr_debug on ENOMEM or EFAULT

[Ard Biesheuvel]

From: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>

Both cases are not worth a debug log message - the error code is telling
enough.

Signed-off-by: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/capsule-loader.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
index 91e91f7a8807..7b57dda2417d 100644
--- a/drivers/firmware/efi/capsule-loader.c
+++ b/drivers/firmware/efi/capsule-loader.c
@@ -88,10 +88,8 @@ static ssize_t efi_capsule_setup_info(struct capsule_info *cap_info,
 	temp_page = krealloc(cap_info->pages,
 			     pages_needed * sizeof(void *),
 			     GFP_KERNEL | __GFP_ZERO);
-	if (!temp_page) {
-		pr_debug("%s: krealloc() failed\n", __func__);
+	if (!temp_page)
 		return -ENOMEM;
-	}
 
 	cap_info->pages = temp_page;
 	cap_info->header_obtained = true;
@@ -111,10 +109,8 @@ static ssize_t efi_capsule_submit_update(struct capsule_info *cap_info)
 
 	cap_hdr_temp = vmap(cap_info->pages, cap_info->index,
 			VM_MAP, PAGE_KERNEL);
-	if (!cap_hdr_temp) {
-		pr_debug("%s: vmap() failed\n", __func__);
+	if (!cap_hdr_temp)
 		return -ENOMEM;
-	}
 
 	ret = efi_capsule_update(cap_hdr_temp, cap_info->pages);
 	vunmap(cap_hdr_temp);
@@ -171,7 +167,6 @@ static ssize_t efi_capsule_write(struct file *file, const char __user *buff,
 	if (!cap_info->page_bytes_remain) {
 		page = alloc_page(GFP_KERNEL);
 		if (!page) {
-			pr_debug("%s: alloc_page() failed\n", __func__);
 			ret = -ENOMEM;
 			goto failed;
 		}
@@ -184,7 +179,6 @@ static ssize_t efi_capsule_write(struct file *file, const char __user *buff,
 
 	kbuff = kmap(page);
 	if (!kbuff) {
-		pr_debug("%s: kmap() failed\n", __func__);
 		ret = -ENOMEM;
 		goto failed;
 	}
@@ -193,7 +187,6 @@ static ssize_t efi_capsule_write(struct file *file, const char __user *buff,
 	/* Copy capsule binary data from user space to kernel space buffer */
 	write_byte = min_t(size_t, count, cap_info->page_bytes_remain);
 	if (copy_from_user(kbuff, buff, write_byte)) {
-		pr_debug("%s: copy_from_user() failed\n", __func__);
 		ret = -EFAULT;
 		goto fail_unmap;
 	}
-- 
2.9.3

Re: [PATCH 2/8] efi/capsule: Remove pr_debug on ENOMEM or EFAULT

[Matt Fleming]

On Wed, 05 Apr, at 10:23:11AM, Ard Biesheuvel wrote:
> From: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
> 
> Both cases are not worth a debug log message - the error code is telling
> enough.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
> Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/firmware/efi/capsule-loader.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)

Reviewed-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>

[PATCH 3/8] efi/capsule: Clean up pr_err/info messages

[Ard Biesheuvel]

From: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>

Avoid __func__, improve the information provided by some of the
messages.

Signed-off-by: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/capsule-loader.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
index 7b57dda2417d..3fb91e1597a9 100644
--- a/drivers/firmware/efi/capsule-loader.c
+++ b/drivers/firmware/efi/capsule-loader.c
@@ -70,7 +70,7 @@ static ssize_t efi_capsule_setup_info(struct capsule_info *cap_info,
 	pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >> PAGE_SHIFT;
 
 	if (pages_needed == 0) {
-		pr_err("%s: pages count invalid\n", __func__);
+		pr_err("invalid capsule size");
 		return -EINVAL;
 	}
 
@@ -79,8 +79,7 @@ static ssize_t efi_capsule_setup_info(struct capsule_info *cap_info,
 				    cap_hdr->imagesize,
 				    &cap_info->reset_type);
 	if (ret) {
-		pr_err("%s: efi_capsule_supported() failed\n",
-		       __func__);
+		pr_err("capsule not supported\n");
 		return ret;
 	}
 
@@ -115,14 +114,14 @@ static ssize_t efi_capsule_submit_update(struct capsule_info *cap_info)
 	ret = efi_capsule_update(cap_hdr_temp, cap_info->pages);
 	vunmap(cap_hdr_temp);
 	if (ret) {
-		pr_err("%s: efi_capsule_update() failed\n", __func__);
+		pr_err("capsule update failed\n");
 		return ret;
 	}
 
 	/* Indicate capsule binary uploading is done */
 	cap_info->index = NO_FURTHER_WRITE_ACTION;
-	pr_info("%s: Successfully upload capsule file with reboot type '%s'\n",
-		__func__, !cap_info->reset_type ? "RESET_COLD" :
+	pr_info("Successfully upload capsule file with reboot type '%s'\n",
+		!cap_info->reset_type ? "RESET_COLD" :
 		cap_info->reset_type == 1 ? "RESET_WARM" :
 		"RESET_SHUTDOWN");
 	return 0;
@@ -207,8 +206,7 @@ static ssize_t efi_capsule_write(struct file *file, const char __user *buff,
 	if (cap_info->header_obtained &&
 	    cap_info->count >= cap_info->total_size) {
 		if (cap_info->count > cap_info->total_size) {
-			pr_err("%s: upload size exceeded header defined size\n",
-			       __func__);
+			pr_err("capsule upload size exceeded header defined size\n");
 			ret = -EINVAL;
 			goto failed;
 		}
@@ -242,7 +240,7 @@ static int efi_capsule_flush(struct file *file, fl_owner_t id)
 	struct capsule_info *cap_info = file->private_data;
 
 	if (cap_info->index > 0) {
-		pr_err("%s: capsule upload not complete\n", __func__);
+		pr_err("capsule upload not complete\n");
 		efi_free_all_buff_pages(cap_info);
 		ret = -ECANCELED;
 	}
@@ -321,8 +319,7 @@ static int __init efi_capsule_loader_init(void)
 
 	ret = misc_register(&efi_capsule_misc);
 	if (ret)
-		pr_err("%s: Failed to register misc char file note\n",
-		       __func__);
+		pr_err("Unable to register capsule loader device\n");
 
 	return ret;
 }
-- 
2.9.3

Re: [PATCH 3/8] efi/capsule: Clean up pr_err/info messages

[Matt Fleming]

On Wed, 05 Apr, at 10:23:12AM, Ard Biesheuvel wrote:
> From: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
> 
> Avoid __func__, improve the information provided by some of the
> messages.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
> Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/firmware/efi/capsule-loader.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)

Reviewed-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>

[PATCH 4/8] efi/capsule: Adjust return type of efi_capsule_setup_info

[Ard Biesheuvel]

From: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>

We actually expect int at the caller and never return any size
information.

Signed-off-by: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/capsule-loader.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
index 3fb91e1597a9..37d3f6ec2d28 100644
--- a/drivers/firmware/efi/capsule-loader.c
+++ b/drivers/firmware/efi/capsule-loader.c
@@ -53,8 +53,8 @@ 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,
-				      void *kbuff, size_t hdr_bytes)
+static int efi_capsule_setup_info(struct capsule_info *cap_info,
+				  void *kbuff, size_t hdr_bytes)
 {
 	efi_capsule_header_t *cap_hdr;
 	size_t pages_needed;
-- 
2.9.3

Re: [PATCH 4/8] efi/capsule: Adjust return type of efi_capsule_setup_info

[Matt Fleming]

On Wed, 05 Apr, at 10:23:13AM, Ard Biesheuvel wrote:
> From: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
> 
> We actually expect int at the caller and never return any size
> information.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
> Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/firmware/efi/capsule-loader.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>

[PATCH 5/8] efi/capsule-loader: use cached copy of capsule header

[Ard Biesheuvel]

Instead of kmapping the capsule data twice, copy the capsule header
into the capsule info struct we keep locally. This is an improvement
by itself, but will also enable handling of non-standard header formats
more easily.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/capsule-loader.c | 41 ++++++++------------
 1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
index 37d3f6ec2d28..5b012a467d7d 100644
--- a/drivers/firmware/efi/capsule-loader.c
+++ b/drivers/firmware/efi/capsule-loader.c
@@ -21,13 +21,13 @@
 #define NO_FURTHER_WRITE_ACTION -1
 
 struct capsule_info {
-	bool		header_obtained;
-	int		reset_type;
-	long		index;
-	size_t		count;
-	size_t		total_size;
-	struct page	**pages;
-	size_t		page_bytes_remain;
+	efi_capsule_header_t	header;
+	int			reset_type;
+	long			index;
+	size_t			count;
+	size_t			total_size;
+	struct page		**pages;
+	size_t			page_bytes_remain;
 };
 
 /**
@@ -56,7 +56,6 @@ static void efi_free_all_buff_pages(struct capsule_info *cap_info)
 static int efi_capsule_setup_info(struct capsule_info *cap_info,
 				  void *kbuff, size_t hdr_bytes)
 {
-	efi_capsule_header_t *cap_hdr;
 	size_t pages_needed;
 	int ret;
 	void *temp_page;
@@ -66,8 +65,9 @@ static int efi_capsule_setup_info(struct capsule_info *cap_info,
 		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;
+	kbuff -= cap_info->count;
+	memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
+	pages_needed = ALIGN(cap_info->header.imagesize, PAGE_SIZE) / PAGE_SIZE;
 
 	if (pages_needed == 0) {
 		pr_err("invalid capsule size");
@@ -75,15 +75,16 @@ static int 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,
+	ret = efi_capsule_supported(cap_info->header.guid,
+				    cap_info->header.flags,
+				    cap_info->header.imagesize,
 				    &cap_info->reset_type);
 	if (ret) {
 		pr_err("capsule not supported\n");
 		return ret;
 	}
 
-	cap_info->total_size = cap_hdr->imagesize;
+	cap_info->total_size = cap_info->header.imagesize;
 	temp_page = krealloc(cap_info->pages,
 			     pages_needed * sizeof(void *),
 			     GFP_KERNEL | __GFP_ZERO);
@@ -91,7 +92,6 @@ static int efi_capsule_setup_info(struct capsule_info *cap_info,
 		return -ENOMEM;
 
 	cap_info->pages = temp_page;
-	cap_info->header_obtained = true;
 
 	return 0;
 }
@@ -104,15 +104,8 @@ static int efi_capsule_setup_info(struct capsule_info *cap_info,
 static ssize_t efi_capsule_submit_update(struct capsule_info *cap_info)
 {
 	int ret;
-	void *cap_hdr_temp;
 
-	cap_hdr_temp = vmap(cap_info->pages, cap_info->index,
-			VM_MAP, PAGE_KERNEL);
-	if (!cap_hdr_temp)
-		return -ENOMEM;
-
-	ret = efi_capsule_update(cap_hdr_temp, cap_info->pages);
-	vunmap(cap_hdr_temp);
+	ret = efi_capsule_update(&cap_info->header, cap_info->pages);
 	if (ret) {
 		pr_err("capsule update failed\n");
 		return ret;
@@ -192,7 +185,7 @@ static ssize_t efi_capsule_write(struct file *file, const char __user *buff,
 	cap_info->page_bytes_remain -= write_byte;
 
 	/* Setup capsule binary info structure */
-	if (!cap_info->header_obtained) {
+	if (cap_info->header.headersize == 0) {
 		ret = efi_capsule_setup_info(cap_info, kbuff,
 					     cap_info->count + write_byte);
 		if (ret)
@@ -203,7 +196,7 @@ static ssize_t efi_capsule_write(struct file *file, const char __user *buff,
 	kunmap(page);
 
 	/* Submit the full binary to efi_capsule_update() API */
-	if (cap_info->header_obtained &&
+	if (cap_info->header.headersize > 0 &&
 	    cap_info->count >= cap_info->total_size) {
 		if (cap_info->count > cap_info->total_size) {
 			pr_err("capsule upload size exceeded header defined size\n");
-- 
2.9.3

Re: [PATCH 5/8] efi/capsule-loader: use cached copy of capsule header

[Matt Fleming]

On Wed, 05 Apr, at 10:23:14AM, Ard Biesheuvel wrote:
> Instead of kmapping the capsule data twice, copy the capsule header
> into the capsule info struct we keep locally. This is an improvement
> by itself, but will also enable handling of non-standard header formats
> more easily.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/firmware/efi/capsule-loader.c | 41 ++++++++------------
>  1 file changed, 17 insertions(+), 24 deletions(-)

Reviewed-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>

[PATCH 6/8] efi/capsule-loader: indirect calls to efi_capsule_setup_info via weak alias

[Ard Biesheuvel]

To allow platform specific code to hook into the capsule loading
routines, indirect calls to efi_capsule_setup_info() via a weak alias
of __efi_capsule_setup_info(), allowing platforms to redefine the former
but still use the latter.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/capsule-loader.c | 52 +++++++++-----------
 include/linux/efi.h                   | 12 +++++
 2 files changed, 35 insertions(+), 29 deletions(-)

diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
index 5b012a467d7d..d68a1ecebbf3 100644
--- a/drivers/firmware/efi/capsule-loader.c
+++ b/drivers/firmware/efi/capsule-loader.c
@@ -20,16 +20,6 @@
 
 #define NO_FURTHER_WRITE_ACTION -1
 
-struct capsule_info {
-	efi_capsule_header_t	header;
-	int			reset_type;
-	long			index;
-	size_t			count;
-	size_t			total_size;
-	struct page		**pages;
-	size_t			page_bytes_remain;
-};
-
 /**
  * efi_free_all_buff_pages - free all previous allocated buffer pages
  * @cap_info: pointer to current instance of capsule_info structure
@@ -46,28 +36,13 @@ static void efi_free_all_buff_pages(struct capsule_info *cap_info)
 	cap_info->index = NO_FURTHER_WRITE_ACTION;
 }
 
-/**
- * efi_capsule_setup_info - obtain the efi capsule header in the binary and
- *			    setup capsule_info structure
- * @cap_info: pointer to current instance of capsule_info structure
- * @kbuff: a mapped first page buffer pointer
- * @hdr_bytes: the total received number of bytes for efi header
- **/
-static int efi_capsule_setup_info(struct capsule_info *cap_info,
-				  void *kbuff, size_t hdr_bytes)
+int __efi_capsule_setup_info(struct capsule_info *cap_info)
 {
 	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))
-		return 0;
-
-	/* Reset back to the correct offset of header */
-	kbuff -= cap_info->count;
-	memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
-	pages_needed = ALIGN(cap_info->header.imagesize, PAGE_SIZE) / PAGE_SIZE;
+	pages_needed = ALIGN(cap_info->total_size, PAGE_SIZE) / PAGE_SIZE;
 
 	if (pages_needed == 0) {
 		pr_err("invalid capsule size");
@@ -84,7 +59,6 @@ static int efi_capsule_setup_info(struct capsule_info *cap_info,
 		return ret;
 	}
 
-	cap_info->total_size = cap_info->header.imagesize;
 	temp_page = krealloc(cap_info->pages,
 			     pages_needed * sizeof(void *),
 			     GFP_KERNEL | __GFP_ZERO);
@@ -97,6 +71,26 @@ static int efi_capsule_setup_info(struct capsule_info *cap_info,
 }
 
 /**
+ * efi_capsule_setup_info - obtain the efi capsule header in the binary and
+ *			    setup capsule_info structure
+ * @cap_info: pointer to current instance of capsule_info structure
+ * @kbuff: a mapped first page buffer pointer
+ * @hdr_bytes: the total received number of bytes for efi header
+ **/
+int __weak efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
+				  size_t hdr_bytes)
+{
+	/* Only process data block that is larger than efi header size */
+	if (hdr_bytes < sizeof(efi_capsule_header_t))
+		return 0;
+
+	memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
+	cap_info->total_size = cap_info->header.imagesize;
+
+	return __efi_capsule_setup_info(cap_info);
+}
+
+/**
  * efi_capsule_submit_update - invoke the efi_capsule_update API once binary
  *			       upload done
  * @cap_info: pointer to current instance of capsule_info structure
@@ -186,7 +180,7 @@ static ssize_t efi_capsule_write(struct file *file, const char __user *buff,
 
 	/* Setup capsule binary info structure */
 	if (cap_info->header.headersize == 0) {
-		ret = efi_capsule_setup_info(cap_info, kbuff,
+		ret = efi_capsule_setup_info(cap_info, kbuff - cap_info->count,
 					     cap_info->count + write_byte);
 		if (ret)
 			goto fail_unmap;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index ec36f42a2add..a7379a2b5680 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -137,6 +137,18 @@ struct efi_boot_memmap {
 #define EFI_CAPSULE_POPULATE_SYSTEM_TABLE	0x00020000
 #define EFI_CAPSULE_INITIATE_RESET		0x00040000
 
+struct capsule_info {
+	efi_capsule_header_t	header;
+	int			reset_type;
+	long			index;
+	size_t			count;
+	size_t			total_size;
+	struct page		**pages;
+	size_t			page_bytes_remain;
+};
+
+int __efi_capsule_setup_info(struct capsule_info *cap_info);
+
 /*
  * Allocation types for calls to boottime->allocate_pages.
  */
-- 
2.9.3

Re: [PATCH 6/8] efi/capsule-loader: indirect calls to efi_capsule_setup_info via weak alias

[Matt Fleming]

On Wed, 05 Apr, at 10:23:15AM, Ard Biesheuvel wrote:
> To allow platform specific code to hook into the capsule loading
> routines, indirect calls to efi_capsule_setup_info() via a weak alias
> of __efi_capsule_setup_info(), allowing platforms to redefine the former
> but still use the latter.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/firmware/efi/capsule-loader.c | 52 +++++++++-----------
>  include/linux/efi.h                   | 12 +++++
>  2 files changed, 35 insertions(+), 29 deletions(-)

[...]

> @@ -97,6 +71,26 @@ static int efi_capsule_setup_info(struct capsule_info *cap_info,
>  }
>  
>  /**
> + * efi_capsule_setup_info - obtain the efi capsule header in the binary and
> + *			    setup capsule_info structure
> + * @cap_info: pointer to current instance of capsule_info structure
> + * @kbuff: a mapped first page buffer pointer
> + * @hdr_bytes: the total received number of bytes for efi header
> + **/
> +int __weak efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
> +				  size_t hdr_bytes)
> +{
> +	/* Only process data block that is larger than efi header size */
> +	if (hdr_bytes < sizeof(efi_capsule_header_t))
> +		return 0;
> +
> +	memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
> +	cap_info->total_size = cap_info->header.imagesize;
> +
> +	return __efi_capsule_setup_info(cap_info);
> +}

It would be good if you provided a little bit of blurb in the function
comment describing why someone might want to override this __weak
function.

Perhaps just something like,

 "Platforms with non-standard capsule update mechanisms can override
  this __weak function so they can perform any required capsule
  image munging. See quark_quirk_function() for an example."

[PATCH 7/8] efi/capsule-loader: use page addresses rather than struct page pointers

[Ard Biesheuvel]

To give some leeway to code that handles non-standard capsule headers,
let's keep an array of page addresses rather than struct page pointers.

This gives special implementations of efi_capsule_setup_info() the
opportunity to mangle the payload a bit before it is presented to the
firmware, without putting any knowledge of the nature of such quirks
into the generic code.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/capsule-loader.c | 12 ++++++++----
 drivers/firmware/efi/capsule.c        |  7 ++++---
 include/linux/efi.h                   |  4 ++--
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
index d68a1ecebbf3..22b2bb73176c 100644
--- a/drivers/firmware/efi/capsule-loader.c
+++ b/drivers/firmware/efi/capsule-loader.c
@@ -20,6 +20,10 @@
 
 #define NO_FURTHER_WRITE_ACTION -1
 
+#ifndef phys_to_page
+#define phys_to_page(x)		virt_to_page((unsigned long)__va(x))
+#endif
+
 /**
  * efi_free_all_buff_pages - free all previous allocated buffer pages
  * @cap_info: pointer to current instance of capsule_info structure
@@ -31,7 +35,7 @@
 static void efi_free_all_buff_pages(struct capsule_info *cap_info)
 {
 	while (cap_info->index > 0)
-		__free_page(cap_info->pages[--cap_info->index]);
+		__free_page(phys_to_page(cap_info->pages[--cap_info->index]));
 
 	cap_info->index = NO_FURTHER_WRITE_ACTION;
 }
@@ -157,12 +161,12 @@ static ssize_t efi_capsule_write(struct file *file, const char __user *buff,
 			goto failed;
 		}
 
-		cap_info->pages[cap_info->index++] = page;
+		cap_info->pages[cap_info->index++] = page_to_phys(page);
 		cap_info->page_bytes_remain = PAGE_SIZE;
+	} else {
+		page = phys_to_page(cap_info->pages[cap_info->index - 1]);
 	}
 
-	page = cap_info->pages[cap_info->index - 1];
-
 	kbuff = kmap(page);
 	if (!kbuff) {
 		ret = -ENOMEM;
diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
index 6eedff45e6d7..57f85256feb2 100644
--- a/drivers/firmware/efi/capsule.c
+++ b/drivers/firmware/efi/capsule.c
@@ -214,7 +214,7 @@ 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, phys_addr_t *pages)
 {
 	u32 imagesize = capsule->imagesize;
 	efi_guid_t guid = capsule->guid;
@@ -253,10 +253,11 @@ 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);
+			u64 sz = min_t(u64, imagesize,
+				       PAGE_SIZE - (u64)*pages % PAGE_SIZE);
 
 			sglist[j].length = sz;
-			sglist[j].data = page_to_phys(*pages++);
+			sglist[j].data = *pages++;
 
 			imagesize -= sz;
 			count--;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index a7379a2b5680..8269bcb8ccf7 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -143,7 +143,7 @@ struct capsule_info {
 	long			index;
 	size_t			count;
 	size_t			total_size;
-	struct page		**pages;
+	phys_addr_t		*pages;
 	size_t			page_bytes_remain;
 };
 
@@ -1415,7 +1415,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);
+			      phys_addr_t *pages);
 
 #ifdef CONFIG_EFI_RUNTIME_MAP
 int efi_runtime_map_init(struct kobject *);
-- 
2.9.3

Re: [PATCH 7/8] efi/capsule-loader: use page addresses rather than struct page pointers

[Matt Fleming]

On Wed, 05 Apr, at 10:23:16AM, Ard Biesheuvel wrote:
> To give some leeway to code that handles non-standard capsule headers,
> let's keep an array of page addresses rather than struct page pointers.
> 
> This gives special implementations of efi_capsule_setup_info() the
> opportunity to mangle the payload a bit before it is presented to the
> firmware, without putting any knowledge of the nature of such quirks
> into the generic code.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/firmware/efi/capsule-loader.c | 12 ++++++++----
>  drivers/firmware/efi/capsule.c        |  7 ++++---
>  include/linux/efi.h                   |  4 ++--
>  3 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
> index d68a1ecebbf3..22b2bb73176c 100644
> --- a/drivers/firmware/efi/capsule-loader.c
> +++ b/drivers/firmware/efi/capsule-loader.c
> @@ -20,6 +20,10 @@
>  
>  #define NO_FURTHER_WRITE_ACTION -1
>  
> +#ifndef phys_to_page
> +#define phys_to_page(x)		virt_to_page((unsigned long)__va(x))
> +#endif

Is this going to work with highmem pages, which presumably, is a
possibility for the 32-bit Quark?

Re: [PATCH 7/8] efi/capsule-loader: use page addresses rather than struct page pointers

[Ard Biesheuvel]

On 18 April 2017 at 13:56, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> On Wed, 05 Apr, at 10:23:16AM, Ard Biesheuvel wrote:
>> To give some leeway to code that handles non-standard capsule headers,
>> let's keep an array of page addresses rather than struct page pointers.
>>
>> This gives special implementations of efi_capsule_setup_info() the
>> opportunity to mangle the payload a bit before it is presented to the
>> firmware, without putting any knowledge of the nature of such quirks
>> into the generic code.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  drivers/firmware/efi/capsule-loader.c | 12 ++++++++----
>>  drivers/firmware/efi/capsule.c        |  7 ++++---
>>  include/linux/efi.h                   |  4 ++--
>>  3 files changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
>> index d68a1ecebbf3..22b2bb73176c 100644
>> --- a/drivers/firmware/efi/capsule-loader.c
>> +++ b/drivers/firmware/efi/capsule-loader.c
>> @@ -20,6 +20,10 @@
>>
>>  #define NO_FURTHER_WRITE_ACTION -1
>>
>> +#ifndef phys_to_page
>> +#define phys_to_page(x)              virt_to_page((unsigned long)__va(x))
>> +#endif
>
> Is this going to work with highmem pages, which presumably, is a
> possibility for the 32-bit Quark?

Good point. Given that we don't really care about the virtual address
anyway, what is the best way to translate physical addresses to struct
page pointers on x86? i suppose pfn_to_page(pa >> PAGE_SHIFT) always
does the trick?

Re: [PATCH 7/8] efi/capsule-loader: use page addresses rather than struct page pointers

[Matt Fleming]

On Tue, 18 Apr, at 02:01:21PM, Ard Biesheuvel wrote:
> On 18 April 2017 at 13:56, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> > On Wed, 05 Apr, at 10:23:16AM, Ard Biesheuvel wrote:
> >> To give some leeway to code that handles non-standard capsule headers,
> >> let's keep an array of page addresses rather than struct page pointers.
> >>
> >> This gives special implementations of efi_capsule_setup_info() the
> >> opportunity to mangle the payload a bit before it is presented to the
> >> firmware, without putting any knowledge of the nature of such quirks
> >> into the generic code.
> >>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> ---
> >>  drivers/firmware/efi/capsule-loader.c | 12 ++++++++----
> >>  drivers/firmware/efi/capsule.c        |  7 ++++---
> >>  include/linux/efi.h                   |  4 ++--
> >>  3 files changed, 14 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
> >> index d68a1ecebbf3..22b2bb73176c 100644
> >> --- a/drivers/firmware/efi/capsule-loader.c
> >> +++ b/drivers/firmware/efi/capsule-loader.c
> >> @@ -20,6 +20,10 @@
> >>
> >>  #define NO_FURTHER_WRITE_ACTION -1
> >>
> >> +#ifndef phys_to_page
> >> +#define phys_to_page(x)              virt_to_page((unsigned long)__va(x))
> >> +#endif
> >
> > Is this going to work with highmem pages, which presumably, is a
> > possibility for the 32-bit Quark?
> 
> Good point. Given that we don't really care about the virtual address
> anyway, what is the best way to translate physical addresses to struct
> page pointers on x86? i suppose pfn_to_page(pa >> PAGE_SHIFT) always
> does the trick?

Yep, I think that's the way to do it.

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

[Ard Biesheuvel]

From: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>

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 real header to __efi_capsule_setup_info.

To be minimally invasive and maximally safe, the quirk version of
efi_capsule_identify_image is only effective on Quark processors.

Signed-off-by: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
[ardb: refactor using an override of efi_capsule_setup_info()]
Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/x86/platform/efi/quirks.c | 112 ++++++++++++++++++++
 drivers/firmware/efi/Kconfig   |   9 ++
 2 files changed, 121 insertions(+)

diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 30031d5293c4..ee922e1b7008 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -13,12 +13,66 @@
 #include <linux/dmi.h>
 #include <asm/efi.h>
 #include <asm/uv/uv.h>
+#include <asm/cpu_device_id.h>
 
 #define EFI_MIN_RESERVE 5120
 
 #define EFI_DUMMY_GUID \
 	EFI_GUID(0x4424ac57, 0xbe4b, 0x47dd, 0x9e, 0x97, 0xed, 0x50, 0xf0, 0x9f, 0x92, 0xa9)
 
+#define QUARK_CSH_SIGNATURE		0x5f435348	/* _CSH */
+#define QUARK_SECURITY_HEADER_SIZE	0x400
+
+/*
+ * Header prepended to the standard EFI capsule on Quark systems the are based
+ * on Intel firmware BSP.
+ * @csh_signature:	Unique identifier to sanity check signed module
+ * 			presence ("_CSH").
+ * @version:		Current version of CSH used. Should be one for Quark A0.
+ * @modulesize:		Size of the entire module including the module header
+ * 			and payload.
+ * @security_version_number_index: Index of SVN to use for validation of signed
+ * 			module.
+ * @security_version_number: Used to prevent against roll back of modules.
+ * @rsvd_module_id:	Currently unused for Clanton (Quark).
+ * @rsvd_module_vendor:	Vendor Identifier. For Intel products value is
+ * 			0x00008086.
+ * @rsvd_date:		BCD representation of build date as yyyymmdd, where
+ * 			yyyy=4 digit year, mm=1-12, dd=1-31.
+ * @headersize:		Total length of the header including including any
+ * 			padding optionally added by the signing tool.
+ * @hash_algo:		What Hash is used in the module signing.
+ * @cryp_algo:		What Crypto is used in the module signing.
+ * @keysize:		Total length of the key data including including any
+ * 			padding optionally added by the signing tool.
+ * @signaturesize:	Total length of the signature including including any
+ * 			padding optionally added by the signing tool.
+ * @rsvd_next_header:	32-bit pointer to the next Secure Boot Module in the
+ * 			chain, if there is a next header.
+ * @rsvd:		Reserved, padding structure to required size.
+ *
+ * See also QuartSecurityHeader_t in
+ * Quark_EDKII_v1.2.1.1/QuarkPlatformPkg/Include/QuarkBootRom.h
+ * from https://downloadcenter.intel.com/download/23197/Intel-Quark-SoC-X1000-Board-Support-Package-BSP
+ */
+struct 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];
+};
+
 static efi_char16_t efi_dummy_name[6] = { 'D', 'U', 'M', 'M', 'Y', 0 };
 
 static bool efi_no_storage_paranoia;
@@ -495,3 +549,61 @@ bool efi_poweroff_required(void)
 {
 	return acpi_gbl_reduced_hardware || acpi_no_s5;
 }
+
+#ifdef CONFIG_EFI_CAPSULE_QUIRK_QUARK_CSH
+
+static const struct x86_cpu_id quark_ids[] = {
+	{ X86_VENDOR_INTEL, 5, 9 },	/* Intel Quark X1000 */
+	{ }
+};
+
+int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
+			   size_t hdr_bytes)
+{
+	struct quark_security_header *csh = kbuff;
+
+	cap_info->total_size = 0;
+
+	if (!x86_match_cpu(quark_ids))
+		goto fallback;
+
+	/* Only process data block that is larger than the security header */
+	if (hdr_bytes < sizeof(struct quark_security_header))
+		return 0;
+
+	if (csh->csh_signature != QUARK_CSH_SIGNATURE ||
+	    csh->headersize != QUARK_SECURITY_HEADER_SIZE)
+		goto fallback;
+
+	/* 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("Quark security header detected\n");
+
+	if (csh->rsvd_next_header != 0) {
+		pr_err("multiple Quark security headers not supported\n");
+		return -EINVAL;
+	}
+
+	kbuff += csh->headersize;
+	cap_info->total_size = csh->headersize;
+
+	/*
+	 * Update the first page pointer to skip over the CSH header.
+	 */
+	cap_info->pages[0] += csh->headersize;
+
+fallback:
+	if (hdr_bytes < sizeof(efi_capsule_header_t))
+		return 0;
+
+	memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
+
+	cap_info->total_size += cap_info->header.imagesize;
+
+	return __efi_capsule_setup_info(cap_info);
+}
+
+#endif
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 2e78b0b96d74..394db40ed374 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -112,6 +112,15 @@ config EFI_CAPSULE_LOADER
 
 	  Most users should say N.
 
+config EFI_CAPSULE_QUIRK_QUARK_CSH
+	boolean "Add support for Quark capsules with non-standard headers"
+	depends on X86 && !64BIT
+	select EFI_CAPSULE_LOADER
+	default y
+	help
+	  Add support for processing Quark X1000 EFI capsules, whose header
+	  layout deviates from the layout mandated by the UEFI specification.
+
 config EFI_TEST
 	tristate "EFI Runtime Service Tests Support"
 	depends on EFI
-- 
2.9.3

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

[Matt Fleming]

On Wed, 05 Apr, at 10:23:17AM, Ard Biesheuvel wrote:
> From: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
> 
> 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 real header to __efi_capsule_setup_info.
> 
> To be minimally invasive and maximally safe, the quirk version of
> efi_capsule_identify_image is only effective on Quark processors.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
> Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> [ardb: refactor using an override of efi_capsule_setup_info()]
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  arch/x86/platform/efi/quirks.c | 112 ++++++++++++++++++++
>  drivers/firmware/efi/Kconfig   |   9 ++
>  2 files changed, 121 insertions(+)

[...]

> @@ -495,3 +549,61 @@ bool efi_poweroff_required(void)
>  {
>  	return acpi_gbl_reduced_hardware || acpi_no_s5;
>  }
> +
> +#ifdef CONFIG_EFI_CAPSULE_QUIRK_QUARK_CSH
> +
> +static const struct x86_cpu_id quark_ids[] = {
> +	{ X86_VENDOR_INTEL, 5, 9 },	/* Intel Quark X1000 */
> +	{ }
> +};
> +
> +int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
> +			   size_t hdr_bytes)
> +{
> +	struct quark_security_header *csh = kbuff;
> +
> +	cap_info->total_size = 0;
> +
> +	if (!x86_match_cpu(quark_ids))
> +		goto fallback;
> +

I'd prefer to see the quark quirk pulled out into its own function and
referenced from the __weak efi_capsule_setup_info() function, which
makes it easier to people to read the EFI capsule code flow if they're
not interested in the Quark quick.

Something like this,

int efi_capsule_setup_info(...)
{
	...

	if (x86_match_cpu(quark_ids))
		return efi_capsule_quark_setup_quirk(cap_info, kbuff, hdr_bytes);

> +	if (hdr_bytes < sizeof(efi_capsule_header_t))
> +		return 0;
> +
> +	memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
> +
> +	cap_info->total_size += cap_info->header.imagesize;
> +
> +	return __efi_capsule_setup_info(cap_info);
> +}

Or something.

Otherwise this looks fine to me.

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

[Andy Shevchenko]

On Tue, Apr 18, 2017 at 3:48 PM, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> On Wed, 05 Apr, at 10:23:17AM, Ard Biesheuvel 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 real header to __efi_capsule_setup_info.
>>
>> To be minimally invasive and maximally safe, the quirk version of
>> efi_capsule_identify_image is only effective on Quark processors.

>> +#ifdef CONFIG_EFI_CAPSULE_QUIRK_QUARK_CSH
>> +
>> +static const struct x86_cpu_id quark_ids[] = {
>> +     { X86_VENDOR_INTEL, 5, 9 },     /* Intel Quark X1000 */
>> +     { }
>> +};
>> +
>> +int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>> +                        size_t hdr_bytes)
>> +{
>> +     struct quark_security_header *csh = kbuff;
>> +
>> +     cap_info->total_size = 0;
>> +
>> +     if (!x86_match_cpu(quark_ids))
>> +             goto fallback;
>> +
>
> I'd prefer to see the quark quirk pulled out into its own function and
> referenced from the __weak efi_capsule_setup_info() function, which
> makes it easier to people to read the EFI capsule code flow if they're
> not interested in the Quark quick.
>
> Something like this,
>
> int efi_capsule_setup_info(...)
> {
>         ...
>
>         if (x86_match_cpu(quark_ids))
>                 return efi_capsule_quark_setup_quirk(cap_info, kbuff, hdr_bytes);

Previously I had proposed to use driver_data field

Something like

struct _info {
int(*setup)(...);
}; <<< this is optional, might be useful in the future, otherwise just
simple typedef.

int efi_capsule_quark_setup_quirk(...)
{
}

struct _info efi_capsule_quark_info = {
 .setup = ..._setup_quirk,
};

#define ICPU(family, model, ddata) ...

static const struct x86_cpu_id quark_ids[] = {
 ICPU(5, 9, efi_capsule_quark_info),
 {}
};

Though didn't hear back on it.
-- 
With Best Regards,
Andy Shevchenko

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

[Jan Kiszka]

On 2017-04-18 14:55, Andy Shevchenko wrote:
> On Tue, Apr 18, 2017 at 3:48 PM, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
>> On Wed, 05 Apr, at 10:23:17AM, Ard Biesheuvel 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 real header to __efi_capsule_setup_info.
>>>
>>> To be minimally invasive and maximally safe, the quirk version of
>>> efi_capsule_identify_image is only effective on Quark processors.
> 
>>> +#ifdef CONFIG_EFI_CAPSULE_QUIRK_QUARK_CSH
>>> +
>>> +static const struct x86_cpu_id quark_ids[] = {
>>> +     { X86_VENDOR_INTEL, 5, 9 },     /* Intel Quark X1000 */
>>> +     { }
>>> +};
>>> +
>>> +int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>> +                        size_t hdr_bytes)
>>> +{
>>> +     struct quark_security_header *csh = kbuff;
>>> +
>>> +     cap_info->total_size = 0;
>>> +
>>> +     if (!x86_match_cpu(quark_ids))
>>> +             goto fallback;
>>> +
>>
>> I'd prefer to see the quark quirk pulled out into its own function and
>> referenced from the __weak efi_capsule_setup_info() function, which
>> makes it easier to people to read the EFI capsule code flow if they're
>> not interested in the Quark quick.
>>
>> Something like this,
>>
>> int efi_capsule_setup_info(...)
>> {
>>         ...
>>
>>         if (x86_match_cpu(quark_ids))
>>                 return efi_capsule_quark_setup_quirk(cap_info, kbuff, hdr_bytes);
> 
> Previously I had proposed to use driver_data field
> 
> Something like
> 
> struct _info {
> int(*setup)(...);
> }; <<< this is optional, might be useful in the future, otherwise just
> simple typedef.
> 
> int efi_capsule_quark_setup_quirk(...)
> {
> }
> 
> struct _info efi_capsule_quark_info = {
>  .setup = ..._setup_quirk,
> };
> 
> #define ICPU(family, model, ddata) ...
> 
> static const struct x86_cpu_id quark_ids[] = {
>  ICPU(5, 9, efi_capsule_quark_info),
>  {}
> };
> 
> Though didn't hear back on it.
> 

I've implemented this, but for the old design, and Ard took over then.
So it never made it to the list.

Whatever layout of these bits is preferred, it can probably be done. I
just need an indication that there is (likely) a consensus.

Thanks,
Jan

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

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

[Matt Fleming]

On Tue, 18 Apr, at 02:59:43PM, Jan Kiszka wrote:
> 
> I've implemented this, but for the old design, and Ard took over then.
> So it never made it to the list.
> 
> Whatever layout of these bits is preferred, it can probably be done. I
> just need an indication that there is (likely) a consensus.

Post what you already have on top of Ard's series and we'll review it.

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

[Ard Biesheuvel]

On 18 April 2017 at 14:44, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> On Tue, 18 Apr, at 02:59:43PM, Jan Kiszka wrote:
>>
>> I've implemented this, but for the old design, and Ard took over then.
>> So it never made it to the list.
>>
>> Whatever layout of these bits is preferred, it can probably be done. I
>> just need an indication that there is (likely) a consensus.
>
> Post what you already have on top of Ard's series and we'll review it.

For the record, other than the change Matt suggested (to take the
Quirk handling out of the normal flow), I don't think there is a need
to add a lot of parametrization just to implement this single quirk.
IMO it can wait for the next one.

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

[Ard Biesheuvel]

On 18 April 2017 at 14:46, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 18 April 2017 at 14:44, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
>> On Tue, 18 Apr, at 02:59:43PM, Jan Kiszka wrote:
>>>
>>> I've implemented this, but for the old design, and Ard took over then.
>>> So it never made it to the list.
>>>
>>> Whatever layout of these bits is preferred, it can probably be done. I
>>> just need an indication that there is (likely) a consensus.
>>
>> Post what you already have on top of Ard's series and we'll review it.
>
> For the record, other than the change Matt suggested (to take the
> Quirk handling out of the normal flow), I don't think there is a need
> to add a lot of parametrization just to implement this single quirk.
> IMO it can wait for the next one.

OK, I have pushed patches 1 - 5 to efi/next branch, and the entire
series (with Matt's review feedback regarding #6 and #7 incorporated)
to my own branch here

https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=quark-capsule

Jan, would you mind taking over again, i.e, update patch #8 and repost
patches #6, #7 and #8 after retesting? (just remove my S-o-b from #8
after you've reworked it)

Thanks,
Ard.

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

[Jan Kiszka]

On 2017-04-18 19:10, Ard Biesheuvel wrote:
> On 18 April 2017 at 14:46, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> On 18 April 2017 at 14:44, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
>>> On Tue, 18 Apr, at 02:59:43PM, Jan Kiszka wrote:
>>>>
>>>> I've implemented this, but for the old design, and Ard took over then.
>>>> So it never made it to the list.
>>>>
>>>> Whatever layout of these bits is preferred, it can probably be done. I
>>>> just need an indication that there is (likely) a consensus.
>>>
>>> Post what you already have on top of Ard's series and we'll review it.
>>
>> For the record, other than the change Matt suggested (to take the
>> Quirk handling out of the normal flow), I don't think there is a need
>> to add a lot of parametrization just to implement this single quirk.
>> IMO it can wait for the next one.
> 
> OK, I have pushed patches 1 - 5 to efi/next branch, and the entire
> series (with Matt's review feedback regarding #6 and #7 incorporated)
> to my own branch here
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=quark-capsule
> 
> Jan, would you mind taking over again, i.e, update patch #8 and repost
> patches #6, #7 and #8 after retesting? (just remove my S-o-b from #8
> after you've reworked it)

Sure, I'll pick them up ASAP.

Jan

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

Re: [PATCH 0/8] efi: add support for non-standard capsule headers

[Jan Kiszka]

On 2017-04-05 11:23, Ard Biesheuvel wrote:
> This is a followup to Jan's series [0] to add support for the non-standard
> and awkward capsule header layout that is used by the Quark platform.
> 
> While we would prefer to adhere to the standard rigorously, the reality
> (and common practice) in Linux is that we sometimes have to deal with
> quirks. So while Jan's aim is to get Quark to work, the reason for my
> involvement is to try and accommodate this in a flexible way without
> putting any handling specific to this quirk in the common code.
> 
> Patches #1 to #4 are minor preparatory cleanups.
> 
> Patch #5 reworks the capsule loader code to use a cached copy of the
> header rather than load it from memory multiple times (which may involve
> a kmap/kunmap sequence if it is in highmem). This also allows some mangling
> to be performed by quirks code.
> 
> Patch #6 splits up efi_capsule_setup_info() into a primary part called
> __efi_capsule_setup_info(), and a __weak wrapper under the original name,
> allowing it to be overridden externally.
> 
> Patch #7 changes the array of struct page pointers maintained by the capsule
> loader into an array of physical addresses. This allows special versions of
> efi_capsule_setup_info() to mangle the contents of the capsule (and skip
> headers by moving pointers around) without putting any intimate knowledge
> of the quirks handling into the common code.
> 
> Patch #8 is Jan's original patch to add the Quark specific quirk to arch/x86,
> but reworked to take advantage of the facilities added in #6 and #7.
> 
> This has been tested by Jan on Quark, but this needs testing on other
> platforms to ensure that the common code still works as expected on
> conforming firmware implementations.
> 
> Ard Biesheuvel (3):
>   efi/capsule-loader: use cached copy of capsule header
>   efi/capsule-loader: indirect calls to efi_capsule_setup_info via weak
>     alias
>   efi/capsule-loader: use page addresses rather than struct page
>     pointers
> 
> Jan Kiszka (5):
>   efi/capsule: Fix return code on failing kmap/vmap
>   efi/capsule: Remove pr_debug on ENOMEM or EFAULT
>   efi/capsule: Clean up pr_err/info messages
>   efi/capsule: Adjust return type of efi_capsule_setup_info
>   efi/capsule: Add support for Quark security header
> 
>  arch/x86/platform/efi/quirks.c        | 112 ++++++++++++++++++++
>  drivers/firmware/efi/Kconfig          |   9 ++
>  drivers/firmware/efi/capsule-loader.c | 111 ++++++++-----------
>  drivers/firmware/efi/capsule.c        |   7 +-
>  include/linux/efi.h                   |  14 ++-
>  5 files changed, 184 insertions(+), 69 deletions(-)
> 

Thanks again for pushing this! I've retested the series in our queue
[1], and it works nicely.

Let me know if you need anything else from us to accelerate the merge.
Unfortunately, I don't think I have a different device with capsule
update support around (does the Seattle / Softiron have that feature?).

Jan

[1] https://github.com/siemens/linux/commits/queues/iot2000

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

Re: [PATCH 0/8] efi: add support for non-standard capsule headers

[Bryan O'Donoghue]

On 05/04/17 10:23, Ard Biesheuvel wrote:
> This is a followup to Jan's series [0] to add support for the non-standard
> and awkward capsule header layout that is used by the Quark platform.
>
> While we would prefer to adhere to the standard rigorously, the reality
> (and common practice) in Linux is that we sometimes have to deal with
> quirks. So while Jan's aim is to get Quark to work, the reason for my
> involvement is to try and accommodate this in a flexible way without
> putting any handling specific to this quirk in the common code.

Hi Ard, Jan.

I've run this series on my Galileo Gen2 and it appears to work well.

Please take my;

Tested-by: Bryan O'Donoghue <pure.logic-SyKdqv6vbfZdzvEItQ6vdLNAH6kLmebB@public.gmane.org>

for the series.

---
bod

Re: [PATCH 0/8] efi: add support for non-standard capsule headers

[Ard Biesheuvel]

On 18 April 2017 at 17:26, Bryan O'Donoghue
<pure.logic-SyKdqv6vbfZdzvEItQ6vdLNAH6kLmebB@public.gmane.org> wrote:
>
>
> On 05/04/17 10:23, Ard Biesheuvel wrote:
>>
>> This is a followup to Jan's series [0] to add support for the non-standard
>> and awkward capsule header layout that is used by the Quark platform.
>>
>> While we would prefer to adhere to the standard rigorously, the reality
>> (and common practice) in Linux is that we sometimes have to deal with
>> quirks. So while Jan's aim is to get Quark to work, the reason for my
>> involvement is to try and accommodate this in a flexible way without
>> putting any handling specific to this quirk in the common code.
>
>
> Hi Ard, Jan.
>
> I've run this series on my Galileo Gen2 and it appears to work well.
>
> Please take my;
>
> Tested-by: Bryan O'Donoghue <pure.logic-SyKdqv6vbfZdzvEItQ6vdLNAH6kLmebB@public.gmane.org>
>

Thanks Bryan,

I take it this is a board that is affected by the quirk?

Re: [PATCH 0/8] efi: add support for non-standard capsule headers

[Jan Kiszka]

On 2017-04-18 18:23, Ard Biesheuvel wrote:
> On 18 April 2017 at 17:26, Bryan O'Donoghue
> <pure.logic-SyKdqv6vbfZdzvEItQ6vdLNAH6kLmebB@public.gmane.org> wrote:
>>
>>
>> On 05/04/17 10:23, Ard Biesheuvel wrote:
>>>
>>> This is a followup to Jan's series [0] to add support for the non-standard
>>> and awkward capsule header layout that is used by the Quark platform.
>>>
>>> While we would prefer to adhere to the standard rigorously, the reality
>>> (and common practice) in Linux is that we sometimes have to deal with
>>> quirks. So while Jan's aim is to get Quark to work, the reason for my
>>> involvement is to try and accommodate this in a flexible way without
>>> putting any handling specific to this quirk in the common code.
>>
>>
>> Hi Ard, Jan.
>>
>> I've run this series on my Galileo Gen2 and it appears to work well.
>>
>> Please take my;
>>
>> Tested-by: Bryan O'Donoghue <pure.logic-SyKdqv6vbfZdzvEItQ6vdLNAH6kLmebB@public.gmane.org>
>>
> 
> Thanks Bryan,
> 
> I take it this is a board that is affected by the quirk?
> 

The Galileo Gen2 firmware doesn't technically require the CSH for
flashing (there is no secure boot with its SoC variant, thus no
verification of the header), but it can swallow such an update format.
It's a third case, so to say.

Jan

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

Re: [PATCH 0/8] efi: add support for non-standard capsule headers

[Bryan O'Donoghue]

On 18/04/17 17:23, Ard Biesheuvel wrote:
> On 18 April 2017 at 17:26, Bryan O'Donoghue
> <pure.logic-SyKdqv6vbfZdzvEItQ6vdLNAH6kLmebB@public.gmane.org> wrote:
>>
>>
>> On 05/04/17 10:23, Ard Biesheuvel wrote:
>>>
>>> This is a followup to Jan's series [0] to add support for the non-standard
>>> and awkward capsule header layout that is used by the Quark platform.
>>>
>>> While we would prefer to adhere to the standard rigorously, the reality
>>> (and common practice) in Linux is that we sometimes have to deal with
>>> quirks. So while Jan's aim is to get Quark to work, the reason for my
>>> involvement is to try and accommodate this in a flexible way without
>>> putting any handling specific to this quirk in the common code.
>>
>>
>> Hi Ard, Jan.
>>
>> I've run this series on my Galileo Gen2 and it appears to work well.
>>
>> Please take my;
>>
>> Tested-by: Bryan O'Donoghue <pure.logic-SyKdqv6vbfZdzvEItQ6vdLNAH6kLmebB@public.gmane.org>
>>
>
> Thanks Bryan,
>
> I take it this is a board that is affected by the quirk?
>

It is indeed.

Galileo Gen2 is a Quark x1000 board. All x1000 parts have the CSH header.

https://tinyurl.com/lhlygzh

cheers
---
bod