| Message ID | 1450440782-5446-2-git-send-email-hock.leong.kweh@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Small nit. On Fri, 2015-12-18 at 20:13 +0800, Kweh, Hock Leong wrote: > From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> > > Introducing a kernel module to expose capsule loader interface > (misc char device file note) for user to upload capsule binaries. This patch ? introduces a kernel module to expose a capsule loader interface... for a user ... > > Example method to load the capsule binary: Example: > cat firmware.bin > /dev/efi_capsule_loader > > This patch also export efi_capsule_supported() function symbol for > verifying the submitted capsule header in this kernel module. 1. Should be a separate patch 2. Suggested, rewording for that patch log 2/2 Title This patch exports efi_capsule_supported to enable verification of the capsule header. That said - who is the user of this symbol ? Why is it needed ? Anyway please consider splitting and rewording. > > Cc: Matt Fleming <matt@codeblueprint.co.uk> > Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com> > --- > drivers/firmware/efi/Kconfig | 10 > drivers/firmware/efi/Makefile | 1 > drivers/firmware/efi/capsule-loader.c | 355 > +++++++++++++++++++++++++++++++++ > drivers/firmware/efi/capsule.c | 1 > 4 files changed, 367 insertions(+) > create mode 100644 drivers/firmware/efi/capsule-loader.c > > diff --git a/drivers/firmware/efi/Kconfig > b/drivers/firmware/efi/Kconfig > index e1670d5..dc2a912 100644 > --- a/drivers/firmware/efi/Kconfig > +++ b/drivers/firmware/efi/Kconfig > @@ -87,6 +87,16 @@ config EFI_RUNTIME_WRAPPERS > config EFI_ARMSTUB > bool > > +config EFI_CAPSULE_LOADER > + tristate "EFI capsule loader" > + depends on EFI > + help > + This option exposes a loader interface > "/dev/efi_capsule_loader" for > + user to load EFI capsule binary and update the EFI > firmware. After > + a successful loading, a system reboot is required. > + > + If unsure, say N. > + This option exposes a loader interface... for a user to load an EFI capsule. A capsule isn't necessarily exclusively for updating of firmware either AFAIK - so I suggest dropping. http://www.intel.com/content/www/us/en/architecture-and-technology/unif ied-extensible-firmware-interface/efi-capsule-specification.html Quote "Capsules were designed for and are intended to be the major vehicle for delivering firmware volume changes. There is no requirement that capsules be limited to that function." > endmenu > > config UEFI_CPER > diff --git a/drivers/firmware/efi/Makefile > b/drivers/firmware/efi/Makefile > index fabf801..cf9d9d3 100644 > --- a/drivers/firmware/efi/Makefile > +++ b/drivers/firmware/efi/Makefile > @@ -18,3 +18,4 @@ obj-$(CONFIG_EFI_RUNTIME_MAP) += > runtime-map.o > obj-$(CONFIG_EFI_RUNTIME_WRAPPERS) += runtime-wrappers.o > obj-$(CONFIG_EFI_STUB) += libstub/ > obj-$(CONFIG_EFI_FAKE_MEMMAP) += fake_mem.o > +obj-$(CONFIG_EFI_CAPSULE_LOADER) += capsule-loader.o > diff --git a/drivers/firmware/efi/capsule-loader.c > b/drivers/firmware/efi/capsule-loader.c > new file mode 100644 > index 0000000..e1214bb > --- /dev/null > +++ b/drivers/firmware/efi/capsule-loader.c > @@ -0,0 +1,355 @@ > +/* > + * EFI capsule loader driver. > + * > + * Copyright 2015 Intel Corporation > + * > + * This file is part of the Linux kernel, and is made available > under > + * the terms of the GNU General Public License version 2. > + */ > + > +#define pr_fmt(fmt) "EFI: " fmt > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/miscdevice.h> > +#include <linux/highmem.h> > +#include <linux/slab.h> > +#include <linux/mutex.h> > +#include <linux/efi.h> > + > +#define NO_FURTHER_WRITE_ACTION -1 > + > +struct capsule_info { > + bool header_obtained; > + int reset_type; > + long index; > + size_t count; > + size_t total_size; > + struct page **pages; > + size_t page_bytes_remain; > +}; > + > +static DEFINE_MUTEX(capsule_loader_lock); > + > +/** > + * efi_free_all_buff_pages - free all previous allocated buffer > pages > + * @cap_info: pointer to current instance of capsule_info structure > + * > + * Besides freeing the buffer pages, it also flagged an > + * NO_FURTHER_WRITE_ACTION flag for indicating to the next > write entries > + * that there is a flaw happened and any subsequence actions > are not > + * valid unless close(2). > + **/ The description needs to be reworded. In addition to freeing buffer pages, we flag NO_FURTHER_WRITE_ACTION on error and will cease to process data in subsequent efi_capsule_write() calls. (or similar) > +static void efi_free_all_buff_pages(struct capsule_info *cap_info) > +{ > + while (cap_info->index > 0) > + __free_page(cap_info->pages[--cap_info->index]); > + > + kfree(cap_info->pages); > + > + cap_info->index = NO_FURTHER_WRITE_ACTION; > +} > + > +/** > + * efi_capsule_setup_info - to obtain the efi capsule header in the > binary and > + * setup capsule_info structure > + * @cap_info: pointer to current instance of capsule_info structure > + * @kbuff: a mapped 1st page buffer pointer first not 1st > + * @hdr_bytes: the total bytes of received efi header the total number of received bytes of the EFI header > + **/ > +static ssize_t efi_capsule_setup_info(struct capsule_info *cap_info, > + void *kbuff, size_t hdr_bytes) > +{ > + int ret; > + void *temp_page; > + > + /* Handling 1st block is less than header size */ first or initial I think you mean to say "Ensure first > + > + /* Check if the capsule binary supported */ > + mutex_lock(&capsule_loader_lock); > + ret = efi_capsule_supported(cap_hdr->guid, cap_hdr > ->flags, > + cap_hdr->imagesize, > + &cap_info->reset_type); > + mutex_unlock(&capsule_loader_lock); What does this mutex really do ? You hold it here around efi_capsule_supported() in the call chain efi_capsule_write() => efi_capsule_setup_info() and then again inside of efi_capsule_submit_update() the call chain chain efi_capsule_write() => efi_capsule_submit_update() So if its valid to ensure you are synchronizing parallel contexts with -respect to calls into efi_capsule_supported() and efi_capsule_submit_update() why aren't you holding that lock for the duration of efi_capsule_write() - couldn't cap_info->page_bytes_remain as an example equally be racy ? > + > +/** > + * efi_capsule_submit_update - invoke the efi_capsule_update API > once binary > + * upload done > + * @cap_info: pointer to current instance of capsule_info structure > + **/ > +static ssize_t efi_capsule_submit_update(struct capsule_info > *cap_info) > +{ > + int ret; > + void *cap_hdr_temp; > + > + cap_hdr_temp = kmap(cap_info->pages[0]); > + if (!cap_hdr_temp) { > + pr_debug("%s: kmap() failed\n", __func__); > + return -EFAULT; > + } > + > + mutex_lock(&capsule_loader_lock); > + ret = efi_capsule_update(cap_hdr_temp, cap_info->pages); > + mutex_unlock(&capsule_loader_lock); > + kunmap(cap_info->pages[0]); > + if (ret) { > + pr_err("%s: efi_capsule_update() failed\n", > __func__); > + return ret; > + } > + > + /* Indicate capsule binary uploading is done */ > + cap_info->index = NO_FURTHER_WRITE_ACTION; Again - if you need a mutex for efi_capsule_update() why don't you need a mutex for cap_info->index updates looks racy... +} > + > +/** > + * efi_capsule_write - store the capsule binary and pass it to > + * efi_capsule_update() API > + * @file: file pointer > + * @buff: buffer pointer > + * @count: number of bytes in @buff > + * @offp: not used > + * > + * Expectation: > + * - User space tool should start at the beginning of capsule > binary and > + * pass data in sequential. A user-space tool.. sequentially > + * - User should close and re-open this file note in order to > upload more > + * capsules. A user should.. > + * - Any error occurred, user should close the file and > restart the > + * operation for the next try otherwise -EIO will be > returned until the > + * file is closed. On error -EIO will be returned and capsule loading will be abandoned. > + * - EFI capsule header must be located at the beginning of > capsule binary > + * file and passed in as 1st block write. An EFI capsule header.... in as the first data in the first write operation > + **/ > +static ssize_t efi_capsule_write(struct file *file, const char > __user *buff, > + size_t count, loff_t *offp) > +{ > + int ret = 0; > + struct capsule_info *cap_info = file->private_data; > + struct page *kbuff_page = NULL; > + void *kbuff = NULL; > + size_t write_byte; > + > + if (count == 0) > + return 0; > + > + /* Return error while NO_FURTHER_WRITE_ACTION is flagged */ > + if (cap_info->index < 0) > + return -EIO; > + > + /* Only alloc a new page when previous page is full */ > + if (!cap_info->page_bytes_remain) { > + kbuff_page = alloc_page(GFP_KERNEL); > + if (!kbuff_page) { > + pr_debug("%s: alloc_page() failed\n", > __func__); Shouldn't this be a pr_err() ? > + ret = -ENOMEM; > + goto failed; > + } > + cap_info->page_bytes_remain = PAGE_SIZE; > + } else { > + kbuff_page = cap_info->pages[--cap_info->index]; How are you guaranteeing this index doesn't go negative ? You compare index < 0 above so the pre-decrement could be negative ... right ? > + } > + > + kbuff = kmap(kbuff_page); > + if (!kbuff) { > + pr_debug("%s: kmap() failed\n", __func__); and here ? > + ret = -EFAULT; > + goto fail_freepage; > + } > + kbuff += PAGE_SIZE - cap_info->page_bytes_remain; > + > + /* Copy capsule binary data from user space to kernel space > buffer */ > + write_byte = min_t(size_t, count, cap_info > ->page_bytes_remain); > + if (copy_from_user(kbuff, buff, write_byte)) { > + pr_debug("%s: copy_from_user() failed\n", __func__); ditto > + ret = -EFAULT; > + goto fail_unmap; > + } > + cap_info->page_bytes_remain -= write_byte; > + > + /* Setup capsule binary info structure */ > + if (!cap_info->header_obtained) { > + ret = efi_capsule_setup_info(cap_info, kbuff, > + cap_info->count + > write_byte); > + if (ret) > + goto fail_unmap; > + } > + > + cap_info->pages[cap_info->index++] = kbuff_page; > + cap_info->count += write_byte; > + kunmap(kbuff_page); > + > + /* Submit the full binary to efi_capsule_update() API */ > + if (cap_info->header_obtained && > + cap_info->count >= cap_info->total_size) { > + if (cap_info->count > cap_info->total_size) { > + pr_err("%s: upload size exceeded header > defined size\n", > + __func__); > + ret = -EINVAL; > + goto failed; Shouldn't this be fail_freepage ? > + } > + > + ret = efi_capsule_submit_update(cap_info); > + if (ret) > + goto failed; Shouldn't this be fail_freepage ? > + } > + > + return write_byte; > + > +fail_unmap: > + kunmap(kbuff_page); > +fail_freepage: > + __free_page(kbuff_page); > +failed: > + efi_free_all_buff_pages(cap_info); > + return ret; > +} > + > +/** > + * efi_capsule_flush - called by file close or file flush > + * @file: file pointer > + * @id: not used > + * > + * If capsule being uploaded partially, calling this function > will be > + * treated as uploading canceled and will free up those > completed buffer > + * pages and then return -ECANCELED. If a capsule is being partially updated then calling this function will be treated as upload termination and will free completed buffer pages; in this case -ECANCELLED will be returned. > + **/ > +static int efi_capsule_flush(struct file *file, fl_owner_t id) > +{ > + int ret = 0; > + struct capsule_info *cap_info = file->private_data; > + > + if (cap_info->index > 0) { > + pr_err("%s: capsule upload not complete\n", > __func__); > + efi_free_all_buff_pages(cap_info); > + ret = -ECANCELED; > + } > + > + return ret; > +} > + > +/** > + * efi_capsule_release - called by file close > + * @inode: not used > + * @file: file pointer > + * > + * We would not free the successful submitted buffer pages > here due to > + * efi update require system reboot with data maintained. > + **/ We will not free successfully submitted pages since EFI update requires data to be maintained across boot. > +static int efi_capsule_open(struct inode *inode, struct file *file) > +{ > + struct capsule_info *cap_info; > + > + cap_info = kzalloc(sizeof(*cap_info), GFP_KERNEL); > + if (!cap_info) > + return -ENOMEM; > + > + file->private_data = cap_info; > + > + return 0; > +} You have a memory leak here don't you ? if I do for (i = 0; i < N; i++) { fd = open(/dev/your_node); close(fd); } You'll leak that kzalloc... -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 18 Dec, at 08:13:01PM, Kweh Hock Leong wrote: > From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> > > Introducing a kernel module to expose capsule loader interface > (misc char device file note) for user to upload capsule binaries. > > Example method to load the capsule binary: > cat firmware.bin > /dev/efi_capsule_loader > > This patch also export efi_capsule_supported() function symbol for > verifying the submitted capsule header in this kernel module. > > Cc: Matt Fleming <matt@codeblueprint.co.uk> > Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com> > --- > drivers/firmware/efi/Kconfig | 10 > drivers/firmware/efi/Makefile | 1 > drivers/firmware/efi/capsule-loader.c | 355 +++++++++++++++++++++++++++++++++ > drivers/firmware/efi/capsule.c | 1 > 4 files changed, 367 insertions(+) > create mode 100644 drivers/firmware/efi/capsule-loader.c [...] > +#define pr_fmt(fmt) "EFI: " fmt > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/miscdevice.h> > +#include <linux/highmem.h> > +#include <linux/slab.h> > +#include <linux/mutex.h> > +#include <linux/efi.h> > + > +#define NO_FURTHER_WRITE_ACTION -1 > + > +struct capsule_info { > + bool header_obtained; > + int reset_type; > + long index; > + size_t count; > + size_t total_size; > + struct page **pages; > + size_t page_bytes_remain; > +}; > + > +static DEFINE_MUTEX(capsule_loader_lock); This mutex is not needed. It doesn't protect anything in your code. > +/** > + * efi_free_all_buff_pages - free all previous allocated buffer pages > + * @cap_info: pointer to current instance of capsule_info structure > + * > + * Besides freeing the buffer pages, it also flagged an > + * NO_FURTHER_WRITE_ACTION flag for indicating to the next write entries > + * that there is a flaw happened and any subsequence actions are not > + * valid unless close(2). > + **/ > +static void efi_free_all_buff_pages(struct capsule_info *cap_info) > +{ > + while (cap_info->index > 0) > + __free_page(cap_info->pages[--cap_info->index]); > + > + kfree(cap_info->pages); > + > + cap_info->index = NO_FURTHER_WRITE_ACTION; > +} > + > +/** > + * efi_capsule_setup_info - to obtain the efi capsule header in the binary and > + * setup capsule_info structure > + * @cap_info: pointer to current instance of capsule_info structure > + * @kbuff: a mapped 1st page buffer pointer > + * @hdr_bytes: the total bytes of received efi header > + **/ > +static ssize_t efi_capsule_setup_info(struct capsule_info *cap_info, > + void *kbuff, size_t hdr_bytes) > +{ > + int ret; > + void *temp_page; > + > + /* Handling 1st block is less than header size */ > + if (hdr_bytes < sizeof(efi_capsule_header_t)) { > + if (!cap_info->pages) { > + cap_info->pages = kzalloc(sizeof(void *), GFP_KERNEL); > + if (!cap_info->pages) { > + pr_debug("%s: kzalloc() failed\n", __func__); > + return -ENOMEM; > + } > + } This function would be much more simple if you handled the above condition differently. Instead of having code in efi_capsule_setup_info() to allocate the initial ->pages memory, why not just allocate it in efi_capsule_open() at the same time as you allocate the private_data? You can then free it in efi_capsule_release() (you're leaking it at the moment). You are always going to need at least one element in ->pages for successful operation, so you might as well allocate it up front. > + } else { > + /* Reset back to the correct offset of header */ > + efi_capsule_header_t *cap_hdr = kbuff - cap_info->count; > + size_t pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >> > + PAGE_SHIFT; > + > + if (pages_needed == 0) { > + pr_err("%s: pages count invalid\n", __func__); > + return -EINVAL; > + } > + > + /* Check if the capsule binary supported */ > + mutex_lock(&capsule_loader_lock); > + ret = efi_capsule_supported(cap_hdr->guid, cap_hdr->flags, > + cap_hdr->imagesize, > + &cap_info->reset_type); > + mutex_unlock(&capsule_loader_lock); > + if (ret) { > + pr_err("%s: efi_capsule_supported() failed\n", > + __func__); > + return ret; > + } > + > + cap_info->total_size = cap_hdr->imagesize; > + temp_page = krealloc(cap_info->pages, > + pages_needed * sizeof(void *), > + GFP_KERNEL | __GFP_ZERO); > + if (!temp_page) { > + pr_debug("%s: krealloc() failed\n", __func__); > + return -ENOMEM; > + } > + > + cap_info->pages = temp_page; > + cap_info->header_obtained = 1; This should be 'true', not 1. > + } > + > + return 0; > +} > > + > +/** > + * efi_capsule_submit_update - invoke the efi_capsule_update API once binary > + * upload done > + * @cap_info: pointer to current instance of capsule_info structure > + **/ > +static ssize_t efi_capsule_submit_update(struct capsule_info *cap_info) > +{ > + int ret; > + void *cap_hdr_temp; > + > + cap_hdr_temp = kmap(cap_info->pages[0]); > + if (!cap_hdr_temp) { > + pr_debug("%s: kmap() failed\n", __func__); > + return -EFAULT; > + } > + > + mutex_lock(&capsule_loader_lock); > + ret = efi_capsule_update(cap_hdr_temp, cap_info->pages); > + mutex_unlock(&capsule_loader_lock); > + kunmap(cap_info->pages[0]); > + if (ret) { > + pr_err("%s: efi_capsule_update() failed\n", __func__); > + return ret; > + } > + > + /* Indicate capsule binary uploading is done */ > + cap_info->index = NO_FURTHER_WRITE_ACTION; > + pr_info("%s: Successfully upload capsule file with reboot type '%s'\n", > + __func__, !cap_info->reset_type ? "RESET_COLD" : > + cap_info->reset_type == 1 ? "RESET_WARM" : > + "RESET_SHUTDOWN"); > + return 0; > +} > + > +/** > + * efi_capsule_write - store the capsule binary and pass it to > + * efi_capsule_update() API > + * @file: file pointer > + * @buff: buffer pointer > + * @count: number of bytes in @buff > + * @offp: not used > + * > + * Expectation: > + * - User space tool should start at the beginning of capsule binary and > + * pass data in sequential. > + * - User should close and re-open this file note in order to upload more > + * capsules. > + * - Any error occurred, user should close the file and restart the > + * operation for the next try otherwise -EIO will be returned until the > + * file is closed. > + * - EFI capsule header must be located at the beginning of capsule binary > + * file and passed in as 1st block write. > + **/ > +static ssize_t efi_capsule_write(struct file *file, const char __user *buff, > + size_t count, loff_t *offp) > +{ > + int ret = 0; > + struct capsule_info *cap_info = file->private_data; > + struct page *kbuff_page = NULL; > + void *kbuff = NULL; > + size_t write_byte; > + > + if (count == 0) > + return 0; > + > + /* Return error while NO_FURTHER_WRITE_ACTION is flagged */ > + if (cap_info->index < 0) > + return -EIO; > + > + /* Only alloc a new page when previous page is full */ > + if (!cap_info->page_bytes_remain) { > + kbuff_page = alloc_page(GFP_KERNEL); > + if (!kbuff_page) { > + pr_debug("%s: alloc_page() failed\n", __func__); > + ret = -ENOMEM; > + goto failed; > + } > + cap_info->page_bytes_remain = PAGE_SIZE; > + } else { > + kbuff_page = cap_info->pages[--cap_info->index]; > + } This shuffling and unshuffling of cap_info->index seems kinda crazy to me because you're treating the current page differently from the rest, which complicates the error paths too (you shouldn't need __free_page() *and* efi_free_all_buff_pages()). Why not make cap_info->index always index the last page? That way you could do, if (!cap_info->page_bytes_remain) { cap_info->pages[++cap_info->index] = alloc_page() cap_info->page_bytes_remain = PAGE_SIZE; } kbuff_page = cap_info->pages[cap_info->index]; ? > + > + kbuff = kmap(kbuff_page); > + if (!kbuff) { > + pr_debug("%s: kmap() failed\n", __func__); > + ret = -EFAULT; > + goto fail_freepage; > + } > + kbuff += PAGE_SIZE - cap_info->page_bytes_remain; > + > + /* Copy capsule binary data from user space to kernel space buffer */ > + write_byte = min_t(size_t, count, cap_info->page_bytes_remain); > + if (copy_from_user(kbuff, buff, write_byte)) { > + pr_debug("%s: copy_from_user() failed\n", __func__); > + ret = -EFAULT; > + goto fail_unmap; > + } > + cap_info->page_bytes_remain -= write_byte; > + > + /* Setup capsule binary info structure */ > + if (!cap_info->header_obtained) { > + ret = efi_capsule_setup_info(cap_info, kbuff, > + cap_info->count + write_byte); > + if (ret) > + goto fail_unmap; > + } > + > + cap_info->pages[cap_info->index++] = kbuff_page; > + cap_info->count += write_byte; > + kunmap(kbuff_page); > + > + /* Submit the full binary to efi_capsule_update() API */ > + if (cap_info->header_obtained && > + cap_info->count >= cap_info->total_size) { > + if (cap_info->count > cap_info->total_size) { > + pr_err("%s: upload size exceeded header defined size\n", > + __func__); > + ret = -EINVAL; > + goto failed; > + } > + > + ret = efi_capsule_submit_update(cap_info); > + if (ret) > + goto failed; > + } > + > + return write_byte; > + > +fail_unmap: > + kunmap(kbuff_page); > +fail_freepage: > + __free_page(kbuff_page); > +failed: > + efi_free_all_buff_pages(cap_info); > + return ret; > +} > + > +/** > + * efi_capsule_flush - called by file close or file flush > + * @file: file pointer > + * @id: not used > + * > + * If capsule being uploaded partially, calling this function will be > + * treated as uploading canceled and will free up those completed buffer > + * pages and then return -ECANCELED. > + **/ > +static int efi_capsule_flush(struct file *file, fl_owner_t id) > +{ > + int ret = 0; > + struct capsule_info *cap_info = file->private_data; > + > + if (cap_info->index > 0) { > + pr_err("%s: capsule upload not complete\n", __func__); > + efi_free_all_buff_pages(cap_info); > + ret = -ECANCELED; > + } > + > + return ret; > +} > + > +/** > + * efi_capsule_release - called by file close > + * @inode: not used > + * @file: file pointer > + * > + * We would not free the successful submitted buffer pages here due to > + * efi update require system reboot with data maintained. > + **/ > +static int efi_capsule_release(struct inode *inode, struct file *file) > +{ > + kfree(file->private_data); > + file->private_data = NULL; > + return 0; > +} > + > +/** > + * efi_capsule_open - called by file open > + * @inode: not used > + * @file: file pointer > + * > + * Will allocate each capsule_info memory for each file open call. > + * This provided the capability to support multiple file open feature > + * where user is not needed to wait for others to finish in order to > + * upload their capsule binary. > + **/ > +static int efi_capsule_open(struct inode *inode, struct file *file) > +{ > + struct capsule_info *cap_info; > + > + cap_info = kzalloc(sizeof(*cap_info), GFP_KERNEL); > + if (!cap_info) > + return -ENOMEM; > + > + file->private_data = cap_info; > + Please allocate one ->pages element here (void *). You need at least one for this code to work and you can easily free it in efi_capsule_release() if it still exists. It would also simplfy efi_capsule_setup_info() immensely. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 21 Dec, at 05:04:11PM, Bryan O'Donoghue wrote: > > +static int efi_capsule_open(struct inode *inode, struct file *file) > > +{ > > + struct capsule_info *cap_info; > > + > > + cap_info = kzalloc(sizeof(*cap_info), GFP_KERNEL); > > + if (!cap_info) > > + return -ENOMEM; > > + > > + file->private_data = cap_info; > > + > > + return 0; > > +} > > You have a memory leak here don't you ? > > if I do > for (i = 0; i < N; i++) { > fd = open(/dev/your_node); > close(fd); > } > > You'll leak that kzalloc... Nope, it gets freed in efi_capsule_release(). Though the code does leak cap_info->pages if a capsule is successfully submitted to the firmware. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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