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
Write() function call.
Tested the code with Intel Quark Galileo platform.
Thanks.
---
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 (2):
efi: export efi_capsule_supported() function symbol
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 | 218 +++++++++++++++++++++++++++++
4 files changed, 230 insertions(+)
create mode 100644 drivers/firmware/efi/efi-capsule-loader.c
--
1.7.9.5
From: "Kweh, Hock Leong" <[email protected]>
This patch export efi_capsule_supported() function symbol for capsule
kernel module to use.
Cc: Matt Fleming <[email protected]>
Signed-off-by: Kweh, Hock Leong <[email protected]>
---
drivers/firmware/efi/capsule.c | 1 +
1 file changed, 1 insertion(+)
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
--
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
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/efi-capsule-loader.c | 218 +++++++++++++++++++++++++++++
3 files changed, 229 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/efi-capsule-loader.c b/drivers/firmware/efi/efi-capsule-loader.c
new file mode 100644
index 0000000..1da8608
--- /dev/null
+++ b/drivers/firmware/efi/efi-capsule-loader.c
@@ -0,0 +1,218 @@
+/*
+ * 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"
+
+struct capsule_info {
+ int curr_index;
+ int curr_size;
+ int total_size;
+ struct page **pages;
+};
+
+static DEFINE_MUTEX(capsule_loader_lock);
+
+/*
+ * This function will store the capsule binary and pass it to
+ * efi_capsule_update() API in capsule.c
+ */
+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;
+ void *kbuff;
+
+ pr_debug("%s: Entering Write()\n", __func__);
+ if (count == 0)
+ return 0;
+
+ if (cap_info->curr_index == -1)
+ return count;
+
+ kbuff_page = alloc_page(GFP_KERNEL);
+ if (!kbuff_page) {
+ pr_err("%s: alloc_page() failed\n", __func__);
+ if (!cap_info->curr_index)
+ return -ENOMEM;
+ ret = -ENOMEM;
+ goto failed;
+ }
+
+ kbuff = kmap(kbuff_page);
+ if (!kbuff) {
+ pr_err("%s: kmap() failed\n", __func__);
+ if (cap_info->curr_index)
+ cap_info->pages[cap_info->curr_index++] = kbuff_page;
+ ret = -EFAULT;
+ goto failed;
+ }
+
+ /* copy capsule binary data from user space to kernel space buffer */
+ if (copy_from_user(kbuff, buff, min_t(size_t, count, PAGE_SIZE))) {
+ pr_err("%s: copy_from_user() failed\n", __func__);
+ if (cap_info->curr_index)
+ cap_info->pages[cap_info->curr_index++] = kbuff_page;
+ kunmap(kbuff_page);
+ ret = -EFAULT;
+ goto failed;
+ }
+
+ /* setup capsule binary info structure */
+ if (cap_info->curr_index == 0) {
+ efi_capsule_header_t *cap_hdr = kbuff;
+ int reset_type;
+ int pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >>
+ PAGE_SHIFT;
+
+ if (pages_needed <= 0) {
+ pr_err("%s: pages count invalid\n", __func__);
+ ret = -EINVAL;
+ kunmap(kbuff_page);
+ goto failed;
+ }
+
+ /* check if the capsule binary supported */
+ ret = efi_capsule_supported(cap_hdr->guid, cap_hdr->flags,
+ cap_hdr->imagesize, &reset_type);
+ if (ret) {
+ pr_err("%s: efi_capsule_supported() failed\n",
+ __func__);
+ kunmap(kbuff_page);
+ goto failed;
+ }
+
+ cap_info->total_size = cap_hdr->imagesize;
+ cap_info->pages = kmalloc_array(pages_needed, sizeof(void *),
+ GFP_KERNEL);
+ if (!cap_info->pages) {
+ pr_err("%s: kmalloc_array() failed\n", __func__);
+ ret = -ENOMEM;
+ kunmap(kbuff_page);
+ goto failed;
+ }
+ }
+
+ cap_info->pages[cap_info->curr_index++] = kbuff_page;
+ cap_info->curr_size += count;
+ kunmap(kbuff_page);
+
+ /* submit the full binary to efi_capsule_update() API */
+ if (cap_info->curr_size >= cap_info->total_size) {
+ ret = efi_capsule_update(kmap(cap_info->pages[0]),
+ cap_info->pages);
+ kunmap(cap_info->pages[0]);
+ if (ret) {
+ pr_err("%s: efi_capsule_update() failed\n", __func__);
+ goto failed;
+ }
+ /* indicate capsule binary uploading is done */
+ cap_info->curr_index = -1;
+ }
+
+ /*
+ * we cannot free the pages here due to reboot need that data
+ * maintained.
+ */
+ return count;
+
+failed:
+ if (!cap_info->curr_index) {
+ __free_page(kbuff_page);
+ } else {
+ while (cap_info->curr_index > 0)
+ __free_page(cap_info->pages[--cap_info->curr_index]);
+ kfree(cap_info->pages);
+ }
+
+ return ret;
+}
+
+static int efi_capsule_release(struct inode *inode, struct file *file)
+{
+ int ret = 0;
+ struct capsule_info *cap_info = file->private_data;
+
+ pr_debug("%s: Exit in Release()\n", __func__);
+ /* release those uncompleted uploaded block */
+ if (cap_info->curr_index > 0) {
+ pr_err("%s: capsule upload not complete\n", __func__);
+ while (cap_info->curr_index > 0)
+ __free_page(cap_info->pages[--cap_info->curr_index]);
+ kfree(cap_info->pages);
+ ret = -ECANCELED;
+ }
+ kfree(file->private_data);
+ file->private_data = NULL;
+ mutex_unlock(&capsule_loader_lock);
+ return ret;
+}
+
+static int efi_capsule_open(struct inode *inode, struct file *file)
+{
+ int ret = -EBUSY;
+ struct capsule_info *cap_info;
+
+ pr_debug("%s: Entering Open()\n", __func__);
+ if (mutex_trylock(&capsule_loader_lock)) {
+ cap_info = kzalloc(sizeof(*cap_info), GFP_KERNEL);
+ file->private_data = cap_info;
+ ret = 0;
+ }
+ return ret;
+}
+
+static const struct file_operations efi_capsule_fops = {
+ .owner = THIS_MODULE,
+ .open = efi_capsule_open,
+ .write = efi_capsule_write,
+ .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: misc_register() failed\n", __func__);
+
+ return ret;
+}
+module_init(efi_capsule_loader_init);
+
+static void __exit efi_capsule_loader_exit(void)
+{
+ mutex_destroy(&capsule_loader_lock);
+ misc_deregister(&efi_capsule_misc);
+}
+module_exit(efi_capsule_loader_exit);
+
+MODULE_DESCRIPTION("EFI capsule firmware binary loader");
+MODULE_LICENSE("GPL v2");
--
1.7.9.5
On Fri, 21 Aug, at 07:34:48PM, 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.
>
> Example method to load the capsule binary:
> cat firmware.bin > /dev/efi_capsule_loader
OK interesting, we're going down the misc char device route - Andy
might be happier, even if there is no ioctl(2) support.
> 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/efi-capsule-loader.c | 218 +++++++++++++++++++++++++++++
> 3 files changed, 229 insertions(+)
> create mode 100644 drivers/firmware/efi/efi-capsule-loader.c
[...]
> diff --git a/drivers/firmware/efi/efi-capsule-loader.c b/drivers/firmware/efi/efi-capsule-loader.c
> new file mode 100644
> index 0000000..1da8608
> --- /dev/null
> +++ b/drivers/firmware/efi/efi-capsule-loader.c
> @@ -0,0 +1,218 @@
> +/*
> + * 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"
> +
> +struct capsule_info {
> + int curr_index;
> + int curr_size;
> + int total_size;
It's totally conceivable that a capsule might be greater than 2GB in
size. In which case, 'int' is the wrong data type for these size
fields. Perhaps 'unsigned long' ?
I'd suggest swapping 'curr_index' for simply 'index' and 'curr_size'
for 'count' or 'bytes'.
> + struct page **pages;
> +};
> +
> +static DEFINE_MUTEX(capsule_loader_lock);
> +
> +/*
> + * This function will store the capsule binary and pass it to
> + * efi_capsule_update() API in capsule.c
> + */
> +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;
> + void *kbuff;
> +
> + pr_debug("%s: Entering Write()\n", __func__);
> + if (count == 0)
> + return 0;
> +
> + if (cap_info->curr_index == -1)
> + return count;
Shouldn't we be returning an error here to signal that the driver
wasn't expecting any more data to be sent? Otherwise the caller will
think everything worked out fine, but it didn't. See my comments below
about the success/failure design.
> +
> + kbuff_page = alloc_page(GFP_KERNEL);
> + if (!kbuff_page) {
> + pr_err("%s: alloc_page() failed\n", __func__);
> + if (!cap_info->curr_index)
> + return -ENOMEM;
> + ret = -ENOMEM;
> + goto failed;
> + }
If the caller writes less than PAGE_SIZE bytes at a time this could be
incredibly wasteful. We should use the remaining space from any
previous page allocations.
> +
> + kbuff = kmap(kbuff_page);
> + if (!kbuff) {
> + pr_err("%s: kmap() failed\n", __func__);
> + if (cap_info->curr_index)
> + cap_info->pages[cap_info->curr_index++] = kbuff_page;
> + ret = -EFAULT;
> + goto failed;
> + }
> +
> + /* copy capsule binary data from user space to kernel space buffer */
> + if (copy_from_user(kbuff, buff, min_t(size_t, count, PAGE_SIZE))) {
> + pr_err("%s: copy_from_user() failed\n", __func__);
> + if (cap_info->curr_index)
> + cap_info->pages[cap_info->curr_index++] = kbuff_page;
Huh? Is this to satisfy the requirements of the code at the 'failed'
label? The error handling could do with some cleanup because it's very
difficult to follow the code flow.
Please try and get rid of the housekeeping code where you add
kbuff_page to the pages array just to make the 'failed' code work.
I'd also ditch the pr_err() calls for all but the most fatal of error
conditions because they make the code harder to read.
> + kunmap(kbuff_page);
> + ret = -EFAULT;
> + goto failed;
> + }
> +
> + /* setup capsule binary info structure */
> + if (cap_info->curr_index == 0) {
> + efi_capsule_header_t *cap_hdr = kbuff;
> + int reset_type;
> + int pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >>
> + PAGE_SHIFT;
> +
> + if (pages_needed <= 0) {
Can ALIGN() even return a genuine negative value? 'pages_needed'
should be 'size_t'.
> + pr_err("%s: pages count invalid\n", __func__);
> + ret = -EINVAL;
> + kunmap(kbuff_page);
> + goto failed;
> + }
> +
> + /* check if the capsule binary supported */
> + ret = efi_capsule_supported(cap_hdr->guid, cap_hdr->flags,
> + cap_hdr->imagesize, &reset_type);
> + if (ret) {
> + pr_err("%s: efi_capsule_supported() failed\n",
> + __func__);
> + kunmap(kbuff_page);
> + goto failed;
> + }
> +
> + cap_info->total_size = cap_hdr->imagesize;
> + cap_info->pages = kmalloc_array(pages_needed, sizeof(void *),
> + GFP_KERNEL);
> + if (!cap_info->pages) {
> + pr_err("%s: kmalloc_array() failed\n", __func__);
> + ret = -ENOMEM;
> + kunmap(kbuff_page);
> + goto failed;
> + }
> + }
> +
> + cap_info->pages[cap_info->curr_index++] = kbuff_page;
> + cap_info->curr_size += count;
> + kunmap(kbuff_page);
> +
> + /* submit the full binary to efi_capsule_update() API */
> + if (cap_info->curr_size >= cap_info->total_size) {
> + ret = efi_capsule_update(kmap(cap_info->pages[0]),
> + cap_info->pages);
But kmap() can fail, so you need to handle that.
> + kunmap(cap_info->pages[0]);
> + if (ret) {
> + pr_err("%s: efi_capsule_update() failed\n", __func__);
> + goto failed;
> + }
> + /* indicate capsule binary uploading is done */
> + cap_info->curr_index = -1;
> + }
> +
> + /*
> + * we cannot free the pages here due to reboot need that data
> + * maintained.
> + */
> + return count;
> +
> +failed:
> + if (!cap_info->curr_index) {
> + __free_page(kbuff_page);
> + } else {
> + while (cap_info->curr_index > 0)
> + __free_page(cap_info->pages[--cap_info->curr_index]);
> + kfree(cap_info->pages);
> + }
> +
> + return ret;
Let's explicitly discuss the failure mode. If we fail in this function
we need to decide what happens if the userspace tool continues writing
data instead of closing the file.
It looks like we expect the userspace tool to start at the beginning
of the capsule data again, but you explicitly prohibit calling
lseek(2) by using the 'no_llseek' file_operations function. That makes
it difficult to verify that the app is starting at the beginning of
the file (I also notice that 'ppos' isn't being updated).
In the success case we expect the user to close/open this file to
write more capsules.
Personally, I think these two approaches are backwards. You should be
able to continue writing capsule data as long as write(2) returns
success, but should have to close/re-open the device file as soon as
an error is encountered. If you don't close/re-open on failure I'd
expect write(2) to return -EIO or somesuch error until you do.
It's worth explicitly documenting these two scenarios in the code.
> +}
> +
> +static int efi_capsule_release(struct inode *inode, struct file *file)
> +{
> + int ret = 0;
> + struct capsule_info *cap_info = file->private_data;
> +
> + pr_debug("%s: Exit in Release()\n", __func__);
> + /* release those uncompleted uploaded block */
> + if (cap_info->curr_index > 0) {
> + pr_err("%s: capsule upload not complete\n", __func__);
> + while (cap_info->curr_index > 0)
> + __free_page(cap_info->pages[--cap_info->curr_index]);
> + kfree(cap_info->pages);
> + ret = -ECANCELED;
Interestingly the return value from ->release() isn't propagated to
userspace, look at __fput(). This is because the actual put'ing of the
file is delayed.
Notice how James used ->flush() in his patch series, which *does*
propagate the return value and most importantly, does so at close(2)
time (and also if the task exits for any reason, like being killed by
a fatal signal).
> + }
> + kfree(file->private_data);
> + file->private_data = NULL;
> + mutex_unlock(&capsule_loader_lock);
> + return ret;
> +}
> +
> +static int efi_capsule_open(struct inode *inode, struct file *file)
> +{
> + int ret = -EBUSY;
> + struct capsule_info *cap_info;
> +
> + pr_debug("%s: Entering Open()\n", __func__);
> + if (mutex_trylock(&capsule_loader_lock)) {
> + cap_info = kzalloc(sizeof(*cap_info), GFP_KERNEL);
> + file->private_data = cap_info;
> + ret = 0;
> + }
> + return ret;
> +}
I think this would be more readable like this,
if (mutex_trylock(&capsule_loader_lock))
return -EBUSY;
cap_info = kzalloc(sizeof(*cap_info), GFP_KERNEL);
file->private_data = cap_info;
return 0;
Also, if we only allow one open at a time, why not make 'cap_info'
statically allocated so that you don't need to handle the kzalloc()
failure in this function?
--
Matt Fleming, Intel Open Source Technology Center