Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753944AbcD2NzA (ORCPT ); Fri, 29 Apr 2016 09:55:00 -0400 Received: from mga14.intel.com ([192.55.52.115]:54577 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753816AbcD2Ny6 (ORCPT ); Fri, 29 Apr 2016 09:54:58 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,551,1455004800"; d="scan'208,223";a="795038630" From: jeremy.compostella@intel.com (Compostella, Jeremy) To: Matt Fleming Cc: Ingo Molnar , , , , , , , , Subject: Re: [tip:efi/core] efibc: Add EFI Bootloader Control module References: <1461614832-17633-26-git-send-email-matt@codeblueprint.co.uk> <20160429095356.GA29957@gmail.com> <20160429103011.GE2839@codeblueprint.co.uk> <87lh3wejfi.fsf@jcompost-MOBL1.tl.intel.com> <20160429121635.GF2839@codeblueprint.co.uk> Date: Fri, 29 Apr 2016 15:53:39 +0200 In-Reply-To: <20160429121635.GF2839@codeblueprint.co.uk> (Matt Fleming's message of "Fri, 29 Apr 2016 13:16:35 +0100") Message-ID: <87h9eked24.fsf@jcompost-MOBL1.tl.intel.com> User-Agent: Gnus/5.130012 (Ma Gnus v0.12) Emacs/25.0.50 (gnu/linux) MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3949 Lines: 135 --=-=-= Content-Type: text/plain Matt Fleming writes: > On Fri, 29 Apr, at 01:36:01PM, Jeremy Compostella wrote: >> I would go for this last proposal because it fixes the issue, it works >> with the potential race condition you mentioned and it is a simple >> solution. Yet, it means that if we really run into a race condition, >> the LoaderEntryOneShot value might be indeterminate. >> >> Though, this is a very unlikely situation and if it happens I cannot >> think of any good reason to prefer to store the first restart target >> or the second. >> >> Do you want me to make this change ? > > Yes please, that'd be greatly appreciated. Be sure to do it on top of > either the EFI 'next' branch or tip:efi/core. Please, find the patch in attachment. I wrote it on top of tip:efi/core. --=-=-= Content-Type: text/x-diff Content-Disposition: attachment; filename=0001-efibc-fix-excessive-stack-footprint-warning.patch >From ef3a2941769e59b11d1ec36117209dc4c90c7cf9 Mon Sep 17 00:00:00 2001 From: Jeremy Compostella Date: Fri, 29 Apr 2016 15:29:59 +0200 Subject: [PATCH] efibc: fix excessive stack footprint warning Use dynamic memory allocation instead of stack memory for the entry object. This patch also fixes a potential buffer override. Signed-off-by: Jeremy Compostella --- drivers/firmware/efi/efibc.c | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/drivers/firmware/efi/efibc.c b/drivers/firmware/efi/efibc.c index 2e0c7cc..dd6d1a6 100644 --- a/drivers/firmware/efi/efibc.c +++ b/drivers/firmware/efi/efibc.c @@ -17,6 +17,7 @@ #include #include #include +#include static void efibc_str_to_str16(const char *str, efi_char16_t *str16) { @@ -28,42 +29,53 @@ 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 -1; + } - 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 -1; + } - 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) - return NOTIFY_DONE; + ret = efibc_set_variable("LoaderEntryRebootReason", reason); + if (ret || !data) + return NOTIFY_DONE; efibc_set_variable("LoaderEntryOneShot", (char *)data); -- 1.9.1 --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable J=C3=A9r=C3=A9my --=20 One Emacs to rule them all --=-=-=--