[PATCH v10 0/1] Enable capsule loader interface for efi firmware updating

[PATCH v10 0/1] 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
file operation write() function call.

Tested the code with Intel Quark Galileo GEN1 platform.

Thanks.

---
changelog v10:
* rebase to v4.4
* added efi runtime services check at efi_capsule_loader_init()
* fixed checkpatch issues
* code clean up base on Borislav's comments

changelog v9:
* squash 2 patches to become 1 patch
* change function param to pass in cap_info instead of file structure
* perform both alloc inside efi_capsule_setup_info()
* change to use multiple exit labels instead of one function call
* further code clean up base on Matt's comments

changelog v8:
* further clean up on kunmap() & efi_free_all_buff_pages()
* design enhanced to support 1st few writes are less than efi header size
* removed support to padding capsule and flag error once the upload size
  bigger than header defined size

changelog v7:
* add successful message printed in dmesg
* shorten the code in efi_capsule_write() by splitting out
  efi_capsule_setup_info() & efi_capsule_submit_update() functions
* design added capability to support multiple file open action
* re-write those comments by following standard format
* design added the "uncomplete" error return through flush() file operation

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 (1):
  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-loader.c |  355 +++++++++++++++++++++++++++++++++
 drivers/firmware/efi/capsule.c        |    1
 4 files changed, 367 insertions(+)
 create mode 100644 drivers/firmware/efi/capsule-loader.c

-- 
1.7.9.5

[PATCH v10 1/1] 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

This patch also export efi_capsule_supported() function symbol for
verifying the submitted capsule header in this kernel module.

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

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index e1670d5..dc2a912 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -87,6 +87,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. After
+	  a successful loading, a system reboot is required.
+
+	  If unsure, say N.
+
 endmenu
 
 config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index fabf801..cf9d9d3 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -18,3 +18,4 @@ obj-$(CONFIG_EFI_RUNTIME_MAP)		+= runtime-map.o
 obj-$(CONFIG_EFI_RUNTIME_WRAPPERS)	+= runtime-wrappers.o
 obj-$(CONFIG_EFI_STUB)			+= libstub/
 obj-$(CONFIG_EFI_FAKE_MEMMAP)		+= fake_mem.o
+obj-$(CONFIG_EFI_CAPSULE_LOADER)	+= capsule-loader.o
diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
new file mode 100644
index 0000000..e1214bb
--- /dev/null
+++ b/drivers/firmware/efi/capsule-loader.c
@@ -0,0 +1,355 @@
+/*
+ * 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) "EFI: " 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 NO_FURTHER_WRITE_ACTION -1
+
+struct capsule_info {
+	bool		header_obtained;
+	int		reset_type;
+	long		index;
+	size_t		count;
+	size_t		total_size;
+	struct page	**pages;
+	size_t		page_bytes_remain;
+};
+
+static DEFINE_MUTEX(capsule_loader_lock);
+
+/**
+ * efi_free_all_buff_pages - free all previous allocated buffer pages
+ * @cap_info: pointer to current instance of capsule_info structure
+ *
+ *	Besides freeing the buffer pages, it also flagged an
+ *	NO_FURTHER_WRITE_ACTION flag for indicating to the next write entries
+ *	that there is a flaw happened and any subsequence actions are not
+ *	valid unless close(2).
+ **/
+static void efi_free_all_buff_pages(struct capsule_info *cap_info)
+{
+	while (cap_info->index > 0)
+		__free_page(cap_info->pages[--cap_info->index]);
+
+	kfree(cap_info->pages);
+
+	cap_info->index = NO_FURTHER_WRITE_ACTION;
+}
+
+/**
+ * efi_capsule_setup_info - to obtain the efi capsule header in the binary and
+ *			    setup capsule_info structure
+ * @cap_info: pointer to current instance of capsule_info structure
+ * @kbuff: a mapped 1st page buffer pointer
+ * @hdr_bytes: the total bytes of received efi header
+ **/
+static ssize_t efi_capsule_setup_info(struct capsule_info *cap_info,
+				      void *kbuff, size_t hdr_bytes)
+{
+	int ret;
+	void *temp_page;
+
+	/* Handling 1st block is less than header size */
+	if (hdr_bytes < sizeof(efi_capsule_header_t)) {
+		if (!cap_info->pages) {
+			cap_info->pages = kzalloc(sizeof(void *), GFP_KERNEL);
+			if (!cap_info->pages) {
+				pr_debug("%s: kzalloc() failed\n", __func__);
+				return -ENOMEM;
+			}
+		}
+	} else {
+		/* Reset back to the correct offset of header */
+		efi_capsule_header_t *cap_hdr = kbuff - cap_info->count;
+		size_t pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >>
+					    PAGE_SHIFT;
+
+		if (pages_needed == 0) {
+			pr_err("%s: pages count invalid\n", __func__);
+			return -EINVAL;
+		}
+
+		/* Check if the capsule binary supported */
+		mutex_lock(&capsule_loader_lock);
+		ret = efi_capsule_supported(cap_hdr->guid, cap_hdr->flags,
+					    cap_hdr->imagesize,
+					    &cap_info->reset_type);
+		mutex_unlock(&capsule_loader_lock);
+		if (ret) {
+			pr_err("%s: efi_capsule_supported() failed\n",
+			       __func__);
+			return ret;
+		}
+
+		cap_info->total_size = cap_hdr->imagesize;
+		temp_page = krealloc(cap_info->pages,
+				     pages_needed * sizeof(void *),
+				     GFP_KERNEL | __GFP_ZERO);
+		if (!temp_page) {
+			pr_debug("%s: krealloc() failed\n", __func__);
+			return -ENOMEM;
+		}
+
+		cap_info->pages = temp_page;
+		cap_info->header_obtained = 1;
+	}
+
+	return 0;
+}
+
+/**
+ * efi_capsule_submit_update - invoke the efi_capsule_update API once binary
+ *			       upload done
+ * @cap_info: pointer to current instance of capsule_info structure
+ **/
+static ssize_t efi_capsule_submit_update(struct capsule_info *cap_info)
+{
+	int ret;
+	void *cap_hdr_temp;
+
+	cap_hdr_temp = kmap(cap_info->pages[0]);
+	if (!cap_hdr_temp) {
+		pr_debug("%s: kmap() failed\n", __func__);
+		return -EFAULT;
+	}
+
+	mutex_lock(&capsule_loader_lock);
+	ret = efi_capsule_update(cap_hdr_temp, cap_info->pages);
+	mutex_unlock(&capsule_loader_lock);
+	kunmap(cap_info->pages[0]);
+	if (ret) {
+		pr_err("%s: efi_capsule_update() failed\n", __func__);
+		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" :
+		cap_info->reset_type == 1 ? "RESET_WARM" :
+		"RESET_SHUTDOWN");
+	return 0;
+}
+
+/**
+ * efi_capsule_write - store the capsule binary and pass it to
+ *		       efi_capsule_update() API
+ * @file: file pointer
+ * @buff: buffer pointer
+ * @count: number of bytes in @buff
+ * @offp: not used
+ *
+ *	Expectation:
+ *	- User space 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.
+ *	- EFI capsule header must be located at the beginning of capsule binary
+ *	  file and passed in as 1st block write.
+ **/
+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 page *kbuff_page = NULL;
+	void *kbuff = NULL;
+	size_t write_byte;
+
+	if (count == 0)
+		return 0;
+
+	/* Return error while NO_FURTHER_WRITE_ACTION is flagged */
+	if (cap_info->index < 0)
+		return -EIO;
+
+	/* Only alloc a new page when previous page is full */
+	if (!cap_info->page_bytes_remain) {
+		kbuff_page = alloc_page(GFP_KERNEL);
+		if (!kbuff_page) {
+			pr_debug("%s: alloc_page() failed\n", __func__);
+			ret = -ENOMEM;
+			goto failed;
+		}
+		cap_info->page_bytes_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__);
+		ret = -EFAULT;
+		goto fail_freepage;
+	}
+	kbuff += PAGE_SIZE - cap_info->page_bytes_remain;
+
+	/* 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;
+	}
+	cap_info->page_bytes_remain -= write_byte;
+
+	/* Setup capsule binary info structure */
+	if (!cap_info->header_obtained) {
+		ret = efi_capsule_setup_info(cap_info, kbuff,
+					     cap_info->count + write_byte);
+		if (ret)
+			goto fail_unmap;
+	}
+
+	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->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__);
+			ret = -EINVAL;
+			goto failed;
+		}
+
+		ret = efi_capsule_submit_update(cap_info);
+		if (ret)
+			goto failed;
+	}
+
+	return write_byte;
+
+fail_unmap:
+	kunmap(kbuff_page);
+fail_freepage:
+	__free_page(kbuff_page);
+failed:
+	efi_free_all_buff_pages(cap_info);
+	return ret;
+}
+
+/**
+ * efi_capsule_flush - called by file close or file flush
+ * @file: file pointer
+ * @id: not used
+ *
+ *	If capsule being uploaded partially, calling this function will be
+ *	treated as uploading canceled and will free up those completed buffer
+ *	pages and then return -ECANCELED.
+ **/
+static int efi_capsule_flush(struct file *file, fl_owner_t id)
+{
+	int ret = 0;
+	struct capsule_info *cap_info = file->private_data;
+
+	if (cap_info->index > 0) {
+		pr_err("%s: capsule upload not complete\n", __func__);
+		efi_free_all_buff_pages(cap_info);
+		ret = -ECANCELED;
+	}
+
+	return ret;
+}
+
+/**
+ * efi_capsule_release - called by file close
+ * @inode: not used
+ * @file: file pointer
+ *
+ *	We would not free the successful submitted buffer pages here due to
+ *	efi update require system reboot with data maintained.
+ **/
+static int efi_capsule_release(struct inode *inode, struct file *file)
+{
+	kfree(file->private_data);
+	file->private_data = NULL;
+	return 0;
+}
+
+/**
+ * efi_capsule_open - called by file open
+ * @inode: not used
+ * @file: file pointer
+ *
+ *	Will allocate each 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;
+
+	cap_info = kzalloc(sizeof(*cap_info), GFP_KERNEL);
+	if (!cap_info)
+		return -ENOMEM;
+
+	file->private_data = cap_info;
+
+	return 0;
+}
+
+static const struct file_operations efi_capsule_fops = {
+	.owner = THIS_MODULE,
+	.open = efi_capsule_open,
+	.write = efi_capsule_write,
+	.flush = efi_capsule_flush,
+	.release = efi_capsule_release,
+	.llseek = no_llseek,
+};
+
+static struct miscdevice efi_capsule_misc = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = "efi_capsule_loader",
+	.fops = &efi_capsule_fops,
+};
+
+static int __init efi_capsule_loader_init(void)
+{
+	int ret;
+
+	if (!efi_enabled(EFI_RUNTIME_SERVICES))
+		return -ENODEV;
+
+	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(&capsule_loader_lock);
+	}
+
+	return ret;
+}
+module_init(efi_capsule_loader_init);
+
+static void __exit efi_capsule_loader_exit(void)
+{
+	misc_deregister(&efi_capsule_misc);
+	mutex_destroy(&capsule_loader_lock);
+}
+module_exit(efi_capsule_loader_exit);
+
+MODULE_DESCRIPTION("EFI capsule firmware binary loader");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
index 475643d..476a960 100644
--- a/drivers/firmware/efi/capsule.c
+++ b/drivers/firmware/efi/capsule.c
@@ -102,6 +102,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 v10 1/1] efi: a misc char interface for user to update efi firmware

[Bryan O'Donoghue]

Small nit.

On Fri, 2015-12-18 at 20:13 +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.

This patch ? introduces a kernel module to expose a capsule loader
interface... for a user ...

> 
> Example method to load the capsule binary:

Example:

> cat firmware.bin > /dev/efi_capsule_loader
> 
> This patch also export efi_capsule_supported() function symbol for
> verifying the submitted capsule header in this kernel module.

1. Should be a separate patch 
2. Suggested, rewording for that patch log

2/2 Title
This patch exports efi_capsule_supported to enable verification of the
capsule header.

That said - who is the user of this symbol ? Why is it needed ? Anyway
please consider splitting and rewording.

> 
> Cc: Matt Fleming <matt@codeblueprint.co.uk>
> Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
> ---
>  drivers/firmware/efi/Kconfig          |   10
>  drivers/firmware/efi/Makefile         |    1
>  drivers/firmware/efi/capsule-loader.c |  355
> +++++++++++++++++++++++++++++++++
>  drivers/firmware/efi/capsule.c        |    1
>  4 files changed, 367 insertions(+)
>  create mode 100644 drivers/firmware/efi/capsule-loader.c
> 
> diff --git a/drivers/firmware/efi/Kconfig
> b/drivers/firmware/efi/Kconfig
> index e1670d5..dc2a912 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -87,6 +87,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. After
> +	  a successful loading, a system reboot is required.
> +
> +	  If unsure, say N.
> +

This option exposes a loader interface... for a user to load an EFI
capsule.

A capsule isn't necessarily exclusively for updating of firmware either
AFAIK - so I suggest dropping. 

http://www.intel.com/content/www/us/en/architecture-and-technology/unif
ied-extensible-firmware-interface/efi-capsule-specification.html

Quote "Capsules were designed for and are intended to be the major
vehicle for delivering firmware volume changes. There is no requirement
that capsules be limited to that function."

>  endmenu
>  
>  config UEFI_CPER
> diff --git a/drivers/firmware/efi/Makefile
> b/drivers/firmware/efi/Makefile
> index fabf801..cf9d9d3 100644
> --- a/drivers/firmware/efi/Makefile
> +++ b/drivers/firmware/efi/Makefile
> @@ -18,3 +18,4 @@ obj-$(CONFIG_EFI_RUNTIME_MAP)		+=
> runtime-map.o
>  obj-$(CONFIG_EFI_RUNTIME_WRAPPERS)	+= runtime-wrappers.o
>  obj-$(CONFIG_EFI_STUB)			+= libstub/
>  obj-$(CONFIG_EFI_FAKE_MEMMAP)		+= fake_mem.o
> +obj-$(CONFIG_EFI_CAPSULE_LOADER)	+= capsule-loader.o
> diff --git a/drivers/firmware/efi/capsule-loader.c
> b/drivers/firmware/efi/capsule-loader.c
> new file mode 100644
> index 0000000..e1214bb
> --- /dev/null
> +++ b/drivers/firmware/efi/capsule-loader.c
> @@ -0,0 +1,355 @@
> +/*
> + * 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) "EFI: " 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 NO_FURTHER_WRITE_ACTION -1
> +
> +struct capsule_info {
> +	bool		header_obtained;
> +	int		reset_type;
> +	long		index;
> +	size_t		count;
> +	size_t		total_size;
> +	struct page	**pages;
> +	size_t		page_bytes_remain;
> +};
> +
> +static DEFINE_MUTEX(capsule_loader_lock);
> +
> +/**
> + * efi_free_all_buff_pages - free all previous allocated buffer
> pages
> + * @cap_info: pointer to current instance of capsule_info structure
> + *
> + *	Besides freeing the buffer pages, it also flagged an
> + *	NO_FURTHER_WRITE_ACTION flag for indicating to the next
> write entries
> + *	that there is a flaw happened and any subsequence actions
> are not
> + *	valid unless close(2).
> + **/

The description needs to be reworded.

In addition to freeing buffer pages, we flag NO_FURTHER_WRITE_ACTION on
error and will cease to process data in subsequent efi_capsule_write()
calls.

(or similar)

> +static void efi_free_all_buff_pages(struct capsule_info *cap_info)
> +{
> +	while (cap_info->index > 0)
> +		__free_page(cap_info->pages[--cap_info->index]);
> +
> +	kfree(cap_info->pages);
> +
> +	cap_info->index = NO_FURTHER_WRITE_ACTION;
> +}
> +
> +/**
> + * efi_capsule_setup_info - to obtain the efi capsule header in the
> binary and
> + *			    setup capsule_info structure
> + * @cap_info: pointer to current instance of capsule_info structure
> + * @kbuff: a mapped 1st page buffer pointer

first not 1st

> + * @hdr_bytes: the total bytes of received efi header

the total number of received bytes of the EFI header

> + **/
> +static ssize_t efi_capsule_setup_info(struct capsule_info *cap_info,
> +				      void *kbuff, size_t hdr_bytes)
> +{
> +	int ret;
> +	void *temp_page;
> +
> +	/* Handling 1st block is less than header size */
first or initial
I think you mean to say "Ensure first 

> +
> +		/* Check if the capsule binary supported */
> +		mutex_lock(&capsule_loader_lock);
> +		ret = efi_capsule_supported(cap_hdr->guid, cap_hdr
> ->flags,
> +					    cap_hdr->imagesize,
> +					    &cap_info->reset_type);
> +		mutex_unlock(&capsule_loader_lock);

What does this mutex really do ? You hold it here around
efi_capsule_supported() in the call

chain efi_capsule_write() => efi_capsule_setup_info()

and then again inside of efi_capsule_submit_update() the call chain 

chain efi_capsule_write() => efi_capsule_submit_update()

So if its valid to ensure you are synchronizing parallel contexts with
-respect to calls into efi_capsule_supported() and
efi_capsule_submit_update() why aren't you holding that lock for the
duration of efi_capsule_write() - couldn't cap_info->page_bytes_remain
as an example equally be racy ?


> +
> +/**
> + * efi_capsule_submit_update - invoke the efi_capsule_update API
> once binary
> + *			       upload done
> + * @cap_info: pointer to current instance of capsule_info structure
> + **/
> +static ssize_t efi_capsule_submit_update(struct capsule_info
> *cap_info)
> +{
> +	int ret;
> +	void *cap_hdr_temp;
> +
> +	cap_hdr_temp = kmap(cap_info->pages[0]);
> +	if (!cap_hdr_temp) {
> +		pr_debug("%s: kmap() failed\n", __func__);
> +		return -EFAULT;
> +	}
> +
> +	mutex_lock(&capsule_loader_lock);
> +	ret = efi_capsule_update(cap_hdr_temp, cap_info->pages);
> +	mutex_unlock(&capsule_loader_lock);
> +	kunmap(cap_info->pages[0]);
> +	if (ret) {
> +		pr_err("%s: efi_capsule_update() failed\n",
> __func__);
> +		return ret;
> +	}
> +
> +	/* Indicate capsule binary uploading is done */
> +	cap_info->index = NO_FURTHER_WRITE_ACTION;

Again - if you need a mutex for efi_capsule_update() why don't you need
a mutex for cap_info->index updates looks racy...

+}
> +
> +/**
> + * efi_capsule_write - store the capsule binary and pass it to
> + *		       efi_capsule_update() API
> + * @file: file pointer
> + * @buff: buffer pointer
> + * @count: number of bytes in @buff
> + * @offp: not used
> + *
> + *	Expectation:
> + *	- User space tool should start at the beginning of capsule
> binary and
> + *	  pass data in sequential.

A user-space tool.. sequentially

> + *	- User should close and re-open this file note in order to
> upload more
> + *	  capsules.

A user should..

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

On error -EIO will be returned and capsule loading will be abandoned.


> + *	- EFI capsule header must be located at the beginning of
> capsule binary
> + *	  file and passed in as 1st block write.

An EFI capsule header.... in as the first data in the first write
operation


> + **/
> +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 page *kbuff_page = NULL;
> +	void *kbuff = NULL;
> +	size_t write_byte;
> +
> +	if (count == 0)
> +		return 0;
> +
> +	/* Return error while NO_FURTHER_WRITE_ACTION is flagged */
> +	if (cap_info->index < 0)
> +		return -EIO;
> +
> +	/* Only alloc a new page when previous page is full */
> +	if (!cap_info->page_bytes_remain) {
> +		kbuff_page = alloc_page(GFP_KERNEL);
> +		if (!kbuff_page) {
> +			pr_debug("%s: alloc_page() failed\n",
> __func__);

Shouldn't this be a pr_err() ?

> +			ret = -ENOMEM;
> +			goto failed;
> +		}
> +		cap_info->page_bytes_remain = PAGE_SIZE;
> +	} else {
> +		kbuff_page = cap_info->pages[--cap_info->index];

How are you guaranteeing this index doesn't go negative ? You compare
index < 0 above so the pre-decrement could be negative ... right ?

> +	}
> +
> +	kbuff = kmap(kbuff_page);
> +	if (!kbuff) {
> +		pr_debug("%s: kmap() failed\n", __func__);

and here ?

> +		ret = -EFAULT;
> +		goto fail_freepage;
> +	}
> +	kbuff += PAGE_SIZE - cap_info->page_bytes_remain;
> +
> +	/* 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__);

ditto

> +		ret = -EFAULT;
> +		goto fail_unmap;
> +	}
> +	cap_info->page_bytes_remain -= write_byte;
> +
> +	/* Setup capsule binary info structure */
> +	if (!cap_info->header_obtained) {
> +		ret = efi_capsule_setup_info(cap_info, kbuff,
> +					     cap_info->count +
> write_byte);
> +		if (ret)
> +			goto fail_unmap;
> +	}
> +
> +	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->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__);
> +			ret = -EINVAL;
> +			goto failed;

Shouldn't this be fail_freepage ?

> +		}
> +
> +		ret = efi_capsule_submit_update(cap_info);
> +		if (ret)
> +			goto failed;

Shouldn't this be fail_freepage ?

> +	}
> +
> +	return write_byte;
> +
> +fail_unmap:
> +	kunmap(kbuff_page);
> +fail_freepage:
> +	__free_page(kbuff_page);
> +failed:
> +	efi_free_all_buff_pages(cap_info);
> +	return ret;
> +}
> +
> +/**
> + * efi_capsule_flush - called by file close or file flush
> + * @file: file pointer
> + * @id: not used
> + *
> + *	If capsule being uploaded partially, calling this function
> will be
> + *	treated as uploading canceled and will free up those



> completed buffer
> + *	pages and then return -ECANCELED.

If a capsule is being partially updated then calling this function will
be treated as upload termination and will free completed buffer pages;
in this case -ECANCELLED will be returned.


> + **/
> +static int efi_capsule_flush(struct file *file, fl_owner_t id)
> +{
> +	int ret = 0;
> +	struct capsule_info *cap_info = file->private_data;
> +
> +	if (cap_info->index > 0) {
> +		pr_err("%s: capsule upload not complete\n",
> __func__);
> +		efi_free_all_buff_pages(cap_info);
> +		ret = -ECANCELED;
> +	}
> +
> +	return ret;
> +}
> +
> +/**
> + * efi_capsule_release - called by file close
> + * @inode: not used
> + * @file: file pointer
> + *
> + *	We would not free the successful submitted buffer pages
> here due to
> + *	efi update require system reboot with data maintained.
> + **/

We will not free successfully submitted pages since EFI update requires
data to be maintained across boot. 


> +static int efi_capsule_open(struct inode *inode, struct file *file)
> +{
> +	struct capsule_info *cap_info;
> +
> +	cap_info = kzalloc(sizeof(*cap_info), GFP_KERNEL);
> +	if (!cap_info)
> +		return -ENOMEM;
> +
> +	file->private_data = cap_info;
> +
> +	return 0;
> +}

You have a memory leak here don't you ?

if I do 
for (i = 0; i < N; i++) {
	fd = open(/dev/your_node);
	close(fd);
}

You'll leak that kzalloc...

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

[Borislav Petkov]

On Mon, Dec 21, 2015 at 05:04:11PM +0000, Bryan O'Donoghue wrote:
> > This patch also export efi_capsule_supported() function symbol for
> > verifying the submitted capsule header in this kernel module.
> 
> 1. Should be a separate patch 
> 2. Suggested, rewording for that patch log
> 
> 2/2 Title
> This patch exports efi_capsule_supported to enable verification of the
> capsule header.
> 
> That said - who is the user of this symbol ? Why is it needed ? Anyway
> please consider splitting and rewording.

Huh?

I still don't really see the reasoning for splitting out the export.

When you do the export and use it in a single patch it is *obvious* why
it is being exported.

And there's no need to mention in the commit message that you're
exporting a symbol. People export symbols all the time.

-- 
Regards/Gruss,
    Boris.

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

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

[Bryan O'Donoghue]

On Mon, 2015-12-21 at 18:37 +0100, Borislav Petkov wrote:
> On Mon, Dec 21, 2015 at 05:04:11PM +0000, Bryan O'Donoghue wrote:
> > > This patch also export efi_capsule_supported() function symbol
> > > for
> > > verifying the submitted capsule header in this kernel module.
> > 
> > 1. Should be a separate patch 
> > 2. Suggested, rewording for that patch log
> > 
> > 2/2 Title
> > This patch exports efi_capsule_supported to enable verification of
> > the
> > capsule header.
> > 
> > That said - who is the user of this symbol ? Why is it needed ?
> > Anyway
> > please consider splitting and rewording.
> 
> Huh?
> 
> I still don't really see the reasoning for splitting out the export.
> 
> When you do the export and use it in a single patch it is *obvious*
> why
> it is being exported.
> 
> And there's no need to mention in the commit message that you're
> exporting a symbol. People export symbols all the time.
> 

Yes - saw the export down the end of the patchset. Feel free to ignore
that comment.

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

[Matt Fleming]

On Fri, 18 Dec, at 08:13:01PM, 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
> 
> This patch also export efi_capsule_supported() function symbol for
> verifying the submitted capsule header in this kernel module.
> 
> Cc: Matt Fleming <matt@codeblueprint.co.uk>
> Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
> ---
>  drivers/firmware/efi/Kconfig          |   10
>  drivers/firmware/efi/Makefile         |    1
>  drivers/firmware/efi/capsule-loader.c |  355 +++++++++++++++++++++++++++++++++
>  drivers/firmware/efi/capsule.c        |    1
>  4 files changed, 367 insertions(+)
>  create mode 100644 drivers/firmware/efi/capsule-loader.c

[...]

> +#define pr_fmt(fmt) "EFI: " 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 NO_FURTHER_WRITE_ACTION -1
> +
> +struct capsule_info {
> +	bool		header_obtained;
> +	int		reset_type;
> +	long		index;
> +	size_t		count;
> +	size_t		total_size;
> +	struct page	**pages;
> +	size_t		page_bytes_remain;
> +};
> +
> +static DEFINE_MUTEX(capsule_loader_lock);

This mutex is not needed. It doesn't protect anything in your code.

> +/**
> + * efi_free_all_buff_pages - free all previous allocated buffer pages
> + * @cap_info: pointer to current instance of capsule_info structure
> + *
> + *	Besides freeing the buffer pages, it also flagged an
> + *	NO_FURTHER_WRITE_ACTION flag for indicating to the next write entries
> + *	that there is a flaw happened and any subsequence actions are not
> + *	valid unless close(2).
> + **/
> +static void efi_free_all_buff_pages(struct capsule_info *cap_info)
> +{
> +	while (cap_info->index > 0)
> +		__free_page(cap_info->pages[--cap_info->index]);
> +
> +	kfree(cap_info->pages);
> +
> +	cap_info->index = NO_FURTHER_WRITE_ACTION;
> +}
> +
> +/**
> + * efi_capsule_setup_info - to obtain the efi capsule header in the binary and
> + *			    setup capsule_info structure
> + * @cap_info: pointer to current instance of capsule_info structure
> + * @kbuff: a mapped 1st page buffer pointer
> + * @hdr_bytes: the total bytes of received efi header
> + **/
> +static ssize_t efi_capsule_setup_info(struct capsule_info *cap_info,
> +				      void *kbuff, size_t hdr_bytes)
> +{
> +	int ret;
> +	void *temp_page;
> +
> +	/* Handling 1st block is less than header size */
> +	if (hdr_bytes < sizeof(efi_capsule_header_t)) {
> +		if (!cap_info->pages) {
> +			cap_info->pages = kzalloc(sizeof(void *), GFP_KERNEL);
> +			if (!cap_info->pages) {
> +				pr_debug("%s: kzalloc() failed\n", __func__);
> +				return -ENOMEM;
> +			}
> +		}

This function would be much more simple if you handled the above
condition differently.

Instead of having code in efi_capsule_setup_info() to allocate the
initial ->pages memory, why not just allocate it in efi_capsule_open()
at the same time as you allocate the private_data? You can then
free it in efi_capsule_release() (you're leaking it at the moment).

You are always going to need at least one element in ->pages for
successful operation, so you might as well allocate it up front.

> +	} else {
> +		/* Reset back to the correct offset of header */
> +		efi_capsule_header_t *cap_hdr = kbuff - cap_info->count;
> +		size_t pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >>
> +					    PAGE_SHIFT;
> +
> +		if (pages_needed == 0) {
> +			pr_err("%s: pages count invalid\n", __func__);
> +			return -EINVAL;
> +		}
> +
> +		/* Check if the capsule binary supported */
> +		mutex_lock(&capsule_loader_lock);
> +		ret = efi_capsule_supported(cap_hdr->guid, cap_hdr->flags,
> +					    cap_hdr->imagesize,
> +					    &cap_info->reset_type);
> +		mutex_unlock(&capsule_loader_lock);
> +		if (ret) {
> +			pr_err("%s: efi_capsule_supported() failed\n",
> +			       __func__);
> +			return ret;
> +		}
> +
> +		cap_info->total_size = cap_hdr->imagesize;
> +		temp_page = krealloc(cap_info->pages,
> +				     pages_needed * sizeof(void *),
> +				     GFP_KERNEL | __GFP_ZERO);
> +		if (!temp_page) {
> +			pr_debug("%s: krealloc() failed\n", __func__);
> +			return -ENOMEM;
> +		}
> +
> +		cap_info->pages = temp_page;
> +		cap_info->header_obtained = 1;

This should be 'true', not 1.

> +	}
> +
> +	return 0;
> +}
>
> +
> +/**
> + * efi_capsule_submit_update - invoke the efi_capsule_update API once binary
> + *			       upload done
> + * @cap_info: pointer to current instance of capsule_info structure
> + **/
> +static ssize_t efi_capsule_submit_update(struct capsule_info *cap_info)
> +{
> +	int ret;
> +	void *cap_hdr_temp;
> +
> +	cap_hdr_temp = kmap(cap_info->pages[0]);
> +	if (!cap_hdr_temp) {
> +		pr_debug("%s: kmap() failed\n", __func__);
> +		return -EFAULT;
> +	}
> +
> +	mutex_lock(&capsule_loader_lock);
> +	ret = efi_capsule_update(cap_hdr_temp, cap_info->pages);
> +	mutex_unlock(&capsule_loader_lock);
> +	kunmap(cap_info->pages[0]);
> +	if (ret) {
> +		pr_err("%s: efi_capsule_update() failed\n", __func__);
> +		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" :
> +		cap_info->reset_type == 1 ? "RESET_WARM" :
> +		"RESET_SHUTDOWN");
> +	return 0;
> +}
> +
> +/**
> + * efi_capsule_write - store the capsule binary and pass it to
> + *		       efi_capsule_update() API
> + * @file: file pointer
> + * @buff: buffer pointer
> + * @count: number of bytes in @buff
> + * @offp: not used
> + *
> + *	Expectation:
> + *	- User space 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.
> + *	- EFI capsule header must be located at the beginning of capsule binary
> + *	  file and passed in as 1st block write.
> + **/
> +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 page *kbuff_page = NULL;
> +	void *kbuff = NULL;
> +	size_t write_byte;
> +
> +	if (count == 0)
> +		return 0;
> +
> +	/* Return error while NO_FURTHER_WRITE_ACTION is flagged */
> +	if (cap_info->index < 0)
> +		return -EIO;
> +
> +	/* Only alloc a new page when previous page is full */
> +	if (!cap_info->page_bytes_remain) {
> +		kbuff_page = alloc_page(GFP_KERNEL);
> +		if (!kbuff_page) {
> +			pr_debug("%s: alloc_page() failed\n", __func__);
> +			ret = -ENOMEM;
> +			goto failed;
> +		}
> +		cap_info->page_bytes_remain = PAGE_SIZE;
> +	} else {
> +		kbuff_page = cap_info->pages[--cap_info->index];
> +	}

This shuffling and unshuffling of cap_info->index seems kinda crazy to
me because you're treating the current page differently from the rest,
which complicates the error paths too (you shouldn't need
__free_page() *and* efi_free_all_buff_pages()).

Why not make cap_info->index always index the last page? That way you
could do,

	if (!cap_info->page_bytes_remain) {
		cap_info->pages[++cap_info->index] = alloc_page()
		cap_info->page_bytes_remain = PAGE_SIZE;
	}

	kbuff_page = cap_info->pages[cap_info->index];

?

> +
> +	kbuff = kmap(kbuff_page);
> +	if (!kbuff) {
> +		pr_debug("%s: kmap() failed\n", __func__);
> +		ret = -EFAULT;
> +		goto fail_freepage;
> +	}
> +	kbuff += PAGE_SIZE - cap_info->page_bytes_remain;
> +
> +	/* 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;
> +	}
> +	cap_info->page_bytes_remain -= write_byte;
> +
> +	/* Setup capsule binary info structure */
> +	if (!cap_info->header_obtained) {
> +		ret = efi_capsule_setup_info(cap_info, kbuff,
> +					     cap_info->count + write_byte);
> +		if (ret)
> +			goto fail_unmap;
> +	}
> +
> +	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->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__);
> +			ret = -EINVAL;
> +			goto failed;
> +		}
> +
> +		ret = efi_capsule_submit_update(cap_info);
> +		if (ret)
> +			goto failed;
> +	}
> +
> +	return write_byte;
> +
> +fail_unmap:
> +	kunmap(kbuff_page);
> +fail_freepage:
> +	__free_page(kbuff_page);
> +failed:
> +	efi_free_all_buff_pages(cap_info);
> +	return ret;
> +}
> +
> +/**
> + * efi_capsule_flush - called by file close or file flush
> + * @file: file pointer
> + * @id: not used
> + *
> + *	If capsule being uploaded partially, calling this function will be
> + *	treated as uploading canceled and will free up those completed buffer
> + *	pages and then return -ECANCELED.
> + **/
> +static int efi_capsule_flush(struct file *file, fl_owner_t id)
> +{
> +	int ret = 0;
> +	struct capsule_info *cap_info = file->private_data;
> +
> +	if (cap_info->index > 0) {
> +		pr_err("%s: capsule upload not complete\n", __func__);
> +		efi_free_all_buff_pages(cap_info);
> +		ret = -ECANCELED;
> +	}
> +
> +	return ret;
> +}
> +
> +/**
> + * efi_capsule_release - called by file close
> + * @inode: not used
> + * @file: file pointer
> + *
> + *	We would not free the successful submitted buffer pages here due to
> + *	efi update require system reboot with data maintained.
> + **/
> +static int efi_capsule_release(struct inode *inode, struct file *file)
> +{
> +	kfree(file->private_data);
> +	file->private_data = NULL;
> +	return 0;
> +}
> +
> +/**
> + * efi_capsule_open - called by file open
> + * @inode: not used
> + * @file: file pointer
> + *
> + *	Will allocate each 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;
> +
> +	cap_info = kzalloc(sizeof(*cap_info), GFP_KERNEL);
> +	if (!cap_info)
> +		return -ENOMEM;
> +
> +	file->private_data = cap_info;
> +

Please allocate one ->pages element here (void *). You need at least
one for this code to work and you can easily free it in
efi_capsule_release() if it still exists. It would also simplfy
efi_capsule_setup_info() immensely.

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

[Matt Fleming]

On Mon, 21 Dec, at 05:04:11PM, Bryan O'Donoghue wrote:
> > +static int efi_capsule_open(struct inode *inode, struct file *file)
> > +{
> > +	struct capsule_info *cap_info;
> > +
> > +	cap_info = kzalloc(sizeof(*cap_info), GFP_KERNEL);
> > +	if (!cap_info)
> > +		return -ENOMEM;
> > +
> > +	file->private_data = cap_info;
> > +
> > +	return 0;
> > +}
> 
> You have a memory leak here don't you ?
> 
> if I do 
> for (i = 0; i < N; i++) {
> 	fd = open(/dev/your_node);
> 	close(fd);
> }
> 
> You'll leak that kzalloc...

Nope, it gets freed in efi_capsule_release(). Though the code does
leak cap_info->pages if a capsule is successfully submitted to the
firmware.