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

[PATCH v9 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 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.c            |    1
 drivers/firmware/efi/efi-capsule-loader.c |  356 +++++++++++++++++++++++++++++
 4 files changed, 368 insertions(+)
 create mode 100644 drivers/firmware/efi/efi-capsule-loader.c

-- 
1.7.9.5

[PATCH v9 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.fleming@intel.com>
Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
---
 drivers/firmware/efi/Kconfig              |   10
 drivers/firmware/efi/Makefile             |    1
 drivers/firmware/efi/capsule.c            |    1
 drivers/firmware/efi/efi-capsule-loader.c |  356 +++++++++++++++++++++++++++++
 4 files changed, 368 insertions(+)
 create mode 100644 drivers/firmware/efi/efi-capsule-loader.c

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index f712d47..0be8ee3 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -60,6 +60,16 @@ config EFI_RUNTIME_WRAPPERS
 config EFI_ARMSTUB
 	bool
 
+config EFI_CAPSULE_LOADER
+	tristate "EFI capsule loader"
+	depends on EFI
+	help
+	  This option exposes a loader interface "/dev/efi_capsule_loader" for
+	  user to load EFI capsule binary and update the EFI firmware through
+	  system reboot.
+
+	  If unsure, say N.
+
 endmenu
 
 config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 698846e..5ab031a 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_UEFI_CPER)			+= cper.o
 obj-$(CONFIG_EFI_RUNTIME_MAP)		+= runtime-map.o
 obj-$(CONFIG_EFI_RUNTIME_WRAPPERS)	+= runtime-wrappers.o
 obj-$(CONFIG_EFI_STUB)			+= libstub/
+obj-$(CONFIG_EFI_CAPSULE_LOADER)	+= efi-capsule-loader.o
diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
index d8cd75c0..738d437 100644
--- a/drivers/firmware/efi/capsule.c
+++ b/drivers/firmware/efi/capsule.c
@@ -101,6 +101,7 @@ out:
 	kfree(capsule);
 	return rv;
 }
+EXPORT_SYMBOL_GPL(efi_capsule_supported);
 
 /**
  * efi_capsule_update - send a capsule to the firmware
diff --git a/drivers/firmware/efi/efi-capsule-loader.c b/drivers/firmware/efi/efi-capsule-loader.c
new file mode 100644
index 0000000..23f7618
--- /dev/null
+++ b/drivers/firmware/efi/efi-capsule-loader.c
@@ -0,0 +1,356 @@
+/*
+ * EFI capsule loader driver.
+ *
+ * Copyright 2015 Intel Corporation
+ *
+ * This file is part of the Linux kernel, and is made available under
+ * the terms of the GNU General Public License version 2.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/miscdevice.h>
+#include <linux/highmem.h>
+#include <linux/slab.h>
+#include <linux/mutex.h>
+#include <linux/efi.h>
+
+#define DEV_NAME "efi_capsule_loader"
+#define UPLOAD_DONE -1
+#define ERR_OCCURED -2
+
+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 ERR_OCCURED
+ *	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);
+
+	/* Indicate to the next that error had occurred */
+	cap_info->index = ERR_OCCURED;
+}
+
+/**
+ * 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 = UPLOAD_DONE;
+	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 error had occurred or capsule uploading is done */
+	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) {
+		pr_debug("%s: kzalloc() failed\n", __func__);
+		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 = DEV_NAME,
+	.fops = &efi_capsule_fops,
+};
+
+static int __init efi_capsule_loader_init(void)
+{
+	int ret;
+
+	mutex_init(&capsule_loader_lock);
+
+	ret = misc_register(&efi_capsule_misc);
+	if (ret) {
+		pr_err("%s: Failed to register misc char file note\n",
+		       __func__);
+		mutex_destroy(&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");
-- 
1.7.9.5

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

[Borislav Petkov]

On Thu, Oct 29, 2015 at 01:58:57AM +0800, Kweh, Hock Leong wrote:
> From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>
> 
> Introducing a kernel module to expose capsule loader interface
> (misc char device file note) for user to upload capsule binaries.
> 
> Example method to load the capsule binary:
> cat firmware.bin > /dev/efi_capsule_loader

$ cat "some_dumb_file" > /dev/efi_capsule_loader
Killed

and in dmesg:

[   34.033982] efi_capsule_loader: efi_capsule_flush: capsule upload not complete
[   58.765683] ------------[ cut here ]------------
[   58.769349] WARNING: CPU: 5 PID: 3904 at drivers/firmware/efi/capsule.c:83 efi_capsule_supported+0x103/0x150()
[   58.775063] Modules linked in:
[   58.776474] CPU: 5 PID: 3904 Comm: cat Not tainted 4.3.0-rc7+ #3
[   58.779044] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
[   58.783387]  ffffffff81957aa0 ffff880079793d78 ffffffff812cb2ea 0000000000000000
[   58.786749]  ffff880079793db0 ffffffff81055981 00010102464c457f 0000000000000000
[   58.790140]  0000000000401e3b 0000000000000001 ffff880078660704 ffff880079793dc0
[   58.793353] Call Trace:
[   58.794343]  [<ffffffff812cb2ea>] dump_stack+0x4e/0x84
[   58.796416]  [<ffffffff81055981>] warn_slowpath_common+0x91/0xd0
[   58.798773]  [<ffffffff81055a7a>] warn_slowpath_null+0x1a/0x20
[   58.800962]  [<ffffffff8157ae93>] efi_capsule_supported+0x103/0x150
[   58.803292]  [<ffffffff8157d559>] efi_capsule_write+0x269/0x390
[   58.805563]  [<ffffffff81183ef8>] __vfs_write+0x28/0xe0
[   58.807591]  [<ffffffff81183e9a>] ? __vfs_read+0xaa/0xe0
[   58.809612]  [<ffffffff811847d5>] vfs_write+0xb5/0x1a0
[   58.811272]  [<ffffffff811a33be>] ? __fget_light+0x6e/0x90
[   58.813073]  [<ffffffff81185412>] SyS_write+0x52/0xc0
[   58.814720]  [<ffffffff816cff5b>] entry_SYSCALL_64_fastpath+0x16/0x73
[   58.816665] ---[ end trace 94c0c141f9b0ec01 ]---
[   58.818179] BUG: unable to handle kernel NULL pointer dereference at           (null)
[   58.820427] IP: [<          (null)>]           (null)
[   58.820630] PGD 79af8067 PUD 79781067 PMD 0 
[   58.820630] Oops: 0010 [#1] PREEMPT SMP 
[   58.820630] Modules linked in:
[   58.820630] CPU: 5 PID: 3904 Comm: cat Tainted: G        W       4.3.0-rc7+ #3
[   58.820630] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
[   58.820630] task: ffff8800771417c0 ti: ffff880079790000 task.ti: ffff880079790000
[   58.820630] RIP: 0010:[<0000000000000000>]  [<          (null)>]           (null)
[   58.820630] RSP: 0018:ffff880079793dc8  EFLAGS: 00010282
[   58.820630] RAX: ffff88007a01b4e0 RBX: 00010102464c457f RCX: ffff880078660704
[   58.820630] RDX: ffff880079793dd8 RSI: 0000000000000001 RDI: ffff880079793dd0
[   58.820630] RBP: ffff880079793e08 R08: 0000000000000000 R09: 0000000000000000
[   58.820630] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
[   58.820630] R13: 0000000000401e3b R14: 0000000000000001 R15: ffff880078660704
[   58.820630] FS:  00007ffff7fe1700(0000) GS:ffff88007c000000(0000) knlGS:0000000000000000
[   58.820630] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[   58.820630] CR2: 0000000000000000 CR3: 000000007ab90000 CR4: 00000000000406e0
[   58.820630] Stack:
[   58.820630]  ffffffff8157ae24 ffff88007a01b4e0 0000000000000002 ffff880078660700
[   58.820630]  ffff880077060000 0000000000001000 ffffea0001dc1800 ffff880077060000
[   58.820630]  ffff880079793e48 ffffffff8157d559 0000000000000402 ffff8800799cbc00
[   58.820630] Call Trace:
[   58.820630]  [<ffffffff8157ae24>] ? efi_capsule_supported+0x94/0x150
[   58.820630]  [<ffffffff8157d559>] efi_capsule_write+0x269/0x390
[   58.820630]  [<ffffffff81183ef8>] __vfs_write+0x28/0xe0
[   58.820630]  [<ffffffff81183e9a>] ? __vfs_read+0xaa/0xe0
[   58.820630]  [<ffffffff811847d5>] vfs_write+0xb5/0x1a0
[   58.820630]  [<ffffffff811a33be>] ? __fget_light+0x6e/0x90
[   58.820630]  [<ffffffff81185412>] SyS_write+0x52/0xc0
[   58.820630]  [<ffffffff816cff5b>] entry_SYSCALL_64_fastpath+0x16/0x73
[   58.820630] Code:  Bad RIP value.
[   58.820630] RIP  [<          (null)>]           (null)
[   58.820630]  RSP <ffff880079793dc8>
[   58.820630] CR2: 0000000000000000
[   58.876221] ---[ end trace 94c0c141f9b0ec02 ]---

> This patch also export efi_capsule_supported() function symbol for
> verifying the submitted capsule header in this kernel module.
> 
> Cc: Matt Fleming <matt.fleming@intel.com>
> Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
> ---
>  drivers/firmware/efi/Kconfig              |   10
>  drivers/firmware/efi/Makefile             |    1
>  drivers/firmware/efi/capsule.c            |    1
>  drivers/firmware/efi/efi-capsule-loader.c |  356 +++++++++++++++++++++++++++++
>  4 files changed, 368 insertions(+)
>  create mode 100644 drivers/firmware/efi/efi-capsule-loader.c

Please integrate checkpatch into your workflow - it can be helpful
sometimes:

WARNING: 'OCCURED' may be misspelled - perhaps 'OCCURRED'?
#114: FILE: drivers/firmware/efi/efi-capsule-loader.c:22:
+#define ERR_OCCURED -2

WARNING: 'OCCURED' may be misspelled - perhaps 'OCCURRED'?
#132: FILE: drivers/firmware/efi/efi-capsule-loader.c:40:
+ *     Besides freeing the buffer pages, it also flagged an ERR_OCCURED

WARNING: 'OCCURED' may be misspelled - perhaps 'OCCURRED'?
#144: FILE: drivers/firmware/efi/efi-capsule-loader.c:52:
+       cap_info->index = ERR_OCCURED;

WARNING: Possible unnecessary 'out of memory' message
#399: FILE: drivers/firmware/efi/efi-capsule-loader.c:307:
+       if (!cap_info) {
+               pr_debug("%s: kzalloc() failed\n", __func__);


> 
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index f712d47..0be8ee3 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -60,6 +60,16 @@ config EFI_RUNTIME_WRAPPERS
>  config EFI_ARMSTUB
>  	bool
>  
> +config EFI_CAPSULE_LOADER
> +	tristate "EFI capsule loader"
> +	depends on EFI
> +	help
> +	  This option exposes a loader interface "/dev/efi_capsule_loader" for
> +	  user to load EFI capsule binary and update the EFI firmware through
> +	  system reboot.

Make this:

	... 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 698846e..5ab031a 100644
> --- a/drivers/firmware/efi/Makefile
> +++ b/drivers/firmware/efi/Makefile
> @@ -8,3 +8,4 @@ obj-$(CONFIG_UEFI_CPER)			+= cper.o
>  obj-$(CONFIG_EFI_RUNTIME_MAP)		+= runtime-map.o
>  obj-$(CONFIG_EFI_RUNTIME_WRAPPERS)	+= runtime-wrappers.o
>  obj-$(CONFIG_EFI_STUB)			+= libstub/
> +obj-$(CONFIG_EFI_CAPSULE_LOADER)	+= efi-capsule-loader.o
> diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
> index d8cd75c0..738d437 100644
> --- a/drivers/firmware/efi/capsule.c
> +++ b/drivers/firmware/efi/capsule.c
> @@ -101,6 +101,7 @@ out:
>  	kfree(capsule);
>  	return rv;
>  }
> +EXPORT_SYMBOL_GPL(efi_capsule_supported);
>  
>  /**
>   * efi_capsule_update - send a capsule to the firmware
> diff --git a/drivers/firmware/efi/efi-capsule-loader.c b/drivers/firmware/efi/efi-capsule-loader.c

All those files under drivers/firmware/efi/ are EFI stuff so this one
doesn't need the "efi-" name prefix either. efi-pstore.c I'm looking at
you too.

> new file mode 100644
> index 0000000..23f7618
> --- /dev/null
> +++ b/drivers/firmware/efi/efi-capsule-loader.c
> @@ -0,0 +1,356 @@
> +/*
> + * EFI capsule loader driver.
> + *
> + * Copyright 2015 Intel Corporation
> + *
> + * This file is part of the Linux kernel, and is made available under
> + * the terms of the GNU General Public License version 2.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

I think this should be

#define pr_fmt(fmt) "EFI: " fmt

or something that all EFI code uses.

> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/miscdevice.h>
> +#include <linux/highmem.h>
> +#include <linux/slab.h>
> +#include <linux/mutex.h>
> +#include <linux/efi.h>
> +
> +#define DEV_NAME "efi_capsule_loader"

Why a define if it is used only in one place? Just put the string there
instead.

> +#define UPLOAD_DONE -1

Isn't the fact that upload was finished a success message? If so, why is
it a negative value?

> +#define ERR_OCCURED -2

WARNING: 'OCCURED' may be misspelled - perhaps 'OCCURRED'?
#114: FILE: drivers/firmware/efi/efi-capsule-loader.c:22:
+#define ERR_OCCURED -2

Ok, that should be enough review for now - I'll take a look at the rest
once you've taken care of the splat above and those minor issues I
pointed out.

Thanks.

-- 
Regards/Gruss,
    Boris.

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

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

[Kweh, Hock Leong]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Sunday, November 01, 2015 6:30 PM
> >
> > Example method to load the capsule binary:
> > cat firmware.bin > /dev/efi_capsule_loader
> 
> $ cat "some_dumb_file" > /dev/efi_capsule_loader Killed
> 
> and in dmesg:
> 
> [   34.033982] efi_capsule_loader: efi_capsule_flush: capsule upload not
> complete
> [   58.765683] ------------[ cut here ]------------
> [   58.769349] WARNING: CPU: 5 PID: 3904 at
> drivers/firmware/efi/capsule.c:83 efi_capsule_supported+0x103/0x150()
> [   58.775063] Modules linked in:
> [   58.776474] CPU: 5 PID: 3904 Comm: cat Not tainted 4.3.0-rc7+ #3
> [   58.779044] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> 1.7.5-20140531_083030-gandalf 04/01/2014
> [   58.783387]  ffffffff81957aa0 ffff880079793d78 ffffffff812cb2ea
> 0000000000000000
> [   58.786749]  ffff880079793db0 ffffffff81055981 00010102464c457f
> 0000000000000000
> [   58.790140]  0000000000401e3b 0000000000000001 ffff880078660704
> ffff880079793dc0
> [   58.793353] Call Trace:
> [   58.794343]  [<ffffffff812cb2ea>] dump_stack+0x4e/0x84
> [   58.796416]  [<ffffffff81055981>] warn_slowpath_common+0x91/0xd0
> [   58.798773]  [<ffffffff81055a7a>] warn_slowpath_null+0x1a/0x20
> [   58.800962]  [<ffffffff8157ae93>] efi_capsule_supported+0x103/0x150
> [   58.803292]  [<ffffffff8157d559>] efi_capsule_write+0x269/0x390
> [   58.805563]  [<ffffffff81183ef8>] __vfs_write+0x28/0xe0
> [   58.807591]  [<ffffffff81183e9a>] ? __vfs_read+0xaa/0xe0
> [   58.809612]  [<ffffffff811847d5>] vfs_write+0xb5/0x1a0
> [   58.811272]  [<ffffffff811a33be>] ? __fget_light+0x6e/0x90
> [   58.813073]  [<ffffffff81185412>] SyS_write+0x52/0xc0
> [   58.814720]  [<ffffffff816cff5b>] entry_SYSCALL_64_fastpath+0x16/0x73
> [   58.816665] ---[ end trace 94c0c141f9b0ec01 ]---
> [   58.818179] BUG: unable to handle kernel NULL pointer dereference at
> (null)
> [   58.820427] IP: [<          (null)>]           (null)
> [   58.820630] PGD 79af8067 PUD 79781067 PMD 0
> [   58.820630] Oops: 0010 [#1] PREEMPT SMP
> [   58.820630] Modules linked in:
> [   58.820630] CPU: 5 PID: 3904 Comm: cat Tainted: G        W       4.3.0-rc7+ #3
> [   58.820630] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> 1.7.5-20140531_083030-gandalf 04/01/2014
> [   58.820630] task: ffff8800771417c0 ti: ffff880079790000 task.ti:
> ffff880079790000
> [   58.820630] RIP: 0010:[<0000000000000000>]  [<          (null)>]           (null)
> [   58.820630] RSP: 0018:ffff880079793dc8  EFLAGS: 00010282
> [   58.820630] RAX: ffff88007a01b4e0 RBX: 00010102464c457f RCX:
> ffff880078660704
> [   58.820630] RDX: ffff880079793dd8 RSI: 0000000000000001 RDI:
> ffff880079793dd0
> [   58.820630] RBP: ffff880079793e08 R08: 0000000000000000 R09:
> 0000000000000000
> [   58.820630] R10: 0000000000000000 R11: 0000000000000001 R12:
> 0000000000000000
> [   58.820630] R13: 0000000000401e3b R14: 0000000000000001 R15:
> ffff880078660704
> [   58.820630] FS:  00007ffff7fe1700(0000) GS:ffff88007c000000(0000)
> knlGS:0000000000000000
> [   58.820630] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> [   58.820630] CR2: 0000000000000000 CR3: 000000007ab90000 CR4:
> 00000000000406e0
> [   58.820630] Stack:
> [   58.820630]  ffffffff8157ae24 ffff88007a01b4e0 0000000000000002
> ffff880078660700
> [   58.820630]  ffff880077060000 0000000000001000 ffffea0001dc1800
> ffff880077060000
> [   58.820630]  ffff880079793e48 ffffffff8157d559 0000000000000402
> ffff8800799cbc00
> [   58.820630] Call Trace:
> [   58.820630]  [<ffffffff8157ae24>] ? efi_capsule_supported+0x94/0x150
> [   58.820630]  [<ffffffff8157d559>] efi_capsule_write+0x269/0x390
> [   58.820630]  [<ffffffff81183ef8>] __vfs_write+0x28/0xe0
> [   58.820630]  [<ffffffff81183e9a>] ? __vfs_read+0xaa/0xe0
> [   58.820630]  [<ffffffff811847d5>] vfs_write+0xb5/0x1a0
> [   58.820630]  [<ffffffff811a33be>] ? __fget_light+0x6e/0x90
> [   58.820630]  [<ffffffff81185412>] SyS_write+0x52/0xc0
> [   58.820630]  [<ffffffff816cff5b>] entry_SYSCALL_64_fastpath+0x16/0x73
> [   58.820630] Code:  Bad RIP value.
> [   58.820630] RIP  [<          (null)>]           (null)
> [   58.820630]  RSP <ffff880079793dc8>
> [   58.820630] CR2: 0000000000000000
> [   58.876221] ---[ end trace 94c0c141f9b0ec02 ]---
> 

Could you share me your dumb file? I did perform negative test, but I did
not get these dump stack in dmesg. Thanks.

> 
> Please integrate checkpatch into your workflow - it can be helpful
> sometimes:
> 
> WARNING: 'OCCURED' may be misspelled - perhaps 'OCCURRED'?
> #114: FILE: drivers/firmware/efi/efi-capsule-loader.c:22:
> +#define ERR_OCCURED -2
> 
> WARNING: 'OCCURED' may be misspelled - perhaps 'OCCURRED'?
> #132: FILE: drivers/firmware/efi/efi-capsule-loader.c:40:
> + *     Besides freeing the buffer pages, it also flagged an ERR_OCCURED
> 
> WARNING: 'OCCURED' may be misspelled - perhaps 'OCCURRED'?
> #144: FILE: drivers/firmware/efi/efi-capsule-loader.c:52:
> +       cap_info->index = ERR_OCCURED;
> 
> WARNING: Possible unnecessary 'out of memory' message
> #399: FILE: drivers/firmware/efi/efi-capsule-loader.c:307:
> +       if (!cap_info) {
> +               pr_debug("%s: kzalloc() failed\n", __func__);
> 

I did use checkpatch. May be my version is different. Will get the
latest version and run it again. Thanks.

> > +config EFI_CAPSULE_LOADER
> > +	tristate "EFI capsule loader"
> > +	depends on EFI
> > +	help
> > +	  This option exposes a loader interface "/dev/efi_capsule_loader"
> for
> > +	  user to load EFI capsule binary and update the EFI firmware through
> > +	  system reboot.
> 
> Make this:
> 
> 	... and update the EFI firmware. After a successful loading, a system
> 	reboot is required."
> 

Ok. Noted.

> >
> >  /**
> >   * efi_capsule_update - send a capsule to the firmware diff --git
> > a/drivers/firmware/efi/efi-capsule-loader.c
> > b/drivers/firmware/efi/efi-capsule-loader.c
> 
> All those files under drivers/firmware/efi/ are EFI stuff so this one doesn't
> need the "efi-" name prefix either. efi-pstore.c I'm looking at you too.
> 

Ok. Noted.

> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> 
> I think this should be
> 
> #define pr_fmt(fmt) "EFI: " fmt
> 
> or something that all EFI code uses.
> 

Ok. Noted.

> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/highmem.h>
> > +#include <linux/slab.h>
> > +#include <linux/mutex.h>
> > +#include <linux/efi.h>
> > +
> > +#define DEV_NAME "efi_capsule_loader"
> 
> Why a define if it is used only in one place? Just put the string there instead.

Ok. Noted.

> 
> > +#define UPLOAD_DONE -1
> 
> Isn't the fact that upload was finished a success message? If so, why is it a
> negative value?

This is to indicate an upload is done and pending for close(2). If a subsequence
write(2) perform, return error. Comments inputted by Matt and Andy.

> 
> > +#define ERR_OCCURED -2
> 
> WARNING: 'OCCURED' may be misspelled - perhaps 'OCCURRED'?
> #114: FILE: drivers/firmware/efi/efi-capsule-loader.c:22:
> +#define ERR_OCCURED -2
> 
> Ok, that should be enough review for now - I'll take a look at the rest once
> you've taken care of the splat above and those minor issues I pointed out.
> 
> Thanks.
> 

Thanks for the review.

Regards,
Wilson

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

[Borislav Petkov]

On Sun, Nov 01, 2015 at 10:52:52AM +0000, Kweh, Hock Leong wrote:
> Could you share me your dumb file? I did perform negative test, but I did
> not get these dump stack in dmesg. Thanks.

I think almost any file works:

cat /bin/ls > /dev/efi_capsule_loader

> > > +#define UPLOAD_DONE -1
> > 
> > Isn't the fact that upload was finished a success message? If so, why is it a
> > negative value?
> 
> This is to indicate an upload is done and pending for close(2). If a subsequence
> write(2) perform, return error. Comments inputted by Matt and Andy.

But in that case you can return ERR_OCCURRED. UPLOAD_DONE still doesn't
look like a negative value to me as it signals that the upload was done
and thus successful as no errors happened during the upload.

-- 
Regards/Gruss,
    Boris.

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

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

[Kweh, Hock Leong]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Sunday, November 01, 2015 6:58 PM
> 
> On Sun, Nov 01, 2015 at 10:52:52AM +0000, Kweh, Hock Leong wrote:
> > Could you share me your dumb file? I did perform negative test, but I did
> > not get these dump stack in dmesg. Thanks.
> 
> I think almost any file works:
> 
> cat /bin/ls > /dev/efi_capsule_loader

Ok. Will try this out.

> 
> > > > +#define UPLOAD_DONE -1
> > >
> > > Isn't the fact that upload was finished a success message? If so, why is it a
> > > negative value?
> >
> > This is to indicate an upload is done and pending for close(2). If a
> subsequence
> > write(2) perform, return error. Comments inputted by Matt and Andy.
> 
> But in that case you can return ERR_OCCURRED. UPLOAD_DONE still doesn't
> look like a negative value to me as it signals that the upload was done
> and thus successful as no errors happened during the upload.
> 

Hmm .... If I combine these 2 flags to become one as "NO_MORE_WRITE_ACTION"
to better describing the situation, you Okay with it?

Regards,
Wilson

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

[Borislav Petkov]

On Sun, Nov 01, 2015 at 11:11:23AM +0000, Kweh, Hock Leong wrote:
> Hmm .... If I combine these 2 flags to become one as
> "NO_MORE_WRITE_ACTION" to better describing the situation, you Okay
> with it?

I don't understand, why combine?

Why not simply make UPLOAD_DONE a positive value:

#define UPLOAD_DONE	1
#define ERR_OCCURRED	-1

0 would obviously mean, no errors occurred whatsoever.

-- 
Regards/Gruss,
    Boris.

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

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

[Kweh, Hock Leong]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Sunday, November 01, 2015 8:59 PM
> 
> On Sun, Nov 01, 2015 at 11:11:23AM +0000, Kweh, Hock Leong wrote:
> > Hmm .... If I combine these 2 flags to become one as
> > "NO_MORE_WRITE_ACTION" to better describing the situation, you Okay
> > with it?
> 
> I don't understand, why combine?
> 
> Why not simply make UPLOAD_DONE a positive value:
> 
> #define UPLOAD_DONE	1
> #define ERR_OCCURRED	-1
> 
> 0 would obviously mean, no errors occurred whatsoever.
> 

Hi Boris,

This is not a return value to indicate what is going now. It is a flag used in
"cap_info->index" which positive value has a meaning of index number.
I am using the negative value for the flag which similar to the implementation
of pointer & error pointer (ERR_PTR).

Thanks & Regards,
Wilson

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

[Borislav Petkov]

On Mon, Nov 02, 2015 at 07:17:28AM +0000, Kweh, Hock Leong wrote:
> This is not a return value to indicate what is going now. It is a flag
> used in "cap_info->index" which positive value has a meaning of index
> number. I am using the negative value for the flag which similar to
> the implementation of pointer & error pointer (ERR_PTR).

Ok, but that doesn't make any sense: you're assigning UPLOAD_DONE to
cap_info->index only once in efi_capsule_submit_update() and you're not
testing it anywhere. Yeah, yeah, you're implicitly testing for it by
doing the "< 0" check.

So simply assign -1 to ->index to mean *any* type of error occurred,
remove the defines and you can always test for "< 0" to mean "did
something fail".

You simply don't need two error values...

-- 
Regards/Gruss,
    Boris.

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

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

[Kweh, Hock Leong]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Wednesday, November 04, 2015 4:15 AM
> 
> On Mon, Nov 02, 2015 at 07:17:28AM +0000, Kweh, Hock Leong wrote:
> > This is not a return value to indicate what is going now. It is a flag
> > used in "cap_info->index" which positive value has a meaning of index
> > number. I am using the negative value for the flag which similar to
> > the implementation of pointer & error pointer (ERR_PTR).
> 
> Ok, but that doesn't make any sense: you're assigning UPLOAD_DONE to
> cap_info->index only once in efi_capsule_submit_update() and you're not
> testing it anywhere. Yeah, yeah, you're implicitly testing for it by
> doing the "< 0" check.
> 
> So simply assign -1 to ->index to mean *any* type of error occurred,
> remove the defines and you can always test for "< 0" to mean "did
> something fail".
> 
> You simply don't need two error values...
> 

Ok. Noted.

Thanks & Regards,
Wilson