linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] lkdtm: Add tests for LIST_POISON and ZERO_SIZE_PTR
@ 2016-11-15 10:59 Michael Ellerman
  2016-11-15 16:25 ` kbuild test robot
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Michael Ellerman @ 2016-11-15 10:59 UTC (permalink / raw)
  To: keescook; +Cc: kernel-hardening, linux-kernel

This adds two tests, to check that a read or write to LIST_POISON1 and
ZERO_SIZE_PTR are blocked.

The default values for both (256 and 16) typically fall in the range
of valid user space addresses. However in general mmap_min_addr is 64K,
which prevents user space from mapping anything at those addresses.

However it's feasible that an attacker will be able to find a way to
cause an access at an offset from either value, and if that offset is
greater than 64K then they can access user space again.

To simulate that case, in the test we create a user mapping at
mmap_min_addr, and offset the pointer by that amount. This gives the
test the greatest chance of failing (ie. an access succeeding).

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 drivers/misc/lkdtm.h      |  2 ++
 drivers/misc/lkdtm_bugs.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/misc/lkdtm_core.c |  2 ++
 3 files changed, 48 insertions(+)

diff --git a/drivers/misc/lkdtm.h b/drivers/misc/lkdtm.h
index fdf954c2107f..cc207f7824f9 100644
--- a/drivers/misc/lkdtm.h
+++ b/drivers/misc/lkdtm.h
@@ -21,6 +21,8 @@ void lkdtm_SPINLOCKUP(void);
 void lkdtm_HUNG_TASK(void);
 void lkdtm_ATOMIC_UNDERFLOW(void);
 void lkdtm_ATOMIC_OVERFLOW(void);
+void lkdtm_ACCESS_LIST_POISON(void);
+void lkdtm_ACCESS_ZERO_SIZE_PTR(void);
 
 /* lkdtm_heap.c */
 void lkdtm_OVERWRITE_ALLOCATION(void);
diff --git a/drivers/misc/lkdtm_bugs.c b/drivers/misc/lkdtm_bugs.c
index 182ae1894b32..35ce9c753b48 100644
--- a/drivers/misc/lkdtm_bugs.c
+++ b/drivers/misc/lkdtm_bugs.c
@@ -5,7 +5,10 @@
  * test source files.
  */
 #include "lkdtm.h"
+#include <linux/mman.h>
 #include <linux/sched.h>
+#include <linux/security.h>
+#include <linux/slab.h>
 
 /*
  * Make sure our attempts to over run the kernel stack doesn't trigger
@@ -146,3 +149,44 @@ void lkdtm_ATOMIC_OVERFLOW(void)
 	pr_info("attempting bad atomic overflow\n");
 	atomic_inc(&over);
 }
+
+static void test_poison_ptr(void *base, const char *desc)
+{
+	unsigned long *ptr, val, uaddr;
+
+	uaddr = vm_mmap(NULL, mmap_min_addr, PAGE_SIZE, PROT_READ | PROT_WRITE,
+			MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, 0);
+	if (uaddr >= TASK_SIZE) {
+		pr_warn("Failed to allocate user memory, can't perform test.\n");
+		return;
+	}
+
+	/*
+	 * Creating a mapping and adding mmap_min_addr to the value is cheating
+	 * in a way. But it simulates the case where an attacker is able to
+	 * cause an access at a small offset from the base value, leading to a
+	 * user space access. If an arch doesn't define CONFIG_ILLEGAL_POINTER_VALUE
+	 * then it's likely this will work in the absence of other protections.
+	 */
+	ptr = mmap_min_addr + base;
+
+	pr_info("attempting read of %s %p\n", desc, ptr);
+	val = *ptr;
+	pr_info("FAIL: Was able to read %s! Got 0x%lx\n", desc, val);
+
+	pr_info("attempting write of %s %p\n", desc, ptr);
+	*ptr = 0xdeadbeefabcd1234;
+	pr_info("FAIL: Was able to write %s! Now = 0x%lx\n", desc, *ptr);
+
+	vm_munmap(uaddr, PAGE_SIZE);
+}
+
+void lkdtm_ACCESS_LIST_POISON(void)
+{
+	test_poison_ptr(LIST_POISON1, "LIST_POISON");
+}
+
+void lkdtm_ACCESS_ZERO_SIZE_PTR(void)
+{
+	test_poison_ptr(ZERO_SIZE_PTR, "ZERO_SIZE_PTR");
+}
diff --git a/drivers/misc/lkdtm_core.c b/drivers/misc/lkdtm_core.c
index f9154b8d67f6..025a0ee8d8ee 100644
--- a/drivers/misc/lkdtm_core.c
+++ b/drivers/misc/lkdtm_core.c
@@ -220,6 +220,8 @@ struct crashtype crashtypes[] = {
 	CRASHTYPE(WRITE_KERN),
 	CRASHTYPE(ATOMIC_UNDERFLOW),
 	CRASHTYPE(ATOMIC_OVERFLOW),
+	CRASHTYPE(ACCESS_LIST_POISON),
+	CRASHTYPE(ACCESS_ZERO_SIZE_PTR),
 	CRASHTYPE(USERCOPY_HEAP_SIZE_TO),
 	CRASHTYPE(USERCOPY_HEAP_SIZE_FROM),
 	CRASHTYPE(USERCOPY_HEAP_FLAG_TO),
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] lkdtm: Add tests for LIST_POISON and ZERO_SIZE_PTR
  2016-11-15 10:59 [PATCH] lkdtm: Add tests for LIST_POISON and ZERO_SIZE_PTR Michael Ellerman
@ 2016-11-15 16:25 ` kbuild test robot
  2016-11-15 18:02 ` Kees Cook
  2016-11-15 22:33 ` kbuild test robot
  2 siblings, 0 replies; 7+ messages in thread
From: kbuild test robot @ 2016-11-15 16:25 UTC (permalink / raw)
  To: Michael Ellerman; +Cc: kbuild-all, keescook, kernel-hardening, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2020 bytes --]

Hi Michael,

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on v4.9-rc5]
[cannot apply to next-20161115]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Michael-Ellerman/lkdtm-Add-tests-for-LIST_POISON-and-ZERO_SIZE_PTR/20161115-235441
config: i386-randconfig-x007-201646 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   drivers/misc/lkdtm_bugs.c: In function 'test_poison_ptr':
>> drivers/misc/lkdtm_bugs.c:178:9: warning: large integer implicitly truncated to unsigned type [-Woverflow]
     *ptr = 0xdeadbeefabcd1234;
            ^~~~~~~~~~~~~~~~~~

vim +178 drivers/misc/lkdtm_bugs.c

   162		}
   163	
   164		/*
   165		 * Creating a mapping and adding mmap_min_addr to the value is cheating
   166		 * in a way. But it simulates the case where an attacker is able to
   167		 * cause an access at a small offset from the base value, leading to a
   168		 * user space access. If an arch doesn't define CONFIG_ILLEGAL_POINTER_VALUE
   169		 * then it's likely this will work in the absence of other protections.
   170		 */
   171		ptr = mmap_min_addr + base;
   172	
   173		pr_info("attempting read of %s %p\n", desc, ptr);
   174		val = *ptr;
   175		pr_info("FAIL: Was able to read %s! Got 0x%lx\n", desc, val);
   176	
   177		pr_info("attempting write of %s %p\n", desc, ptr);
 > 178		*ptr = 0xdeadbeefabcd1234;
   179		pr_info("FAIL: Was able to write %s! Now = 0x%lx\n", desc, *ptr);
   180	
   181		vm_munmap(uaddr, PAGE_SIZE);
   182	}
   183	
   184	void lkdtm_ACCESS_LIST_POISON(void)
   185	{
   186		test_poison_ptr(LIST_POISON1, "LIST_POISON");

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24141 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] lkdtm: Add tests for LIST_POISON and ZERO_SIZE_PTR
  2016-11-15 10:59 [PATCH] lkdtm: Add tests for LIST_POISON and ZERO_SIZE_PTR Michael Ellerman
  2016-11-15 16:25 ` kbuild test robot
@ 2016-11-15 18:02 ` Kees Cook
  2016-11-15 22:33 ` kbuild test robot
  2 siblings, 0 replies; 7+ messages in thread
From: Kees Cook @ 2016-11-15 18:02 UTC (permalink / raw)
  To: Michael Ellerman; +Cc: kernel-hardening, LKML

On Tue, Nov 15, 2016 at 2:59 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> This adds two tests, to check that a read or write to LIST_POISON1 and
> ZERO_SIZE_PTR are blocked.

Awesome. I think this addition!

> The default values for both (256 and 16) typically fall in the range
> of valid user space addresses. However in general mmap_min_addr is 64K,
> which prevents user space from mapping anything at those addresses.
>
> However it's feasible that an attacker will be able to find a way to
> cause an access at an offset from either value, and if that offset is
> greater than 64K then they can access user space again.
>
> To simulate that case, in the test we create a user mapping at
> mmap_min_addr, and offset the pointer by that amount. This gives the
> test the greatest chance of failing (ie. an access succeeding).
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  drivers/misc/lkdtm.h      |  2 ++
>  drivers/misc/lkdtm_bugs.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/misc/lkdtm_core.c |  2 ++
>  3 files changed, 48 insertions(+)
>
> diff --git a/drivers/misc/lkdtm.h b/drivers/misc/lkdtm.h
> index fdf954c2107f..cc207f7824f9 100644
> --- a/drivers/misc/lkdtm.h
> +++ b/drivers/misc/lkdtm.h
> @@ -21,6 +21,8 @@ void lkdtm_SPINLOCKUP(void);
>  void lkdtm_HUNG_TASK(void);
>  void lkdtm_ATOMIC_UNDERFLOW(void);
>  void lkdtm_ATOMIC_OVERFLOW(void);
> +void lkdtm_ACCESS_LIST_POISON(void);
> +void lkdtm_ACCESS_ZERO_SIZE_PTR(void);
>
>  /* lkdtm_heap.c */
>  void lkdtm_OVERWRITE_ALLOCATION(void);
> diff --git a/drivers/misc/lkdtm_bugs.c b/drivers/misc/lkdtm_bugs.c
> index 182ae1894b32..35ce9c753b48 100644
> --- a/drivers/misc/lkdtm_bugs.c
> +++ b/drivers/misc/lkdtm_bugs.c
> @@ -5,7 +5,10 @@
>   * test source files.
>   */
>  #include "lkdtm.h"
> +#include <linux/mman.h>
>  #include <linux/sched.h>
> +#include <linux/security.h>
> +#include <linux/slab.h>
>
>  /*
>   * Make sure our attempts to over run the kernel stack doesn't trigger
> @@ -146,3 +149,44 @@ void lkdtm_ATOMIC_OVERFLOW(void)
>         pr_info("attempting bad atomic overflow\n");
>         atomic_inc(&over);
>  }
> +
> +static void test_poison_ptr(void *base, const char *desc)
> +{
> +       unsigned long *ptr, val, uaddr;
> +
> +       uaddr = vm_mmap(NULL, mmap_min_addr, PAGE_SIZE, PROT_READ | PROT_WRITE,
> +                       MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, 0);
> +       if (uaddr >= TASK_SIZE) {
> +               pr_warn("Failed to allocate user memory, can't perform test.\n");
> +               return;
> +       }
> +
> +       /*
> +        * Creating a mapping and adding mmap_min_addr to the value is cheating
> +        * in a way. But it simulates the case where an attacker is able to
> +        * cause an access at a small offset from the base value, leading to a
> +        * user space access. If an arch doesn't define CONFIG_ILLEGAL_POINTER_VALUE
> +        * then it's likely this will work in the absence of other protections.
> +        */
> +       ptr = mmap_min_addr + base;
> +
> +       pr_info("attempting read of %s %p\n", desc, ptr);
> +       val = *ptr;
> +       pr_info("FAIL: Was able to read %s! Got 0x%lx\n", desc, val);
> +
> +       pr_info("attempting write of %s %p\n", desc, ptr);
> +       *ptr = 0xdeadbeefabcd1234;

I've traditionally used int pointers to avoid build warnings (as
kbuild mentioned), see lkdtm_READ_AFTER_FREE() for example.

> +       pr_info("FAIL: Was able to write %s! Now = 0x%lx\n", desc, *ptr);
> +
> +       vm_munmap(uaddr, PAGE_SIZE);
> +}
> +
> +void lkdtm_ACCESS_LIST_POISON(void)
> +{
> +       test_poison_ptr(LIST_POISON1, "LIST_POISON");
> +}
> +
> +void lkdtm_ACCESS_ZERO_SIZE_PTR(void)
> +{
> +       test_poison_ptr(ZERO_SIZE_PTR, "ZERO_SIZE_PTR");
> +}
> diff --git a/drivers/misc/lkdtm_core.c b/drivers/misc/lkdtm_core.c
> index f9154b8d67f6..025a0ee8d8ee 100644
> --- a/drivers/misc/lkdtm_core.c
> +++ b/drivers/misc/lkdtm_core.c
> @@ -220,6 +220,8 @@ struct crashtype crashtypes[] = {
>         CRASHTYPE(WRITE_KERN),
>         CRASHTYPE(ATOMIC_UNDERFLOW),
>         CRASHTYPE(ATOMIC_OVERFLOW),
> +       CRASHTYPE(ACCESS_LIST_POISON),
> +       CRASHTYPE(ACCESS_ZERO_SIZE_PTR),
>         CRASHTYPE(USERCOPY_HEAP_SIZE_TO),
>         CRASHTYPE(USERCOPY_HEAP_SIZE_FROM),
>         CRASHTYPE(USERCOPY_HEAP_FLAG_TO),
> --
> 2.7.4
>

Thanks, I like this. Architectures with PAN/SMAP will be protected due
to the "unexpected" direct user memory access, and architectures with
a memory hole will trip over the bad memory area. And those without
need to fix something. :)

-Kees

-- 
Kees Cook
Nexus Security

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] lkdtm: Add tests for LIST_POISON and ZERO_SIZE_PTR
  2016-11-15 10:59 [PATCH] lkdtm: Add tests for LIST_POISON and ZERO_SIZE_PTR Michael Ellerman
  2016-11-15 16:25 ` kbuild test robot
  2016-11-15 18:02 ` Kees Cook
@ 2016-11-15 22:33 ` kbuild test robot
  2016-11-17 10:53   ` [kernel-hardening] " Michael Ellerman
  2 siblings, 1 reply; 7+ messages in thread
From: kbuild test robot @ 2016-11-15 22:33 UTC (permalink / raw)
  To: Michael Ellerman; +Cc: kbuild-all, keescook, kernel-hardening, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 818 bytes --]

Hi Michael,

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on v4.9-rc5]
[cannot apply to next-20161115]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Michael-Ellerman/lkdtm-Add-tests-for-LIST_POISON-and-ZERO_SIZE_PTR/20161115-235441
config: i386-randconfig-i0-201646 (attached as .config)
compiler: gcc-4.8 (Debian 4.8.4-1) 4.8.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

>> ERROR: "mmap_min_addr" [drivers/misc/lkdtm.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26219 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [kernel-hardening] Re: [PATCH] lkdtm: Add tests for LIST_POISON and ZERO_SIZE_PTR
  2016-11-15 22:33 ` kbuild test robot
@ 2016-11-17 10:53   ` Michael Ellerman
  2016-11-17 19:56     ` Kees Cook
  0 siblings, 1 reply; 7+ messages in thread
From: Michael Ellerman @ 2016-11-17 10:53 UTC (permalink / raw)
  To: keescook; +Cc: kernel-hardening, linux-kernel

kbuild test robot <lkp@intel.com> writes:
> [auto build test ERROR on char-misc/char-misc-testing]
> [also build test ERROR on v4.9-rc5]
> [cannot apply to next-20161115]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url:    https://github.com/0day-ci/linux/commits/Michael-Ellerman/lkdtm-Add-tests-for-LIST_POISON-and-ZERO_SIZE_PTR/20161115-235441
> config: i386-randconfig-i0-201646 (attached as .config)
> compiler: gcc-4.8 (Debian 4.8.4-1) 4.8.4
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386 
>
> All errors (new ones prefixed by >>):
>
>>> ERROR: "mmap_min_addr" [drivers/misc/lkdtm.ko] undefined!

I couldn't see what was causing this, but of course it's just that
mmap_min_addr is not exported, and in that config LKDTM is a module.

I always build it in which is why I didn't notice.

I'm not sure we want to EXPORT_SYMBOL mmap_min_addr just for this test
do we? I could change the test to use mmap_min_addr if LKDTM is
built-in, and otherwise just map at zero. Dunno.

cheers

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [kernel-hardening] Re: [PATCH] lkdtm: Add tests for LIST_POISON and ZERO_SIZE_PTR
  2016-11-17 10:53   ` [kernel-hardening] " Michael Ellerman
@ 2016-11-17 19:56     ` Kees Cook
  2016-11-18  2:37       ` Michael Ellerman
  0 siblings, 1 reply; 7+ messages in thread
From: Kees Cook @ 2016-11-17 19:56 UTC (permalink / raw)
  To: Michael Ellerman; +Cc: kernel-hardening, LKML

On Thu, Nov 17, 2016 at 2:53 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> kbuild test robot <lkp@intel.com> writes:
>> [auto build test ERROR on char-misc/char-misc-testing]
>> [also build test ERROR on v4.9-rc5]
>> [cannot apply to next-20161115]
>> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>>
>> url:    https://github.com/0day-ci/linux/commits/Michael-Ellerman/lkdtm-Add-tests-for-LIST_POISON-and-ZERO_SIZE_PTR/20161115-235441
>> config: i386-randconfig-i0-201646 (attached as .config)
>> compiler: gcc-4.8 (Debian 4.8.4-1) 4.8.4
>> reproduce:
>>         # save the attached .config to linux build tree
>>         make ARCH=i386
>>
>> All errors (new ones prefixed by >>):
>>
>>>> ERROR: "mmap_min_addr" [drivers/misc/lkdtm.ko] undefined!
>
> I couldn't see what was causing this, but of course it's just that
> mmap_min_addr is not exported, and in that config LKDTM is a module.
>
> I always build it in which is why I didn't notice.
>
> I'm not sure we want to EXPORT_SYMBOL mmap_min_addr just for this test
> do we? I could change the test to use mmap_min_addr if LKDTM is
> built-in, and otherwise just map at zero. Dunno.

Because of lkdtm's moduleness, I've had to put a lot of weird stuff
into EXPORT_SYMBOL_GPL(). mmap_min_addr is more sensitive than most,
though. How about just hard-coding it as 64k instead of using
mmap_min_addr? Anyone using >64k is likely going to understand why
lkdtm failed.

-Kees

-- 
Kees Cook
Nexus Security

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [kernel-hardening] Re: [PATCH] lkdtm: Add tests for LIST_POISON and ZERO_SIZE_PTR
  2016-11-17 19:56     ` Kees Cook
@ 2016-11-18  2:37       ` Michael Ellerman
  0 siblings, 0 replies; 7+ messages in thread
From: Michael Ellerman @ 2016-11-18  2:37 UTC (permalink / raw)
  To: Kees Cook; +Cc: kernel-hardening, LKML

Kees Cook <keescook@chromium.org> writes:

> On Thu, Nov 17, 2016 at 2:53 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>> kbuild test robot <lkp@intel.com> writes:
>>> [auto build test ERROR on char-misc/char-misc-testing]
>>> [also build test ERROR on v4.9-rc5]
>>> [cannot apply to next-20161115]
>>> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>>>
>>> url:    https://github.com/0day-ci/linux/commits/Michael-Ellerman/lkdtm-Add-tests-for-LIST_POISON-and-ZERO_SIZE_PTR/20161115-235441
>>> config: i386-randconfig-i0-201646 (attached as .config)
>>> compiler: gcc-4.8 (Debian 4.8.4-1) 4.8.4
>>> reproduce:
>>>         # save the attached .config to linux build tree
>>>         make ARCH=i386
>>>
>>> All errors (new ones prefixed by >>):
>>>
>>>>> ERROR: "mmap_min_addr" [drivers/misc/lkdtm.ko] undefined!
>>
>> I couldn't see what was causing this, but of course it's just that
>> mmap_min_addr is not exported, and in that config LKDTM is a module.
>>
>> I always build it in which is why I didn't notice.
>>
>> I'm not sure we want to EXPORT_SYMBOL mmap_min_addr just for this test
>> do we? I could change the test to use mmap_min_addr if LKDTM is
>> built-in, and otherwise just map at zero. Dunno.
>
> Because of lkdtm's moduleness, I've had to put a lot of weird stuff
> into EXPORT_SYMBOL_GPL(). mmap_min_addr is more sensitive than most,
> though. How about just hard-coding it as 64k instead of using
> mmap_min_addr?

We can just use CONFIG_DEFAULT_MMAP_MIN_ADDR, which should work unless
someone's actually changed the value at runtime. Will test.

I'd rather that than 64K hard-coded, as I'm thinking of increasing the
minimum on ppc64.

cheers

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-11-18  2:38 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-15 10:59 [PATCH] lkdtm: Add tests for LIST_POISON and ZERO_SIZE_PTR Michael Ellerman
2016-11-15 16:25 ` kbuild test robot
2016-11-15 18:02 ` Kees Cook
2016-11-15 22:33 ` kbuild test robot
2016-11-17 10:53   ` [kernel-hardening] " Michael Ellerman
2016-11-17 19:56     ` Kees Cook
2016-11-18  2:37       ` Michael Ellerman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).