linux-kernel - Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

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: <AANLkTi=AtYTL-w5Jajypnbj=G_MGzyeLxtusu5V72eG+@mail.gmail.com>
Date:	Mon, 29 Nov 2010 11:46:40 +0200
From:	Ohad Ben-Cohen <ohad@...ery.com>
To:	Russell King - ARM Linux <linux@....linux.org.uk>
Cc:	Olof Johansson <olof@...om.net>,
	Hari Kanigeri <h-kanigeri2@...com>, Suman Anna <s-anna@...com>,
	Benoit Cousson <b-cousson@...com>,
	Arnd Bergmann <arnd@...db.de>,
	Tony Lindgren <tony@...mide.com>, Greg KH <greg@...ah.com>,
	linux-kernel@...r.kernel.org,
	Grant Likely <grant.likely@...retlab.ca>,
	Kevin Hilman <khilman@...prootsystems.com>,
	akpm@...ux-foundation.org, linux-omap@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

On Sat, Nov 27, 2010 at 12:53 AM, Russell King - ARM Linux
<linux@....linux.org.uk> wrote:
> On Sat, Nov 27, 2010 at 12:18:55AM +0200, Ohad Ben-Cohen wrote:
>> But then there's the other (quite reasonable) claim that says we
>> shouldn't crash the machine because of a non fatal bug: if a crappy
>> driver messes up, the user (not the developer) will most probably
>> prefer the machine to keep running with degraded functionality rather
>> than boot.
>
> There's also the quite reasonable expectation that we shouldn't corrupt
> user data.  With locking interfaces, if someone abuses them and they
> fail to work, then the risk is data corruption due to races.  The safe
> thing in that case is to panic - terminate that thread before it does
> anything unsafe, thereby preventing data corruption.

Makes sense.

I considered hwspinlock as a peripheral which doesn't qualify to take
down the system on failure, but in general I agree that there indeed
might be critical user data involved. Especially if this is a
framework and not just another driver.

But as a framework that can be used on non ARM architectures as well,
I do prefer to check for NULL pointers and not rely on the Oops.

If we had a macro that would be compiled out on architectures that
reliably produces an Oops on NULL dereference, but otherwise, would
BUG_ON on them, that should satisfy everyone.

The BUG_ON_MAPPABLE_NULL() macro below should achieve exactly that,
please tell me what you think, thanks!

 arch/arm/include/asm/mman.h |    1 +
 include/asm-generic/bug.h   |   40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/mman.h b/arch/arm/include/asm/mman.h
index 41f99c5..d4ee003 100644
--- a/arch/arm/include/asm/mman.h
+++ b/arch/arm/include/asm/mman.h
@@ -1,4 +1,5 @@
 #include <asm-generic/mman.h>
+#include <asm/pgtable.h>

 #define arch_mmap_check(addr, len, flags) \
 	(((flags) & MAP_FIXED && (addr) < FIRST_USER_ADDRESS) ? -EINVAL : 0)
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index c2c9ba0..d211d9c 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -2,6 +2,11 @@
 #define _ASM_GENERIC_BUG_H

 #include <linux/compiler.h>
+#include <asm/mman.h>
+
+#ifndef arch_mmap_check
+#define arch_mmap_check(addr, len, flags)	(0)
+#endif

 #ifdef CONFIG_BUG

@@ -53,6 +58,41 @@ struct bug_entry {
 #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while(0)
 #endif

+/**
+ * BUG_ON_MAPPABLE_NULL() - panic on NULL only if address 0 is mappable
+ * @addr:	address to check
+ *
+ * In general, NULL dereference Oopses are not desirable, since they take down
+ * the system with them and make the user extremely unhappy. So as a general
+ * rule kernel code should avoid dereferencing NULL pointers by doing a
+ * simple check (when appropriate), and if needed, continue operating
+ * with reduced functionality rather than crash.
+ *
+ * _Critical_ kernel code, OTOH, that should not (/cannot) keep running when
+ * given an unexpected NULL pointer, should just crash. On some architectures,
+ * a NULL dereference will always reliably produce an Oops. On others, where
+ * the zero address can be mmapped, an Oops is not guaranteed. Relying on
+ * NULL dereference Oopses to happen on these architectures might lead to
+ * data corruptions (system will keep running despite a critical bug and
+ * the results will be horribly undefined). In addition, these situations
+ * can also have security implications - we have seen several privilege
+ * escalation exploits with which an attacker gained full control over the
+ * system due to NULL dereference bugs.
+ *
+ * This macro will BUG_ON if @addr is NULL on architectures where the zero
+ * addresses can be mapped. On other architectures, where the zero address
+ * can never be mapped, this macro is compiled out, so the system will just
+ * Oops when the @addr is dereferenced.
+ *
+ * As with BUG_ON, use this macro only if a NULL @addr cannot be tolerated.
+ * If proceeding with degraded functionality is an option, it's much
+ * better to just simply check for the NULL and returning instead of crashing
+ * the system.
+ */
+#define BUG_ON_MAPPABLE_NULL(addr) do { \
+	if (arch_mmap_check(0, 1, MAP_FIXED) == 0) BUG_ON(!addr); \
+} while(0)
+
 /*
  * WARN(), WARN_ON(), WARN_ON_ONCE, and so on can be used to report
  * significant issues that need prompt attention if they should ever
-- 
1.7.0.4
--
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/