[PATCH v6 0/2] Enable capsule loader interface for efi firmware updating

[PATCH v6 0/2] Enable capsule loader interface for efi firmware updating

[Kweh, Hock Leong]

From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>

Dear maintainers & communities,

This patchset is created on top of Matt's patchset:
1.)https://lkml.org/lkml/2014/10/7/390
"[PATCH 1/2] efi: Move efi_status_to_err() to efi.h"
2.)https://lkml.org/lkml/2014/10/7/391
"[PATCH 2/2] efi: Capsule update support"

It expose a misc char interface for user to upload the capsule binary and
calling efi_capsule_update() API to pass the binary to EFI firmware.

The steps to update efi firmware are:
1.) cat firmware.cap > /dev/efi_capsule_loader
2.) reboot

Any failed upload error message will be returned while doing "cat" through
Write() function call.

Tested the code with Intel Quark Galileo platform.

Thanks.

---
changelog v6:
* clean up on error handling for better code flow and review
* clean up on pr_err() for critical error only
* design taking care writing block that below PAGE_SIZE
* once error has occurred, design will return -EIO until file close
* document design expectations/scenarios in the code
* change the dynamic allocation cap_info struct to statically allocated

changelog v5:
* changed to new design without leveraging firmware_class API
* use misc_char device interface instead of sysfs
* error return through file Write() function call


Kweh, Hock Leong (2):
  efi: export efi_capsule_supported() function symbol
  efi: a misc char interface for user to update efi firmware

 drivers/firmware/efi/Kconfig              |   10 ++
 drivers/firmware/efi/Makefile             |    1
 drivers/firmware/efi/capsule.c            |    1
 drivers/firmware/efi/efi-capsule-loader.c |  246 +++++++++++++++++++++++++++++
 4 files changed, 258 insertions(+)
 create mode 100644 drivers/firmware/efi/efi-capsule-loader.c

-- 
1.7.9.5

[PATCH v6 1/2] efi: export efi_capsule_supported() function symbol

[Kweh, Hock Leong]

From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>

This patch export efi_capsule_supported() function symbol for capsule
kernel module to use.

Cc: Matt Fleming <matt.fleming@intel.com>
Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
---
 drivers/firmware/efi/capsule.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
index d8cd75c0..738d437 100644
--- a/drivers/firmware/efi/capsule.c
+++ b/drivers/firmware/efi/capsule.c
@@ -101,6 +101,7 @@ out:
 	kfree(capsule);
 	return rv;
 }
+EXPORT_SYMBOL_GPL(efi_capsule_supported);
 
 /**
  * efi_capsule_update - send a capsule to the firmware
-- 
1.7.9.5

Re: [PATCH v6 1/2] efi: export efi_capsule_supported() function symbol

[Borislav Petkov]

On Fri, Oct 02, 2015 at 05:05:53AM +0800, Kweh, Hock Leong wrote:
> From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>
> 
> This patch export efi_capsule_supported() function symbol for capsule
> kernel module to use.
> 
> Cc: Matt Fleming <matt.fleming@intel.com>
> Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
> ---
>  drivers/firmware/efi/capsule.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
> index d8cd75c0..738d437 100644
> --- a/drivers/firmware/efi/capsule.c
> +++ b/drivers/firmware/efi/capsule.c
> @@ -101,6 +101,7 @@ out:
>  	kfree(capsule);
>  	return rv;
>  }
> +EXPORT_SYMBOL_GPL(efi_capsule_supported);
>  
>  /**
>   * efi_capsule_update - send a capsule to the firmware

Why is this a separate patch?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

RE: [PATCH v6 1/2] efi: export efi_capsule_supported() function symbol

[Kweh, Hock Leong]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1607 bytes --]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Saturday, October 03, 2015 1:37 AM
> To: Kweh, Hock Leong
> Cc: Matt Fleming; Greg Kroah-Hartman; Ong, Boon Leong; LKML; linux-
> efi@vger.kernel.org; Sam Protsenko; Peter Jones; Andy Lutomirski; Roy
> Franz; James Bottomley; Linux FS Devel; Fleming, Matt
> Subject: Re: [PATCH v6 1/2] efi: export efi_capsule_supported() function
> symbol
> 
> On Fri, Oct 02, 2015 at 05:05:53AM +0800, Kweh, Hock Leong wrote:
> > From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>
> >
> > This patch export efi_capsule_supported() function symbol for capsule
> > kernel module to use.
> >
> > Cc: Matt Fleming <matt.fleming@intel.com>
> > Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
> > ---
> >  drivers/firmware/efi/capsule.c |    1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/firmware/efi/capsule.c
> > b/drivers/firmware/efi/capsule.c index d8cd75c0..738d437 100644
> > --- a/drivers/firmware/efi/capsule.c
> > +++ b/drivers/firmware/efi/capsule.c
> > @@ -101,6 +101,7 @@ out:
> >  	kfree(capsule);
> >  	return rv;
> >  }
> > +EXPORT_SYMBOL_GPL(efi_capsule_supported);
> >
> >  /**
> >   * efi_capsule_update - send a capsule to the firmware
> 
> Why is this a separate patch?
> 

It is because the author of this code is Matt. Submitting this,
allows him to easily squash into his patch:
https://lkml.org/lkml/2014/10/7/391

Thanks.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v6 1/2] efi: export efi_capsule_supported() function symbol

[Borislav Petkov]

On Sat, Oct 03, 2015 at 03:02:11AM +0000, Kweh, Hock Leong wrote:
> It is because the author of this code is Matt. Submitting this,
> allows him to easily squash into his patch:
> https://lkml.org/lkml/2014/10/7/391

Then you should say that in the commit message or above it or under the
"---" line that this patch will either be merged into Matt's patchset or
you should simply merge it into your 2/2 patch when submitting.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[PATCH v6 2/2] efi: a misc char interface for user to update efi firmware

[Kweh, Hock Leong]

From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>

Introducing a kernel module to expose capsule loader interface
(misc char device file note) for user to upload capsule binaries.

Example method to load the capsule binary:
cat firmware.bin > /dev/efi_capsule_loader

Cc: Matt Fleming <matt.fleming@intel.com>
Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
---
 drivers/firmware/efi/Kconfig              |   10 ++
 drivers/firmware/efi/Makefile             |    1 +
 drivers/firmware/efi/efi-capsule-loader.c |  246 +++++++++++++++++++++++++++++
 3 files changed, 257 insertions(+)
 create mode 100644 drivers/firmware/efi/efi-capsule-loader.c

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index f712d47..0be8ee3 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -60,6 +60,16 @@ config EFI_RUNTIME_WRAPPERS
 config EFI_ARMSTUB
 	bool
 
+config EFI_CAPSULE_LOADER
+	tristate "EFI capsule loader"
+	depends on EFI
+	help
+	  This option exposes a loader interface "/dev/efi_capsule_loader" for
+	  user to load EFI capsule binary and update the EFI firmware through
+	  system reboot.
+
+	  If unsure, say N.
+
 endmenu
 
 config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 698846e..5ab031a 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_UEFI_CPER)			+= cper.o
 obj-$(CONFIG_EFI_RUNTIME_MAP)		+= runtime-map.o
 obj-$(CONFIG_EFI_RUNTIME_WRAPPERS)	+= runtime-wrappers.o
 obj-$(CONFIG_EFI_STUB)			+= libstub/
+obj-$(CONFIG_EFI_CAPSULE_LOADER)	+= efi-capsule-loader.o
diff --git a/drivers/firmware/efi/efi-capsule-loader.c b/drivers/firmware/efi/efi-capsule-loader.c
new file mode 100644
index 0000000..571e0c8
--- /dev/null
+++ b/drivers/firmware/efi/efi-capsule-loader.c
@@ -0,0 +1,246 @@
+/*
+ * EFI capsule loader driver.
+ *
+ * Copyright 2015 Intel Corporation
+ *
+ * This file is part of the Linux kernel, and is made available under
+ * the terms of the GNU General Public License version 2.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/miscdevice.h>
+#include <linux/highmem.h>
+#include <linux/slab.h>
+#include <linux/mutex.h>
+#include <linux/efi.h>
+
+#define DEV_NAME "efi_capsule_loader"
+
+static struct capsule_info {
+	char		header_obtained;
+	long		index;
+	unsigned long	count;
+	unsigned long	total_size;
+	struct page	**pages;
+	unsigned long	page_count_remain;
+} cap_info;
+
+static DEFINE_MUTEX(capsule_loader_lock);
+
+/*
+ * This function will free the current & all previous allocated buffer pages.
+ * The input parameter is the allocated buffer page for current execution which
+ * has not yet assigned to pages array. The input param can be NULL if the
+ * current execution has not allocated any buffer page.
+ */
+static void efi_free_all_buff_pages(struct page *kbuff_page)
+{
+	if (!kbuff_page)
+		__free_page(kbuff_page);
+
+	if (cap_info.index > 0)
+		while (cap_info.index > 0)
+			__free_page(cap_info.pages[--cap_info.index]);
+
+	if (cap_info.header_obtained)
+		kfree(cap_info.pages);
+
+	/* indicate to the next that error had occurred */
+	cap_info.index = -2;
+}
+
+/*
+ * This function will store the capsule binary and pass it to
+ * efi_capsule_update() API in capsule.c.
+ *
+ * Expectation:
+ * - userspace tool should start at the beginning of capsule binary and pass
+ *   data in sequential.
+ * - user should close and re-open this file note in order to upload more
+ *   capsules.
+ * - any error occurred, user should close the file and restart the operation
+ *   for the next try otherwise -EIO will be returned until the file is closed.
+ */
+static ssize_t efi_capsule_write(struct file *file, const char __user *buff,
+				 size_t count, loff_t *offp)
+{
+	int ret = 0;
+	struct page *kbuff_page;
+	void *kbuff;
+	size_t write_byte;
+
+	pr_debug("%s: Entering Write()\n", __func__);
+	if (count == 0)
+		return 0;
+
+	/* return error while error had occurred or capsule uploading is done */
+	if (cap_info.index < 0)
+		return -EIO;
+
+	/* only alloc a new page when the current page is full */
+	if (!cap_info.page_count_remain) {
+		kbuff_page = alloc_page(GFP_KERNEL);
+		if (!kbuff_page) {
+			pr_debug("%s: alloc_page() failed\n", __func__);
+			efi_free_all_buff_pages(NULL);
+			return -ENOMEM;
+		}
+		cap_info.page_count_remain = PAGE_SIZE;
+	} else {
+		kbuff_page = cap_info.pages[--cap_info.index];
+	}
+	kbuff = kmap(kbuff_page);
+	if (!kbuff) {
+		pr_debug("%s: kmap() failed\n", __func__);
+		efi_free_all_buff_pages(kbuff_page);
+		return -EFAULT;
+	}
+	kbuff += PAGE_SIZE - cap_info.page_count_remain;
+
+	/* copy capsule binary data from user space to kernel space buffer */
+	write_byte = min_t(size_t, count, cap_info.page_count_remain);
+	if (copy_from_user(kbuff, buff, write_byte)) {
+		pr_debug("%s: copy_from_user() failed\n", __func__);
+		kunmap(kbuff_page);
+		efi_free_all_buff_pages(kbuff_page);
+		return -EFAULT;
+	}
+	cap_info.page_count_remain -= write_byte;
+
+	/* setup capsule binary info structure */
+	if (cap_info.header_obtained == 0 && cap_info.index == 0) {
+		efi_capsule_header_t *cap_hdr = kbuff;
+		int reset_type;
+		size_t pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >>
+					PAGE_SHIFT;
+
+		if (pages_needed == 0) {
+			pr_err("%s: pages count invalid\n", __func__);
+			kunmap(kbuff_page);
+			efi_free_all_buff_pages(kbuff_page);
+			return -EINVAL;
+		}
+
+		/* check if the capsule binary supported */
+		ret = efi_capsule_supported(cap_hdr->guid, cap_hdr->flags,
+					    cap_hdr->imagesize, &reset_type);
+		if (ret) {
+			pr_err("%s: efi_capsule_supported() failed\n",
+			       __func__);
+			kunmap(kbuff_page);
+			efi_free_all_buff_pages(kbuff_page);
+			return ret;
+		}
+
+		cap_info.total_size = cap_hdr->imagesize;
+		cap_info.pages = kmalloc_array(pages_needed, sizeof(void *),
+						GFP_KERNEL);
+		if (!cap_info.pages) {
+			pr_debug("%s: kmalloc_array() failed\n", __func__);
+			kunmap(kbuff_page);
+			efi_free_all_buff_pages(kbuff_page);
+			return -ENOMEM;
+		}
+
+		cap_info.header_obtained = 1;
+	}
+
+	cap_info.pages[cap_info.index++] = kbuff_page;
+	cap_info.count += write_byte;
+	kunmap(kbuff_page);
+
+	/* submit the full binary to efi_capsule_update() API */
+	if (cap_info.count >= cap_info.total_size) {
+		void *cap_hdr_temp;
+
+		cap_hdr_temp = kmap(cap_info.pages[0]);
+		if (!cap_hdr_temp) {
+			pr_debug("%s: kmap() failed\n", __func__);
+			efi_free_all_buff_pages(NULL);
+			return -EFAULT;
+		}
+		ret = efi_capsule_update(cap_hdr_temp, cap_info.pages);
+		kunmap(cap_info.pages[0]);
+		if (ret) {
+			pr_err("%s: efi_capsule_update() failed\n", __func__);
+			efi_free_all_buff_pages(NULL);
+			return ret;
+		}
+		/* indicate capsule binary uploading is done */
+		cap_info.index = -1;
+	}
+
+	return write_byte;
+}
+
+static int efi_capsule_release(struct inode *inode, struct file *file)
+{
+	int ret = 0;
+
+	pr_debug("%s: Exit in Release()\n", __func__);
+	/* release uncompleted capsule upload */
+	if (cap_info.index > 0) {
+		pr_err("%s: capsule upload not complete\n", __func__);
+		efi_free_all_buff_pages(NULL);
+		ret = -ECANCELED;
+	}
+	mutex_unlock(&capsule_loader_lock);
+
+	/*
+	 * we would not free the successful submitted buffer pages here due to
+	 * efi update require system reboot with data maintained.
+	 */
+	return ret;
+}
+
+static int efi_capsule_open(struct inode *inode, struct file *file)
+{
+	pr_debug("%s: Entering Open()\n", __func__);
+	if (!mutex_trylock(&capsule_loader_lock))
+		return -EBUSY;
+
+	memset(&cap_info, 0, sizeof(cap_info));
+	return 0;
+}
+
+static const struct file_operations efi_capsule_fops = {
+	.owner = THIS_MODULE,
+	.open = efi_capsule_open,
+	.write = efi_capsule_write,
+	.release = efi_capsule_release,
+	.llseek = no_llseek,
+};
+
+static struct miscdevice efi_capsule_misc = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = DEV_NAME,
+	.fops = &efi_capsule_fops,
+};
+
+static int __init efi_capsule_loader_init(void)
+{
+	int ret;
+
+	mutex_init(&capsule_loader_lock);
+
+	ret = misc_register(&efi_capsule_misc);
+	if (ret)
+		pr_err("%s: Failed to register misc char file note\n",
+		       __func__);
+
+	return ret;
+}
+module_init(efi_capsule_loader_init);
+
+static void __exit efi_capsule_loader_exit(void)
+{
+	mutex_destroy(&capsule_loader_lock);
+	misc_deregister(&efi_capsule_misc);
+}
+module_exit(efi_capsule_loader_exit);
+
+MODULE_DESCRIPTION("EFI capsule firmware binary loader");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH v6 2/2] efi: a misc char interface for user to update efi firmware

[Borislav Petkov]

On Fri, Oct 02, 2015 at 05:05:54AM +0800, Kweh, Hock Leong wrote:
> From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>
> 
> Introducing a kernel module to expose capsule loader interface
> (misc char device file note) for user to upload capsule binaries.
> 
> Example method to load the capsule binary:
> cat firmware.bin > /dev/efi_capsule_loader
> 
> Cc: Matt Fleming <matt.fleming@intel.com>
> Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
> ---
>  drivers/firmware/efi/Kconfig              |   10 ++
>  drivers/firmware/efi/Makefile             |    1 +
>  drivers/firmware/efi/efi-capsule-loader.c |  246 +++++++++++++++++++++++++++++
>  3 files changed, 257 insertions(+)
>  create mode 100644 drivers/firmware/efi/efi-capsule-loader.c
> 
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index f712d47..0be8ee3 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -60,6 +60,16 @@ config EFI_RUNTIME_WRAPPERS
>  config EFI_ARMSTUB
>  	bool
>  
> +config EFI_CAPSULE_LOADER
> +	tristate "EFI capsule loader"
> +	depends on EFI
> +	help
> +	  This option exposes a loader interface "/dev/efi_capsule_loader" for
> +	  user to load EFI capsule binary and update the EFI firmware through
> +	  system reboot.

Do you mean here: user has to cat the fw binary *and* *then* reboot the box?

If so, we probably should issue such note to dmesg after successful
write...

> +
> +	  If unsure, say N.
> +
>  endmenu
>  
>  config UEFI_CPER
> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> index 698846e..5ab031a 100644
> --- a/drivers/firmware/efi/Makefile
> +++ b/drivers/firmware/efi/Makefile
> @@ -8,3 +8,4 @@ obj-$(CONFIG_UEFI_CPER)			+= cper.o
>  obj-$(CONFIG_EFI_RUNTIME_MAP)		+= runtime-map.o
>  obj-$(CONFIG_EFI_RUNTIME_WRAPPERS)	+= runtime-wrappers.o
>  obj-$(CONFIG_EFI_STUB)			+= libstub/
> +obj-$(CONFIG_EFI_CAPSULE_LOADER)	+= efi-capsule-loader.o
> diff --git a/drivers/firmware/efi/efi-capsule-loader.c b/drivers/firmware/efi/efi-capsule-loader.c
> new file mode 100644
> index 0000000..571e0c8
> --- /dev/null
> +++ b/drivers/firmware/efi/efi-capsule-loader.c
> @@ -0,0 +1,246 @@
> +/*
> + * EFI capsule loader driver.
> + *
> + * Copyright 2015 Intel Corporation
> + *
> + * This file is part of the Linux kernel, and is made available under
> + * the terms of the GNU General Public License version 2.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/miscdevice.h>
> +#include <linux/highmem.h>
> +#include <linux/slab.h>
> +#include <linux/mutex.h>
> +#include <linux/efi.h>
> +
> +#define DEV_NAME "efi_capsule_loader"
> +
> +static struct capsule_info {
> +	char		header_obtained;
> +	long		index;
> +	unsigned long	count;
> +	unsigned long	total_size;
> +	struct page	**pages;
> +	unsigned long	page_count_remain;
> +} cap_info;
> +
> +static DEFINE_MUTEX(capsule_loader_lock);
> +
> +/*
> + * This function will free the current & all previous allocated buffer pages.

No need to start the comment with "This function" - that's implicitly the case.
Same for the other comments below.

> + * The input parameter is the allocated buffer page for current execution which
> + * has not yet assigned to pages array. The input param can be NULL if the
> + * current execution has not allocated any buffer page.

We have the standard format of documenting those:

 * @kbuff_page: the allocated buffer page...

> + */
> +static void efi_free_all_buff_pages(struct page *kbuff_page)
> +{
> +	if (!kbuff_page)
> +		__free_page(kbuff_page);
> +
> +	if (cap_info.index > 0)
> +		while (cap_info.index > 0)
> +			__free_page(cap_info.pages[--cap_info.index]);
> +
> +	if (cap_info.header_obtained)
> +		kfree(cap_info.pages);
> +
> +	/* indicate to the next that error had occurred */
> +	cap_info.index = -2;
> +}
> +
> +/*
> + * This function will store the capsule binary and pass it to
> + * efi_capsule_update() API in capsule.c.
> + *
> + * Expectation:
> + * - userspace tool should start at the beginning of capsule binary and pass
> + *   data in sequential.
> + * - user should close and re-open this file note in order to upload more
> + *   capsules.
> + * - any error occurred, user should close the file and restart the operation
> + *   for the next try otherwise -EIO will be returned until the file is closed.
> + */
> +static ssize_t efi_capsule_write(struct file *file, const char __user *buff,
> +				 size_t count, loff_t *offp)
> +{
> +	int ret = 0;
> +	struct page *kbuff_page;
> +	void *kbuff;
> +	size_t write_byte;
> +
> +	pr_debug("%s: Entering Write()\n", __func__);
> +	if (count == 0)
> +		return 0;
> +
> +	/* return error while error had occurred or capsule uploading is done */
> +	if (cap_info.index < 0)
> +		return -EIO;
> +
> +	/* only alloc a new page when the current page is full */
> +	if (!cap_info.page_count_remain) {
> +		kbuff_page = alloc_page(GFP_KERNEL);
> +		if (!kbuff_page) {
> +			pr_debug("%s: alloc_page() failed\n", __func__);
> +			efi_free_all_buff_pages(NULL);
> +			return -ENOMEM;
> +		}
> +		cap_info.page_count_remain = PAGE_SIZE;
> +	} else {
> +		kbuff_page = cap_info.pages[--cap_info.index];
> +	}
> +	kbuff = kmap(kbuff_page);
> +	if (!kbuff) {
> +		pr_debug("%s: kmap() failed\n", __func__);
> +		efi_free_all_buff_pages(kbuff_page);
> +		return -EFAULT;
> +	}
> +	kbuff += PAGE_SIZE - cap_info.page_count_remain;
> +
> +	/* copy capsule binary data from user space to kernel space buffer */
> +	write_byte = min_t(size_t, count, cap_info.page_count_remain);
> +	if (copy_from_user(kbuff, buff, write_byte)) {
> +		pr_debug("%s: copy_from_user() failed\n", __func__);
> +		kunmap(kbuff_page);
> +		efi_free_all_buff_pages(kbuff_page);
> +		return -EFAULT;
> +	}
> +	cap_info.page_count_remain -= write_byte;
> +
> +	/* setup capsule binary info structure */
> +	if (cap_info.header_obtained == 0 && cap_info.index == 0) {
> +		efi_capsule_header_t *cap_hdr = kbuff;
> +		int reset_type;
> +		size_t pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >>
> +					PAGE_SHIFT;
> +
> +		if (pages_needed == 0) {
> +			pr_err("%s: pages count invalid\n", __func__);
> +			kunmap(kbuff_page);
> +			efi_free_all_buff_pages(kbuff_page);
> +			return -EINVAL;
> +		}
> +
> +		/* check if the capsule binary supported */
> +		ret = efi_capsule_supported(cap_hdr->guid, cap_hdr->flags,
> +					    cap_hdr->imagesize, &reset_type);
> +		if (ret) {
> +			pr_err("%s: efi_capsule_supported() failed\n",
> +			       __func__);
> +			kunmap(kbuff_page);
> +			efi_free_all_buff_pages(kbuff_page);
> +			return ret;
> +		}
> +
> +		cap_info.total_size = cap_hdr->imagesize;
> +		cap_info.pages = kmalloc_array(pages_needed, sizeof(void *),
> +						GFP_KERNEL);
> +		if (!cap_info.pages) {
> +			pr_debug("%s: kmalloc_array() failed\n", __func__);
> +			kunmap(kbuff_page);
> +			efi_free_all_buff_pages(kbuff_page);
> +			return -ENOMEM;
> +		}
> +
> +		cap_info.header_obtained = 1;
> +	}
> +
> +	cap_info.pages[cap_info.index++] = kbuff_page;
> +	cap_info.count += write_byte;
> +	kunmap(kbuff_page);
> +
> +	/* submit the full binary to efi_capsule_update() API */
> +	if (cap_info.count >= cap_info.total_size) {
> +		void *cap_hdr_temp;
> +
> +		cap_hdr_temp = kmap(cap_info.pages[0]);
> +		if (!cap_hdr_temp) {
> +			pr_debug("%s: kmap() failed\n", __func__);
> +			efi_free_all_buff_pages(NULL);
> +			return -EFAULT;
> +		}
> +		ret = efi_capsule_update(cap_hdr_temp, cap_info.pages);
> +		kunmap(cap_info.pages[0]);
> +		if (ret) {
> +			pr_err("%s: efi_capsule_update() failed\n", __func__);
> +			efi_free_all_buff_pages(NULL);
> +			return ret;
> +		}
> +		/* indicate capsule binary uploading is done */
> +		cap_info.index = -1;
> +	}
> +
> +	return write_byte;
> +}

This function is huuuge and it could use some splitting. At least the
code blocks under

	/* setup capsule binary info structure */

and

	/* submit the full binary to efi_capsule_update() API */

could be separate, helper functions.

And you have the same repeated pattern on the error path:

	kunmap();
	efi_free_all_buff_pages(NULL);
	return -<errno>

Mind rewriting that with goto err_... labels like it is normally done in
the kernel.

> +
> +static int efi_capsule_release(struct inode *inode, struct file *file)
> +{
> +	int ret = 0;
> +
> +	pr_debug("%s: Exit in Release()\n", __func__);
> +	/* release uncompleted capsule upload */

That should be above the function.

> +	if (cap_info.index > 0) {
> +		pr_err("%s: capsule upload not complete\n", __func__);
> +		efi_free_all_buff_pages(NULL);
> +		ret = -ECANCELED;
> +	}
> +	mutex_unlock(&capsule_loader_lock);
> +
> +	/*
> +	 * we would not free the successful submitted buffer pages here due to
> +	 * efi update require system reboot with data maintained.
> +	 */

That too.

> +	return ret;
> +}
> +
> +static int efi_capsule_open(struct inode *inode, struct file *file)
> +{
> +	pr_debug("%s: Entering Open()\n", __func__);

Do we really need those?

> +	if (!mutex_trylock(&capsule_loader_lock))
> +		return -EBUSY;
> +
> +	memset(&cap_info, 0, sizeof(cap_info));
> +	return 0;
> +}
> +
> +static const struct file_operations efi_capsule_fops = {
> +	.owner = THIS_MODULE,
> +	.open = efi_capsule_open,
> +	.write = efi_capsule_write,
> +	.release = efi_capsule_release,
> +	.llseek = no_llseek,
> +};
> +
> +static struct miscdevice efi_capsule_misc = {
> +	.minor = MISC_DYNAMIC_MINOR,
> +	.name = DEV_NAME,
> +	.fops = &efi_capsule_fops,
> +};
> +
> +static int __init efi_capsule_loader_init(void)
> +{
> +	int ret;
> +
> +	mutex_init(&capsule_loader_lock);
> +
> +	ret = misc_register(&efi_capsule_misc);
> +	if (ret)
> +		pr_err("%s: Failed to register misc char file note\n",
> +		       __func__);

mutex_destroy() on the error path.

> +
> +	return ret;
> +}
> +module_init(efi_capsule_loader_init);
> +
> +static void __exit efi_capsule_loader_exit(void)
> +{
> +	mutex_destroy(&capsule_loader_lock);
> +	misc_deregister(&efi_capsule_misc);

Those are in the wrong order - you need to destroy the mutex last.

> +}
> +module_exit(efi_capsule_loader_exit);
> +
> +MODULE_DESCRIPTION("EFI capsule firmware binary loader");
> +MODULE_LICENSE("GPL v2");
> -- 
> 1.7.9.5
> 
> 

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH v6 2/2] efi: a misc char interface for user to update efi firmware

[Andy Lutomirski]

On Thu, Oct 1, 2015 at 2:05 PM, Kweh, Hock Leong
<hock.leong.kweh@intel.com> wrote:
> From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>
>
> Introducing a kernel module to expose capsule loader interface
> (misc char device file note) for user to upload capsule binaries.
>
> Example method to load the capsule binary:
> cat firmware.bin > /dev/efi_capsule_loader
>
> Cc: Matt Fleming <matt.fleming@intel.com>
> Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
> ---
>  drivers/firmware/efi/Kconfig              |   10 ++
>  drivers/firmware/efi/Makefile             |    1 +
>  drivers/firmware/efi/efi-capsule-loader.c |  246 +++++++++++++++++++++++++++++
>  3 files changed, 257 insertions(+)
>  create mode 100644 drivers/firmware/efi/efi-capsule-loader.c
>
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index f712d47..0be8ee3 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -60,6 +60,16 @@ config EFI_RUNTIME_WRAPPERS
>  config EFI_ARMSTUB
>         bool
>
> +config EFI_CAPSULE_LOADER
> +       tristate "EFI capsule loader"
> +       depends on EFI
> +       help
> +         This option exposes a loader interface "/dev/efi_capsule_loader" for
> +         user to load EFI capsule binary and update the EFI firmware through
> +         system reboot.
> +
> +         If unsure, say N.
> +
>  endmenu
>
>  config UEFI_CPER
> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> index 698846e..5ab031a 100644
> --- a/drivers/firmware/efi/Makefile
> +++ b/drivers/firmware/efi/Makefile
> @@ -8,3 +8,4 @@ obj-$(CONFIG_UEFI_CPER)                 += cper.o
>  obj-$(CONFIG_EFI_RUNTIME_MAP)          += runtime-map.o
>  obj-$(CONFIG_EFI_RUNTIME_WRAPPERS)     += runtime-wrappers.o
>  obj-$(CONFIG_EFI_STUB)                 += libstub/
> +obj-$(CONFIG_EFI_CAPSULE_LOADER)       += efi-capsule-loader.o
> diff --git a/drivers/firmware/efi/efi-capsule-loader.c b/drivers/firmware/efi/efi-capsule-loader.c
> new file mode 100644
> index 0000000..571e0c8
> --- /dev/null
> +++ b/drivers/firmware/efi/efi-capsule-loader.c
> @@ -0,0 +1,246 @@
> +/*
> + * EFI capsule loader driver.
> + *
> + * Copyright 2015 Intel Corporation
> + *
> + * This file is part of the Linux kernel, and is made available under
> + * the terms of the GNU General Public License version 2.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/miscdevice.h>
> +#include <linux/highmem.h>
> +#include <linux/slab.h>
> +#include <linux/mutex.h>
> +#include <linux/efi.h>
> +
> +#define DEV_NAME "efi_capsule_loader"
> +
> +static struct capsule_info {
> +       char            header_obtained;
> +       long            index;
> +       unsigned long   count;
> +       unsigned long   total_size;
> +       struct page     **pages;
> +       unsigned long   page_count_remain;
> +} cap_info;
> +

Why is this static?

> +static DEFINE_MUTEX(capsule_loader_lock);
> +
> +/*
> + * This function will free the current & all previous allocated buffer pages.
> + * The input parameter is the allocated buffer page for current execution which
> + * has not yet assigned to pages array. The input param can be NULL if the
> + * current execution has not allocated any buffer page.
> + */
> +static void efi_free_all_buff_pages(struct page *kbuff_page)
> +{
> +       if (!kbuff_page)
> +               __free_page(kbuff_page);
> +
> +       if (cap_info.index > 0)
> +               while (cap_info.index > 0)
> +                       __free_page(cap_info.pages[--cap_info.index]);
> +
> +       if (cap_info.header_obtained)
> +               kfree(cap_info.pages);
> +
> +       /* indicate to the next that error had occurred */
> +       cap_info.index = -2;

This -2 to too magical.

> +}
> +
> +/*
> + * This function will store the capsule binary and pass it to
> + * efi_capsule_update() API in capsule.c.
> + *
> + * Expectation:
> + * - userspace tool should start at the beginning of capsule binary and pass
> + *   data in sequential.
> + * - user should close and re-open this file note in order to upload more
> + *   capsules.
> + * - any error occurred, user should close the file and restart the operation
> + *   for the next try otherwise -EIO will be returned until the file is closed.
> + */
> +static ssize_t efi_capsule_write(struct file *file, const char __user *buff,
> +                                size_t count, loff_t *offp)
> +{
> +       int ret = 0;
> +       struct page *kbuff_page;
> +       void *kbuff;
> +       size_t write_byte;
> +
> +       pr_debug("%s: Entering Write()\n", __func__);

Unnecessary.

> +       if (count == 0)
> +               return 0;
> +
> +       /* return error while error had occurred or capsule uploading is done */
> +       if (cap_info.index < 0)
> +               return -EIO;
> +
> +       /* only alloc a new page when the current page is full */
> +       if (!cap_info.page_count_remain) {
> +               kbuff_page = alloc_page(GFP_KERNEL);
> +               if (!kbuff_page) {
> +                       pr_debug("%s: alloc_page() failed\n", __func__);
> +                       efi_free_all_buff_pages(NULL);
> +                       return -ENOMEM;
> +               }
> +               cap_info.page_count_remain = PAGE_SIZE;
> +       } else {
> +               kbuff_page = cap_info.pages[--cap_info.index];
> +       }
> +       kbuff = kmap(kbuff_page);
> +       if (!kbuff) {
> +               pr_debug("%s: kmap() failed\n", __func__);
> +               efi_free_all_buff_pages(kbuff_page);
> +               return -EFAULT;
> +       }
> +       kbuff += PAGE_SIZE - cap_info.page_count_remain;
> +
> +       /* copy capsule binary data from user space to kernel space buffer */
> +       write_byte = min_t(size_t, count, cap_info.page_count_remain);
> +       if (copy_from_user(kbuff, buff, write_byte)) {
> +               pr_debug("%s: copy_from_user() failed\n", __func__);
> +               kunmap(kbuff_page);
> +               efi_free_all_buff_pages(kbuff_page);
> +               return -EFAULT;
> +       }
> +       cap_info.page_count_remain -= write_byte;
> +
> +       /* setup capsule binary info structure */
> +       if (cap_info.header_obtained == 0 && cap_info.index == 0) {
> +               efi_capsule_header_t *cap_hdr = kbuff;
> +               int reset_type;
> +               size_t pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >>
> +                                       PAGE_SHIFT;
> +
> +               if (pages_needed == 0) {
> +                       pr_err("%s: pages count invalid\n", __func__);
> +                       kunmap(kbuff_page);
> +                       efi_free_all_buff_pages(kbuff_page);
> +                       return -EINVAL;
> +               }
> +
> +               /* check if the capsule binary supported */
> +               ret = efi_capsule_supported(cap_hdr->guid, cap_hdr->flags,
> +                                           cap_hdr->imagesize, &reset_type);

And what if cap_hdr isn't written yet?

> +               if (ret) {
> +                       pr_err("%s: efi_capsule_supported() failed\n",
> +                              __func__);
> +                       kunmap(kbuff_page);
> +                       efi_free_all_buff_pages(kbuff_page);
> +                       return ret;
> +               }
> +
> +               cap_info.total_size = cap_hdr->imagesize;
> +               cap_info.pages = kmalloc_array(pages_needed, sizeof(void *),
> +                                               GFP_KERNEL);
> +               if (!cap_info.pages) {
> +                       pr_debug("%s: kmalloc_array() failed\n", __func__);
> +                       kunmap(kbuff_page);
> +                       efi_free_all_buff_pages(kbuff_page);
> +                       return -ENOMEM;
> +               }
> +
> +               cap_info.header_obtained = 1;

I don't see how you know that the header is obtained.

> +       }
> +
> +       cap_info.pages[cap_info.index++] = kbuff_page;

Huh?  You might now have allocated a whole page.

> +       cap_info.count += write_byte;
> +       kunmap(kbuff_page);
> +
> +       /* submit the full binary to efi_capsule_update() API */
> +       if (cap_info.count >= cap_info.total_size) {
> +               void *cap_hdr_temp;
> +
> +               cap_hdr_temp = kmap(cap_info.pages[0]);
> +               if (!cap_hdr_temp) {
> +                       pr_debug("%s: kmap() failed\n", __func__);
> +                       efi_free_all_buff_pages(NULL);
> +                       return -EFAULT;
> +               }
> +               ret = efi_capsule_update(cap_hdr_temp, cap_info.pages);
> +               kunmap(cap_info.pages[0]);
> +               if (ret) {
> +                       pr_err("%s: efi_capsule_update() failed\n", __func__);
> +                       efi_free_all_buff_pages(NULL);
> +                       return ret;
> +               }
> +               /* indicate capsule binary uploading is done */
> +               cap_info.index = -1;

Should count > cap_info.total_size be an error?

--Andy

> +static int efi_capsule_release(struct inode *inode, struct file *file)
> +{
> +       int ret = 0;
> +
> +       pr_debug("%s: Exit in Release()\n", __func__);
> +       /* release uncompleted capsule upload */
> +       if (cap_info.index > 0) {
> +               pr_err("%s: capsule upload not complete\n", __func__);
> +               efi_free_all_buff_pages(NULL);
> +               ret = -ECANCELED;
> +       }
> +       mutex_unlock(&capsule_loader_lock);

Holding mutexes in userspace seems like an error.  In fact, doesn't
lockdep warn when you do that?

--Andy

Re: [PATCH v6 0/2] Enable capsule loader interface for efi firmware updating

[Borislav Petkov]

On Fri, Oct 02, 2015 at 05:05:52AM +0800, Kweh, Hock Leong wrote:
> From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>
> 
> Dear maintainers & communities,
> 
> This patchset is created on top of Matt's patchset:
> 1.)https://lkml.org/lkml/2014/10/7/390
> "[PATCH 1/2] efi: Move efi_status_to_err() to efi.h"
> 2.)https://lkml.org/lkml/2014/10/7/391
> "[PATCH 2/2] efi: Capsule update support"
> 
> It expose a misc char interface for user to upload the capsule binary and
> calling efi_capsule_update() API to pass the binary to EFI firmware.
> 
> The steps to update efi firmware are:
> 1.) cat firmware.cap > /dev/efi_capsule_loader
> 2.) reboot
> 
> Any failed upload error message will be returned while doing "cat" through
> Write() function call.
> 
> Tested the code with Intel Quark Galileo platform.

What does the error case look like? A standard glibc message about
write(2) failing?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

RE: [PATCH v6 0/2] Enable capsule loader interface for efi firmware updating

[Kweh, Hock Leong]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1657 bytes --]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Saturday, October 03, 2015 1:37 AM
> To: Kweh, Hock Leong
> Cc: Matt Fleming; Greg Kroah-Hartman; Ong, Boon Leong; LKML; linux-
> efi@vger.kernel.org; Sam Protsenko; Peter Jones; Andy Lutomirski; Roy
> Franz; James Bottomley; Linux FS Devel; Fleming, Matt
> Subject: Re: [PATCH v6 0/2] Enable capsule loader interface for efi firmware
> updating
> 
> On Fri, Oct 02, 2015 at 05:05:52AM +0800, Kweh, Hock Leong wrote:
> > From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>
> >
> > Dear maintainers & communities,
> >
> > This patchset is created on top of Matt's patchset:
> > 1.)https://lkml.org/lkml/2014/10/7/390
> > "[PATCH 1/2] efi: Move efi_status_to_err() to efi.h"
> > 2.)https://lkml.org/lkml/2014/10/7/391
> > "[PATCH 2/2] efi: Capsule update support"
> >
> > It expose a misc char interface for user to upload the capsule binary
> > and calling efi_capsule_update() API to pass the binary to EFI firmware.
> >
> > The steps to update efi firmware are:
> > 1.) cat firmware.cap > /dev/efi_capsule_loader
> > 2.) reboot
> >
> > Any failed upload error message will be returned while doing "cat"
> > through
> > Write() function call.
> >
> > Tested the code with Intel Quark Galileo platform.
> 
> What does the error case look like? A standard glibc message about
> write(2) failing?
> 

Any upload fail error like -ENOMEM, -EINVAL, -EIO as well as error returned
by efi_capsule_update() API.

Thanks.

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v6 0/2] Enable capsule loader interface for efi firmware updating

[Borislav Petkov]

On Sat, Oct 03, 2015 at 03:18:41AM +0000, Kweh, Hock Leong wrote:
> > What does the error case look like? A standard glibc message about
> > write(2) failing?
> > 
> 
> Any upload fail error like -ENOMEM, -EINVAL, -EIO as well as error returned
> by efi_capsule_update() API.

All I'm asking is, how does the user know that the upload didn't succeed?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

RE: [PATCH v6 0/2] Enable capsule loader interface for efi firmware updating

[Kweh, Hock Leong]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1313 bytes --]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Saturday, October 03, 2015 5:06 PM
> On Sat, Oct 03, 2015 at 03:18:41AM +0000, Kweh, Hock Leong wrote:
> > > What does the error case look like? A standard glibc message about
> > > write(2) failing?
> > >
> >
> > Any upload fail error like -ENOMEM, -EINVAL, -EIO as well as error
> > returned by efi_capsule_update() API.
> 
> All I'm asking is, how does the user know that the upload didn't succeed?
> 

I think it should depend on user app about which API they are using.
If they are using syscall then errors would be returned through write(2).
If they are using libc APIs fwrite, fputs and fprintf, then the errors would
return through those APIs. However, this design is targeting the simple
upload action "cat capsule.bin > /dev/efi_capsule_loader", so the errors
should be returned through cat() or I/O redirection mechanism from
shell terminal. Am I answered your question?

Btw, I have an out topic question: I do notice you guys wrote in the message
that a function look like write(2) or close(2). What actually the "2" mean there?

Thanks & regards,
Wilson

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v6 0/2] Enable capsule loader interface for efi firmware updating

[Andy Lutomirski]

On Mon, Oct 5, 2015 at 8:33 AM, Kweh, Hock Leong
<hock.leong.kweh@intel.com> wrote:
>> -----Original Message-----
>> From: Borislav Petkov [mailto:bp@alien8.de]
>> Sent: Saturday, October 03, 2015 5:06 PM
>> On Sat, Oct 03, 2015 at 03:18:41AM +0000, Kweh, Hock Leong wrote:
>> > > What does the error case look like? A standard glibc message about
>> > > write(2) failing?
>> > >
>> >
>> > Any upload fail error like -ENOMEM, -EINVAL, -EIO as well as error
>> > returned by efi_capsule_update() API.
>>
>> All I'm asking is, how does the user know that the upload didn't succeed?
>>
>
> I think it should depend on user app about which API they are using.
> If they are using syscall then errors would be returned through write(2).
> If they are using libc APIs fwrite, fputs and fprintf, then the errors would
> return through those APIs. However, this design is targeting the simple
> upload action "cat capsule.bin > /dev/efi_capsule_loader", so the errors
> should be returned through cat() or I/O redirection mechanism from
> shell terminal. Am I answered your question?
>
> Btw, I have an out topic question: I do notice you guys wrote in the message
> that a function look like write(2) or close(2). What actually the "2" mean there?

It's the manpage section.  Typing 'man 2 write' will give you the
manpage 'WRITE(2)'.  Section 2 is syscalls.

--Andy

Re: [PATCH v6 0/2] Enable capsule loader interface for efi firmware updating

[Andy Lutomirski]

On Oct 2, 2015 10:37 AM, "Borislav Petkov" <bp@alien8.de> wrote:
>
> On Fri, Oct 02, 2015 at 05:05:52AM +0800, Kweh, Hock Leong wrote:
> > From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>
> >
> > Dear maintainers & communities,
> >
> > This patchset is created on top of Matt's patchset:
> > 1.)https://lkml.org/lkml/2014/10/7/390
> > "[PATCH 1/2] efi: Move efi_status_to_err() to efi.h"
> > 2.)https://lkml.org/lkml/2014/10/7/391
> > "[PATCH 2/2] efi: Capsule update support"
> >
> > It expose a misc char interface for user to upload the capsule binary and
> > calling efi_capsule_update() API to pass the binary to EFI firmware.
> >
> > The steps to update efi firmware are:
> > 1.) cat firmware.cap > /dev/efi_capsule_loader
> > 2.) reboot
> >
> > Any failed upload error message will be returned while doing "cat" through
> > Write() function call.
> >
> > Tested the code with Intel Quark Galileo platform.
>
> What does the error case look like? A standard glibc message about
> write(2) failing?
>

close(2), right?

--Andy

Re: [PATCH v6 0/2] Enable capsule loader interface for efi firmware updating

[Borislav Petkov]

On Fri, Oct 02, 2015 at 09:18:05PM -0700, Andy Lutomirski wrote:
> > What does the error case look like? A standard glibc message about
> > write(2) failing?
> >
> 
> close(2), right?

I'm looking at those retvals of efi_capsule_write(). They are returned
to userspace during write(2), no?

/me has no idea how the whole plumbing actually works that's why me is
asking stupid questions.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

RE: [PATCH v6 2/2] efi: a misc char interface for user to update efi firmware

[Kweh, Hock Leong]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4589 bytes --]

> -----Original Message-----
> From: Andy Lutomirski [mailto:luto@amacapital.net]
> Sent: Sunday, October 04, 2015 7:16 AM
> > +
> > +       /* setup capsule binary info structure */
> > +       if (cap_info.header_obtained == 0 && cap_info.index == 0) {
> > +               efi_capsule_header_t *cap_hdr = kbuff;
> > +               int reset_type;
> > +               size_t pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >>
> > +                                       PAGE_SHIFT;
> > +
> > +               if (pages_needed == 0) {
> > +                       pr_err("%s: pages count invalid\n", __func__);
> > +                       kunmap(kbuff_page);
> > +                       efi_free_all_buff_pages(kbuff_page);
> > +                       return -EINVAL;
> > +               }
> > +
> > +               /* check if the capsule binary supported */
> > +               ret = efi_capsule_supported(cap_hdr->guid, cap_hdr->flags,
> > +                                           cap_hdr->imagesize,
> > + &reset_type);
> 
> And what if cap_hdr isn't written yet?

This design mainly targeting a simplest interface that user could upload efi
capsule in a single command action: cat capsule.bin > /dev/efi_capsule_loader

So, it is expected that efi capsule header is at the starting of the binary file.
Already capture this into efi_capsule_write() comment in v7 patchset: 
https://lkml.org/lkml/2015/10/5/232

If you want to enhance this module to support creating efi capsule header for
your binary, strongly believe this design can cater the implementation such as
adding ioctl to pass in efi guid, flags and so on parameters to create the header.


> 
> > +               if (ret) {
> > +                       pr_err("%s: efi_capsule_supported() failed\n",
> > +                              __func__);
> > +                       kunmap(kbuff_page);
> > +                       efi_free_all_buff_pages(kbuff_page);
> > +                       return ret;
> > +               }
> > +
> > +               cap_info.total_size = cap_hdr->imagesize;
> > +               cap_info.pages = kmalloc_array(pages_needed, sizeof(void *),
> > +                                               GFP_KERNEL);
> > +               if (!cap_info.pages) {
> > +                       pr_debug("%s: kmalloc_array() failed\n", __func__);
> > +                       kunmap(kbuff_page);
> > +                       efi_free_all_buff_pages(kbuff_page);
> > +                       return -ENOMEM;
> > +               }
> > +
> > +               cap_info.header_obtained = 1;
> 
> I don't see how you know that the header is obtained.

Capsule header is at the starting block of image binary. We can
obtain the header through the 1st block of write action. So,
user app is expected to upload the image binary sequentially.

Also captured this into efi_capsule_write() comment in v7 patchset: 
https://lkml.org/lkml/2015/10/5/232

> 
> > +       }
> > +
> > +       cap_info.pages[cap_info.index++] = kbuff_page;
> 
> Huh?  You might now have allocated a whole page.

Yes, the efi capsule header does tell the whole image size.

> 
> > +       cap_info.count += write_byte;
> > +       kunmap(kbuff_page);
> > +
> > +       /* submit the full binary to efi_capsule_update() API */
> > +       if (cap_info.count >= cap_info.total_size) {
> > +               void *cap_hdr_temp;
> > +
> > +               cap_hdr_temp = kmap(cap_info.pages[0]);
> > +               if (!cap_hdr_temp) {
> > +                       pr_debug("%s: kmap() failed\n", __func__);
> > +                       efi_free_all_buff_pages(NULL);
> > +                       return -EFAULT;
> > +               }
> > +               ret = efi_capsule_update(cap_hdr_temp, cap_info.pages);
> > +               kunmap(cap_info.pages[0]);
> > +               if (ret) {
> > +                       pr_err("%s: efi_capsule_update() failed\n", __func__);
> > +                       efi_free_all_buff_pages(NULL);
> > +                       return ret;
> > +               }
> > +               /* indicate capsule binary uploading is done */
> > +               cap_info.index = -1;
> 
> Should count > cap_info.total_size be an error?
> 
> --Andy

Yes, this is why after the write count already reaches the image size stated in
efi capsule header, an indicator will be flagged for subsequence write to be
returned -EIO as what Matt has commented.


Thanks & Regards,
Wilson

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v6 2/2] efi: a misc char interface for user to update efi firmware

[Andy Lutomirski]

On Mon, Oct 5, 2015 at 9:07 AM, Kweh, Hock Leong
<hock.leong.kweh@intel.com> wrote:
>> -----Original Message-----
>> From: Andy Lutomirski [mailto:luto@amacapital.net]
>> Sent: Sunday, October 04, 2015 7:16 AM
>> > +
>> > +       /* setup capsule binary info structure */
>> > +       if (cap_info.header_obtained == 0 && cap_info.index == 0) {
>> > +               efi_capsule_header_t *cap_hdr = kbuff;
>> > +               int reset_type;
>> > +               size_t pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >>
>> > +                                       PAGE_SHIFT;
>> > +
>> > +               if (pages_needed == 0) {
>> > +                       pr_err("%s: pages count invalid\n", __func__);
>> > +                       kunmap(kbuff_page);
>> > +                       efi_free_all_buff_pages(kbuff_page);
>> > +                       return -EINVAL;
>> > +               }
>> > +
>> > +               /* check if the capsule binary supported */
>> > +               ret = efi_capsule_supported(cap_hdr->guid, cap_hdr->flags,
>> > +                                           cap_hdr->imagesize,
>> > + &reset_type);
>>
>> And what if cap_hdr isn't written yet?
>
> This design mainly targeting a simplest interface that user could upload efi
> capsule in a single command action: cat capsule.bin > /dev/efi_capsule_loader
>
> So, it is expected that efi capsule header is at the starting of the binary file.
> Already capture this into efi_capsule_write() comment in v7 patchset:
> https://lkml.org/lkml/2015/10/5/232
>
> If you want to enhance this module to support creating efi capsule header for
> your binary, strongly believe this design can cater the implementation such as
> adding ioctl to pass in efi guid, flags and so on parameters to create the header.
>

No, that's not what I mean.  What I mean is: what if cat writes too
little in the first write call (e.g. 3 bytes).

>
>>
>> > +               if (ret) {
>> > +                       pr_err("%s: efi_capsule_supported() failed\n",
>> > +                              __func__);
>> > +                       kunmap(kbuff_page);
>> > +                       efi_free_all_buff_pages(kbuff_page);
>> > +                       return ret;
>> > +               }
>> > +
>> > +               cap_info.total_size = cap_hdr->imagesize;
>> > +               cap_info.pages = kmalloc_array(pages_needed, sizeof(void *),
>> > +                                               GFP_KERNEL);
>> > +               if (!cap_info.pages) {
>> > +                       pr_debug("%s: kmalloc_array() failed\n", __func__);
>> > +                       kunmap(kbuff_page);
>> > +                       efi_free_all_buff_pages(kbuff_page);
>> > +                       return -ENOMEM;
>> > +               }
>> > +
>> > +               cap_info.header_obtained = 1;
>>
>> I don't see how you know that the header is obtained.
>
> Capsule header is at the starting block of image binary. We can
> obtain the header through the 1st block of write action.

That's quite an assumption to make.

> So,
> user app is expected to upload the image binary sequentially.
>

>> > +       cap_info.pages[cap_info.index++] = kbuff_page;
>>
>> Huh?  You might now have allocated a whole page.
>
> Yes, the efi capsule header does tell the whole image size.

So what?  Did you allocate a page in this particular write call?  If
so, then cap_info.index++, etc is okay.  If not, it's wrong.

>> > +       cap_info.count += write_byte;
>> > +       kunmap(kbuff_page);
>> > +
>> > +       /* submit the full binary to efi_capsule_update() API */
>> > +       if (cap_info.count >= cap_info.total_size) {
>> > +               void *cap_hdr_temp;
>> > +
>> > +               cap_hdr_temp = kmap(cap_info.pages[0]);
>> > +               if (!cap_hdr_temp) {
>> > +                       pr_debug("%s: kmap() failed\n", __func__);
>> > +                       efi_free_all_buff_pages(NULL);
>> > +                       return -EFAULT;
>> > +               }
>> > +               ret = efi_capsule_update(cap_hdr_temp, cap_info.pages);
>> > +               kunmap(cap_info.pages[0]);
>> > +               if (ret) {
>> > +                       pr_err("%s: efi_capsule_update() failed\n", __func__);
>> > +                       efi_free_all_buff_pages(NULL);
>> > +                       return ret;
>> > +               }
>> > +               /* indicate capsule binary uploading is done */
>> > +               cap_info.index = -1;
>>
>> Should count > cap_info.total_size be an error?
>>
>> --Andy
>
> Yes, this is why after the write count already reaches the image size stated in
> efi capsule header, an indicator will be flagged for subsequence write to be
> returned -EIO as what Matt has commented.

What if *this very same write* writes too much data?

--Andy

RE: [PATCH v6 2/2] efi: a misc char interface for user to update efi firmware

[Kweh, Hock Leong]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3981 bytes --]

> -----Original Message-----
> From: Andy Lutomirski [mailto:luto@amacapital.net]
> Sent: Tuesday, October 06, 2015 3:06 AM
>
> >> And what if cap_hdr isn't written yet?
> >
> > This design mainly targeting a simplest interface that user could
> > upload efi capsule in a single command action: cat capsule.bin >
> > /dev/efi_capsule_loader
> >
> > So, it is expected that efi capsule header is at the starting of the binary file.
> > Already capture this into efi_capsule_write() comment in v7 patchset:
> > https://lkml.org/lkml/2015/10/5/232
> >
> > If you want to enhance this module to support creating efi capsule
> > header for your binary, strongly believe this design can cater the
> > implementation such as adding ioctl to pass in efi guid, flags and so on
> parameters to create the header.
> >
> 
> No, that's not what I mean.  What I mean is: what if cat writes too little in the
> first write call (e.g. 3 bytes).

Yes, I could add a condition checking for this:
if (write_byte < sizeof(efi_capsule_header_t) { ... }
to ensure the 1st block count does not less than the capsule header size.
If not, will return error.

Do you have any idea that in what kind of situation user app will pass in less than
28 bytes each time?

> 
> >
> >>
> >> > +               if (ret) {
> >> > +                       pr_err("%s: efi_capsule_supported() failed\n",
> >> > +                              __func__);
> >> > +                       kunmap(kbuff_page);
> >> > +                       efi_free_all_buff_pages(kbuff_page);
> >> > +                       return ret;
> >> > +               }
> >> > +
> >> > +               cap_info.total_size = cap_hdr->imagesize;
> >> > +               cap_info.pages = kmalloc_array(pages_needed, sizeof(void *),
> >> > +                                               GFP_KERNEL);
> >> > +               if (!cap_info.pages) {
> >> > +                       pr_debug("%s: kmalloc_array() failed\n", __func__);
> >> > +                       kunmap(kbuff_page);
> >> > +                       efi_free_all_buff_pages(kbuff_page);
> >> > +                       return -ENOMEM;
> >> > +               }
> >> > +
> >> > +               cap_info.header_obtained = 1;
> >>
> >> I don't see how you know that the header is obtained.
> >
> > Capsule header is at the starting block of image binary. We can obtain
> > the header through the 1st block of write action.
> 
> That's quite an assumption to make.

Answered as above.

> 
> >> > +       cap_info.pages[cap_info.index++] = kbuff_page;
> >>
> >> Huh?  You might now have allocated a whole page.
> >
> > Yes, the efi capsule header does tell the whole image size.
> 
> So what?  Did you allocate a page in this particular write call?  If so, then
> cap_info.index++, etc is okay.  If not, it's wrong.

Yes, the allocation is at:
cap_info.pages = kmalloc_array(pages_needed, sizeof(void *),
                                                                 GFP_KERNEL);
before line:
cap_info.header_obtained = 1;

> 
> >> > +               }
> >> > +               /* indicate capsule binary uploading is done */
> >> > +               cap_info.index = -1;
> >>
> >> Should count > cap_info.total_size be an error?
> >>
> >> --Andy
> >
> > Yes, this is why after the write count already reaches the image size
> > stated in efi capsule header, an indicator will be flagged for
> > subsequence write to be returned -EIO as what Matt has commented.
> 
> What if *this very same write* writes too much data?
> 

I think it is still okay as the data is still within a page and this could cater the image
binary that padding to page size. Whatever next write that more than the current
page, will return error -EIO.

If you think that should flag an error, I could simply add the condition checking
to it.


Thanks & Regards,
Wilson

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥