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

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

[Jan Kiszka]

This addresses the review feedback provided on round 1, specifically
- refactored queue to keep the Quark mess in 
- only check for CSH on Quark CPUs
- added some smaller cleanups of the capsule loader
- documented capsule header / linked to original code

See last patch for the background of the series.

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

Jan

Jan Kiszka (7):
  efi/capsule: Fix return code on failing kmap/vmap
  efi/capsule: Remove pr_debug on ENOMEM or EFAULT
  efi/capsule: Clean up pr_err/info messages
  efi/capsule: Adjust return type of efi_capsule_setup_info
  efi/capsule: Prepare for loading images with security header
  efi/capsule: Factor out overloadable efi_capsule_identify_image
  efi/capsule: Add support for Quark security header

 arch/x86/platform/efi/quirks.c        |  95 ++++++++++++++++++++++++
 drivers/firmware/efi/capsule-loader.c | 136 +++++++++++++++++++---------------
 drivers/firmware/efi/capsule.c        |  21 +++++-
 include/linux/efi.h                   |  19 +++++
 4 files changed, 208 insertions(+), 63 deletions(-)

-- 
2.10.2

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

[Jan Kiszka]

This addresses the review feedback provided on round 1, specifically
- refactored queue to keep the Quark mess in 
- only check for CSH on Quark CPUs
- added some smaller cleanups of the capsule loader
- documented capsule header / linked to original code

See last patch for the background of the series.

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

Jan

Jan Kiszka (7):
  efi/capsule: Fix return code on failing kmap/vmap
  efi/capsule: Remove pr_debug on ENOMEM or EFAULT
  efi/capsule: Clean up pr_err/info messages
  efi/capsule: Adjust return type of efi_capsule_setup_info
  efi/capsule: Prepare for loading images with security header
  efi/capsule: Factor out overloadable efi_capsule_identify_image
  efi/capsule: Add support for Quark security header

 arch/x86/platform/efi/quirks.c        |  95 ++++++++++++++++++++++++
 drivers/firmware/efi/capsule-loader.c | 136 +++++++++++++++++++---------------
 drivers/firmware/efi/capsule.c        |  21 +++++-
 include/linux/efi.h                   |  19 +++++
 4 files changed, 208 insertions(+), 63 deletions(-)

-- 
2.10.2

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

[Jan Kiszka]

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@siemens.com>
---
 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 9ae6c11..91e91f7 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.10.2

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

[Jan Kiszka]

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>
---
 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 9ae6c11..91e91f7 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.10.2

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

[Ard Biesheuvel]

On 24 March 2017 at 17:34, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 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@siemens.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.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 9ae6c11..91e91f7 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.10.2
>

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

[Jan Kiszka]

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

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 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 91e91f7..7b57dda 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.10.2

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

[Jan Kiszka]

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>
---
 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 91e91f7..7b57dda 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.10.2

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

[Ard Biesheuvel]

On 24 March 2017 at 17:34, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> Both cases are not worth a debug log message - the error code is telling
> enough.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.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 91e91f7..7b57dda 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.10.2
>

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

[Ard Biesheuvel]

On 24 March 2017 at 17:34, Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org> wrote:
> 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>

Reviewed-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 91e91f7..7b57dda 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.10.2
>

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

[Jan Kiszka]

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

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 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 7b57dda..3fb91e1 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.10.2

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

[Jan Kiszka]

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

Signed-off-by: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@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 7b57dda..3fb91e1 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.10.2

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

[Ard Biesheuvel]

On 24 March 2017 at 17:34, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> Avoid __func__, improve the information provided by some of the
> messages.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.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 7b57dda..3fb91e1 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.10.2
>

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

[Ard Biesheuvel]

On 24 March 2017 at 17:34, Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org> wrote:
> Avoid __func__, improve the information provided by some of the
> messages.
>
> Signed-off-by: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>

Reviewed-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 7b57dda..3fb91e1 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.10.2
>

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

[Jan Kiszka]

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

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 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 3fb91e1..37d3f6e 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.10.2

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

[Jan Kiszka]

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

Signed-off-by: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@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 3fb91e1..37d3f6e 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.10.2

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

[Ard Biesheuvel]

On 24 March 2017 at 17:34, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> We actually expect int at the caller and never return any size
> information.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.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 3fb91e1..37d3f6e 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.10.2
>

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

[Ard Biesheuvel]

On 24 March 2017 at 17:34, Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org> wrote:
> We actually expect int at the caller and never return any size
> information.
>
> Signed-off-by: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>

Reviewed-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 3fb91e1..37d3f6e 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.10.2
>

[PATCH v2 5/7] 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 and its user for this by
defining and evaluating a EFI header displacement in the image located
in memory. For standard-conforming capsules, this displacement is 0.

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

diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
index 37d3f6e..59e2694 100644
--- a/drivers/firmware/efi/capsule-loader.c
+++ b/drivers/firmware/efi/capsule-loader.c
@@ -26,6 +26,7 @@ struct capsule_info {
 	long		index;
 	size_t		count;
 	size_t		total_size;
+	unsigned int	efi_hdr_displacement;
 	struct page	**pages;
 	size_t		page_bytes_remain;
 };
@@ -83,6 +84,8 @@ static int efi_capsule_setup_info(struct capsule_info *cap_info,
 		return ret;
 	}
 
+	cap_info->efi_hdr_displacement = 0;
+
 	cap_info->total_size = cap_hdr->imagesize;
 	temp_page = krealloc(cap_info->pages,
 			     pages_needed * sizeof(void *),
@@ -103,16 +106,20 @@ static int 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,
-			VM_MAP, PAGE_KERNEL);
-	if (!cap_hdr_temp)
+	mapped_pages = vmap(cap_info->pages, cap_info->index,
+			    VM_MAP, PAGE_KERNEL);
+	if (!mapped_pages)
 		return -ENOMEM;
 
-	ret = efi_capsule_update(cap_hdr_temp, cap_info->pages);
-	vunmap(cap_hdr_temp);
+	cap_hdr = mapped_pages + cap_info->efi_hdr_displacement;
+
+	ret = efi_capsule_update(cap_hdr, cap_info->efi_hdr_displacement,
+				 cap_info->pages);
+	vunmap(mapped_pages);
 	if (ret) {
 		pr_err("capsule update failed\n");
 		return ret;
diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
index 6eedff4..a60c4c4 100644
--- a/drivers/firmware/efi/capsule.c
+++ b/drivers/firmware/efi/capsule.c
@@ -184,6 +184,8 @@ efi_capsule_update_locked(efi_capsule_header_t *capsule,
 /**
  * efi_capsule_update - send a capsule to the firmware
  * @capsule: capsule to send to firmware
+ * @efi_hdr_displacement: EFI header offset on first data page (only needed for
+ * non-conforming CSH capsules)
  * @pages: an array of capsule data pages
  *
  * Build a scatter gather list with EFI capsule block descriptors to
@@ -214,9 +216,12 @@ 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 efi_hdr_displacement,
+		       struct page **pages)
 {
 	u32 imagesize = capsule->imagesize;
+	u32 total_size = imagesize + efi_hdr_displacement;
 	efi_guid_t guid = capsule->guid;
 	unsigned int count, sg_count;
 	u32 flags = capsule->flags;
@@ -224,11 +229,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 (efi_hdr_displacement > PAGE_SIZE - sizeof(efi_capsule_header_t))
+		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 +263,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 (efi_hdr_displacement > 0) {
+				sglist[j].data += efi_hdr_displacement;
+				sz -= efi_hdr_displacement;
+				efi_hdr_displacement = 0;
+			}
+			sglist[j].length = sz;
 
 			imagesize -= sz;
 			count--;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 94d34e0..d83095c6 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1403,6 +1403,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,
+			      unsigned int efi_hdr_displacement,
 			      struct page **pages);
 
 #ifdef CONFIG_EFI_RUNTIME_MAP
-- 
2.10.2

[PATCH v2 5/7] 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 and its user for this by
defining and evaluating a EFI header displacement in the image located
in memory. For standard-conforming capsules, this displacement is 0.

Signed-off-by: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
---
 drivers/firmware/efi/capsule-loader.c | 19 +++++++++++++------
 drivers/firmware/efi/capsule.c        | 21 +++++++++++++++++----
 include/linux/efi.h                   |  1 +
 3 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
index 37d3f6e..59e2694 100644
--- a/drivers/firmware/efi/capsule-loader.c
+++ b/drivers/firmware/efi/capsule-loader.c
@@ -26,6 +26,7 @@ struct capsule_info {
 	long		index;
 	size_t		count;
 	size_t		total_size;
+	unsigned int	efi_hdr_displacement;
 	struct page	**pages;
 	size_t		page_bytes_remain;
 };
@@ -83,6 +84,8 @@ static int efi_capsule_setup_info(struct capsule_info *cap_info,
 		return ret;
 	}
 
+	cap_info->efi_hdr_displacement = 0;
+
 	cap_info->total_size = cap_hdr->imagesize;
 	temp_page = krealloc(cap_info->pages,
 			     pages_needed * sizeof(void *),
@@ -103,16 +106,20 @@ static int 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,
-			VM_MAP, PAGE_KERNEL);
-	if (!cap_hdr_temp)
+	mapped_pages = vmap(cap_info->pages, cap_info->index,
+			    VM_MAP, PAGE_KERNEL);
+	if (!mapped_pages)
 		return -ENOMEM;
 
-	ret = efi_capsule_update(cap_hdr_temp, cap_info->pages);
-	vunmap(cap_hdr_temp);
+	cap_hdr = mapped_pages + cap_info->efi_hdr_displacement;
+
+	ret = efi_capsule_update(cap_hdr, cap_info->efi_hdr_displacement,
+				 cap_info->pages);
+	vunmap(mapped_pages);
 	if (ret) {
 		pr_err("capsule update failed\n");
 		return ret;
diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
index 6eedff4..a60c4c4 100644
--- a/drivers/firmware/efi/capsule.c
+++ b/drivers/firmware/efi/capsule.c
@@ -184,6 +184,8 @@ efi_capsule_update_locked(efi_capsule_header_t *capsule,
 /**
  * efi_capsule_update - send a capsule to the firmware
  * @capsule: capsule to send to firmware
+ * @efi_hdr_displacement: EFI header offset on first data page (only needed for
+ * non-conforming CSH capsules)
  * @pages: an array of capsule data pages
  *
  * Build a scatter gather list with EFI capsule block descriptors to
@@ -214,9 +216,12 @@ 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 efi_hdr_displacement,
+		       struct page **pages)
 {
 	u32 imagesize = capsule->imagesize;
+	u32 total_size = imagesize + efi_hdr_displacement;
 	efi_guid_t guid = capsule->guid;
 	unsigned int count, sg_count;
 	u32 flags = capsule->flags;
@@ -224,11 +229,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 (efi_hdr_displacement > PAGE_SIZE - sizeof(efi_capsule_header_t))
+		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 +263,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 (efi_hdr_displacement > 0) {
+				sglist[j].data += efi_hdr_displacement;
+				sz -= efi_hdr_displacement;
+				efi_hdr_displacement = 0;
+			}
+			sglist[j].length = sz;
 
 			imagesize -= sz;
 			count--;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 94d34e0..d83095c6 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1403,6 +1403,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,
+			      unsigned int efi_hdr_displacement,
 			      struct page **pages);
 
 #ifdef CONFIG_EFI_RUNTIME_MAP
-- 
2.10.2

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

[Andy Shevchenko]

On Fri, Mar 24, 2017 at 7:34 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 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 and its user for this by
> defining and evaluating a EFI header displacement in the image located
> in memory. For standard-conforming capsules, this displacement is 0.

To me sounds like new field should be of type size_t.

>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  drivers/firmware/efi/capsule-loader.c | 19 +++++++++++++------
>  drivers/firmware/efi/capsule.c        | 21 +++++++++++++++++----
>  include/linux/efi.h                   |  1 +
>  3 files changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
> index 37d3f6e..59e2694 100644
> --- a/drivers/firmware/efi/capsule-loader.c
> +++ b/drivers/firmware/efi/capsule-loader.c
> @@ -26,6 +26,7 @@ struct capsule_info {
>         long            index;
>         size_t          count;
>         size_t          total_size;
> +       unsigned int    efi_hdr_displacement;
>         struct page     **pages;
>         size_t          page_bytes_remain;
>  };
> @@ -83,6 +84,8 @@ static int efi_capsule_setup_info(struct capsule_info *cap_info,
>                 return ret;
>         }
>
> +       cap_info->efi_hdr_displacement = 0;
> +
>         cap_info->total_size = cap_hdr->imagesize;
>         temp_page = krealloc(cap_info->pages,
>                              pages_needed * sizeof(void *),
> @@ -103,16 +106,20 @@ static int 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,
> -                       VM_MAP, PAGE_KERNEL);
> -       if (!cap_hdr_temp)
> +       mapped_pages = vmap(cap_info->pages, cap_info->index,
> +                           VM_MAP, PAGE_KERNEL);
> +       if (!mapped_pages)
>                 return -ENOMEM;
>
> -       ret = efi_capsule_update(cap_hdr_temp, cap_info->pages);
> -       vunmap(cap_hdr_temp);
> +       cap_hdr = mapped_pages + cap_info->efi_hdr_displacement;
> +
> +       ret = efi_capsule_update(cap_hdr, cap_info->efi_hdr_displacement,
> +                                cap_info->pages);
> +       vunmap(mapped_pages);
>         if (ret) {
>                 pr_err("capsule update failed\n");
>                 return ret;
> diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
> index 6eedff4..a60c4c4 100644
> --- a/drivers/firmware/efi/capsule.c
> +++ b/drivers/firmware/efi/capsule.c
> @@ -184,6 +184,8 @@ efi_capsule_update_locked(efi_capsule_header_t *capsule,
>  /**
>   * efi_capsule_update - send a capsule to the firmware
>   * @capsule: capsule to send to firmware
> + * @efi_hdr_displacement: EFI header offset on first data page (only needed for
> + * non-conforming CSH capsules)
>   * @pages: an array of capsule data pages
>   *
>   * Build a scatter gather list with EFI capsule block descriptors to
> @@ -214,9 +216,12 @@ 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 efi_hdr_displacement,
> +                      struct page **pages)
>  {
>         u32 imagesize = capsule->imagesize;
> +       u32 total_size = imagesize + efi_hdr_displacement;
>         efi_guid_t guid = capsule->guid;
>         unsigned int count, sg_count;
>         u32 flags = capsule->flags;
> @@ -224,11 +229,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 (efi_hdr_displacement > PAGE_SIZE - sizeof(efi_capsule_header_t))
> +               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 +263,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 (efi_hdr_displacement > 0) {
> +                               sglist[j].data += efi_hdr_displacement;
> +                               sz -= efi_hdr_displacement;
> +                               efi_hdr_displacement = 0;
> +                       }
> +                       sglist[j].length = sz;
>
>                         imagesize -= sz;
>                         count--;
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 94d34e0..d83095c6 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1403,6 +1403,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,
> +                             unsigned int efi_hdr_displacement,
>                               struct page **pages);
>
>  #ifdef CONFIG_EFI_RUNTIME_MAP
> --
> 2.10.2
>



-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Mar 24, 2017 at 7:34 PM, Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org> wrote:
> 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 and its user for this by
> defining and evaluating a EFI header displacement in the image located
> in memory. For standard-conforming capsules, this displacement is 0.

To me sounds like new field should be of type size_t.

>
> Signed-off-by: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/firmware/efi/capsule-loader.c | 19 +++++++++++++------
>  drivers/firmware/efi/capsule.c        | 21 +++++++++++++++++----
>  include/linux/efi.h                   |  1 +
>  3 files changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
> index 37d3f6e..59e2694 100644
> --- a/drivers/firmware/efi/capsule-loader.c
> +++ b/drivers/firmware/efi/capsule-loader.c
> @@ -26,6 +26,7 @@ struct capsule_info {
>         long            index;
>         size_t          count;
>         size_t          total_size;
> +       unsigned int    efi_hdr_displacement;
>         struct page     **pages;
>         size_t          page_bytes_remain;
>  };
> @@ -83,6 +84,8 @@ static int efi_capsule_setup_info(struct capsule_info *cap_info,
>                 return ret;
>         }
>
> +       cap_info->efi_hdr_displacement = 0;
> +
>         cap_info->total_size = cap_hdr->imagesize;
>         temp_page = krealloc(cap_info->pages,
>                              pages_needed * sizeof(void *),
> @@ -103,16 +106,20 @@ static int 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,
> -                       VM_MAP, PAGE_KERNEL);
> -       if (!cap_hdr_temp)
> +       mapped_pages = vmap(cap_info->pages, cap_info->index,
> +                           VM_MAP, PAGE_KERNEL);
> +       if (!mapped_pages)
>                 return -ENOMEM;
>
> -       ret = efi_capsule_update(cap_hdr_temp, cap_info->pages);
> -       vunmap(cap_hdr_temp);
> +       cap_hdr = mapped_pages + cap_info->efi_hdr_displacement;
> +
> +       ret = efi_capsule_update(cap_hdr, cap_info->efi_hdr_displacement,
> +                                cap_info->pages);
> +       vunmap(mapped_pages);
>         if (ret) {
>                 pr_err("capsule update failed\n");
>                 return ret;
> diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
> index 6eedff4..a60c4c4 100644
> --- a/drivers/firmware/efi/capsule.c
> +++ b/drivers/firmware/efi/capsule.c
> @@ -184,6 +184,8 @@ efi_capsule_update_locked(efi_capsule_header_t *capsule,
>  /**
>   * efi_capsule_update - send a capsule to the firmware
>   * @capsule: capsule to send to firmware
> + * @efi_hdr_displacement: EFI header offset on first data page (only needed for
> + * non-conforming CSH capsules)
>   * @pages: an array of capsule data pages
>   *
>   * Build a scatter gather list with EFI capsule block descriptors to
> @@ -214,9 +216,12 @@ 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 efi_hdr_displacement,
> +                      struct page **pages)
>  {
>         u32 imagesize = capsule->imagesize;
> +       u32 total_size = imagesize + efi_hdr_displacement;
>         efi_guid_t guid = capsule->guid;
>         unsigned int count, sg_count;
>         u32 flags = capsule->flags;
> @@ -224,11 +229,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 (efi_hdr_displacement > PAGE_SIZE - sizeof(efi_capsule_header_t))
> +               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 +263,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 (efi_hdr_displacement > 0) {
> +                               sglist[j].data += efi_hdr_displacement;
> +                               sz -= efi_hdr_displacement;
> +                               efi_hdr_displacement = 0;
> +                       }
> +                       sglist[j].length = sz;
>
>                         imagesize -= sz;
>                         count--;
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 94d34e0..d83095c6 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1403,6 +1403,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,
> +                             unsigned int efi_hdr_displacement,
>                               struct page **pages);
>
>  #ifdef CONFIG_EFI_RUNTIME_MAP
> --
> 2.10.2
>



-- 
With Best Regards,
Andy Shevchenko

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

[Ard Biesheuvel]

On 24 March 2017 at 17:34, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 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 and its user for this by
> defining and evaluating a EFI header displacement in the image located
> in memory. For standard-conforming capsules, this displacement is 0.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Hello Jan,

Thanks for taking the time to respin this.

I played around with these patches a bit (I can't really test them
since I don't have the hardware), and I am not really happy with the
non-trivial changes to the generic code, only to allow a header
displacement.

So instead, I attempted to come up with an alternative which does not
use a displacement field, but makes the core capsule routines work
with a copy of the capsule header rather than mandating that it exists
at the start of the buffer. This way, we can override the code that
performs the copy, and make it originate from somewhere else.

Could you please have a look at

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

and tell me if that would work for you? I will send them out for
proper review in any case, but to avoid confusion (if I missed
something obvious), I don't want to send them out just yet.

Thanks,
Ard.


> ---
>  drivers/firmware/efi/capsule-loader.c | 19 +++++++++++++------
>  drivers/firmware/efi/capsule.c        | 21 +++++++++++++++++----
>  include/linux/efi.h                   |  1 +
>  3 files changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
> index 37d3f6e..59e2694 100644
> --- a/drivers/firmware/efi/capsule-loader.c
> +++ b/drivers/firmware/efi/capsule-loader.c
> @@ -26,6 +26,7 @@ struct capsule_info {
>         long            index;
>         size_t          count;
>         size_t          total_size;
> +       unsigned int    efi_hdr_displacement;
>         struct page     **pages;
>         size_t          page_bytes_remain;
>  };
> @@ -83,6 +84,8 @@ static int efi_capsule_setup_info(struct capsule_info *cap_info,
>                 return ret;
>         }
>
> +       cap_info->efi_hdr_displacement = 0;
> +
>         cap_info->total_size = cap_hdr->imagesize;
>         temp_page = krealloc(cap_info->pages,
>                              pages_needed * sizeof(void *),
> @@ -103,16 +106,20 @@ static int 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,
> -                       VM_MAP, PAGE_KERNEL);
> -       if (!cap_hdr_temp)
> +       mapped_pages = vmap(cap_info->pages, cap_info->index,
> +                           VM_MAP, PAGE_KERNEL);
> +       if (!mapped_pages)
>                 return -ENOMEM;
>
> -       ret = efi_capsule_update(cap_hdr_temp, cap_info->pages);
> -       vunmap(cap_hdr_temp);
> +       cap_hdr = mapped_pages + cap_info->efi_hdr_displacement;
> +
> +       ret = efi_capsule_update(cap_hdr, cap_info->efi_hdr_displacement,
> +                                cap_info->pages);
> +       vunmap(mapped_pages);
>         if (ret) {
>                 pr_err("capsule update failed\n");
>                 return ret;
> diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
> index 6eedff4..a60c4c4 100644
> --- a/drivers/firmware/efi/capsule.c
> +++ b/drivers/firmware/efi/capsule.c
> @@ -184,6 +184,8 @@ efi_capsule_update_locked(efi_capsule_header_t *capsule,
>  /**
>   * efi_capsule_update - send a capsule to the firmware
>   * @capsule: capsule to send to firmware
> + * @efi_hdr_displacement: EFI header offset on first data page (only needed for
> + * non-conforming CSH capsules)
>   * @pages: an array of capsule data pages
>   *
>   * Build a scatter gather list with EFI capsule block descriptors to
> @@ -214,9 +216,12 @@ 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 efi_hdr_displacement,
> +                      struct page **pages)
>  {
>         u32 imagesize = capsule->imagesize;
> +       u32 total_size = imagesize + efi_hdr_displacement;
>         efi_guid_t guid = capsule->guid;
>         unsigned int count, sg_count;
>         u32 flags = capsule->flags;
> @@ -224,11 +229,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 (efi_hdr_displacement > PAGE_SIZE - sizeof(efi_capsule_header_t))
> +               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 +263,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 (efi_hdr_displacement > 0) {
> +                               sglist[j].data += efi_hdr_displacement;
> +                               sz -= efi_hdr_displacement;
> +                               efi_hdr_displacement = 0;
> +                       }
> +                       sglist[j].length = sz;
>
>                         imagesize -= sz;
>                         count--;
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 94d34e0..d83095c6 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1403,6 +1403,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,
> +                             unsigned int efi_hdr_displacement,
>                               struct page **pages);
>
>  #ifdef CONFIG_EFI_RUNTIME_MAP
> --
> 2.10.2
>

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

[Ard Biesheuvel]

On 24 March 2017 at 17:34, Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org> wrote:
> 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 and its user for this by
> defining and evaluating a EFI header displacement in the image located
> in memory. For standard-conforming capsules, this displacement is 0.
>
> Signed-off-by: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>

Hello Jan,

Thanks for taking the time to respin this.

I played around with these patches a bit (I can't really test them
since I don't have the hardware), and I am not really happy with the
non-trivial changes to the generic code, only to allow a header
displacement.

So instead, I attempted to come up with an alternative which does not
use a displacement field, but makes the core capsule routines work
with a copy of the capsule header rather than mandating that it exists
at the start of the buffer. This way, we can override the code that
performs the copy, and make it originate from somewhere else.

Could you please have a look at

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

and tell me if that would work for you? I will send them out for
proper review in any case, but to avoid confusion (if I missed
something obvious), I don't want to send them out just yet.

Thanks,
Ard.


> ---
>  drivers/firmware/efi/capsule-loader.c | 19 +++++++++++++------
>  drivers/firmware/efi/capsule.c        | 21 +++++++++++++++++----
>  include/linux/efi.h                   |  1 +
>  3 files changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
> index 37d3f6e..59e2694 100644
> --- a/drivers/firmware/efi/capsule-loader.c
> +++ b/drivers/firmware/efi/capsule-loader.c
> @@ -26,6 +26,7 @@ struct capsule_info {
>         long            index;
>         size_t          count;
>         size_t          total_size;
> +       unsigned int    efi_hdr_displacement;
>         struct page     **pages;
>         size_t          page_bytes_remain;
>  };
> @@ -83,6 +84,8 @@ static int efi_capsule_setup_info(struct capsule_info *cap_info,
>                 return ret;
>         }
>
> +       cap_info->efi_hdr_displacement = 0;
> +
>         cap_info->total_size = cap_hdr->imagesize;
>         temp_page = krealloc(cap_info->pages,
>                              pages_needed * sizeof(void *),
> @@ -103,16 +106,20 @@ static int 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,
> -                       VM_MAP, PAGE_KERNEL);
> -       if (!cap_hdr_temp)
> +       mapped_pages = vmap(cap_info->pages, cap_info->index,
> +                           VM_MAP, PAGE_KERNEL);
> +       if (!mapped_pages)
>                 return -ENOMEM;
>
> -       ret = efi_capsule_update(cap_hdr_temp, cap_info->pages);
> -       vunmap(cap_hdr_temp);
> +       cap_hdr = mapped_pages + cap_info->efi_hdr_displacement;
> +
> +       ret = efi_capsule_update(cap_hdr, cap_info->efi_hdr_displacement,
> +                                cap_info->pages);
> +       vunmap(mapped_pages);
>         if (ret) {
>                 pr_err("capsule update failed\n");
>                 return ret;
> diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
> index 6eedff4..a60c4c4 100644
> --- a/drivers/firmware/efi/capsule.c
> +++ b/drivers/firmware/efi/capsule.c
> @@ -184,6 +184,8 @@ efi_capsule_update_locked(efi_capsule_header_t *capsule,
>  /**
>   * efi_capsule_update - send a capsule to the firmware
>   * @capsule: capsule to send to firmware
> + * @efi_hdr_displacement: EFI header offset on first data page (only needed for
> + * non-conforming CSH capsules)
>   * @pages: an array of capsule data pages
>   *
>   * Build a scatter gather list with EFI capsule block descriptors to
> @@ -214,9 +216,12 @@ 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 efi_hdr_displacement,
> +                      struct page **pages)
>  {
>         u32 imagesize = capsule->imagesize;
> +       u32 total_size = imagesize + efi_hdr_displacement;
>         efi_guid_t guid = capsule->guid;
>         unsigned int count, sg_count;
>         u32 flags = capsule->flags;
> @@ -224,11 +229,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 (efi_hdr_displacement > PAGE_SIZE - sizeof(efi_capsule_header_t))
> +               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 +263,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 (efi_hdr_displacement > 0) {
> +                               sglist[j].data += efi_hdr_displacement;
> +                               sz -= efi_hdr_displacement;
> +                               efi_hdr_displacement = 0;
> +                       }
> +                       sglist[j].length = sz;
>
>                         imagesize -= sz;
>                         count--;
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 94d34e0..d83095c6 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1403,6 +1403,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,
> +                             unsigned int efi_hdr_displacement,
>                               struct page **pages);
>
>  #ifdef CONFIG_EFI_RUNTIME_MAP
> --
> 2.10.2
>

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

[Jan Kiszka]

On 2017-03-28 15:49, Ard Biesheuvel wrote:
> On 24 March 2017 at 17:34, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> 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 and its user for this by
>> defining and evaluating a EFI header displacement in the image located
>> in memory. For standard-conforming capsules, this displacement is 0.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Hello Jan,
> 
> Thanks for taking the time to respin this.
> 
> I played around with these patches a bit (I can't really test them
> since I don't have the hardware), and I am not really happy with the
> non-trivial changes to the generic code, only to allow a header
> displacement.
> 
> So instead, I attempted to come up with an alternative which does not
> use a displacement field, but makes the core capsule routines work
> with a copy of the capsule header rather than mandating that it exists
> at the start of the buffer. This way, we can override the code that
> performs the copy, and make it originate from somewhere else.
> 
> Could you please have a look at
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=quark-capsule
> 
> and tell me if that would work for you? I will send them out for
> proper review in any case, but to avoid confusion (if I missed
> something obvious), I don't want to send them out just yet.

There is more needed to make things work again, maybe around passing the
right image size. I'm looking into this.

Another observation: Making EFI_CAPSULE_QUIRK_QUARK_CSH select
EFI_CAPSULE_LOADER technically resolves the problem that the platform
code would otherwise need something from a capsule loader module.
However, a logical configuration would rather make the quirk depend on
the loader, wouldn't it? But I'm fine with both.

Thanks,
Jan

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

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

[Jan Kiszka]

On 2017-03-28 15:49, Ard Biesheuvel wrote:
> On 24 March 2017 at 17:34, Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org> wrote:
>> 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 and its user for this by
>> defining and evaluating a EFI header displacement in the image located
>> in memory. For standard-conforming capsules, this displacement is 0.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
> 
> Hello Jan,
> 
> Thanks for taking the time to respin this.
> 
> I played around with these patches a bit (I can't really test them
> since I don't have the hardware), and I am not really happy with the
> non-trivial changes to the generic code, only to allow a header
> displacement.
> 
> So instead, I attempted to come up with an alternative which does not
> use a displacement field, but makes the core capsule routines work
> with a copy of the capsule header rather than mandating that it exists
> at the start of the buffer. This way, we can override the code that
> performs the copy, and make it originate from somewhere else.
> 
> Could you please have a look at
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=quark-capsule
> 
> and tell me if that would work for you? I will send them out for
> proper review in any case, but to avoid confusion (if I missed
> something obvious), I don't want to send them out just yet.

There is more needed to make things work again, maybe around passing the
right image size. I'm looking into this.

Another observation: Making EFI_CAPSULE_QUIRK_QUARK_CSH select
EFI_CAPSULE_LOADER technically resolves the problem that the platform
code would otherwise need something from a capsule loader module.
However, a logical configuration would rather make the quirk depend on
the loader, wouldn't it? But I'm fine with both.

Thanks,
Jan

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

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

[Jan Kiszka]

On 2017-03-28 17:13, Jan Kiszka wrote:
> On 2017-03-28 15:49, Ard Biesheuvel wrote:
>> On 24 March 2017 at 17:34, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> 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 and its user for this by
>>> defining and evaluating a EFI header displacement in the image located
>>> in memory. For standard-conforming capsules, this displacement is 0.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Hello Jan,
>>
>> Thanks for taking the time to respin this.
>>
>> I played around with these patches a bit (I can't really test them
>> since I don't have the hardware), and I am not really happy with the
>> non-trivial changes to the generic code, only to allow a header
>> displacement.
>>
>> So instead, I attempted to come up with an alternative which does not
>> use a displacement field, but makes the core capsule routines work
>> with a copy of the capsule header rather than mandating that it exists
>> at the start of the buffer. This way, we can override the code that
>> performs the copy, and make it originate from somewhere else.
>>
>> Could you please have a look at
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=quark-capsule
>>
>> and tell me if that would work for you? I will send them out for
>> proper review in any case, but to avoid confusion (if I missed
>> something obvious), I don't want to send them out just yet.
> 
> There is more needed to make things work again, maybe around passing the
> right image size. I'm looking into this.
> 

This makes CSH images being accepted again:

diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 4b6f93f..a4e2311 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -562,6 +562,8 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
 {
 	struct quark_security_header *csh = kbuff;
 
+	cap_info->total_size = 0;
+
 	if (!x86_match_cpu(quark_ids))
 		goto fallback;
 
@@ -587,12 +589,16 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
 
 	kbuff += csh->headersize;
 
+	cap_info->total_size = csh->headersize;
+
 fallback:
 	if (hdr_bytes < sizeof(efi_capsule_header_t))
 		return 0;
 
 	memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
 
+	cap_info->total_size += cap_info->header.imagesize;
+
 	return __efi_capsule_setup_info(cap_info);
 }
 
diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
index e851951..40dc354 100644
--- a/drivers/firmware/efi/capsule-loader.c
+++ b/drivers/firmware/efi/capsule-loader.c
@@ -42,7 +42,7 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
 	int ret;
 	void *temp_page;
 
-	pages_needed = ALIGN(cap_info->header.imagesize, PAGE_SIZE) / PAGE_SIZE;
+	pages_needed = ALIGN(cap_info->total_size, PAGE_SIZE) / PAGE_SIZE;
 
 	if (pages_needed == 0) {
 		pr_err("invalid capsule size");
@@ -59,7 +59,6 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
 		return ret;
 	}
 
-	cap_info->total_size = cap_info->header.imagesize;
 	temp_page = krealloc(cap_info->pages,
 			     pages_needed * sizeof(void *),
 			     GFP_KERNEL | __GFP_ZERO);
@@ -87,6 +86,8 @@ int __weak efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
 
 	memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
 
+	cap_info->total_size = cap_info->header.imagesize;
+
 	return __efi_capsule_setup_info(cap_info);
 }
 
But then my changes to efi_capsule_update are missing the present the
right format to the loader. As efi_capsule_update needs to lay out the
sg-list in as special way, excluding the CSH on the first page, it needs
to know about the displacement.

Any suggestions how to address that without rolling back to my aproach?

Jan

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

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

[Ard Biesheuvel]

On 28 March 2017 at 16:43, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-03-28 17:13, Jan Kiszka wrote:
>> On 2017-03-28 15:49, Ard Biesheuvel wrote:
[..]
>>> Could you please have a look at
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=quark-capsule
>>>
>>> and tell me if that would work for you? I will send them out for
>>> proper review in any case, but to avoid confusion (if I missed
>>> something obvious), I don't want to send them out just yet.
>>
>> There is more needed to make things work again, maybe around passing the
>> right image size. I'm looking into this.
>>
>
> This makes CSH images being accepted again:
>
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index 4b6f93f..a4e2311 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -562,6 +562,8 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>  {
>         struct quark_security_header *csh = kbuff;
>
> +       cap_info->total_size = 0;
> +
>         if (!x86_match_cpu(quark_ids))
>                 goto fallback;
>
> @@ -587,12 +589,16 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>
>         kbuff += csh->headersize;
>
> +       cap_info->total_size = csh->headersize;
> +
>  fallback:
>         if (hdr_bytes < sizeof(efi_capsule_header_t))
>                 return 0;
>
>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>
> +       cap_info->total_size += cap_info->header.imagesize;
> +
>         return __efi_capsule_setup_info(cap_info);
>  }
>
> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
> index e851951..40dc354 100644
> --- a/drivers/firmware/efi/capsule-loader.c
> +++ b/drivers/firmware/efi/capsule-loader.c
> @@ -42,7 +42,7 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>         int ret;
>         void *temp_page;
>
> -       pages_needed = ALIGN(cap_info->header.imagesize, PAGE_SIZE) / PAGE_SIZE;
> +       pages_needed = ALIGN(cap_info->total_size, PAGE_SIZE) / PAGE_SIZE;
>
>         if (pages_needed == 0) {
>                 pr_err("invalid capsule size");
> @@ -59,7 +59,6 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>                 return ret;
>         }
>
> -       cap_info->total_size = cap_info->header.imagesize;
>         temp_page = krealloc(cap_info->pages,
>                              pages_needed * sizeof(void *),
>                              GFP_KERNEL | __GFP_ZERO);
> @@ -87,6 +86,8 @@ int __weak efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>
>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>
> +       cap_info->total_size = cap_info->header.imagesize;
> +
>         return __efi_capsule_setup_info(cap_info);
>  }
>

OK, thanks for debugging that.

> But then my changes to efi_capsule_update are missing the present the
> right format to the loader. As efi_capsule_update needs to lay out the
> sg-list in as special way, excluding the CSH on the first page, it needs
> to know about the displacement.
>
> Any suggestions how to address that without rolling back to my aproach?
>

OK, I'm a bit lost now: for my understanding, could you please
reiterate how the CSH image deviates from the ordinary one? Or more
specifically, what exactly is preventing us from simply chopping off
the CSH header and pushing the capsule header + payload into
/dev/capsule_loader?

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

[Jan Kiszka]

On 2017-03-28 17:52, Ard Biesheuvel wrote:
> On 28 March 2017 at 16:43, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2017-03-28 17:13, Jan Kiszka wrote:
>>> On 2017-03-28 15:49, Ard Biesheuvel wrote:
> [..]
>>>> Could you please have a look at
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=quark-capsule
>>>>
>>>> and tell me if that would work for you? I will send them out for
>>>> proper review in any case, but to avoid confusion (if I missed
>>>> something obvious), I don't want to send them out just yet.
>>>
>>> There is more needed to make things work again, maybe around passing the
>>> right image size. I'm looking into this.
>>>
>>
>> This makes CSH images being accepted again:
>>
>> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
>> index 4b6f93f..a4e2311 100644
>> --- a/arch/x86/platform/efi/quirks.c
>> +++ b/arch/x86/platform/efi/quirks.c
>> @@ -562,6 +562,8 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>  {
>>         struct quark_security_header *csh = kbuff;
>>
>> +       cap_info->total_size = 0;
>> +
>>         if (!x86_match_cpu(quark_ids))
>>                 goto fallback;
>>
>> @@ -587,12 +589,16 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>
>>         kbuff += csh->headersize;
>>
>> +       cap_info->total_size = csh->headersize;
>> +
>>  fallback:
>>         if (hdr_bytes < sizeof(efi_capsule_header_t))
>>                 return 0;
>>
>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>
>> +       cap_info->total_size += cap_info->header.imagesize;
>> +
>>         return __efi_capsule_setup_info(cap_info);
>>  }
>>
>> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
>> index e851951..40dc354 100644
>> --- a/drivers/firmware/efi/capsule-loader.c
>> +++ b/drivers/firmware/efi/capsule-loader.c
>> @@ -42,7 +42,7 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>         int ret;
>>         void *temp_page;
>>
>> -       pages_needed = ALIGN(cap_info->header.imagesize, PAGE_SIZE) / PAGE_SIZE;
>> +       pages_needed = ALIGN(cap_info->total_size, PAGE_SIZE) / PAGE_SIZE;
>>
>>         if (pages_needed == 0) {
>>                 pr_err("invalid capsule size");
>> @@ -59,7 +59,6 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>                 return ret;
>>         }
>>
>> -       cap_info->total_size = cap_info->header.imagesize;
>>         temp_page = krealloc(cap_info->pages,
>>                              pages_needed * sizeof(void *),
>>                              GFP_KERNEL | __GFP_ZERO);
>> @@ -87,6 +86,8 @@ int __weak efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>
>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>
>> +       cap_info->total_size = cap_info->header.imagesize;
>> +
>>         return __efi_capsule_setup_info(cap_info);
>>  }
>>
> 
> OK, thanks for debugging that.
> 
>> But then my changes to efi_capsule_update are missing the present the
>> right format to the loader. As efi_capsule_update needs to lay out the
>> sg-list in as special way, excluding the CSH on the first page, it needs
>> to know about the displacement.
>>
>> Any suggestions how to address that without rolling back to my aproach?
>>
> 
> OK, I'm a bit lost now: for my understanding, could you please
> reiterate how the CSH image deviates from the ordinary one? Or more
> specifically, what exactly is preventing us from simply chopping off
> the CSH header and pushing the capsule header + payload into
> /dev/capsule_loader?
> 

Devices that mandate a signed capsule for updates expect the CSH in
front of the regular capsule image. The interface to UEFI remains
unchanged, i.e. you pass the virtually chopped off capsule, but you have
to leave the CSH in RAM right in front of that very same image. It's a
"nice" side-channel API.

Jan

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

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

[Jan Kiszka]

On 2017-03-28 17:52, Ard Biesheuvel wrote:
> On 28 March 2017 at 16:43, Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org> wrote:
>> On 2017-03-28 17:13, Jan Kiszka wrote:
>>> On 2017-03-28 15:49, Ard Biesheuvel wrote:
> [..]
>>>> Could you please have a look at
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=quark-capsule
>>>>
>>>> and tell me if that would work for you? I will send them out for
>>>> proper review in any case, but to avoid confusion (if I missed
>>>> something obvious), I don't want to send them out just yet.
>>>
>>> There is more needed to make things work again, maybe around passing the
>>> right image size. I'm looking into this.
>>>
>>
>> This makes CSH images being accepted again:
>>
>> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
>> index 4b6f93f..a4e2311 100644
>> --- a/arch/x86/platform/efi/quirks.c
>> +++ b/arch/x86/platform/efi/quirks.c
>> @@ -562,6 +562,8 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>  {
>>         struct quark_security_header *csh = kbuff;
>>
>> +       cap_info->total_size = 0;
>> +
>>         if (!x86_match_cpu(quark_ids))
>>                 goto fallback;
>>
>> @@ -587,12 +589,16 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>
>>         kbuff += csh->headersize;
>>
>> +       cap_info->total_size = csh->headersize;
>> +
>>  fallback:
>>         if (hdr_bytes < sizeof(efi_capsule_header_t))
>>                 return 0;
>>
>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>
>> +       cap_info->total_size += cap_info->header.imagesize;
>> +
>>         return __efi_capsule_setup_info(cap_info);
>>  }
>>
>> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
>> index e851951..40dc354 100644
>> --- a/drivers/firmware/efi/capsule-loader.c
>> +++ b/drivers/firmware/efi/capsule-loader.c
>> @@ -42,7 +42,7 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>         int ret;
>>         void *temp_page;
>>
>> -       pages_needed = ALIGN(cap_info->header.imagesize, PAGE_SIZE) / PAGE_SIZE;
>> +       pages_needed = ALIGN(cap_info->total_size, PAGE_SIZE) / PAGE_SIZE;
>>
>>         if (pages_needed == 0) {
>>                 pr_err("invalid capsule size");
>> @@ -59,7 +59,6 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>                 return ret;
>>         }
>>
>> -       cap_info->total_size = cap_info->header.imagesize;
>>         temp_page = krealloc(cap_info->pages,
>>                              pages_needed * sizeof(void *),
>>                              GFP_KERNEL | __GFP_ZERO);
>> @@ -87,6 +86,8 @@ int __weak efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>
>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>
>> +       cap_info->total_size = cap_info->header.imagesize;
>> +
>>         return __efi_capsule_setup_info(cap_info);
>>  }
>>
> 
> OK, thanks for debugging that.
> 
>> But then my changes to efi_capsule_update are missing the present the
>> right format to the loader. As efi_capsule_update needs to lay out the
>> sg-list in as special way, excluding the CSH on the first page, it needs
>> to know about the displacement.
>>
>> Any suggestions how to address that without rolling back to my aproach?
>>
> 
> OK, I'm a bit lost now: for my understanding, could you please
> reiterate how the CSH image deviates from the ordinary one? Or more
> specifically, what exactly is preventing us from simply chopping off
> the CSH header and pushing the capsule header + payload into
> /dev/capsule_loader?
> 

Devices that mandate a signed capsule for updates expect the CSH in
front of the regular capsule image. The interface to UEFI remains
unchanged, i.e. you pass the virtually chopped off capsule, but you have
to leave the CSH in RAM right in front of that very same image. It's a
"nice" side-channel API.

Jan

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

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

[Ard Biesheuvel]

On 28 March 2017 at 17:18, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-03-28 17:52, Ard Biesheuvel wrote:
>> On 28 March 2017 at 16:43, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> On 2017-03-28 17:13, Jan Kiszka wrote:
>>>> On 2017-03-28 15:49, Ard Biesheuvel wrote:
>> [..]
>>>>> Could you please have a look at
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=quark-capsule
>>>>>
>>>>> and tell me if that would work for you? I will send them out for
>>>>> proper review in any case, but to avoid confusion (if I missed
>>>>> something obvious), I don't want to send them out just yet.
>>>>
>>>> There is more needed to make things work again, maybe around passing the
>>>> right image size. I'm looking into this.
>>>>
>>>
>>> This makes CSH images being accepted again:
>>>
>>> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
>>> index 4b6f93f..a4e2311 100644
>>> --- a/arch/x86/platform/efi/quirks.c
>>> +++ b/arch/x86/platform/efi/quirks.c
>>> @@ -562,6 +562,8 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>  {
>>>         struct quark_security_header *csh = kbuff;
>>>
>>> +       cap_info->total_size = 0;
>>> +
>>>         if (!x86_match_cpu(quark_ids))
>>>                 goto fallback;
>>>
>>> @@ -587,12 +589,16 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>
>>>         kbuff += csh->headersize;
>>>
>>> +       cap_info->total_size = csh->headersize;
>>> +
>>>  fallback:
>>>         if (hdr_bytes < sizeof(efi_capsule_header_t))
>>>                 return 0;
>>>
>>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>>
>>> +       cap_info->total_size += cap_info->header.imagesize;
>>> +
>>>         return __efi_capsule_setup_info(cap_info);
>>>  }
>>>
>>> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
>>> index e851951..40dc354 100644
>>> --- a/drivers/firmware/efi/capsule-loader.c
>>> +++ b/drivers/firmware/efi/capsule-loader.c
>>> @@ -42,7 +42,7 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>>         int ret;
>>>         void *temp_page;
>>>
>>> -       pages_needed = ALIGN(cap_info->header.imagesize, PAGE_SIZE) / PAGE_SIZE;
>>> +       pages_needed = ALIGN(cap_info->total_size, PAGE_SIZE) / PAGE_SIZE;
>>>
>>>         if (pages_needed == 0) {
>>>                 pr_err("invalid capsule size");
>>> @@ -59,7 +59,6 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>>                 return ret;
>>>         }
>>>
>>> -       cap_info->total_size = cap_info->header.imagesize;
>>>         temp_page = krealloc(cap_info->pages,
>>>                              pages_needed * sizeof(void *),
>>>                              GFP_KERNEL | __GFP_ZERO);
>>> @@ -87,6 +86,8 @@ int __weak efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>
>>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>>
>>> +       cap_info->total_size = cap_info->header.imagesize;
>>> +
>>>         return __efi_capsule_setup_info(cap_info);
>>>  }
>>>
>>
>> OK, thanks for debugging that.
>>
>>> But then my changes to efi_capsule_update are missing the present the
>>> right format to the loader. As efi_capsule_update needs to lay out the
>>> sg-list in as special way, excluding the CSH on the first page, it needs
>>> to know about the displacement.
>>>
>>> Any suggestions how to address that without rolling back to my aproach?
>>>
>>
>> OK, I'm a bit lost now: for my understanding, could you please
>> reiterate how the CSH image deviates from the ordinary one? Or more
>> specifically, what exactly is preventing us from simply chopping off
>> the CSH header and pushing the capsule header + payload into
>> /dev/capsule_loader?
>>
>
> Devices that mandate a signed capsule for updates expect the CSH in
> front of the regular capsule image. The interface to UEFI remains
> unchanged, i.e. you pass the virtually chopped off capsule, but you have
> to leave the CSH in RAM right in front of that very same image. It's a
> "nice" side-channel API.
>

Wow, that is worse than I thought.

So my suggestion (which I coded up, please pull again), is to replace
the array of struct page pointers with an array of physical addresses
in the capsule_info struct. This way, your special version of
efi_capsule_setup_info() can advance the first one by the size of the
header.

I hope this works for you. I am not sure whether imagesize needs to be
modified, so I left it alone for now (but I suspect it should be).

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

[Ard Biesheuvel]

On 28 March 2017 at 17:18, Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org> wrote:
> On 2017-03-28 17:52, Ard Biesheuvel wrote:
>> On 28 March 2017 at 16:43, Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org> wrote:
>>> On 2017-03-28 17:13, Jan Kiszka wrote:
>>>> On 2017-03-28 15:49, Ard Biesheuvel wrote:
>> [..]
>>>>> Could you please have a look at
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=quark-capsule
>>>>>
>>>>> and tell me if that would work for you? I will send them out for
>>>>> proper review in any case, but to avoid confusion (if I missed
>>>>> something obvious), I don't want to send them out just yet.
>>>>
>>>> There is more needed to make things work again, maybe around passing the
>>>> right image size. I'm looking into this.
>>>>
>>>
>>> This makes CSH images being accepted again:
>>>
>>> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
>>> index 4b6f93f..a4e2311 100644
>>> --- a/arch/x86/platform/efi/quirks.c
>>> +++ b/arch/x86/platform/efi/quirks.c
>>> @@ -562,6 +562,8 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>  {
>>>         struct quark_security_header *csh = kbuff;
>>>
>>> +       cap_info->total_size = 0;
>>> +
>>>         if (!x86_match_cpu(quark_ids))
>>>                 goto fallback;
>>>
>>> @@ -587,12 +589,16 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>
>>>         kbuff += csh->headersize;
>>>
>>> +       cap_info->total_size = csh->headersize;
>>> +
>>>  fallback:
>>>         if (hdr_bytes < sizeof(efi_capsule_header_t))
>>>                 return 0;
>>>
>>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>>
>>> +       cap_info->total_size += cap_info->header.imagesize;
>>> +
>>>         return __efi_capsule_setup_info(cap_info);
>>>  }
>>>
>>> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
>>> index e851951..40dc354 100644
>>> --- a/drivers/firmware/efi/capsule-loader.c
>>> +++ b/drivers/firmware/efi/capsule-loader.c
>>> @@ -42,7 +42,7 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>>         int ret;
>>>         void *temp_page;
>>>
>>> -       pages_needed = ALIGN(cap_info->header.imagesize, PAGE_SIZE) / PAGE_SIZE;
>>> +       pages_needed = ALIGN(cap_info->total_size, PAGE_SIZE) / PAGE_SIZE;
>>>
>>>         if (pages_needed == 0) {
>>>                 pr_err("invalid capsule size");
>>> @@ -59,7 +59,6 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>>                 return ret;
>>>         }
>>>
>>> -       cap_info->total_size = cap_info->header.imagesize;
>>>         temp_page = krealloc(cap_info->pages,
>>>                              pages_needed * sizeof(void *),
>>>                              GFP_KERNEL | __GFP_ZERO);
>>> @@ -87,6 +86,8 @@ int __weak efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>
>>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>>
>>> +       cap_info->total_size = cap_info->header.imagesize;
>>> +
>>>         return __efi_capsule_setup_info(cap_info);
>>>  }
>>>
>>
>> OK, thanks for debugging that.
>>
>>> But then my changes to efi_capsule_update are missing the present the
>>> right format to the loader. As efi_capsule_update needs to lay out the
>>> sg-list in as special way, excluding the CSH on the first page, it needs
>>> to know about the displacement.
>>>
>>> Any suggestions how to address that without rolling back to my aproach?
>>>
>>
>> OK, I'm a bit lost now: for my understanding, could you please
>> reiterate how the CSH image deviates from the ordinary one? Or more
>> specifically, what exactly is preventing us from simply chopping off
>> the CSH header and pushing the capsule header + payload into
>> /dev/capsule_loader?
>>
>
> Devices that mandate a signed capsule for updates expect the CSH in
> front of the regular capsule image. The interface to UEFI remains
> unchanged, i.e. you pass the virtually chopped off capsule, but you have
> to leave the CSH in RAM right in front of that very same image. It's a
> "nice" side-channel API.
>

Wow, that is worse than I thought.

So my suggestion (which I coded up, please pull again), is to replace
the array of struct page pointers with an array of physical addresses
in the capsule_info struct. This way, your special version of
efi_capsule_setup_info() can advance the first one by the size of the
header.

I hope this works for you. I am not sure whether imagesize needs to be
modified, so I left it alone for now (but I suspect it should be).

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

[Ard Biesheuvel]

On 28 March 2017 at 18:17, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 28 March 2017 at 17:18, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2017-03-28 17:52, Ard Biesheuvel wrote:
>>> On 28 March 2017 at 16:43, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>> On 2017-03-28 17:13, Jan Kiszka wrote:
>>>>> On 2017-03-28 15:49, Ard Biesheuvel wrote:
>>> [..]
>>>>>> Could you please have a look at
>>>>>>
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=quark-capsule
>>>>>>
>>>>>> and tell me if that would work for you? I will send them out for
>>>>>> proper review in any case, but to avoid confusion (if I missed
>>>>>> something obvious), I don't want to send them out just yet.
>>>>>
>>>>> There is more needed to make things work again, maybe around passing the
>>>>> right image size. I'm looking into this.
>>>>>
>>>>
>>>> This makes CSH images being accepted again:
>>>>
>>>> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
>>>> index 4b6f93f..a4e2311 100644
>>>> --- a/arch/x86/platform/efi/quirks.c
>>>> +++ b/arch/x86/platform/efi/quirks.c
>>>> @@ -562,6 +562,8 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>>  {
>>>>         struct quark_security_header *csh = kbuff;
>>>>
>>>> +       cap_info->total_size = 0;
>>>> +
>>>>         if (!x86_match_cpu(quark_ids))
>>>>                 goto fallback;
>>>>
>>>> @@ -587,12 +589,16 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>>
>>>>         kbuff += csh->headersize;
>>>>
>>>> +       cap_info->total_size = csh->headersize;
>>>> +
>>>>  fallback:
>>>>         if (hdr_bytes < sizeof(efi_capsule_header_t))
>>>>                 return 0;
>>>>
>>>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>>>
>>>> +       cap_info->total_size += cap_info->header.imagesize;
>>>> +
>>>>         return __efi_capsule_setup_info(cap_info);
>>>>  }
>>>>
>>>> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
>>>> index e851951..40dc354 100644
>>>> --- a/drivers/firmware/efi/capsule-loader.c
>>>> +++ b/drivers/firmware/efi/capsule-loader.c
>>>> @@ -42,7 +42,7 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>>>         int ret;
>>>>         void *temp_page;
>>>>
>>>> -       pages_needed = ALIGN(cap_info->header.imagesize, PAGE_SIZE) / PAGE_SIZE;
>>>> +       pages_needed = ALIGN(cap_info->total_size, PAGE_SIZE) / PAGE_SIZE;
>>>>
>>>>         if (pages_needed == 0) {
>>>>                 pr_err("invalid capsule size");
>>>> @@ -59,7 +59,6 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>>>                 return ret;
>>>>         }
>>>>
>>>> -       cap_info->total_size = cap_info->header.imagesize;
>>>>         temp_page = krealloc(cap_info->pages,
>>>>                              pages_needed * sizeof(void *),
>>>>                              GFP_KERNEL | __GFP_ZERO);
>>>> @@ -87,6 +86,8 @@ int __weak efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>>
>>>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>>>
>>>> +       cap_info->total_size = cap_info->header.imagesize;
>>>> +
>>>>         return __efi_capsule_setup_info(cap_info);
>>>>  }
>>>>
>>>
>>> OK, thanks for debugging that.
>>>
>>>> But then my changes to efi_capsule_update are missing the present the
>>>> right format to the loader. As efi_capsule_update needs to lay out the
>>>> sg-list in as special way, excluding the CSH on the first page, it needs
>>>> to know about the displacement.
>>>>
>>>> Any suggestions how to address that without rolling back to my aproach?
>>>>
>>>
>>> OK, I'm a bit lost now: for my understanding, could you please
>>> reiterate how the CSH image deviates from the ordinary one? Or more
>>> specifically, what exactly is preventing us from simply chopping off
>>> the CSH header and pushing the capsule header + payload into
>>> /dev/capsule_loader?
>>>
>>
>> Devices that mandate a signed capsule for updates expect the CSH in
>> front of the regular capsule image. The interface to UEFI remains
>> unchanged, i.e. you pass the virtually chopped off capsule, but you have
>> to leave the CSH in RAM right in front of that very same image. It's a
>> "nice" side-channel API.
>>
>
> Wow, that is worse than I thought.
>
> So my suggestion (which I coded up, please pull again),

Hmm, it does not build on x86 atm. Let me fix that up first (~15 min)

> is to replace
> the array of struct page pointers with an array of physical addresses
> in the capsule_info struct. This way, your special version of
> efi_capsule_setup_info() can advance the first one by the size of the
> header.
>
> I hope this works for you. I am not sure whether imagesize needs to be
> modified, so I left it alone for now (but I suspect it should be).

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

[Ard Biesheuvel]

On 28 March 2017 at 18:17, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 28 March 2017 at 17:18, Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org> wrote:
>> On 2017-03-28 17:52, Ard Biesheuvel wrote:
>>> On 28 March 2017 at 16:43, Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org> wrote:
>>>> On 2017-03-28 17:13, Jan Kiszka wrote:
>>>>> On 2017-03-28 15:49, Ard Biesheuvel wrote:
>>> [..]
>>>>>> Could you please have a look at
>>>>>>
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=quark-capsule
>>>>>>
>>>>>> and tell me if that would work for you? I will send them out for
>>>>>> proper review in any case, but to avoid confusion (if I missed
>>>>>> something obvious), I don't want to send them out just yet.
>>>>>
>>>>> There is more needed to make things work again, maybe around passing the
>>>>> right image size. I'm looking into this.
>>>>>
>>>>
>>>> This makes CSH images being accepted again:
>>>>
>>>> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
>>>> index 4b6f93f..a4e2311 100644
>>>> --- a/arch/x86/platform/efi/quirks.c
>>>> +++ b/arch/x86/platform/efi/quirks.c
>>>> @@ -562,6 +562,8 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>>  {
>>>>         struct quark_security_header *csh = kbuff;
>>>>
>>>> +       cap_info->total_size = 0;
>>>> +
>>>>         if (!x86_match_cpu(quark_ids))
>>>>                 goto fallback;
>>>>
>>>> @@ -587,12 +589,16 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>>
>>>>         kbuff += csh->headersize;
>>>>
>>>> +       cap_info->total_size = csh->headersize;
>>>> +
>>>>  fallback:
>>>>         if (hdr_bytes < sizeof(efi_capsule_header_t))
>>>>                 return 0;
>>>>
>>>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>>>
>>>> +       cap_info->total_size += cap_info->header.imagesize;
>>>> +
>>>>         return __efi_capsule_setup_info(cap_info);
>>>>  }
>>>>
>>>> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
>>>> index e851951..40dc354 100644
>>>> --- a/drivers/firmware/efi/capsule-loader.c
>>>> +++ b/drivers/firmware/efi/capsule-loader.c
>>>> @@ -42,7 +42,7 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>>>         int ret;
>>>>         void *temp_page;
>>>>
>>>> -       pages_needed = ALIGN(cap_info->header.imagesize, PAGE_SIZE) / PAGE_SIZE;
>>>> +       pages_needed = ALIGN(cap_info->total_size, PAGE_SIZE) / PAGE_SIZE;
>>>>
>>>>         if (pages_needed == 0) {
>>>>                 pr_err("invalid capsule size");
>>>> @@ -59,7 +59,6 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>>>                 return ret;
>>>>         }
>>>>
>>>> -       cap_info->total_size = cap_info->header.imagesize;
>>>>         temp_page = krealloc(cap_info->pages,
>>>>                              pages_needed * sizeof(void *),
>>>>                              GFP_KERNEL | __GFP_ZERO);
>>>> @@ -87,6 +86,8 @@ int __weak efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>>
>>>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>>>
>>>> +       cap_info->total_size = cap_info->header.imagesize;
>>>> +
>>>>         return __efi_capsule_setup_info(cap_info);
>>>>  }
>>>>
>>>
>>> OK, thanks for debugging that.
>>>
>>>> But then my changes to efi_capsule_update are missing the present the
>>>> right format to the loader. As efi_capsule_update needs to lay out the
>>>> sg-list in as special way, excluding the CSH on the first page, it needs
>>>> to know about the displacement.
>>>>
>>>> Any suggestions how to address that without rolling back to my aproach?
>>>>
>>>
>>> OK, I'm a bit lost now: for my understanding, could you please
>>> reiterate how the CSH image deviates from the ordinary one? Or more
>>> specifically, what exactly is preventing us from simply chopping off
>>> the CSH header and pushing the capsule header + payload into
>>> /dev/capsule_loader?
>>>
>>
>> Devices that mandate a signed capsule for updates expect the CSH in
>> front of the regular capsule image. The interface to UEFI remains
>> unchanged, i.e. you pass the virtually chopped off capsule, but you have
>> to leave the CSH in RAM right in front of that very same image. It's a
>> "nice" side-channel API.
>>
>
> Wow, that is worse than I thought.
>
> So my suggestion (which I coded up, please pull again),

Hmm, it does not build on x86 atm. Let me fix that up first (~15 min)

> is to replace
> the array of struct page pointers with an array of physical addresses
> in the capsule_info struct. This way, your special version of
> efi_capsule_setup_info() can advance the first one by the size of the
> header.
>
> I hope this works for you. I am not sure whether imagesize needs to be
> modified, so I left it alone for now (but I suspect it should be).

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

[Jan Kiszka]

On 2017-03-28 19:23, Ard Biesheuvel wrote:
> On 28 March 2017 at 18:17, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 28 March 2017 at 17:18, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> On 2017-03-28 17:52, Ard Biesheuvel wrote:
>>>> On 28 March 2017 at 16:43, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>> On 2017-03-28 17:13, Jan Kiszka wrote:
>>>>>> On 2017-03-28 15:49, Ard Biesheuvel wrote:
>>>> [..]
>>>>>>> Could you please have a look at
>>>>>>>
>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=quark-capsule
>>>>>>>
>>>>>>> and tell me if that would work for you? I will send them out for
>>>>>>> proper review in any case, but to avoid confusion (if I missed
>>>>>>> something obvious), I don't want to send them out just yet.
>>>>>>
>>>>>> There is more needed to make things work again, maybe around passing the
>>>>>> right image size. I'm looking into this.
>>>>>>
>>>>>
>>>>> This makes CSH images being accepted again:
>>>>>
>>>>> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
>>>>> index 4b6f93f..a4e2311 100644
>>>>> --- a/arch/x86/platform/efi/quirks.c
>>>>> +++ b/arch/x86/platform/efi/quirks.c
>>>>> @@ -562,6 +562,8 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>>>  {
>>>>>         struct quark_security_header *csh = kbuff;
>>>>>
>>>>> +       cap_info->total_size = 0;
>>>>> +
>>>>>         if (!x86_match_cpu(quark_ids))
>>>>>                 goto fallback;
>>>>>
>>>>> @@ -587,12 +589,16 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>>>
>>>>>         kbuff += csh->headersize;
>>>>>
>>>>> +       cap_info->total_size = csh->headersize;
>>>>> +
>>>>>  fallback:
>>>>>         if (hdr_bytes < sizeof(efi_capsule_header_t))
>>>>>                 return 0;
>>>>>
>>>>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>>>>
>>>>> +       cap_info->total_size += cap_info->header.imagesize;
>>>>> +
>>>>>         return __efi_capsule_setup_info(cap_info);
>>>>>  }
>>>>>
>>>>> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
>>>>> index e851951..40dc354 100644
>>>>> --- a/drivers/firmware/efi/capsule-loader.c
>>>>> +++ b/drivers/firmware/efi/capsule-loader.c
>>>>> @@ -42,7 +42,7 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>>>>         int ret;
>>>>>         void *temp_page;
>>>>>
>>>>> -       pages_needed = ALIGN(cap_info->header.imagesize, PAGE_SIZE) / PAGE_SIZE;
>>>>> +       pages_needed = ALIGN(cap_info->total_size, PAGE_SIZE) / PAGE_SIZE;
>>>>>
>>>>>         if (pages_needed == 0) {
>>>>>                 pr_err("invalid capsule size");
>>>>> @@ -59,7 +59,6 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>>>>                 return ret;
>>>>>         }
>>>>>
>>>>> -       cap_info->total_size = cap_info->header.imagesize;
>>>>>         temp_page = krealloc(cap_info->pages,
>>>>>                              pages_needed * sizeof(void *),
>>>>>                              GFP_KERNEL | __GFP_ZERO);
>>>>> @@ -87,6 +86,8 @@ int __weak efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>>>
>>>>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>>>>
>>>>> +       cap_info->total_size = cap_info->header.imagesize;
>>>>> +
>>>>>         return __efi_capsule_setup_info(cap_info);
>>>>>  }
>>>>>
>>>>
>>>> OK, thanks for debugging that.
>>>>
>>>>> But then my changes to efi_capsule_update are missing the present the
>>>>> right format to the loader. As efi_capsule_update needs to lay out the
>>>>> sg-list in as special way, excluding the CSH on the first page, it needs
>>>>> to know about the displacement.
>>>>>
>>>>> Any suggestions how to address that without rolling back to my aproach?
>>>>>
>>>>
>>>> OK, I'm a bit lost now: for my understanding, could you please
>>>> reiterate how the CSH image deviates from the ordinary one? Or more
>>>> specifically, what exactly is preventing us from simply chopping off
>>>> the CSH header and pushing the capsule header + payload into
>>>> /dev/capsule_loader?
>>>>
>>>
>>> Devices that mandate a signed capsule for updates expect the CSH in
>>> front of the regular capsule image. The interface to UEFI remains
>>> unchanged, i.e. you pass the virtually chopped off capsule, but you have
>>> to leave the CSH in RAM right in front of that very same image. It's a
>>> "nice" side-channel API.
>>>
>>
>> Wow, that is worse than I thought.
>>
>> So my suggestion (which I coded up, please pull again),
> 
> Hmm, it does not build on x86 atm. Let me fix that up first (~15 min)
> 

I'm without access to the test device till next Tuesday. Will come back
to you then.

Jan

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

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

[Jan Kiszka]

On 2017-03-28 19:23, Ard Biesheuvel wrote:
> On 28 March 2017 at 18:17, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 28 March 2017 at 17:18, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> On 2017-03-28 17:52, Ard Biesheuvel wrote:
>>>> On 28 March 2017 at 16:43, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>> On 2017-03-28 17:13, Jan Kiszka wrote:
>>>>>> On 2017-03-28 15:49, Ard Biesheuvel wrote:
>>>> [..]
>>>>>>> Could you please have a look at
>>>>>>>
>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=quark-capsule
>>>>>>>
>>>>>>> and tell me if that would work for you? I will send them out for
>>>>>>> proper review in any case, but to avoid confusion (if I missed
>>>>>>> something obvious), I don't want to send them out just yet.
>>>>>>
>>>>>> There is more needed to make things work again, maybe around passing the
>>>>>> right image size. I'm looking into this.
>>>>>>
>>>>>
>>>>> This makes CSH images being accepted again:
>>>>>
>>>>> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
>>>>> index 4b6f93f..a4e2311 100644
>>>>> --- a/arch/x86/platform/efi/quirks.c
>>>>> +++ b/arch/x86/platform/efi/quirks.c
>>>>> @@ -562,6 +562,8 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>>>  {
>>>>>         struct quark_security_header *csh = kbuff;
>>>>>
>>>>> +       cap_info->total_size = 0;
>>>>> +
>>>>>         if (!x86_match_cpu(quark_ids))
>>>>>                 goto fallback;
>>>>>
>>>>> @@ -587,12 +589,16 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>>>
>>>>>         kbuff += csh->headersize;
>>>>>
>>>>> +       cap_info->total_size = csh->headersize;
>>>>> +
>>>>>  fallback:
>>>>>         if (hdr_bytes < sizeof(efi_capsule_header_t))
>>>>>                 return 0;
>>>>>
>>>>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>>>>
>>>>> +       cap_info->total_size += cap_info->header.imagesize;
>>>>> +
>>>>>         return __efi_capsule_setup_info(cap_info);
>>>>>  }
>>>>>
>>>>> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
>>>>> index e851951..40dc354 100644
>>>>> --- a/drivers/firmware/efi/capsule-loader.c
>>>>> +++ b/drivers/firmware/efi/capsule-loader.c
>>>>> @@ -42,7 +42,7 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>>>>         int ret;
>>>>>         void *temp_page;
>>>>>
>>>>> -       pages_needed = ALIGN(cap_info->header.imagesize, PAGE_SIZE) / PAGE_SIZE;
>>>>> +       pages_needed = ALIGN(cap_info->total_size, PAGE_SIZE) / PAGE_SIZE;
>>>>>
>>>>>         if (pages_needed == 0) {
>>>>>                 pr_err("invalid capsule size");
>>>>> @@ -59,7 +59,6 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>>>>                 return ret;
>>>>>         }
>>>>>
>>>>> -       cap_info->total_size = cap_info->header.imagesize;
>>>>>         temp_page = krealloc(cap_info->pages,
>>>>>                              pages_needed * sizeof(void *),
>>>>>                              GFP_KERNEL | __GFP_ZERO);
>>>>> @@ -87,6 +86,8 @@ int __weak efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>>>
>>>>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>>>>
>>>>> +       cap_info->total_size = cap_info->header.imagesize;
>>>>> +
>>>>>         return __efi_capsule_setup_info(cap_info);
>>>>>  }
>>>>>
>>>>
>>>> OK, thanks for debugging that.
>>>>
>>>>> But then my changes to efi_capsule_update are missing the present the
>>>>> right format to the loader. As efi_capsule_update needs to lay out the
>>>>> sg-list in as special way, excluding the CSH on the first page, it needs
>>>>> to know about the displacement.
>>>>>
>>>>> Any suggestions how to address that without rolling back to my aproach?
>>>>>
>>>>
>>>> OK, I'm a bit lost now: for my understanding, could you please
>>>> reiterate how the CSH image deviates from the ordinary one? Or more
>>>> specifically, what exactly is preventing us from simply chopping off
>>>> the CSH header and pushing the capsule header + payload into
>>>> /dev/capsule_loader?
>>>>
>>>
>>> Devices that mandate a signed capsule for updates expect the CSH in
>>> front of the regular capsule image. The interface to UEFI remains
>>> unchanged, i.e. you pass the virtually chopped off capsule, but you have
>>> to leave the CSH in RAM right in front of that very same image. It's a
>>> "nice" side-channel API.
>>>
>>
>> Wow, that is worse than I thought.
>>
>> So my suggestion (which I coded up, please pull again),
> 
> Hmm, it does not build on x86 atm. Let me fix that up first (~15 min)
> 

Just tested the patches from b51f20c780 on top of our queue, and this
time the flashing worked fine!

Thanks,
Jan

>> is to replace
>> the array of struct page pointers with an array of physical addresses
>> in the capsule_info struct. This way, your special version of
>> efi_capsule_setup_info() can advance the first one by the size of the
>> header.
>>
>> I hope this works for you. I am not sure whether imagesize needs to be
>> modified, so I left it alone for now (but I suspect it should be).

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

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

[Jan Kiszka]

On 2017-03-28 19:23, Ard Biesheuvel wrote:
> On 28 March 2017 at 18:17, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> On 28 March 2017 at 17:18, Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org> wrote:
>>> On 2017-03-28 17:52, Ard Biesheuvel wrote:
>>>> On 28 March 2017 at 16:43, Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org> wrote:
>>>>> On 2017-03-28 17:13, Jan Kiszka wrote:
>>>>>> On 2017-03-28 15:49, Ard Biesheuvel wrote:
>>>> [..]
>>>>>>> Could you please have a look at
>>>>>>>
>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=quark-capsule
>>>>>>>
>>>>>>> and tell me if that would work for you? I will send them out for
>>>>>>> proper review in any case, but to avoid confusion (if I missed
>>>>>>> something obvious), I don't want to send them out just yet.
>>>>>>
>>>>>> There is more needed to make things work again, maybe around passing the
>>>>>> right image size. I'm looking into this.
>>>>>>
>>>>>
>>>>> This makes CSH images being accepted again:
>>>>>
>>>>> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
>>>>> index 4b6f93f..a4e2311 100644
>>>>> --- a/arch/x86/platform/efi/quirks.c
>>>>> +++ b/arch/x86/platform/efi/quirks.c
>>>>> @@ -562,6 +562,8 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>>>  {
>>>>>         struct quark_security_header *csh = kbuff;
>>>>>
>>>>> +       cap_info->total_size = 0;
>>>>> +
>>>>>         if (!x86_match_cpu(quark_ids))
>>>>>                 goto fallback;
>>>>>
>>>>> @@ -587,12 +589,16 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>>>
>>>>>         kbuff += csh->headersize;
>>>>>
>>>>> +       cap_info->total_size = csh->headersize;
>>>>> +
>>>>>  fallback:
>>>>>         if (hdr_bytes < sizeof(efi_capsule_header_t))
>>>>>                 return 0;
>>>>>
>>>>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>>>>
>>>>> +       cap_info->total_size += cap_info->header.imagesize;
>>>>> +
>>>>>         return __efi_capsule_setup_info(cap_info);
>>>>>  }
>>>>>
>>>>> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
>>>>> index e851951..40dc354 100644
>>>>> --- a/drivers/firmware/efi/capsule-loader.c
>>>>> +++ b/drivers/firmware/efi/capsule-loader.c
>>>>> @@ -42,7 +42,7 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>>>>         int ret;
>>>>>         void *temp_page;
>>>>>
>>>>> -       pages_needed = ALIGN(cap_info->header.imagesize, PAGE_SIZE) / PAGE_SIZE;
>>>>> +       pages_needed = ALIGN(cap_info->total_size, PAGE_SIZE) / PAGE_SIZE;
>>>>>
>>>>>         if (pages_needed == 0) {
>>>>>                 pr_err("invalid capsule size");
>>>>> @@ -59,7 +59,6 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info)
>>>>>                 return ret;
>>>>>         }
>>>>>
>>>>> -       cap_info->total_size = cap_info->header.imagesize;
>>>>>         temp_page = krealloc(cap_info->pages,
>>>>>                              pages_needed * sizeof(void *),
>>>>>                              GFP_KERNEL | __GFP_ZERO);
>>>>> @@ -87,6 +86,8 @@ int __weak efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>>>>>
>>>>>         memcpy(&cap_info->header, kbuff, sizeof(cap_info->header));
>>>>>
>>>>> +       cap_info->total_size = cap_info->header.imagesize;
>>>>> +
>>>>>         return __efi_capsule_setup_info(cap_info);
>>>>>  }
>>>>>
>>>>
>>>> OK, thanks for debugging that.
>>>>
>>>>> But then my changes to efi_capsule_update are missing the present the
>>>>> right format to the loader. As efi_capsule_update needs to lay out the
>>>>> sg-list in as special way, excluding the CSH on the first page, it needs
>>>>> to know about the displacement.
>>>>>
>>>>> Any suggestions how to address that without rolling back to my aproach?
>>>>>
>>>>
>>>> OK, I'm a bit lost now: for my understanding, could you please
>>>> reiterate how the CSH image deviates from the ordinary one? Or more
>>>> specifically, what exactly is preventing us from simply chopping off
>>>> the CSH header and pushing the capsule header + payload into
>>>> /dev/capsule_loader?
>>>>
>>>
>>> Devices that mandate a signed capsule for updates expect the CSH in
>>> front of the regular capsule image. The interface to UEFI remains
>>> unchanged, i.e. you pass the virtually chopped off capsule, but you have
>>> to leave the CSH in RAM right in front of that very same image. It's a
>>> "nice" side-channel API.
>>>
>>
>> Wow, that is worse than I thought.
>>
>> So my suggestion (which I coded up, please pull again),
> 
> Hmm, it does not build on x86 atm. Let me fix that up first (~15 min)
> 

Just tested the patches from b51f20c780 on top of our queue, and this
time the flashing worked fine!

Thanks,
Jan

>> is to replace
>> the array of struct page pointers with an array of physical addresses
>> in the capsule_info struct. This way, your special version of
>> efi_capsule_setup_info() can advance the first one by the size of the
>> header.
>>
>> I hope this works for you. I am not sure whether imagesize needs to be
>> modified, so I left it alone for now (but I suspect it should be).

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

[PATCH v2 6/7] efi/capsule: Factor out overloadable efi_capsule_identify_image

[Jan Kiszka]

Another step to prepare Quark's CSH capsule format: Factor out the weak
efi_capsule_identify_image function which is supposed to tell standard-
conforming images apart from the special ones. The conforming version of
it is __efi_capsule_identify_image, and that is called unless
efi_capsule_identify_image is overloaded by an architecture-specific
quirk implementation.

efi_capsule_setup_info calls the image identification callback and is
prepared for the case, efi_hdr_displacement becomes > 0 and the total
image size > than what the standard EFI header reports.

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

diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
index 59e2694..50cacd4 100644
--- a/drivers/firmware/efi/capsule-loader.c
+++ b/drivers/firmware/efi/capsule-loader.c
@@ -20,26 +20,15 @@
 
 #define NO_FURTHER_WRITE_ACTION -1
 
-struct capsule_info {
-	bool		header_obtained;
-	int		reset_type;
-	long		index;
-	size_t		count;
-	size_t		total_size;
-	unsigned int	efi_hdr_displacement;
-	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
+ * @cap_info: pointer to current instance of efi_capsule_info structure
  *
  *	In addition to freeing buffer pages, it flags NO_FURTHER_WRITE_ACTION
  *	to cease processing data in subsequent write(2) calls until close(2)
  *	is called.
  **/
-static void efi_free_all_buff_pages(struct capsule_info *cap_info)
+static void efi_free_all_buff_pages(struct efi_capsule_info *cap_info)
 {
 	while (cap_info->index > 0)
 		__free_page(cap_info->pages[--cap_info->index]);
@@ -47,28 +36,63 @@ static void efi_free_all_buff_pages(struct capsule_info *cap_info)
 	cap_info->index = NO_FURTHER_WRITE_ACTION;
 }
 
+int __efi_capsule_identify_image(struct efi_capsule_info *cap_info,
+				 void *header, size_t hdr_bytes)
+{
+	efi_capsule_header_t *cap_hdr = header;
+
+	/* Only process data block that is larger than efi header size */
+	if (hdr_bytes < sizeof(efi_capsule_header_t))
+		return 0;
+
+	cap_info->total_size = cap_hdr->imagesize;
+	cap_info->efi_hdr_displacement = 0;
+
+	return 1;
+}
+
+/**
+ * efi_capsule_identify_image - identify the capsule image layout and initialize
+ * 				efi_capsule_info fields accordingly
+ * @cap_info: pointer to current instance of efi_capsule_info structure
+ * @header: mapped image header
+ * @hdr_bytes: the total received number of bytes for header
+ *
+ * Return 1 on success, 0 if insufficient data was read so far, otherwise
+ * negative error code.
+ */
+int __weak efi_capsule_identify_image(struct efi_capsule_info *cap_info,
+				      void *header, size_t hdr_bytes)
+{
+	return __efi_capsule_identify_image(cap_info, header, hdr_bytes);
+}
+
 /**
  * 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
+ *			    setup efi_capsule_info structure
+ * @cap_info: pointer to current instance of efi_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,
+static int efi_capsule_setup_info(struct efi_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;
-
-	/* Only process data block that is larger than efi header size */
-	if (hdr_bytes < sizeof(efi_capsule_header_t))
-		return 0;
+	void *header;
+	int ret;
 
 	/* Reset back to the correct offset of header */
-	cap_hdr = kbuff - cap_info->count;
-	pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >> PAGE_SHIFT;
+	header = kbuff - cap_info->count;
+
+	ret = efi_capsule_identify_image(cap_info, header, hdr_bytes);
+	if (ret <= 0)
+		return ret;
+
+	cap_hdr = header + cap_info->efi_hdr_displacement;
+
+	pages_needed = ALIGN(cap_info->total_size, PAGE_SIZE) >> PAGE_SHIFT;
 
 	if (pages_needed == 0) {
 		pr_err("invalid capsule size");
@@ -77,16 +101,13 @@ 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,
+				    cap_info->total_size,
 				    &cap_info->reset_type);
 	if (ret) {
 		pr_err("capsule not supported\n");
 		return ret;
 	}
 
-	cap_info->efi_hdr_displacement = 0;
-
-	cap_info->total_size = cap_hdr->imagesize;
 	temp_page = krealloc(cap_info->pages,
 			     pages_needed * sizeof(void *),
 			     GFP_KERNEL | __GFP_ZERO);
@@ -102,9 +123,9 @@ static int efi_capsule_setup_info(struct capsule_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
+ * @cap_info: pointer to current instance of efi_capsule_info structure
  **/
-static ssize_t efi_capsule_submit_update(struct capsule_info *cap_info)
+static ssize_t efi_capsule_submit_update(struct efi_capsule_info *cap_info)
 {
 	efi_capsule_header_t *cap_hdr;
 	void *mapped_pages;
@@ -157,7 +178,7 @@ static ssize_t efi_capsule_write(struct file *file, const char __user *buff,
 				 size_t count, loff_t *offp)
 {
 	int ret = 0;
-	struct capsule_info *cap_info = file->private_data;
+	struct efi_capsule_info *cap_info = file->private_data;
 	struct page *page;
 	void *kbuff = NULL;
 	size_t write_byte;
@@ -244,7 +265,7 @@ static ssize_t efi_capsule_write(struct file *file, const char __user *buff,
 static int efi_capsule_flush(struct file *file, fl_owner_t id)
 {
 	int ret = 0;
-	struct capsule_info *cap_info = file->private_data;
+	struct efi_capsule_info *cap_info = file->private_data;
 
 	if (cap_info->index > 0) {
 		pr_err("capsule upload not complete\n");
@@ -265,7 +286,7 @@ static int efi_capsule_flush(struct file *file, fl_owner_t id)
  **/
 static int efi_capsule_release(struct inode *inode, struct file *file)
 {
-	struct capsule_info *cap_info = file->private_data;
+	struct efi_capsule_info *cap_info = file->private_data;
 
 	kfree(cap_info->pages);
 	kfree(file->private_data);
@@ -278,14 +299,14 @@ static int efi_capsule_release(struct inode *inode, struct file *file)
  * @inode: not used
  * @file: file pointer
  *
- *	Will allocate each capsule_info memory for each file open call.
+ *	Will allocate each efi_capsule_info memory for each file open call.
  *	This provided the capability to support multiple file open feature
  *	where user is not needed to wait for others to finish in order to
  *	upload their capsule binary.
  **/
 static int efi_capsule_open(struct inode *inode, struct file *file)
 {
-	struct capsule_info *cap_info;
+	struct efi_capsule_info *cap_info;
 
 	cap_info = kzalloc(sizeof(*cap_info), GFP_KERNEL);
 	if (!cap_info)
diff --git a/include/linux/efi.h b/include/linux/efi.h
index d83095c6..5561817 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1397,6 +1397,18 @@ int efivars_sysfs_init(void);
 #define EFIVARS_DATA_SIZE_MAX 1024
 
 #endif /* CONFIG_EFI_VARS */
+
+struct efi_capsule_info {
+	bool		header_obtained;
+	int		reset_type;
+	long		index;
+	size_t		count;
+	size_t		total_size;
+	unsigned int	efi_hdr_displacement;
+	struct page	**pages;
+	size_t		page_bytes_remain;
+};
+
 extern bool efi_capsule_pending(int *reset_type);
 
 extern int efi_capsule_supported(efi_guid_t guid, u32 flags,
@@ -1406,6 +1418,12 @@ extern int efi_capsule_update(efi_capsule_header_t *capsule,
 			      unsigned int efi_hdr_displacement,
 			      struct page **pages);
 
+int __efi_capsule_identify_image(struct efi_capsule_info *cap_info,
+				 void *header, size_t hdr_bytes);
+
+int efi_capsule_identify_image(struct efi_capsule_info *cap_info, void *header,
+			       size_t hdr_bytes);
+
 #ifdef CONFIG_EFI_RUNTIME_MAP
 int efi_runtime_map_init(struct kobject *);
 int efi_get_runtime_map_size(void);
-- 
2.10.2

[PATCH v2 6/7] efi/capsule: Factor out overloadable efi_capsule_identify_image

[Jan Kiszka]

Another step to prepare Quark's CSH capsule format: Factor out the weak
efi_capsule_identify_image function which is supposed to tell standard-
conforming images apart from the special ones. The conforming version of
it is __efi_capsule_identify_image, and that is called unless
efi_capsule_identify_image is overloaded by an architecture-specific
quirk implementation.

efi_capsule_setup_info calls the image identification callback and is
prepared for the case, efi_hdr_displacement becomes > 0 and the total
image size > than what the standard EFI header reports.

Signed-off-by: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
---
 drivers/firmware/efi/capsule-loader.c | 89 ++++++++++++++++++++++-------------
 include/linux/efi.h                   | 18 +++++++
 2 files changed, 73 insertions(+), 34 deletions(-)

diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
index 59e2694..50cacd4 100644
--- a/drivers/firmware/efi/capsule-loader.c
+++ b/drivers/firmware/efi/capsule-loader.c
@@ -20,26 +20,15 @@
 
 #define NO_FURTHER_WRITE_ACTION -1
 
-struct capsule_info {
-	bool		header_obtained;
-	int		reset_type;
-	long		index;
-	size_t		count;
-	size_t		total_size;
-	unsigned int	efi_hdr_displacement;
-	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
+ * @cap_info: pointer to current instance of efi_capsule_info structure
  *
  *	In addition to freeing buffer pages, it flags NO_FURTHER_WRITE_ACTION
  *	to cease processing data in subsequent write(2) calls until close(2)
  *	is called.
  **/
-static void efi_free_all_buff_pages(struct capsule_info *cap_info)
+static void efi_free_all_buff_pages(struct efi_capsule_info *cap_info)
 {
 	while (cap_info->index > 0)
 		__free_page(cap_info->pages[--cap_info->index]);
@@ -47,28 +36,63 @@ static void efi_free_all_buff_pages(struct capsule_info *cap_info)
 	cap_info->index = NO_FURTHER_WRITE_ACTION;
 }
 
+int __efi_capsule_identify_image(struct efi_capsule_info *cap_info,
+				 void *header, size_t hdr_bytes)
+{
+	efi_capsule_header_t *cap_hdr = header;
+
+	/* Only process data block that is larger than efi header size */
+	if (hdr_bytes < sizeof(efi_capsule_header_t))
+		return 0;
+
+	cap_info->total_size = cap_hdr->imagesize;
+	cap_info->efi_hdr_displacement = 0;
+
+	return 1;
+}
+
+/**
+ * efi_capsule_identify_image - identify the capsule image layout and initialize
+ * 				efi_capsule_info fields accordingly
+ * @cap_info: pointer to current instance of efi_capsule_info structure
+ * @header: mapped image header
+ * @hdr_bytes: the total received number of bytes for header
+ *
+ * Return 1 on success, 0 if insufficient data was read so far, otherwise
+ * negative error code.
+ */
+int __weak efi_capsule_identify_image(struct efi_capsule_info *cap_info,
+				      void *header, size_t hdr_bytes)
+{
+	return __efi_capsule_identify_image(cap_info, header, hdr_bytes);
+}
+
 /**
  * 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
+ *			    setup efi_capsule_info structure
+ * @cap_info: pointer to current instance of efi_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,
+static int efi_capsule_setup_info(struct efi_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;
-
-	/* Only process data block that is larger than efi header size */
-	if (hdr_bytes < sizeof(efi_capsule_header_t))
-		return 0;
+	void *header;
+	int ret;
 
 	/* Reset back to the correct offset of header */
-	cap_hdr = kbuff - cap_info->count;
-	pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >> PAGE_SHIFT;
+	header = kbuff - cap_info->count;
+
+	ret = efi_capsule_identify_image(cap_info, header, hdr_bytes);
+	if (ret <= 0)
+		return ret;
+
+	cap_hdr = header + cap_info->efi_hdr_displacement;
+
+	pages_needed = ALIGN(cap_info->total_size, PAGE_SIZE) >> PAGE_SHIFT;
 
 	if (pages_needed == 0) {
 		pr_err("invalid capsule size");
@@ -77,16 +101,13 @@ 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,
+				    cap_info->total_size,
 				    &cap_info->reset_type);
 	if (ret) {
 		pr_err("capsule not supported\n");
 		return ret;
 	}
 
-	cap_info->efi_hdr_displacement = 0;
-
-	cap_info->total_size = cap_hdr->imagesize;
 	temp_page = krealloc(cap_info->pages,
 			     pages_needed * sizeof(void *),
 			     GFP_KERNEL | __GFP_ZERO);
@@ -102,9 +123,9 @@ static int efi_capsule_setup_info(struct capsule_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
+ * @cap_info: pointer to current instance of efi_capsule_info structure
  **/
-static ssize_t efi_capsule_submit_update(struct capsule_info *cap_info)
+static ssize_t efi_capsule_submit_update(struct efi_capsule_info *cap_info)
 {
 	efi_capsule_header_t *cap_hdr;
 	void *mapped_pages;
@@ -157,7 +178,7 @@ static ssize_t efi_capsule_write(struct file *file, const char __user *buff,
 				 size_t count, loff_t *offp)
 {
 	int ret = 0;
-	struct capsule_info *cap_info = file->private_data;
+	struct efi_capsule_info *cap_info = file->private_data;
 	struct page *page;
 	void *kbuff = NULL;
 	size_t write_byte;
@@ -244,7 +265,7 @@ static ssize_t efi_capsule_write(struct file *file, const char __user *buff,
 static int efi_capsule_flush(struct file *file, fl_owner_t id)
 {
 	int ret = 0;
-	struct capsule_info *cap_info = file->private_data;
+	struct efi_capsule_info *cap_info = file->private_data;
 
 	if (cap_info->index > 0) {
 		pr_err("capsule upload not complete\n");
@@ -265,7 +286,7 @@ static int efi_capsule_flush(struct file *file, fl_owner_t id)
  **/
 static int efi_capsule_release(struct inode *inode, struct file *file)
 {
-	struct capsule_info *cap_info = file->private_data;
+	struct efi_capsule_info *cap_info = file->private_data;
 
 	kfree(cap_info->pages);
 	kfree(file->private_data);
@@ -278,14 +299,14 @@ static int efi_capsule_release(struct inode *inode, struct file *file)
  * @inode: not used
  * @file: file pointer
  *
- *	Will allocate each capsule_info memory for each file open call.
+ *	Will allocate each efi_capsule_info memory for each file open call.
  *	This provided the capability to support multiple file open feature
  *	where user is not needed to wait for others to finish in order to
  *	upload their capsule binary.
  **/
 static int efi_capsule_open(struct inode *inode, struct file *file)
 {
-	struct capsule_info *cap_info;
+	struct efi_capsule_info *cap_info;
 
 	cap_info = kzalloc(sizeof(*cap_info), GFP_KERNEL);
 	if (!cap_info)
diff --git a/include/linux/efi.h b/include/linux/efi.h
index d83095c6..5561817 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1397,6 +1397,18 @@ int efivars_sysfs_init(void);
 #define EFIVARS_DATA_SIZE_MAX 1024
 
 #endif /* CONFIG_EFI_VARS */
+
+struct efi_capsule_info {
+	bool		header_obtained;
+	int		reset_type;
+	long		index;
+	size_t		count;
+	size_t		total_size;
+	unsigned int	efi_hdr_displacement;
+	struct page	**pages;
+	size_t		page_bytes_remain;
+};
+
 extern bool efi_capsule_pending(int *reset_type);
 
 extern int efi_capsule_supported(efi_guid_t guid, u32 flags,
@@ -1406,6 +1418,12 @@ extern int efi_capsule_update(efi_capsule_header_t *capsule,
 			      unsigned int efi_hdr_displacement,
 			      struct page **pages);
 
+int __efi_capsule_identify_image(struct efi_capsule_info *cap_info,
+				 void *header, size_t hdr_bytes);
+
+int efi_capsule_identify_image(struct efi_capsule_info *cap_info, void *header,
+			       size_t hdr_bytes);
+
 #ifdef CONFIG_EFI_RUNTIME_MAP
 int efi_runtime_map_init(struct kobject *);
 int efi_get_runtime_map_size(void);
-- 
2.10.2

[PATCH v2 7/7] 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 displacement in cap_info.

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

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/platform/efi/quirks.c | 95 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)

diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 30031d5..7f16295 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,44 @@ bool efi_poweroff_required(void)
 {
 	return acpi_gbl_reduced_hardware || acpi_no_s5;
 }
+
+static const struct x86_cpu_id quark_ids[] = {
+	{ X86_VENDOR_INTEL, 5, 9 },	/* Intel Quark X1000 */
+	{ }
+};
+
+int efi_capsule_identify_image(struct efi_capsule_info *cap_info, void *header,
+			       size_t hdr_bytes)
+{
+	struct quark_security_header *csh = header;
+
+	if (!x86_match_cpu(quark_ids))
+		return __efi_capsule_identify_image(cap_info, header,
+						    hdr_bytes);
+
+	/* 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)
+		return __efi_capsule_identify_image(cap_info, header,
+						    hdr_bytes);
+
+	/* 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;
+	}
+
+	cap_info->total_size = csh->modulesize;
+	cap_info->efi_hdr_displacement = csh->headersize;
+
+	return 1;
+}
-- 
2.10.2

[PATCH v2 7/7] 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 displacement in cap_info.

To be minimal invasive and maximal 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>
---
 arch/x86/platform/efi/quirks.c | 95 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)

diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 30031d5..7f16295 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,44 @@ bool efi_poweroff_required(void)
 {
 	return acpi_gbl_reduced_hardware || acpi_no_s5;
 }
+
+static const struct x86_cpu_id quark_ids[] = {
+	{ X86_VENDOR_INTEL, 5, 9 },	/* Intel Quark X1000 */
+	{ }
+};
+
+int efi_capsule_identify_image(struct efi_capsule_info *cap_info, void *header,
+			       size_t hdr_bytes)
+{
+	struct quark_security_header *csh = header;
+
+	if (!x86_match_cpu(quark_ids))
+		return __efi_capsule_identify_image(cap_info, header,
+						    hdr_bytes);
+
+	/* 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)
+		return __efi_capsule_identify_image(cap_info, header,
+						    hdr_bytes);
+
+	/* 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;
+	}
+
+	cap_info->total_size = csh->modulesize;
+	cap_info->efi_hdr_displacement = csh->headersize;
+
+	return 1;
+}
-- 
2.10.2

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

[Andy Shevchenko]

On Fri, Mar 24, 2017 at 7:34 PM, Jan Kiszka <jan.kiszka@siemens.com> 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 displacement in cap_info.
>
> To be minimal invasive and maximal safe, the quirk version of
> efi_capsule_identify_image is only effective on Quark processors.


> +static const struct x86_cpu_id quark_ids[] = {
> +       { X86_VENDOR_INTEL, 5, 9 },     /* Intel Quark X1000 */
> +       { }
> +};
> +
> +int efi_capsule_identify_image(struct efi_capsule_info *cap_info, void *header,
> +                              size_t hdr_bytes)
> +{
> +       struct quark_security_header *csh = header;
> +

> +       if (!x86_match_cpu(quark_ids))
> +               return __efi_capsule_identify_image(cap_info, header,
> +                                                   hdr_bytes);

I would slightly differently, i.e. introduce a helper
capsule_identify_image_qrk() and do here something like


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

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

...
       id = x86_match_cpu(efi_capsule_quirk_ids);
       if (id)
          return ((...)id->data)(...);

       return __efi_capsule_identify_image(cap_info, header,  hdr_bytes);

> +
> +       /* 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)
> +               return __efi_capsule_identify_image(cap_info, header,
> +                                                   hdr_bytes);
> +
> +       /* 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;
> +       }
> +
> +       cap_info->total_size = csh->modulesize;
> +       cap_info->efi_hdr_displacement = csh->headersize;
> +
> +       return 1;
> +}


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Mar 24, 2017 at 7:34 PM, Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org> 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 displacement in cap_info.
>
> To be minimal invasive and maximal safe, the quirk version of
> efi_capsule_identify_image is only effective on Quark processors.


> +static const struct x86_cpu_id quark_ids[] = {
> +       { X86_VENDOR_INTEL, 5, 9 },     /* Intel Quark X1000 */
> +       { }
> +};
> +
> +int efi_capsule_identify_image(struct efi_capsule_info *cap_info, void *header,
> +                              size_t hdr_bytes)
> +{
> +       struct quark_security_header *csh = header;
> +

> +       if (!x86_match_cpu(quark_ids))
> +               return __efi_capsule_identify_image(cap_info, header,
> +                                                   hdr_bytes);

I would slightly differently, i.e. introduce a helper
capsule_identify_image_qrk() and do here something like


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

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

...
       id = x86_match_cpu(efi_capsule_quirk_ids);
       if (id)
          return ((...)id->data)(...);

       return __efi_capsule_identify_image(cap_info, header,  hdr_bytes);

> +
> +       /* 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)
> +               return __efi_capsule_identify_image(cap_info, header,
> +                                                   hdr_bytes);
> +
> +       /* 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;
> +       }
> +
> +       cap_info->total_size = csh->modulesize;
> +       cap_info->efi_hdr_displacement = csh->headersize;
> +
> +       return 1;
> +}


-- 
With Best Regards,
Andy Shevchenko

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 855 bytes --]

Hi Jan,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.11-rc3 next-20170324]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jan-Kiszka/efi-capsule-Fix-return-code-on-failing-kmap-vmap/20170326-052032
config: x86_64-acpi-redef (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   arch/x86/built-in.o: In function `efi_capsule_identify_image':
>> (.text+0x75cfc): undefined reference to `__efi_capsule_identify_image'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29511 bytes --]

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

[Andy Shevchenko]

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

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

I wasn't able to test it, thus FWIW:
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

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



-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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

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

I wasn't able to test it, thus FWIW:
Reviewed-by: Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

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



-- 
With Best Regards,
Andy Shevchenko

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

[Jan Kiszka]

On 2017-03-24 21:39, Andy Shevchenko wrote:
> On Fri, Mar 24, 2017 at 7:34 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> This addresses the review feedback provided on round 1, specifically
>> - refactored queue to keep the Quark mess in
>> - only check for CSH on Quark CPUs
>> - added some smaller cleanups of the capsule loader
>> - documented capsule header / linked to original code
>>
>> See last patch for the background of the series.
>>
>> The series has been tested on the Galileo Gen2, to exclude regressions,
>> with a firmware.cap with AND without security header and the SIMATIC
>> IOT2040 which requires the header because of its mandatory secure boot.
> 
> The series looks good to me from code prospective. It's clean and
> understandable, though I have couple of comments (see related
> patches).
> Please, address them.

Done, also the kbuild reboot finding and another build issue along that.
Will send v3 after testing again (unfortunately not on the Galileo - my
board died).

> 
> I wasn't able to test it, thus FWIW:
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Thanks,
Jan

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

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

[Jan Kiszka]

On 2017-03-24 21:39, Andy Shevchenko wrote:
> On Fri, Mar 24, 2017 at 7:34 PM, Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org> wrote:
>> This addresses the review feedback provided on round 1, specifically
>> - refactored queue to keep the Quark mess in
>> - only check for CSH on Quark CPUs
>> - added some smaller cleanups of the capsule loader
>> - documented capsule header / linked to original code
>>
>> See last patch for the background of the series.
>>
>> The series has been tested on the Galileo Gen2, to exclude regressions,
>> with a firmware.cap with AND without security header and the SIMATIC
>> IOT2040 which requires the header because of its mandatory secure boot.
> 
> The series looks good to me from code prospective. It's clean and
> understandable, though I have couple of comments (see related
> patches).
> Please, address them.

Done, also the kbuild reboot finding and another build issue along that.
Will send v3 after testing again (unfortunately not on the Galileo - my
board died).

> 
> I wasn't able to test it, thus FWIW:
> Reviewed-by: Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Thanks,
Jan

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

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

[Bryan O'Donoghue]

On 24/03/17 17:34, Jan Kiszka wrote:
> This addresses the review feedback provided on round 1, specifically
> - refactored queue to keep the Quark mess in
> - only check for CSH on Quark CPUs
> - added some smaller cleanups of the capsule loader
> - documented capsule header / linked to original code
>
> See last patch for the background of the series.
>
> The series has been tested on the Galileo Gen2, to exclude regressions,
> with a firmware.cap with AND without security header and the SIMATIC
> IOT2040 which requires the header because of its mandatory secure boot.
>
> Jan
>
> Jan Kiszka (7):
>   efi/capsule: Fix return code on failing kmap/vmap
>   efi/capsule: Remove pr_debug on ENOMEM or EFAULT
>   efi/capsule: Clean up pr_err/info messages
>   efi/capsule: Adjust return type of efi_capsule_setup_info
>   efi/capsule: Prepare for loading images with security header
>   efi/capsule: Factor out overloadable efi_capsule_identify_image
>   efi/capsule: Add support for Quark security header
>
>  arch/x86/platform/efi/quirks.c        |  95 ++++++++++++++++++++++++
>  drivers/firmware/efi/capsule-loader.c | 136 +++++++++++++++++++---------------
>  drivers/firmware/efi/capsule.c        |  21 +++++-
>  include/linux/efi.h                   |  19 +++++
>  4 files changed, 208 insertions(+), 63 deletions(-)
>

BTW,

Thanks for taking the time to remove the __func__ stuff all over the place.

I'll try to test this out for you. I found that the current BSP Intel is 
releasing has some sort of GUI that downloads an image to a board (which 
completely fails for me on the Galileo I have)... not sure if you have 
different results with the stuff from the Intel website but it's 
non-functional for me :(

I'd like to suggest to you adding something to Documentation describing 
how to load and trigger a capsule update. For example on Quark you need 
to use the EFI reset method to cause capsule update to work.

Could you add a patch to your series for Documentation detailing:

1. Entry criteria (needing to boot in EFI reset mode)
1. Description of loading a capsule
3. Description of triggering the update (reboot)
4. Verifying the update succeeded (actually is this possible right now?)

---
bod

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

[Bryan O'Donoghue]

On 24/03/17 17:34, Jan Kiszka wrote:
> This addresses the review feedback provided on round 1, specifically
> - refactored queue to keep the Quark mess in
> - only check for CSH on Quark CPUs
> - added some smaller cleanups of the capsule loader
> - documented capsule header / linked to original code
>
> See last patch for the background of the series.
>
> The series has been tested on the Galileo Gen2, to exclude regressions,
> with a firmware.cap with AND without security header and the SIMATIC
> IOT2040 which requires the header because of its mandatory secure boot.
>
> Jan
>
> Jan Kiszka (7):
>   efi/capsule: Fix return code on failing kmap/vmap
>   efi/capsule: Remove pr_debug on ENOMEM or EFAULT
>   efi/capsule: Clean up pr_err/info messages
>   efi/capsule: Adjust return type of efi_capsule_setup_info
>   efi/capsule: Prepare for loading images with security header
>   efi/capsule: Factor out overloadable efi_capsule_identify_image
>   efi/capsule: Add support for Quark security header
>
>  arch/x86/platform/efi/quirks.c        |  95 ++++++++++++++++++++++++
>  drivers/firmware/efi/capsule-loader.c | 136 +++++++++++++++++++---------------
>  drivers/firmware/efi/capsule.c        |  21 +++++-
>  include/linux/efi.h                   |  19 +++++
>  4 files changed, 208 insertions(+), 63 deletions(-)
>

BTW,

Thanks for taking the time to remove the __func__ stuff all over the place.

I'll try to test this out for you. I found that the current BSP Intel is 
releasing has some sort of GUI that downloads an image to a board (which 
completely fails for me on the Galileo I have)... not sure if you have 
different results with the stuff from the Intel website but it's 
non-functional for me :(

I'd like to suggest to you adding something to Documentation describing 
how to load and trigger a capsule update. For example on Quark you need 
to use the EFI reset method to cause capsule update to work.

Could you add a patch to your series for Documentation detailing:

1. Entry criteria (needing to boot in EFI reset mode)
1. Description of loading a capsule
3. Description of triggering the update (reboot)
4. Verifying the update succeeded (actually is this possible right now?)

---
bod

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

[Jan Kiszka]

On 2017-03-27 12:29, Bryan O'Donoghue wrote:
> 
> 
> On 24/03/17 17:34, Jan Kiszka wrote:
>> This addresses the review feedback provided on round 1, specifically
>> - refactored queue to keep the Quark mess in
>> - only check for CSH on Quark CPUs
>> - added some smaller cleanups of the capsule loader
>> - documented capsule header / linked to original code
>>
>> See last patch for the background of the series.
>>
>> The series has been tested on the Galileo Gen2, to exclude regressions,
>> with a firmware.cap with AND without security header and the SIMATIC
>> IOT2040 which requires the header because of its mandatory secure boot.
>>
>> Jan
>>
>> Jan Kiszka (7):
>>   efi/capsule: Fix return code on failing kmap/vmap
>>   efi/capsule: Remove pr_debug on ENOMEM or EFAULT
>>   efi/capsule: Clean up pr_err/info messages
>>   efi/capsule: Adjust return type of efi_capsule_setup_info
>>   efi/capsule: Prepare for loading images with security header
>>   efi/capsule: Factor out overloadable efi_capsule_identify_image
>>   efi/capsule: Add support for Quark security header
>>
>>  arch/x86/platform/efi/quirks.c        |  95 ++++++++++++++++++++++++
>>  drivers/firmware/efi/capsule-loader.c | 136
>> +++++++++++++++++++---------------
>>  drivers/firmware/efi/capsule.c        |  21 +++++-
>>  include/linux/efi.h                   |  19 +++++
>>  4 files changed, 208 insertions(+), 63 deletions(-)
>>
> 
> BTW,
> 
> Thanks for taking the time to remove the __func__ stuff all over the place.
> 
> I'll try to test this out for you. I found that the current BSP Intel is
> releasing has some sort of GUI that downloads an image to a board (which
> completely fails for me on the Galileo I have)... not sure if you have
> different results with the stuff from the Intel website but it's
> non-functional for me :(

I found the Galileo capsules *.cap in the jar archives of the Galileo
firmware update packages, and they work.
> 
> I'd like to suggest to you adding something to Documentation describing
> how to load and trigger a capsule update. For example on Quark you need
> to use the EFI reset method to cause capsule update to work.

cat /path/to/capsule.cap > /dev/efi_capsule_loader

The reset method is auto-adjusted by the kernel when an update is
pending. So firmware update is now seriously simple. But I can write a 2
or 3-lines readme if it's preferred.

> 
> Could you add a patch to your series for Documentation detailing:
> 
> 1. Entry criteria (needing to boot in EFI reset mode)
> 1. Description of loading a capsule
> 3. Description of triggering the update (reboot)
> 4. Verifying the update succeeded (actually is this possible right now?)

dmidecode -> check BIOS version information (vendor-specific)

Jan

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

[Jan Kiszka]

On 2017-03-27 12:29, Bryan O'Donoghue wrote:
> 
> 
> On 24/03/17 17:34, Jan Kiszka wrote:
>> This addresses the review feedback provided on round 1, specifically
>> - refactored queue to keep the Quark mess in
>> - only check for CSH on Quark CPUs
>> - added some smaller cleanups of the capsule loader
>> - documented capsule header / linked to original code
>>
>> See last patch for the background of the series.
>>
>> The series has been tested on the Galileo Gen2, to exclude regressions,
>> with a firmware.cap with AND without security header and the SIMATIC
>> IOT2040 which requires the header because of its mandatory secure boot.
>>
>> Jan
>>
>> Jan Kiszka (7):
>>   efi/capsule: Fix return code on failing kmap/vmap
>>   efi/capsule: Remove pr_debug on ENOMEM or EFAULT
>>   efi/capsule: Clean up pr_err/info messages
>>   efi/capsule: Adjust return type of efi_capsule_setup_info
>>   efi/capsule: Prepare for loading images with security header
>>   efi/capsule: Factor out overloadable efi_capsule_identify_image
>>   efi/capsule: Add support for Quark security header
>>
>>  arch/x86/platform/efi/quirks.c        |  95 ++++++++++++++++++++++++
>>  drivers/firmware/efi/capsule-loader.c | 136
>> +++++++++++++++++++---------------
>>  drivers/firmware/efi/capsule.c        |  21 +++++-
>>  include/linux/efi.h                   |  19 +++++
>>  4 files changed, 208 insertions(+), 63 deletions(-)
>>
> 
> BTW,
> 
> Thanks for taking the time to remove the __func__ stuff all over the place.
> 
> I'll try to test this out for you. I found that the current BSP Intel is
> releasing has some sort of GUI that downloads an image to a board (which
> completely fails for me on the Galileo I have)... not sure if you have
> different results with the stuff from the Intel website but it's
> non-functional for me :(

I found the Galileo capsules *.cap in the jar archives of the Galileo
firmware update packages, and they work.
> 
> I'd like to suggest to you adding something to Documentation describing
> how to load and trigger a capsule update. For example on Quark you need
> to use the EFI reset method to cause capsule update to work.

cat /path/to/capsule.cap > /dev/efi_capsule_loader

The reset method is auto-adjusted by the kernel when an update is
pending. So firmware update is now seriously simple. But I can write a 2
or 3-lines readme if it's preferred.

> 
> Could you add a patch to your series for Documentation detailing:
> 
> 1. Entry criteria (needing to boot in EFI reset mode)
> 1. Description of loading a capsule
> 3. Description of triggering the update (reboot)
> 4. Verifying the update succeeded (actually is this possible right now?)

dmidecode -> check BIOS version information (vendor-specific)

Jan

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

[Bryan O'Donoghue]

On 27/03/17 12:01, Jan Kiszka wrote:
> The reset method is auto-adjusted by the kernel when an update is
> pending. So firmware update is now seriously simple. But I can write a 2
> or 3-lines readme if it's preferred.

Using myself as a benchmark I'd say, never assume anything is idiot 
proof to a sufficiently talented idiot, better to document it to be sure 
and give you're spinning a new patch for some comments from Andy I'd say 
it's a worthwhile addition to the set.

---
bod

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

[Bryan O'Donoghue]

On 27/03/17 12:01, Jan Kiszka wrote:
> The reset method is auto-adjusted by the kernel when an update is
> pending. So firmware update is now seriously simple. But I can write a 2
> or 3-lines readme if it's preferred.

Using myself as a benchmark I'd say, never assume anything is idiot 
proof to a sufficiently talented idiot, better to document it to be sure 
and give you're spinning a new patch for some comments from Andy I'd say 
it's a worthwhile addition to the set.

---
bod