[GIT PULL 0/5] EFI changes for v4.7

[GIT PULL 0/5] EFI changes for v4.7

[Matt Fleming]

Folks, this is the second pull request containing v4.7 material. The
commits are listed in priority order, with the first patch fixing an
oops in the EFI capsule code sitting in tip/efi/core, and the rest
being a compiler warning fix, static checker fix, and a couple of
cleanups.

The following changes since commit 0ec7ae928a9c19c2b7b8054507d5694a2597065e:

  efi: Remove unnecessary (and buggy) .memmap initialization from the Xen EFI driver (2016-04-29 11:06:15 +0200)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git tags/efi-next

for you to fetch changes up to 20948c1d9fdefa7acfaa84046f59adce9ef00f2e:

  efivarfs: Make efivarfs_file_ioctl static (2016-05-05 16:52:19 +0100)

----------------------------------------------------------------
 * Fix an oops in the EFI capsule code reported by the 0day bot
   because efi_capsule_pending() was grabbing a mutex in the emergency
   reboot path - Matt Fleming

 * Fix a compiler warning about excessive stack usage in the new efibc
   driver by kmalloc'ing the efivar_entry object - Jeremy Compostella

 * Dan Carpenter reported that it's potentially unsafe to pass the
   address of a pointer to the firmware in efi_capsule_supported().
   Instead we can skip the dynamic allocation entirely and put the
   capsule object on the stack - Matt Fleming

 * Simplify the locking in the efivars code by merging two of
   efivar_init()'s parameters into one - Julia Lawall

 * Cleanup efivarfs_file_ioctl by marking it as static since it has no
   external users - Peter Jones

----------------------------------------------------------------
Jeremy Compostella (1):
      efibc: Fix excessive stack footprint warning

Julia Lawall (1):
      efi: Merge boolean flag arguments

Matt Fleming (2):
      efi/capsule: Make efi_capsule_pending() lockless
      efi/capsule: Move 'capsule' to the stack in efi_capsule_supported()

Peter Jones (1):
      efivarfs: Make efivarfs_file_ioctl static

 drivers/firmware/efi/capsule.c | 65 ++++++++++++++++++++++++------------------
 drivers/firmware/efi/efibc.c   | 34 +++++++++++++++-------
 drivers/firmware/efi/efivars.c |  5 ++--
 drivers/firmware/efi/vars.c    | 23 +++++++--------
 fs/efivarfs/file.c             |  2 +-
 fs/efivarfs/super.c            |  3 +-
 include/linux/efi.h            |  3 +-
 7 files changed, 75 insertions(+), 60 deletions(-)

[PATCH 1/5] efi/capsule: Make efi_capsule_pending() lockless

[Matt Fleming]

Taking a mutex in the reboot path is bogus because we cannot sleep
with interrupts disabled, such as when rebooting due to panic(),

  BUG: sleeping function called from invalid context at kernel/locking/mutex.c:97
  in_atomic(): 0, irqs_disabled(): 1, pid: 7, name: rcu_sched
  Call Trace:
    dump_stack+0x63/0x89
    ___might_sleep+0xd8/0x120
    __might_sleep+0x49/0x80
    mutex_lock+0x20/0x50
    efi_capsule_pending+0x1d/0x60
    native_machine_emergency_restart+0x59/0x280
    machine_emergency_restart+0x19/0x20
    emergency_restart+0x18/0x20
    panic+0x1ba/0x217

In this case all other CPUs will have been stopped by the time we
execute the platform reboot code, so 'capsule_pending' cannot change
under our feet. We wouldn't care even if it could since we cannot wait
for it complete.

Also, instead of relying on the external 'system_state' variable just
use a reboot notifier, so we can set 'stop_capsules' while holding
'capsule_mutex', thereby avoiding a race where system_state is updated
while we're in the middle of efi_capsule_update_locked() (since CPUs
won't have been stopped at that point).

Cc: Borislav Petkov <bp@alien8.de>
Cc: Kweh Hock Leong <hock.leong.kweh@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Bryan O'Donoghue <pure.logic@nexus-software.ie>
Cc: joeyli <jlee@suse.com>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 drivers/firmware/efi/capsule.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
index 0de55944ac0b..4703dc9b8fbd 100644
--- a/drivers/firmware/efi/capsule.c
+++ b/drivers/firmware/efi/capsule.c
@@ -22,11 +22,12 @@ typedef struct {
 } efi_capsule_block_desc_t;
 
 static bool capsule_pending;
+static bool stop_capsules;
 static int efi_reset_type = -1;
 
 /*
  * capsule_mutex serialises access to both capsule_pending and
- * efi_reset_type.
+ * efi_reset_type and stop_capsules.
  */
 static DEFINE_MUTEX(capsule_mutex);
 
@@ -50,18 +51,13 @@ static DEFINE_MUTEX(capsule_mutex);
  */
 bool efi_capsule_pending(int *reset_type)
 {
-	bool rv = false;
-
-	mutex_lock(&capsule_mutex);
 	if (!capsule_pending)
-		goto out;
+		return false;
 
 	if (reset_type)
 		*reset_type = efi_reset_type;
-	rv = true;
-out:
-	mutex_unlock(&capsule_mutex);
-	return rv;
+
+	return true;
 }
 
 /*
@@ -176,7 +172,7 @@ efi_capsule_update_locked(efi_capsule_header_t *capsule,
 	 * whether to force an EFI reboot), and we're racing against
 	 * that call. Abort in that case.
 	 */
-	if (unlikely(system_state == SYSTEM_RESTART)) {
+	if (unlikely(stop_capsules)) {
 		pr_warn("Capsule update raced with reboot, aborting.\n");
 		return -EINVAL;
 	}
@@ -298,3 +294,23 @@ out:
 	return rv;
 }
 EXPORT_SYMBOL_GPL(efi_capsule_update);
+
+static int capsule_reboot_notify(struct notifier_block *nb,
+				 unsigned long event, void *cmd)
+{
+	mutex_lock(&capsule_mutex);
+	stop_capsules = true;
+	mutex_unlock(&capsule_mutex);
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block capsule_reboot_nb = {
+	.notifier_call = capsule_reboot_notify,
+};
+
+static int __init capsule_reboot_register(void)
+{
+	return register_reboot_notifier(&capsule_reboot_nb);
+}
+core_initcall(capsule_reboot_register);
-- 
2.7.3

[PATCH 2/5] efibc: Fix excessive stack footprint warning

[Matt Fleming]

From: Jeremy Compostella <jeremy.compostella@intel.com>

gcc complains about a newly added file for the EFI Bootloader Control:

drivers/firmware/efi/efibc.c: In function 'efibc_set_variable':
drivers/firmware/efi/efibc.c:53:1: error: the frame size of 2272 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]

The problem is the declaration of a local variable of type struct
efivar_entry, which is by itself larger than the warning limit of 1024
bytes.

Use dynamic memory allocation instead of stack memory for the entry
object.

This patch also fixes a potential buffer overflow.

Reported-by: Ingo Molnar <mingo@kernel.org>
Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Jeremy Compostella <jeremy.compostella@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
[ Updated changelog to include gcc error ]
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 drivers/firmware/efi/efibc.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/firmware/efi/efibc.c b/drivers/firmware/efi/efibc.c
index 2e0c7ccd9d9e..8dd0c7085e59 100644
--- a/drivers/firmware/efi/efibc.c
+++ b/drivers/firmware/efi/efibc.c
@@ -17,6 +17,7 @@
 #include <linux/efi.h>
 #include <linux/module.h>
 #include <linux/reboot.h>
+#include <linux/slab.h>
 
 static void efibc_str_to_str16(const char *str, efi_char16_t *str16)
 {
@@ -28,41 +29,52 @@ static void efibc_str_to_str16(const char *str, efi_char16_t *str16)
 	str16[i] = '\0';
 }
 
-static void efibc_set_variable(const char *name, const char *value)
+static int efibc_set_variable(const char *name, const char *value)
 {
 	int ret;
 	efi_guid_t guid = LINUX_EFI_LOADER_ENTRY_GUID;
-	struct efivar_entry entry;
+	struct efivar_entry *entry;
 	size_t size = (strlen(value) + 1) * sizeof(efi_char16_t);
 
-	if (size > sizeof(entry.var.Data))
+	if (size > sizeof(entry->var.Data)) {
 		pr_err("value is too large");
+		return -EINVAL;
+	}
 
-	efibc_str_to_str16(name, entry.var.VariableName);
-	efibc_str_to_str16(value, (efi_char16_t *)entry.var.Data);
-	memcpy(&entry.var.VendorGuid, &guid, sizeof(guid));
+	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
+	if (!entry) {
+		pr_err("failed to allocate efivar entry");
+		return -ENOMEM;
+	}
 
-	ret = efivar_entry_set(&entry,
+	efibc_str_to_str16(name, entry->var.VariableName);
+	efibc_str_to_str16(value, (efi_char16_t *)entry->var.Data);
+	memcpy(&entry->var.VendorGuid, &guid, sizeof(guid));
+
+	ret = efivar_entry_set(entry,
 			       EFI_VARIABLE_NON_VOLATILE
 			       | EFI_VARIABLE_BOOTSERVICE_ACCESS
 			       | EFI_VARIABLE_RUNTIME_ACCESS,
-			       size, entry.var.Data, NULL);
+			       size, entry->var.Data, NULL);
 	if (ret)
 		pr_err("failed to set %s EFI variable: 0x%x\n",
 		       name, ret);
+
+	kfree(entry);
+	return ret;
 }
 
 static int efibc_reboot_notifier_call(struct notifier_block *notifier,
 				      unsigned long event, void *data)
 {
 	const char *reason = "shutdown";
+	int ret;
 
 	if (event == SYS_RESTART)
 		reason = "reboot";
 
-	efibc_set_variable("LoaderEntryRebootReason", reason);
-
-	if (!data)
+	ret = efibc_set_variable("LoaderEntryRebootReason", reason);
+	if (ret || !data)
 		return NOTIFY_DONE;
 
 	efibc_set_variable("LoaderEntryOneShot", (char *)data);
-- 
2.7.3

RE: [PATCH 2/5] efibc: Fix excessive stack footprint warning

[Elliott, Robert (Persistent Memory)]

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Matt Fleming
> Sent: Friday, May 06, 2016 4:39 PM
...
> Subject: [PATCH 2/5] efibc: Fix excessive stack footprint warning
> 
> From: Jeremy Compostella <jeremy.compostella@intel.com>
> 
...
> 
> -static void efibc_set_variable(const char *name, const char *value)
> +static int efibc_set_variable(const char *name, const char *value)
>  {
>  	int ret;
>  	efi_guid_t guid = LINUX_EFI_LOADER_ENTRY_GUID;
> -	struct efivar_entry entry;
> +	struct efivar_entry *entry;
>  	size_t size = (strlen(value) + 1) * sizeof(efi_char16_t);
> 
> -	if (size > sizeof(entry.var.Data))
> +	if (size > sizeof(entry->var.Data)) {
>  		pr_err("value is too large");

That pr_err is introduced by patch 25/40 of the first series.

How about including the name of the variable for which this is failing, 
like the final pr_err?

> +		return -EINVAL;
> +	}
> 
> -	efibc_str_to_str16(name, entry.var.VariableName);
> -	efibc_str_to_str16(value, (efi_char16_t *)entry.var.Data);
> -	memcpy(&entry.var.VendorGuid, &guid, sizeof(guid));
> +	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> +	if (!entry) {
> +		pr_err("failed to allocate efivar entry");

How about including the name of the variable for which this
is failing, like the final pr_err?

> +		return -ENOMEM;
> +	}
> 
> -	ret = efivar_entry_set(&entry,
> +	efibc_str_to_str16(name, entry->var.VariableName);
> +	efibc_str_to_str16(value, (efi_char16_t *)entry->var.Data);
> +	memcpy(&entry->var.VendorGuid, &guid, sizeof(guid));
> +
> +	ret = efivar_entry_set(entry,
>  			       EFI_VARIABLE_NON_VOLATILE
>  			       | EFI_VARIABLE_BOOTSERVICE_ACCESS
>  			       | EFI_VARIABLE_RUNTIME_ACCESS,
> -			       size, entry.var.Data, NULL);
> +			       size, entry->var.Data, NULL);
>  	if (ret)
>  		pr_err("failed to set %s EFI variable: 0x%x\n",
>  		       name, ret);
> +
> +	kfree(entry);
> +	return ret;
>  }

Re: [PATCH 2/5] efibc: Fix excessive stack footprint warning

[Compostella, Jeremy]

[-- Attachment #1: Type: text/plain, Size: 61 bytes --]

Why not.  See patch as attachment.

Thanks,

Jérémy


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-efibc-report-the-EFI-variable-name-in-the-error-mess.patch --]
[-- Type: text/x-diff, Size: 1247 bytes --]

>From 8a9b07e2d7242fa8a36157f1025202a96c3c7c9a Mon Sep 17 00:00:00 2001
From: Jeremy Compostella <jeremy.compostella@intel.com>
Date: Tue, 10 May 2016 10:34:21 +0200
Subject: [PATCH] efibc: report the EFI variable name in the error messages

Report the name of the EFI variable if the value is incorrect or if
efibc_set_variable() fails to allocate the struct efivar_entry object.

Signed-off-by: Jeremy Compostella <jeremy.compostella@intel.com>
---
 drivers/firmware/efi/efibc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/efibc.c b/drivers/firmware/efi/efibc.c
index cb4f573..93d34a1 100644
--- a/drivers/firmware/efi/efibc.c
+++ b/drivers/firmware/efi/efibc.c
@@ -37,13 +37,14 @@ static int efibc_set_variable(const char *name, const char *value)
 	size_t size = (strlen(value) + 1) * sizeof(efi_char16_t);
 
 	if (size > sizeof(entry->var.Data)) {
-		pr_err("value is too large");
+		pr_err("value is too large for %s EFI variable", name);
 		return -EINVAL;
 	}
 
 	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
 	if (!entry) {
-		pr_err("failed to allocate efivar entry");
+		pr_err("failed to allocate efivar entry for %s EFI variable",
+		       name);
 		return -ENOMEM;
 	}
 
-- 
1.9.1


[-- Attachment #3: Type: text/plain, Size: 2390 bytes --]



"Elliott, Robert (Persistent Memory)" <elliott@hpe.com> writes:

>> -----Original Message-----
>> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
>> owner@vger.kernel.org] On Behalf Of Matt Fleming
>> Sent: Friday, May 06, 2016 4:39 PM
> ...
>> Subject: [PATCH 2/5] efibc: Fix excessive stack footprint warning
>>
>> From: Jeremy Compostella <jeremy.compostella@intel.com>
>>
> ...
>>
>> -static void efibc_set_variable(const char *name, const char *value)
>> +static int efibc_set_variable(const char *name, const char *value)
>>  {
>>       int ret;
>>       efi_guid_t guid = LINUX_EFI_LOADER_ENTRY_GUID;
>> -     struct efivar_entry entry;
>> +     struct efivar_entry *entry;
>>       size_t size = (strlen(value) + 1) * sizeof(efi_char16_t);
>>
>> -     if (size > sizeof(entry.var.Data))
>> +     if (size > sizeof(entry->var.Data)) {
>>               pr_err("value is too large");
>
> That pr_err is introduced by patch 25/40 of the first series.
>
> How about including the name of the variable for which this is failing,
> like the final pr_err?
>
>> +             return -EINVAL;
>> +     }
>>
>> -     efibc_str_to_str16(name, entry.var.VariableName);
>> -     efibc_str_to_str16(value, (efi_char16_t *)entry.var.Data);
>> -     memcpy(&entry.var.VendorGuid, &guid, sizeof(guid));
>> +     entry = kmalloc(sizeof(*entry), GFP_KERNEL);
>> +     if (!entry) {
>> +             pr_err("failed to allocate efivar entry");
>
> How about including the name of the variable for which this
> is failing, like the final pr_err?
>
>> +             return -ENOMEM;
>> +     }
>>
>> -     ret = efivar_entry_set(&entry,
>> +     efibc_str_to_str16(name, entry->var.VariableName);
>> +     efibc_str_to_str16(value, (efi_char16_t *)entry->var.Data);
>> +     memcpy(&entry->var.VendorGuid, &guid, sizeof(guid));
>> +
>> +     ret = efivar_entry_set(entry,
>>                              EFI_VARIABLE_NON_VOLATILE
>>                              | EFI_VARIABLE_BOOTSERVICE_ACCESS
>>                              | EFI_VARIABLE_RUNTIME_ACCESS,
>> -                            size, entry.var.Data, NULL);
>> +                            size, entry->var.Data, NULL);
>>       if (ret)
>>               pr_err("failed to set %s EFI variable: 0x%x\n",
>>                      name, ret);
>> +
>> +     kfree(entry);
>> +     return ret;
>>  }
>

-- 
One Emacs to rule them all

Re: [PATCH 2/5] efibc: Fix excessive stack footprint warning

[Matt Fleming]

On Tue, 10 May, at 10:40:22AM, Jeremy Compostella wrote:
> Why not.  See patch as attachment.
> 
> Thanks,
> 
> Jérémy
> 

> From 8a9b07e2d7242fa8a36157f1025202a96c3c7c9a Mon Sep 17 00:00:00 2001
> From: Jeremy Compostella <jeremy.compostella-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Date: Tue, 10 May 2016 10:34:21 +0200
> Subject: [PATCH] efibc: report the EFI variable name in the error messages
> 
> Report the name of the EFI variable if the value is incorrect or if
> efibc_set_variable() fails to allocate the struct efivar_entry object.
> 
> Signed-off-by: Jeremy Compostella <jeremy.compostella-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/firmware/efi/efibc.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/efi/efibc.c b/drivers/firmware/efi/efibc.c
> index cb4f573..93d34a1 100644
> --- a/drivers/firmware/efi/efibc.c
> +++ b/drivers/firmware/efi/efibc.c
> @@ -37,13 +37,14 @@ static int efibc_set_variable(const char *name, const char *value)
>  	size_t size = (strlen(value) + 1) * sizeof(efi_char16_t);
>  
>  	if (size > sizeof(entry->var.Data)) {
> -		pr_err("value is too large");
> +		pr_err("value is too large for %s EFI variable", name);
>  		return -EINVAL;
>  	}

It'd be a good idea to print 'size' too.

>  
>  	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
>  	if (!entry) {
> -		pr_err("failed to allocate efivar entry");
> +		pr_err("failed to allocate efivar entry for %s EFI variable",
> +		       name);
>  		return -ENOMEM;
>  	}

Aren't these pr_err() calls missing newline characters?

Re: [PATCH 2/5] efibc: Fix excessive stack footprint warning

[Compostella, Jeremy]

[-- Attachment #1: Type: text/plain, Size: 64 bytes --]

You're right.  Here is the new patch.

Thanks,

Jérémy


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-efibc-report-more-information-in-the-error-messages.patch --]
[-- Type: text/x-diff, Size: 1461 bytes --]

>From 3a54e6872e220e1ac4db0eae126a20b5383dae3e Mon Sep 17 00:00:00 2001
From: Jeremy Compostella <jeremy.compostella-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Date: Tue, 10 May 2016 10:34:21 +0200
Subject: [PATCH] efibc: report more information in the error messages

Report the name of the EFI variable if the value size is too large or
if efibc_set_variable() fails to allocate the struct efivar_entry
object.  If efibc_set_variable() fails because the value size is too
large, it also reports the value size in the error message.

Signed-off-by: Jeremy Compostella <jeremy.compostella-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efi/efibc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/efibc.c b/drivers/firmware/efi/efibc.c
index cb4f573..369edb5 100644
--- a/drivers/firmware/efi/efibc.c
+++ b/drivers/firmware/efi/efibc.c
@@ -37,13 +37,15 @@ static int efibc_set_variable(const char *name, const char *value)
 	size_t size = (strlen(value) + 1) * sizeof(efi_char16_t);
 
 	if (size > sizeof(entry->var.Data)) {
-		pr_err("value is too large");
+		pr_err("value is too large (%zu bytes) for %s EFI variable\n",
+		       size, name);
 		return -EINVAL;
 	}
 
 	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
 	if (!entry) {
-		pr_err("failed to allocate efivar entry");
+		pr_err("failed to allocate efivar entry for %s EFI variable\n",
+		       name);
 		return -ENOMEM;
 	}
 
-- 
1.9.1


[-- Attachment #3: Type: text/plain, Size: 1797 bytes --]


Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> writes:

> On Tue, 10 May, at 10:40:22AM, Jeremy Compostella wrote:
>> Why not.  See patch as attachment.
>> 
>> Thanks,
>> 
>> Jérémy
>> 
>
>> From 8a9b07e2d7242fa8a36157f1025202a96c3c7c9a Mon Sep 17 00:00:00 2001
>> From: Jeremy Compostella <jeremy.compostella-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> Date: Tue, 10 May 2016 10:34:21 +0200
>> Subject: [PATCH] efibc: report the EFI variable name in the error messages
>> 
>> Report the name of the EFI variable if the value is incorrect or if
>> efibc_set_variable() fails to allocate the struct efivar_entry object.
>> 
>> Signed-off-by: Jeremy Compostella <jeremy.compostella-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/firmware/efi/efibc.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/firmware/efi/efibc.c b/drivers/firmware/efi/efibc.c
>> index cb4f573..93d34a1 100644
>> --- a/drivers/firmware/efi/efibc.c
>> +++ b/drivers/firmware/efi/efibc.c
>> @@ -37,13 +37,14 @@ static int efibc_set_variable(const char *name, const char *value)
>>  	size_t size = (strlen(value) + 1) * sizeof(efi_char16_t);
>>  
>>  	if (size > sizeof(entry->var.Data)) {
>> -		pr_err("value is too large");
>> +		pr_err("value is too large for %s EFI variable", name);
>>  		return -EINVAL;
>>  	}
>
> It'd be a good idea to print 'size' too.
>
>>  
>>  	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
>>  	if (!entry) {
>> -		pr_err("failed to allocate efivar entry");
>> +		pr_err("failed to allocate efivar entry for %s EFI variable",
>> +		       name);
>>  		return -ENOMEM;
>>  	}
>
> Aren't these pr_err() calls missing newline characters?

-- 
One Emacs to rule them all

Re: [PATCH 2/5] efibc: Fix excessive stack footprint warning

[Matt Fleming]

On Wed, 11 May, at 05:16:24PM, Jeremy Compostella wrote:
> From 3a54e6872e220e1ac4db0eae126a20b5383dae3e Mon Sep 17 00:00:00 2001
> From: Jeremy Compostella <jeremy.compostella-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Date: Tue, 10 May 2016 10:34:21 +0200
> Subject: [PATCH] efibc: report more information in the error messages
> 
> Report the name of the EFI variable if the value size is too large or
> if efibc_set_variable() fails to allocate the struct efivar_entry
> object.  If efibc_set_variable() fails because the value size is too
> large, it also reports the value size in the error message.
> 
> Signed-off-by: Jeremy Compostella <jeremy.compostella-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/firmware/efi/efibc.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Applied, thanks Jeremy.

[PATCH 3/5] efi/capsule: Move 'capsule' to the stack in efi_capsule_supported()

[Matt Fleming]

Dan reports that passing the address of the pointer to the kmalloc()'d
memory for 'capsule' is dangerous,

 "drivers/firmware/efi/capsule.c:109 efi_capsule_supported()
  warn: did you mean to pass the address of 'capsule'

   108
   109          status = efi.query_capsule_caps(&capsule, 1, &max_size, reset);
                                                ^^^^^^^^
  If we modify capsule inside this function call then at the end of the
  function we aren't freeing the original pointer that we allocated."

Ard noted that we don't even need to call kmalloc() since the object
we allocate isn't very big and doesn't need to persist after the
function returns.

Place 'capsule' on the stack instead.

Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Kweh Hock Leong <hock.leong.kweh@intel.com>
Cc: Bryan O'Donoghue <pure.logic@nexus-software.ie>
Cc: joeyli <jlee@suse.com>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 drivers/firmware/efi/capsule.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
index 4703dc9b8fbd..7593108f5402 100644
--- a/drivers/firmware/efi/capsule.c
+++ b/drivers/firmware/efi/capsule.c
@@ -86,33 +86,26 @@ bool efi_capsule_pending(int *reset_type)
  */
 int efi_capsule_supported(efi_guid_t guid, u32 flags, size_t size, int *reset)
 {
-	efi_capsule_header_t *capsule;
+	efi_capsule_header_t capsule;
+	efi_capsule_header_t *cap_list[] = { &capsule };
 	efi_status_t status;
 	u64 max_size;
-	int rv = 0;
 
 	if (flags & ~EFI_CAPSULE_SUPPORTED_FLAG_MASK)
 		return -EINVAL;
 
-	capsule = kmalloc(sizeof(*capsule), GFP_KERNEL);
-	if (!capsule)
-		return -ENOMEM;
-
-	capsule->headersize = capsule->imagesize = sizeof(*capsule);
-	memcpy(&capsule->guid, &guid, sizeof(efi_guid_t));
-	capsule->flags = flags;
+	capsule.headersize = capsule.imagesize = sizeof(capsule);
+	memcpy(&capsule.guid, &guid, sizeof(efi_guid_t));
+	capsule.flags = flags;
 
-	status = efi.query_capsule_caps(&capsule, 1, &max_size, reset);
-	if (status != EFI_SUCCESS) {
-		rv = efi_status_to_err(status);
-		goto out;
-	}
+	status = efi.query_capsule_caps(cap_list, 1, &max_size, reset);
+	if (status != EFI_SUCCESS)
+		return efi_status_to_err(status);
 
 	if (size > max_size)
-		rv = -ENOSPC;
-out:
-	kfree(capsule);
-	return rv;
+		return -ENOSPC;
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(efi_capsule_supported);
 
-- 
2.7.3

[PATCH 4/5] efi: Merge boolean flag arguments

[Matt Fleming]

From: Julia Lawall <Julia.Lawall@lip6.fr>

The parameters atomic and duplicates of efivar_init always have opposite
values.  Drop the parameter atomic, replace the uses of !atomic with
duplicates, and update the call sites accordingly.

The code using duplicates is slightly reorganized with an else, to avoid
duplicating the lock code.

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: Jeremy Kerr <jk@ozlabs.org>
Cc: Vaishali Thakkar <vaishali.thakkar@oracle.com>
Cc: Saurabh Sengar <saurabh.truth@gmail.com>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 drivers/firmware/efi/efivars.c |  5 ++---
 drivers/firmware/efi/vars.c    | 23 ++++++++++-------------
 fs/efivarfs/super.c            |  3 +--
 include/linux/efi.h            |  3 +--
 4 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
index 096adcbcb5a9..116b244dee68 100644
--- a/drivers/firmware/efi/efivars.c
+++ b/drivers/firmware/efi/efivars.c
@@ -661,7 +661,7 @@ static void efivar_update_sysfs_entries(struct work_struct *work)
 			return;
 
 		err = efivar_init(efivar_update_sysfs_entry, entry,
-				  true, false, &efivar_sysfs_list);
+				  false, &efivar_sysfs_list);
 		if (!err)
 			break;
 
@@ -730,8 +730,7 @@ int efivars_sysfs_init(void)
 		return -ENOMEM;
 	}
 
-	efivar_init(efivars_sysfs_callback, NULL, false,
-		    true, &efivar_sysfs_list);
+	efivar_init(efivars_sysfs_callback, NULL, true, &efivar_sysfs_list);
 
 	error = create_efivars_bin_attributes();
 	if (error) {
diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 0012331d5a3d..d3b751383286 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -419,8 +419,7 @@ static void dup_variable_bug(efi_char16_t *str16, efi_guid_t *vendor_guid,
  * Returns 0 on success, or a kernel error code on failure.
  */
 int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
-		void *data, bool atomic, bool duplicates,
-		struct list_head *head)
+		void *data, bool duplicates, struct list_head *head)
 {
 	const struct efivar_operations *ops = __efivars->ops;
 	unsigned long variable_name_size = 1024;
@@ -450,7 +449,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 						&vendor_guid);
 		switch (status) {
 		case EFI_SUCCESS:
-			if (!atomic)
+			if (duplicates)
 				spin_unlock_irq(&__efivars->lock);
 
 			variable_name_size = var_name_strnsize(variable_name,
@@ -465,21 +464,19 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 			 * and may end up looping here forever.
 			 */
 			if (duplicates &&
-			    variable_is_present(variable_name, &vendor_guid, head)) {
+			    variable_is_present(variable_name, &vendor_guid,
+						head)) {
 				dup_variable_bug(variable_name, &vendor_guid,
 						 variable_name_size);
-				if (!atomic)
-					spin_lock_irq(&__efivars->lock);
-
 				status = EFI_NOT_FOUND;
-				break;
+			} else {
+				err = func(variable_name, vendor_guid,
+					   variable_name_size, data);
+				if (err)
+					status = EFI_NOT_FOUND;
 			}
 
-			err = func(variable_name, vendor_guid, variable_name_size, data);
-			if (err)
-				status = EFI_NOT_FOUND;
-
-			if (!atomic)
+			if (duplicates)
 				spin_lock_irq(&__efivars->lock);
 
 			break;
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index 553c5d2db4a4..9cb54a38832d 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -216,8 +216,7 @@ static int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
 
 	INIT_LIST_HEAD(&efivarfs_list);
 
-	err = efivar_init(efivarfs_callback, (void *)sb, false,
-			  true, &efivarfs_list);
+	err = efivar_init(efivarfs_callback, (void *)sb, true, &efivarfs_list);
 	if (err)
 		__efivar_entry_iter(efivarfs_destroy, &efivarfs_list, NULL, NULL);
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index aa36fb8bea4b..df7acb51f3cc 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1336,8 +1336,7 @@ int efivars_unregister(struct efivars *efivars);
 struct kobject *efivars_kobject(void);
 
 int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
-		void *data, bool atomic, bool duplicates,
-		struct list_head *head);
+		void *data, bool duplicates, struct list_head *head);
 
 void efivar_entry_add(struct efivar_entry *entry, struct list_head *head);
 void efivar_entry_remove(struct efivar_entry *entry);
-- 
2.7.3

[PATCH 5/5] efivarfs: Make efivarfs_file_ioctl static

[Matt Fleming]

From: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

There are no callers except through the file_operations struct below
this, so it should be static like everything else here.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
---
 fs/efivarfs/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index d48e0d261d78..5f22e74bbade 100644
--- a/fs/efivarfs/file.c
+++ b/fs/efivarfs/file.c
@@ -157,7 +157,7 @@ efivarfs_ioc_setxflags(struct file *file, void __user *arg)
 	return 0;
 }
 
-long
+static long
 efivarfs_file_ioctl(struct file *file, unsigned int cmd, unsigned long p)
 {
 	void __user *arg = (void __user *)p;
-- 
2.7.3