[RFC] add BUG_ON_MAPPABLE_NULL macro

[RFC] add BUG_ON_MAPPABLE_NULL macro

[Ohad Ben-Cohen]

Introduce BUG_ON_MAPPABLE_NULL in order to eliminate redundant BUG_ON
code, checking for NULL addresses, on architectures where the zero
address can never be mapped.

Originally proposed by Russell King <linux@arm.linux.org.uk>

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
Compile tested on ARM and x86-64.

Relevant threads:
1. Original proposal by Russell - http://lkml.org/lkml/2010/1/21/238
2. Recent discussion - https://lkml.org/lkml/2010/11/26/78

Notes:
* Implementation still feels hacky, especially since we don't care about the len param of arch_mmap_check.
* Just like BUG_ON, this new macro is compiled out on !CONFIG_BUG. We might want to add a CONFIG_BUG commentary, so users will at least be aware of the security implications of compiling this out.
* To get an (extremely!) rough upper bound of the profit of this macro, I did:

1. find . -name '*.[ch]' | xargs sed -i 's/BUG_ON(!/BUG_ON_MAPPABLE_NULL(!/'
2. removed some obviously bogus sed hits

With omap2plus_defconfig, uImage shrank by ~4Kb (obviously this doesn't include the potential gain in modules)

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

diff --git a/arch/arm/include/asm/mman.h b/arch/arm/include/asm/mman.h
index 41f99c5..f0c5d58 100644
--- a/arch/arm/include/asm/mman.h
+++ b/arch/arm/include/asm/mman.h
@@ -1,4 +1,6 @@
 #include <asm-generic/mman.h>
+#include <asm/pgtable.h>
+#include <asm/errno.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..0171a30 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,47 @@ struct bug_entry {
 #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while(0)
 #endif
 
+/**
+ * BUG_ON_MAPPABLE_NULL() - BUG_ON(condition) only if address 0 is mappable
+ * @condition:	condition to check, should contain a NULL 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 drivers should avoid dereferencing NULL pointers by doing a simple
+ * check (when appropriate), and just return an error rather than crash.
+ * This way the system, despite having reduced functionality, will just keep
+ * running rather than immediately reboot.
+ *
+ * _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 @condition is true on architectures where the zero
+ * address can be mapped. On other architectures, where the zero address
+ * can never be mapped, this macro is compiled out. It only makes sense to
+ * use this macro if @condition contains a NULL check, in order to optimize that
+ * check out on architectures where the zero address can never be mapped.
+ * On such architectures, those checks are not necessary, since the code
+ * itself will reliably reproduce an Oops as soon as the NULL address will
+ * be dereferenced.
+ *
+ * As with BUG_ON, use this macro only if @condition cannot be tolerated.
+ * If proceeding with degraded functionality is an option, it's much
+ * better to just simply check for @condition and return some error code rather
+ * than crash the system.
+ */
+#define BUG_ON_MAPPABLE_NULL(cond) do { \
+	if (arch_mmap_check(0, 1, MAP_FIXED) == 0) \
+		BUG_ON(cond); \
+} 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

Re: [RFC] add BUG_ON_MAPPABLE_NULL macro

[Andrew Morton]

On Sun,  5 Dec 2010 12:06:03 +0200
Ohad Ben-Cohen <ohad@wizery.com> wrote:

> Introduce BUG_ON_MAPPABLE_NULL in order to eliminate redundant BUG_ON
> code, checking for NULL addresses, on architectures where the zero
> address can never be mapped.
> 
> Originally proposed by Russell King <linux@arm.linux.org.uk>
> 
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
> ---
> Compile tested on ARM and x86-64.
> 
> Relevant threads:
> 1. Original proposal by Russell - http://lkml.org/lkml/2010/1/21/238
> 2. Recent discussion - https://lkml.org/lkml/2010/11/26/78
> 
> Notes:
> * Implementation still feels hacky, especially since we don't care about the len param of arch_mmap_check.
> * Just like BUG_ON, this new macro is compiled out on !CONFIG_BUG. We might want to add a CONFIG_BUG commentary, so users will at least be aware of the security implications of compiling this out.
> * To get an (extremely!) rough upper bound of the profit of this macro, I did:
> 
> 1. find . -name '*.[ch]' | xargs sed -i 's/BUG_ON(!/BUG_ON_MAPPABLE_NULL(!/'
> 2. removed some obviously bogus sed hits
> 
> With omap2plus_defconfig, uImage shrank by ~4Kb (obviously this doesn't include the potential gain in modules)
> 

Every time someone sends me a patch with text after the "---", I decide
it was good changelog material and I promote it to above the "---". 
How's about you save us the effort :)

The changelog is a bit vague really, but the code comment explains it
all, and that's a good place to explain things.

> +/**
> + * BUG_ON_MAPPABLE_NULL() - BUG_ON(condition) only if address 0 is mappable
> + * @condition:	condition to check, should contain a NULL 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 drivers should avoid dereferencing NULL pointers by doing a simple

s/drivers/code/

> + * check (when appropriate), and just return an error rather than crash.
> + * This way the system, despite having reduced functionality, will just keep
> + * running rather than immediately reboot.
> + *
> + * _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.

yup.

> + * This macro will BUG_ON if @condition is true on architectures where the zero
> + * address can be mapped. On other architectures, where the zero address
> + * can never be mapped, this macro is compiled out. It only makes sense to
> + * use this macro if @condition contains a NULL check, in order to optimize that
> + * check out on architectures where the zero address can never be mapped.
> + * On such architectures, those checks are not necessary, since the code
> + * itself will reliably reproduce an Oops as soon as the NULL address will
> + * be dereferenced.
> + *
> + * As with BUG_ON, use this macro only if @condition cannot be tolerated.
> + * If proceeding with degraded functionality is an option, it's much
> + * better to just simply check for @condition and return some error code rather
> + * than crash the system.
> + */
> +#define BUG_ON_MAPPABLE_NULL(cond) do { \
> +	if (arch_mmap_check(0, 1, MAP_FIXED) == 0) \
> +		BUG_ON(cond); \
> +} while (0)

- arch_mmap_check() didn't have any documentation.  Please fix?

- arch_mmap_check() is a pretty poor identifier (identifiers which
  include the word "check" are usually poor ones).  Maybe
  arch_address_accessible()?

- I worry about arch_mmap_check().  Is it expected that it will
  perform a runtime check, like probe_kernel_address()?  Spell out the
  expectations, please.

- BUG_ON_MAPPABLE_NULL() will evaluate arch_mmap_check() even if
  CONFIG_BUG=n.  Seems bad, depending on what those unspelled-out
  expectations are!

- BUG_ON_MAPPABLE_NULL() is a mouthful.  I can't immedaitely think of
  anythinjg nicer :(

- Is BUG_ON_MAPPABLE_NULL() really the interface you want?  Or do we
  just want an interface which checks a pointer for nearly-nullness?

Re: [RFC] add BUG_ON_MAPPABLE_NULL macro

[Ohad Ben-Cohen]

On Wed, Dec 8, 2010 at 2:10 AM, Andrew Morton <akpm@linux-foundation.org> wrote:
> Every time someone sends me a patch with text after the "---", I decide
> it was good changelog material and I promote it to above the "---".
> How's about you save us the effort :)

sure :)

>> +/**
>> + * BUG_ON_MAPPABLE_NULL() - BUG_ON(condition) only if address 0 is mappable
>> + * @condition:       condition to check, should contain a NULL 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 drivers should avoid dereferencing NULL pointers by doing a simple
>
> s/drivers/code/

definitely.

>> + * check (when appropriate), and just return an error rather than crash.
>> + * This way the system, despite having reduced functionality, will just keep
>> + * running rather than immediately reboot.
>> + *
>> + * _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.
>
> yup.
>
>> + * This macro will BUG_ON if @condition is true on architectures where the zero
>> + * address can be mapped. On other architectures, where the zero address
>> + * can never be mapped, this macro is compiled out. It only makes sense to
>> + * use this macro if @condition contains a NULL check, in order to optimize that
>> + * check out on architectures where the zero address can never be mapped.
>> + * On such architectures, those checks are not necessary, since the code
>> + * itself will reliably reproduce an Oops as soon as the NULL address will
>> + * be dereferenced.
>> + *
>> + * As with BUG_ON, use this macro only if @condition cannot be tolerated.
>> + * If proceeding with degraded functionality is an option, it's much
>> + * better to just simply check for @condition and return some error code rather
>> + * than crash the system.
>> + */
>> +#define BUG_ON_MAPPABLE_NULL(cond) do { \
>> +     if (arch_mmap_check(0, 1, MAP_FIXED) == 0) \
>> +             BUG_ON(cond); \
>> +} while (0)
>
> - arch_mmap_check() didn't have any documentation.  Please fix?

Sure.

>
> - arch_mmap_check() is a pretty poor identifier (identifiers which
>  include the word "check" are usually poor ones).  Maybe
>  arch_address_accessible()?

Maybe arch_address_mappable() might be more accurate (an address might
be accessible but not mappable due to some other arch-specific usage)
?

But, do you think it makes sense to rename arch_mmap_check() without
adding any functional benefit to it ?

arch_mmap_check is already defined today for ARM, IA64, MN10300, S390
and SPARC, and used by get_unmapped_area() [mmap.c], and this patch
adds a second usage of it.

To rename it, the patch will have to touch all of those architectures,
and usually, patches that carry stylistic changes in areas which they
don't add any functional benefit to, are considered pretty annoying...

> - I worry about arch_mmap_check().  Is it expected that it will
>  perform a runtime check, like probe_kernel_address()?  Spell out the
>  expectations, please.

Yes, arch_mmap_check does perform a runtime check: get_unmapped_area()
calls it with its addr, len and flags parameters.

But, since BUG_ON_MAPPABLE_NULL() calls it with constant values, I
expect arch_mmap_check() to be compiled out (tested on ARM and
X86_64).

> - BUG_ON_MAPPABLE_NULL() will evaluate arch_mmap_check() even if
>  CONFIG_BUG=n.  Seems bad, depending on what those unspelled-out
>  expectations are!

Definitely. I'll add:

#define BUG_ON_MAPPABLE_NULL(condition)  do { if (condition) ; } while(0)

in case CONFIG_BUG=n.

>
> - BUG_ON_MAPPABLE_NULL() is a mouthful.  I can't immedaitely think of
>  anythinjg nicer :(

Heartily agree...

> - Is BUG_ON_MAPPABLE_NULL() really the interface you want?  Or do we
>  just want an interface which checks a pointer for nearly-nullness?

I guess we can start with BUG_ON_MAPPABLE_NULL(), since this is what
people commonly check today, and we will now be able to optimize those
checks out when they are unnecessary.

Thanks a lot,
Ohad.

>
>
>