linux-kernel - [PATCH 2/2] efi: Delete the in_nmi() conditional runtime locking

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1412165200-32141-3-git-send-email-matt@console-pimps.org>
Date:	Wed,  1 Oct 2014 13:06:40 +0100
From:	Matt Fleming <matt@...sole-pimps.org>
To:	Ingo Molnar <mingo@...nel.org>, "H. Peter Anvin" <hpa@...or.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Peter Zijlstra <peterz@...radead.org>
Cc:	linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org,
	Ard Biesheuvel <ard.biesheuvel@...aro.org>,
	Leif Lindholm <leif.lindholm@...aro.org>,
	Matthew Garrett <mjg59@...f.ucam.org>,
	Matt Fleming <matt.fleming@...el.com>
Subject: [PATCH 2/2] efi: Delete the in_nmi() conditional runtime locking

From: Matt Fleming <matt.fleming@...el.com>

commit 5dc3826d9f08 ("efi: Implement mandatory locking for UEFI Runtime
Services") implemented some conditional locking when accessing variable
runtime services that Ingo described as "pretty disgusting".

The intention with the !efi_in_nmi() checks was to avoid live-locks when
trying to write pstore crash data into an EFI variable. Such lockless
accesses are allowed according to the UEFI specification when we're in a
"non-recoverable" state, but whether or not things are implemented
correctly in actual firmware implementations remains an unanswered
question, and so it would seem sensible to avoid doing any kind of
unsynchronized variable accesses.

Furthermore, the efi_in_nmi() tests are inadequate because they don't
account for the case where we call EFI variable services from panic or
oops callbacks and aren't executing in NMI context. In other words,
live-locking is still possible.

Let's just remove the conditional locking altogether. Now we've got the
->set_variable_nonblocking() EFI variable operation we can abort if the
runtime lock is already held. Aborting is by far the safest option.

Cc: Peter Zijlstra <peterz@...radead.org>
Cc: Ingo Molnar <mingo@...nel.org>
Cc: Ard Biesheuvel <ard.biesheuvel@...aro.org>
Cc: Matthew Garrett <mjg59@...f.ucam.org>
Signed-off-by: Matt Fleming <matt.fleming@...el.com>
---
 arch/x86/include/asm/efi.h              |  2 --
 drivers/firmware/efi/runtime-wrappers.c | 17 ++++-------------
 2 files changed, 4 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index abba1fe1db5e..408ad6d3222e 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -86,8 +86,6 @@ extern void __iomem *__init efi_ioremap(unsigned long addr, unsigned long size,
 
 #endif /* CONFIG_X86_32 */
 
-#define efi_in_nmi()	in_nmi()
-
 extern struct efi_scratch efi_scratch;
 extern void __init efi_set_executable(efi_memory_desc_t *md, bool executable);
 extern int __init efi_memblock_x86_reserve_range(void);
diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 4349206198b2..228bbf910461 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -86,9 +86,6 @@ static DEFINE_SPINLOCK(efi_runtime_lock);
  * for QueryVariableInfo() and SetVariable(), as these can be reached in NMI
  * context through efi_pstore_write().
  */
-#ifndef efi_in_nmi
-#define efi_in_nmi()	(0)
-#endif
 
 /*
  * As per commit ef68c8f87ed1 ("x86: Serialize EFI time accesses on rtc_lock"),
@@ -189,14 +186,11 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name,
 {
 	unsigned long flags;
 	efi_status_t status;
-	bool __in_nmi = efi_in_nmi();
 
-	if (!__in_nmi)
-		spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock_irqsave(&efi_runtime_lock, flags);
 	status = efi_call_virt(set_variable, name, vendor, attr, data_size,
 			       data);
-	if (!__in_nmi)
-		spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock_irqrestore(&efi_runtime_lock, flags);
 	return status;
 }
 
@@ -225,17 +219,14 @@ static efi_status_t virt_efi_query_variable_info(u32 attr,
 {
 	unsigned long flags;
 	efi_status_t status;
-	bool __in_nmi = efi_in_nmi();
 
 	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
-	if (!__in_nmi)
-		spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock_irqsave(&efi_runtime_lock, flags);
 	status = efi_call_virt(query_variable_info, attr, storage_space,
 			       remaining_space, max_variable_size);
-	if (!__in_nmi)
-		spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock_irqrestore(&efi_runtime_lock, flags);
 	return status;
 }
 
-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/