From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753217AbcEBH6R (ORCPT ); Mon, 2 May 2016 03:58:17 -0400 Received: from mga02.intel.com ([134.134.136.20]:28857 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752472AbcEBH6G (ORCPT ); Mon, 2 May 2016 03:58:06 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,566,1455004800"; d="scan'208,223";a="944267414" 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> <87h9eked24.fsf@jcompost-MOBL1.tl.intel.com> <20160430200844.GK2839@codeblueprint.co.uk> Date: Mon, 02 May 2016 09:56:09 +0200 In-Reply-To: <20160430200844.GK2839@codeblueprint.co.uk> (Matt Fleming's message of "Sat, 30 Apr 2016 21:08:44 +0100") Message-ID: <87shy0dhba.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-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Matt Fleming writes: > On Fri, 29 Apr, at 03:53:39PM, Jeremy Compostella wrote: >> 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 >>=20 >> Use dynamic memory allocation instead of stack memory for the entry >> object. >>=20 >> This patch also fixes a potential buffer override. >>=20 >> Signed-off-by: Jeremy Compostella >> --- >> drivers/firmware/efi/efibc.c | 36 ++++++++++++++++++++++++------------ >> 1 file changed, 24 insertions(+), 12 deletions(-) >>=20 >> 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 >>=20=20 >> 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] =3D '\0'; >> } >>=20=20 >> -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 =3D LINUX_EFI_LOADER_ENTRY_GUID; >> - struct efivar_entry entry; >> + struct efivar_entry *entry; >> size_t size =3D (strlen(value) + 1) * sizeof(efi_char16_t); >>=20=20 >> - if (size > sizeof(entry.var.Data)) >> + if (size > sizeof(entry->var.Data)) { >> pr_err("value is too large"); >> + return -1; >> + } > > This isn't right. The usual return value for this scenario would be > either -ENOMEM or -EINVAL. Personally I'd lean towards -EINVAL. Right. Done. >>=20=20 >> - 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 =3D kmalloc(sizeof(*entry), GFP_KERNEL); >> + if (!entry) { >> + pr_err("failed to allocate efivar entry"); >> + return -1; >> + } > > This should be -ENOMEM. Done. >>=20=20 >> - ret =3D 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 =3D 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; >> } >>=20=20 >> static int efibc_reboot_notifier_call(struct notifier_block *notifier, >> unsigned long event, void *data) >> { >> const char *reason =3D "shutdown"; >> + int ret; >>=20=20 >> if (event =3D=3D SYS_RESTART) >> reason =3D "reboot"; >>=20=20 >> - efibc_set_variable("LoaderEntryRebootReason", reason); >> - >> - if (!data) >> - return NOTIFY_DONE; >> + ret =3D efibc_set_variable("LoaderEntryRebootReason", reason); >> + if (ret || !data) >> + return NOTIFY_DONE; >>=20=20 >> efibc_set_variable("LoaderEntryOneShot", (char *)data); > > You need to check this return value too now. We are leaving this function returning NOTIFY_DONE whatever the return value of that last efibc_set_variable() call. Please find the updated patch in attachment. Also, thank you for answering Ard's questions. I promised I won't look at my emails on Labor day ;) J=C3=A9r=C3=A9my --=-=-= Content-Type: text/x-diff Content-Disposition: attachment; filename=0001-efibc-fix-excessive-stack-footprint-warning.patch >>From e50a50dce0762edbc63f15d951b43b21fa7ca784 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..cb4f573 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 -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) - 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 -- One Emacs to rule them all --=-=-=--