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

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

[Kweh, Hock Leong]

> -----Original Message-----
> From: Bryan O'Donoghue [mailto:pure.logic@nexus-software.ie]
> Sent: Tuesday, December 22, 2015 1:04 AM
> 
> Small nit.
> 

Hi, Sorry for the late response. Happy New Year.

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

Noted.

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

Answered by Borislav.

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

Noted.

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

Noted.

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

Noted.

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

Noted.

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

No. I mean handling. I will change to "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 ?
> 

This is to protect multiple instant calling open(2) and still able to synchronize
the calling to efi_capsule_supported() and efi_capsule_update() when they
are uploading the capsule concurrently.

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

Answered as above.

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

Noted.

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

Will change to use plural.

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

What I am trying to tell is that after an error happen (for e.g. -ENOMEM),
then user should close it and restart the file open, write & close operation.
If not, then only -EIO will be returned.

I do not means that any error happen, only -EIO is returned.

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

Noted.

> 
> > + **/
> > +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() ?
> 

As mentioned by Borislav, error will be propagated by -ENOMEM and there is not needed
to have double printing.

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

Taking care by ->page_bytes_remain, if ->index is zero then ->page_bytes_remain
must be zero.

> > +	}
> > +
> > +	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 ?
> 

No, at this stage, kbuff_page no longer valid and have been assigned to
->pages[].

> > +		}
> > +
> > +		ret = efi_capsule_submit_update(cap_info);
> > +		if (ret)
> > +			goto failed;
> 
> Shouldn't this be fail_freepage ?
> 

Same as above.

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

Noted.

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

Noted.

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

When you perform close(fd), efi_capsule_release() will be called and
cap_info will be freed there.


Thanks & Regards,
Wilson

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

[Kweh, Hock Leong]

> -----Original Message-----
> From: Matt Fleming [mailto:matt@console-pimps.org]
> Sent: Thursday, January 21, 2016 7:52 PM
> 
> 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.

This is to synchronize/serializes one of the instant is calling efi_capsule_supported()
and the other one is calling efi_capsule_update() which they are exported symbol
from capsule.c.

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

Just want to double check I understand you correctly. Do you mean
I should free ->pages during close(2) but not free the ->pages[X] if
there is any successfully submitted?

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

Noted.

> 
> > +	}
> > +
> > +	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];
> 
> ?

Noted.

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

Note.

Thanks & Regards,
Wilson

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

[Matt Fleming]

On Tue, 26 Jan, at 03:10:03AM, Kweh Hock Leong wrote:
> > 
> > This mutex is not needed. It doesn't protect anything in your code.
> 
> This is to synchronize/serializes one of the instant is calling efi_capsule_supported()
> and the other one is calling efi_capsule_update() which they are exported symbol
> from capsule.c.
 
You don't need to synchronise them.

Looking at my original capsule patches I can see why you might be
doing this locking, but it's unnecessary. You don't need to serialise
access to efi_capsule_supported() and efi_capsule_update().

Internally to the core EFI capsule code we need to ensure that we
don't allow capsules with conflicting reset types to be sent to the
firmware (how would we know the type of reset to perform?), but that
should be entirely transparent to your driver.  

I'm planning on re-sending my capsule patches, so all this should
become clearer.

> > 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.
> 
> Just want to double check I understand you correctly. Do you mean
> I should free ->pages during close(2) but not free the ->pages[X] if
> there is any successfully submitted?

Yes, that's correct.

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

[Kweh, Hock Leong]

> -----Original Message-----
> From: Matt Fleming [mailto:matt@codeblueprint.co.uk]
> Sent: Thursday, January 28, 2016 8:16 PM
> 
> On Tue, 26 Jan, at 03:10:03AM, Kweh Hock Leong wrote:
> > >
> > > This mutex is not needed. It doesn't protect anything in your code.
> >
> > This is to synchronize/serializes one of the instant is calling
> efi_capsule_supported()
> > and the other one is calling efi_capsule_update() which they are exported
> symbol
> > from capsule.c.
> 
> You don't need to synchronise them.
> 
> Looking at my original capsule patches I can see why you might be
> doing this locking, but it's unnecessary. You don't need to serialise
> access to efi_capsule_supported() and efi_capsule_update().
> 
> Internally to the core EFI capsule code we need to ensure that we
> don't allow capsules with conflicting reset types to be sent to the
> firmware (how would we know the type of reset to perform?), but that
> should be entirely transparent to your driver.
> 
> I'm planning on re-sending my capsule patches, so all this should
> become clearer.
> 

Ok. Noted. Will remove it and submit for v11.

> > > 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.
> >
> > Just want to double check I understand you correctly. Do you mean
> > I should free ->pages during close(2) but not free the ->pages[X] if
> > there is any successfully submitted?
> 
> Yes, that's correct.

Ok. Noted.

Thanks & Regards,
Wilson