| Message ID | 1479207422-6535-1-git-send-email-mpe@ellerman.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <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
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
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.
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)
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(-)