[PATCH] slab: Add POISON_POINTER_DELTA to ZERO_SIZE_PTR

[PATCH] slab: Add POISON_POINTER_DELTA to ZERO_SIZE_PTR

[Michael Ellerman]

POISON_POINTER_DELTA is defined in poison.h, and is intended to be used
to shift poison values so that they don't alias userspace.

We should add it to ZERO_SIZE_PTR so that attackers can't use
ZERO_SIZE_PTR as a way to get a pointer to userspace.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 include/linux/slab.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 084b12bad198..17ddd7aea2dd 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -12,6 +12,7 @@
 #define	_LINUX_SLAB_H
 
 #include <linux/gfp.h>
+#include <linux/poison.h>
 #include <linux/types.h>
 #include <linux/workqueue.h>
 
@@ -109,7 +110,7 @@
  * ZERO_SIZE_PTR can be passed to kfree though in the same way that NULL can.
  * Both make kfree a no-op.
  */
-#define ZERO_SIZE_PTR ((void *)16)
+#define ZERO_SIZE_PTR ((void *)(16 + POISON_POINTER_DELTA))
 
 #define ZERO_OR_NULL_PTR(x) ((unsigned long)(x) <= \
 				(unsigned long)ZERO_SIZE_PTR)
-- 
2.7.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] slab: Add POISON_POINTER_DELTA to ZERO_SIZE_PTR

[Kees Cook]

On Tue, Nov 15, 2016 at 2:57 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> POISON_POINTER_DELTA is defined in poison.h, and is intended to be used
> to shift poison values so that they don't alias userspace.
>
> We should add it to ZERO_SIZE_PTR so that attackers can't use
> ZERO_SIZE_PTR as a way to get a pointer to userspace.

Ah, when dealing with a 0-sized malloc or similar? Do you have
pointers to exploits that rely on this?

Regardless, normally PAN/SMAP-like things should be sufficient to
protect against this. Additionally, on everything but x86_64 and
arm64, POISON_POINTER_DELTA == 0, if I'm reading correctly:

#ifdef CONFIG_ILLEGAL_POINTER_VALUE
# define POISON_POINTER_DELTA _AC(CONFIG_ILLEGAL_POINTER_VALUE, UL)
#else
# define POISON_POINTER_DELTA 0
#endif

...

config ILLEGAL_POINTER_VALUE
       hex
       default 0 if X86_32
       default 0xdead000000000000 if X86_64

...

config ILLEGAL_POINTER_VALUE
        hex
        default 0xdead000000000000

Is the plan to add ILLEGAL_POINTER_VALUE for powerpc too? And either
way, this patch, IIUC, will break the ZERO_OR_NULL_PTR() check, since
suddenly all of userspace will match it. (Though maybe that's okay?)

-Kees

>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  include/linux/slab.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 084b12bad198..17ddd7aea2dd 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -12,6 +12,7 @@
>  #define        _LINUX_SLAB_H
>
>  #include <linux/gfp.h>
> +#include <linux/poison.h>
>  #include <linux/types.h>
>  #include <linux/workqueue.h>
>
> @@ -109,7 +110,7 @@
>   * ZERO_SIZE_PTR can be passed to kfree though in the same way that NULL can.
>   * Both make kfree a no-op.
>   */
> -#define ZERO_SIZE_PTR ((void *)16)
> +#define ZERO_SIZE_PTR ((void *)(16 + POISON_POINTER_DELTA))
>
>  #define ZERO_OR_NULL_PTR(x) ((unsigned long)(x) <= \
>                                 (unsigned long)ZERO_SIZE_PTR)
> --
> 2.7.4
>



-- 
Kees Cook
Nexus Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [kernel-hardening] Re: [PATCH] slab: Add POISON_POINTER_DELTA to ZERO_SIZE_PTR

[Michael Ellerman]

Kees Cook <keescook@chromium.org> writes:

> On Tue, Nov 15, 2016 at 2:57 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>> POISON_POINTER_DELTA is defined in poison.h, and is intended to be used
>> to shift poison values so that they don't alias userspace.
>>
>> We should add it to ZERO_SIZE_PTR so that attackers can't use
>> ZERO_SIZE_PTR as a way to get a pointer to userspace.
>
> Ah, when dealing with a 0-sized malloc or similar?

Yeah as returned by a 0-sized kmalloc for example.

> Do you have pointers to exploits that rely on this?

Not real ones, it was used in the StringIPC challenge:

https://poppopret.org/2015/11/16/csaw-ctf-2015-kernel-exploitation-challenge/

Though that included the ability to seek to an arbitrary offset from the
zero size pointer, so this wouldn't have helped.

> Regardless, normally PAN/SMAP-like things should be sufficient to
> protect against this.

True. Not everyone has PAN/SMAP though :)

> Additionally, on everything but x86_64 and arm64, POISON_POINTER_DELTA
> == 0, if I'm reading correctly:

You are reading correctly. All 64-bit arches should be able to define it
to something though.

> Is the plan to add ILLEGAL_POINTER_VALUE for powerpc too?

Yep. I should have CC'ed you on the patch :)

> And either way, this patch, IIUC, will break the ZERO_OR_NULL_PTR()
> check, since suddenly all of userspace will match it. (Though maybe
> that's okay?)

Yeah I wasn't sure what to do with that.

I don't think it breaks it, but it does become a bit fishy because as
you say all of userspace (and more) will now match.

It should probably just become two separate tests, though that
potentially has issues with double evaluation of the argument. AFAICS
none of the callers pass an expression though.

cheers

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [kernel-hardening] Re: [PATCH] slab: Add POISON_POINTER_DELTA to ZERO_SIZE_PTR

[Kees Cook]

On Tue, Nov 15, 2016 at 3:50 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> Kees Cook <keescook@chromium.org> writes:
>
>> On Tue, Nov 15, 2016 at 2:57 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>>> POISON_POINTER_DELTA is defined in poison.h, and is intended to be used
>>> to shift poison values so that they don't alias userspace.
>>>
>>> We should add it to ZERO_SIZE_PTR so that attackers can't use
>>> ZERO_SIZE_PTR as a way to get a pointer to userspace.
>>
>> Ah, when dealing with a 0-sized malloc or similar?
>
> Yeah as returned by a 0-sized kmalloc for example.
>
>> Do you have pointers to exploits that rely on this?
>
> Not real ones, it was used in the StringIPC challenge:
>
> https://poppopret.org/2015/11/16/csaw-ctf-2015-kernel-exploitation-challenge/
>
> Though that included the ability to seek to an arbitrary offset from the
> zero size pointer, so this wouldn't have helped.
>
>> Regardless, normally PAN/SMAP-like things should be sufficient to
>> protect against this.
>
> True. Not everyone has PAN/SMAP though :)

Right, mostly just thinking out loud about the threat model and the
existing results.

>> Additionally, on everything but x86_64 and arm64, POISON_POINTER_DELTA
>> == 0, if I'm reading correctly:
>
> You are reading correctly. All 64-bit arches should be able to define it
> to something though.
>
>> Is the plan to add ILLEGAL_POINTER_VALUE for powerpc too?
>
> Yep. I should have CC'ed you on the patch :)

I suspected I was missing something. ;)

>> And either way, this patch, IIUC, will break the ZERO_OR_NULL_PTR()
>> check, since suddenly all of userspace will match it. (Though maybe
>> that's okay?)
>
> Yeah I wasn't sure what to do with that.

Yeah, though there are shockingly few callers of that macro. I think
building with HARDENED_USERCOPY would totally break the kernel,
though, since check_bogus_address() is looking at ZERO_OR_NULL even
for things destined for userspace.

> I don't think it breaks it, but it does become a bit fishy because as
> you say all of userspace (and more) will now match.
>
> It should probably just become two separate tests, though that
> potentially has issues with double evaluation of the argument. AFAICS
> none of the callers pass an expression though.

That shouldn't be a problem. I think we can use fancy magic like:

#define ZERO_OR_NULL_PTR(x) \
 ({ \
    unsigned long p = (unsigned long)(x); \
    (p == NULL || p == ZERO_SIZE_PTR); \
   })

Though this technically loses the check for values 1 through 15...

-Kees

-- 
Kees Cook
Nexus Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] slab: Add POISON_POINTER_DELTA to ZERO_SIZE_PTR

[Balbir Singh]

On 15/11/16 21:57, Michael Ellerman wrote:
> POISON_POINTER_DELTA is defined in poison.h, and is intended to be used
> to shift poison values so that they don't alias userspace.
> 
> We should add it to ZERO_SIZE_PTR so that attackers can't use
> ZERO_SIZE_PTR as a way to get a pointer to userspace.
> 
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  include/linux/slab.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 084b12bad198..17ddd7aea2dd 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -12,6 +12,7 @@
>  #define	_LINUX_SLAB_H
>  
>  #include <linux/gfp.h>
> +#include <linux/poison.h>
>  #include <linux/types.h>
>  #include <linux/workqueue.h>
>  
> @@ -109,7 +110,7 @@
>   * ZERO_SIZE_PTR can be passed to kfree though in the same way that NULL can.
>   * Both make kfree a no-op.
>   */
> -#define ZERO_SIZE_PTR ((void *)16)
> +#define ZERO_SIZE_PTR ((void *)(16 + POISON_POINTER_DELTA))
>  
>  #define ZERO_OR_NULL_PTR(x) ((unsigned long)(x) <= \
>  				(unsigned long)ZERO_SIZE_PTR)
> 

I wonder if we should make this a variable with boot time entropy
within a certain region

Balbir Singh.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>