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

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

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

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

Re: [PATCH 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]

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

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

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

Re: [PATCH 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]

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

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

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

Re: [PATCH 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]

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

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

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

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

[Kweh, Hock Leong]

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

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Wednesday, December 16, 2015 7:26 PM
> To: Kweh, Hock Leong
> Cc: Matt Fleming; Greg Kroah-Hartman; Ong, Boon Leong; LKML; linux-
> efi@vger.kernel.org; Sam Protsenko; Peter Jones; Andy Lutomirski; Roy
> Franz; James Bottomley; Linux FS Devel; Anvin, H Peter; 'Matt Fleming'
> Subject: Re: [PATCH v9 1/1] efi: a misc char interface for user to update efi
> firmware
> 
> On Wed, Dec 16, 2015 at 11:09:50AM +0000, Kweh, Hock Leong wrote:
> > So, my conclusion is that this module is not able to be tested on QEMU
> > environment.
> 
> That's not the point.
> 
> The module should better handle writing to the device file gracefully
> and not explode. Regardless of whether it is running on an EFI system or
> not.
> 
> efi_capsule_loader_init() simply loads the driver on *any* system,
> even a !UEFI one. And when I write some garbage to the device file, it
> explodes.
> 
> What it should do instead is check whether it is being loaded on en EFI
> system and whether all it needs to function properly is initialized
> already, like runtime services. If not, it should refuse to load.
> 
> --
> Regards/Gruss,
>     Boris.

Hi Borislav,

I catch your point now. I will fix that in v10 patch.

Thanks & Regards,
Wilson

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

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

[Borislav Petkov]

On Wed, Dec 16, 2015 at 11:09:50AM +0000, Kweh, Hock Leong wrote:
> So, my conclusion is that this module is not able to be tested on QEMU
> environment.

That's not the point.

The module should better handle writing to the device file gracefully
and not explode. Regardless of whether it is running on an EFI system or
not.

efi_capsule_loader_init() simply loads the driver on *any* system,
even a !UEFI one. And when I write some garbage to the device file, it
explodes.

What it should do instead is check whether it is being loaded on en EFI
system and whether all it needs to function properly is initialized
already, like runtime services. If not, it should refuse to load.

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

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

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Wednesday, November 04, 2015 4:00 AM
> 
> On Mon, Nov 02, 2015 at 06:47:29AM +0000, Kweh, Hock Leong wrote:
> > By looking at your dmesg log, the above print out message seem that
> > someone has called the flush() after the write(2). In my environment,
> flush()
> > only being called in 2 places which are before write(2) and during close(2).
> > The dmesg log seems that your environment is running write(2) and flush()
> in
> > different threads and are parallel. Could you help me to double confirm this
> and it
> > would be good if you could told me when the flush() is exactly being called
> in
> > your environment. The info really help me on debugging.
> 
> I don't know what you mean: I simply do
> 
> cat /bin/ls > /dev/efi_capsule_loader
> 
> as root in an SMP kvm guest. And it explodes. Nothing special, just this
> one command.
> 
> I guess you could try to reproduce it, here's how I start it:
> 
> qemu-system-x86_64
> -enable-kvm
> -gdb tcp::1234
> -cpu Opteron_G5
> -m 2048
> -hda /home/boris/kvm/debian/sid-x86_64.img
> -hdb /home/boris/kvm/swap.img
> -boot menu=off,order=c
> -localtime
> -net nic,model=rtl8139
> -net user,hostfwd=tcp::1235-:22
> -usbdevice tablet
> -kernel /home/boris/kernel/linux-2.6/arch/x86/boot/bzImage
> -append "root=/dev/sda1 resume=/dev/sdb1 debug ignore_loglevel
> log_buf_len=16M earlyprintk=ttyS0,115200 console=ttyS0,115200
> console=tty0"
> -monitor pty
> -virtfs local,path=/tmp,mount_tag=tmp,security_model=none
> -serial file:/home/boris/kvm/test-x86_64-1235.log
> -snapshot
> -smp 8
> 
> HTH.
> 
> --
> Regards/Gruss,
>     Boris.

Hi Borislav,

Finally able to free up 25GB space to setup a QEMU VM with Debian v8.2.0
system and look into this issue. Located the NULL pointer happened at code line:

status = efi.query_capsule_caps(&capsule, 1, &max_size, reset);

which is inside function efi_capsule_supported(). This function call is initialized by
EFI Firmware run-time service table. So, I believe the QEMU do not emulate the
EFI Firmware run-time service API calls. This is why when come to this line it hit
the NULL pointer issue.

So, my conclusion is that this module is not able to be tested on QEMU environment.

Thanks & Regards,
Wilson

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

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

[Borislav Petkov]

On Mon, Nov 02, 2015 at 06:47:29AM +0000, Kweh, Hock Leong wrote:
> By looking at your dmesg log, the above print out message seem that
> someone has called the flush() after the write(2). In my environment, flush()
> only being called in 2 places which are before write(2) and during close(2).
> The dmesg log seems that your environment is running write(2) and flush() in
> different threads and are parallel. Could you help me to double confirm this and it 
> would be good if you could told me when the flush() is exactly being called in
> your environment. The info really help me on debugging.

I don't know what you mean: I simply do

cat /bin/ls > /dev/efi_capsule_loader

as root in an SMP kvm guest. And it explodes. Nothing special, just this
one command.

I guess you could try to reproduce it, here's how I start it:

qemu-system-x86_64
-enable-kvm
-gdb tcp::1234
-cpu Opteron_G5
-m 2048
-hda /home/boris/kvm/debian/sid-x86_64.img
-hdb /home/boris/kvm/swap.img
-boot menu=off,order=c
-localtime
-net nic,model=rtl8139
-net user,hostfwd=tcp::1235-:22
-usbdevice tablet
-kernel /home/boris/kernel/linux-2.6/arch/x86/boot/bzImage
-append "root=/dev/sda1 resume=/dev/sdb1 debug ignore_loglevel log_buf_len=16M earlyprintk=ttyS0,115200 console=ttyS0,115200 console=tty0"
-monitor pty
-virtfs local,path=/tmp,mount_tag=tmp,security_model=none
-serial file:/home/boris/kvm/test-x86_64-1235.log
-snapshot
-smp 8

HTH.

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

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

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

Hi Boris,

I have tested "cat /bin/ls > /dev/efi_capsule_loader" in my environment,
but I am not able to reproduce the issue. So, it is a bit hard for me to debug
the issue with my environment and may need your help on this.

By looking at your dmesg log, the above print out message seem that
someone has called the flush() after the write(2). In my environment, flush()
only being called in 2 places which are before write(2) and during close(2).
The dmesg log seems that your environment is running write(2) and flush() in
different threads and are parallel. Could you help me to double confirm this and it 
would be good if you could told me when the flush() is exactly being called in
your environment. The info really help me on debugging.

Thanks & Regards,
Wilson

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

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