| Message ID | 1446055138-26047-2-git-send-email-hock.leong.kweh@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
> -----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
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.
PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBCb3Jpc2xhdiBQZXRrb3YgW21h aWx0bzpicEBhbGllbjguZGVdDQo+IFNlbnQ6IFN1bmRheSwgTm92ZW1iZXIgMDEsIDIwMTUgNjo1 OCBQTQ0KPiANCj4gT24gU3VuLCBOb3YgMDEsIDIwMTUgYXQgMTA6NTI6NTJBTSArMDAwMCwgS3dl aCwgSG9jayBMZW9uZyB3cm90ZToNCj4gPiBDb3VsZCB5b3Ugc2hhcmUgbWUgeW91ciBkdW1iIGZp bGU/IEkgZGlkIHBlcmZvcm0gbmVnYXRpdmUgdGVzdCwgYnV0IEkgZGlkDQo+ID4gbm90IGdldCB0 aGVzZSBkdW1wIHN0YWNrIGluIGRtZXNnLiBUaGFua3MuDQo+IA0KPiBJIHRoaW5rIGFsbW9zdCBh bnkgZmlsZSB3b3JrczoNCj4gDQo+IGNhdCAvYmluL2xzID4gL2Rldi9lZmlfY2Fwc3VsZV9sb2Fk ZXINCg0KT2suIFdpbGwgdHJ5IHRoaXMgb3V0Lg0KDQo+IA0KPiA+ID4gPiArI2RlZmluZSBVUExP QURfRE9ORSAtMQ0KPiA+ID4NCj4gPiA+IElzbid0IHRoZSBmYWN0IHRoYXQgdXBsb2FkIHdhcyBm aW5pc2hlZCBhIHN1Y2Nlc3MgbWVzc2FnZT8gSWYgc28sIHdoeSBpcyBpdCBhDQo+ID4gPiBuZWdh dGl2ZSB2YWx1ZT8NCj4gPg0KPiA+IFRoaXMgaXMgdG8gaW5kaWNhdGUgYW4gdXBsb2FkIGlzIGRv bmUgYW5kIHBlbmRpbmcgZm9yIGNsb3NlKDIpLiBJZiBhDQo+IHN1YnNlcXVlbmNlDQo+ID4gd3Jp dGUoMikgcGVyZm9ybSwgcmV0dXJuIGVycm9yLiBDb21tZW50cyBpbnB1dHRlZCBieSBNYXR0IGFu ZCBBbmR5Lg0KPiANCj4gQnV0IGluIHRoYXQgY2FzZSB5b3UgY2FuIHJldHVybiBFUlJfT0NDVVJS RUQuIFVQTE9BRF9ET05FIHN0aWxsIGRvZXNuJ3QNCj4gbG9vayBsaWtlIGEgbmVnYXRpdmUgdmFs dWUgdG8gbWUgYXMgaXQgc2lnbmFscyB0aGF0IHRoZSB1cGxvYWQgd2FzIGRvbmUNCj4gYW5kIHRo dXMgc3VjY2Vzc2Z1bCBhcyBubyBlcnJvcnMgaGFwcGVuZWQgZHVyaW5nIHRoZSB1cGxvYWQuDQo+ IA0KDQpIbW0gLi4uLiBJZiBJIGNvbWJpbmUgdGhlc2UgMiBmbGFncyB0byBiZWNvbWUgb25lIGFz ICJOT19NT1JFX1dSSVRFX0FDVElPTiINCnRvIGJldHRlciBkZXNjcmliaW5nIHRoZSBzaXR1YXRp b24sIHlvdSBPa2F5IHdpdGggaXQ/DQoNClJlZ2FyZHMsDQpXaWxzb24NCg0K -- 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 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.
PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBCb3Jpc2xhdiBQZXRrb3YgW21h aWx0bzpicEBhbGllbjguZGVdDQo+IFNlbnQ6IFN1bmRheSwgTm92ZW1iZXIgMDEsIDIwMTUgODo1 OSBQTQ0KPiANCj4gT24gU3VuLCBOb3YgMDEsIDIwMTUgYXQgMTE6MTE6MjNBTSArMDAwMCwgS3dl aCwgSG9jayBMZW9uZyB3cm90ZToNCj4gPiBIbW0gLi4uLiBJZiBJIGNvbWJpbmUgdGhlc2UgMiBm bGFncyB0byBiZWNvbWUgb25lIGFzDQo+ID4gIk5PX01PUkVfV1JJVEVfQUNUSU9OIiB0byBiZXR0 ZXIgZGVzY3JpYmluZyB0aGUgc2l0dWF0aW9uLCB5b3UgT2theQ0KPiA+IHdpdGggaXQ/DQo+IA0K PiBJIGRvbid0IHVuZGVyc3RhbmQsIHdoeSBjb21iaW5lPw0KPiANCj4gV2h5IG5vdCBzaW1wbHkg bWFrZSBVUExPQURfRE9ORSBhIHBvc2l0aXZlIHZhbHVlOg0KPiANCj4gI2RlZmluZSBVUExPQURf RE9ORQkxDQo+ICNkZWZpbmUgRVJSX09DQ1VSUkVECS0xDQo+IA0KPiAwIHdvdWxkIG9idmlvdXNs eSBtZWFuLCBubyBlcnJvcnMgb2NjdXJyZWQgd2hhdHNvZXZlci4NCj4gDQoNCkhpIEJvcmlzLA0K DQpUaGlzIGlzIG5vdCBhIHJldHVybiB2YWx1ZSB0byBpbmRpY2F0ZSB3aGF0IGlzIGdvaW5nIG5v dy4gSXQgaXMgYSBmbGFnIHVzZWQgaW4NCiJjYXBfaW5mby0+aW5kZXgiIHdoaWNoIHBvc2l0aXZl IHZhbHVlIGhhcyBhIG1lYW5pbmcgb2YgaW5kZXggbnVtYmVyLg0KSSBhbSB1c2luZyB0aGUgbmVn YXRpdmUgdmFsdWUgZm9yIHRoZSBmbGFnIHdoaWNoIHNpbWlsYXIgdG8gdGhlIGltcGxlbWVudGF0 aW9uDQpvZiBwb2ludGVyICYgZXJyb3IgcG9pbnRlciAoRVJSX1BUUikuDQoNClRoYW5rcyAmIFJl Z2FyZHMsDQpXaWxzb24NCg0K -- 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, 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...
> -----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
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");