From: "Kweh, Hock Leong" <[email protected]>
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
From: "Kweh, Hock Leong" <[email protected]>
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 <[email protected]>
Signed-off-by: Kweh, Hock Leong <[email protected]>
---
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
Small nit.
On Fri, 2015-12-18 at 20:13 +0800, Kweh, Hock Leong wrote:
> From: "Kweh, Hock Leong" <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Kweh, Hock Leong <[email protected]>
> ---
> 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...
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.
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.