[PATCH] Make LIST_POISON less deadly

[PATCH] Make LIST_POISON less deadly

[Avi Kivity]

The list macros use LIST_POISON1 and LIST_POISON2 as undereferencable
pointers in order to trap erronous use of freed list_heads.  Unfortunately
userspace can arrange for those pointers to actually be dereferencable,
potentially turning an oops to an expolit.

To avoid this allow architectures (currently x86_64 only) to override
the default values for these pointers with truly-undereferncable values.
This is easy on x86_64 as the virtual address space is smaller than
the range spanned by pointer values.

Signed-off-by: Avi Kivity <avi@qumranet.com>
---
 arch/x86/Kconfig         |    1 +
 include/asm-x86/poison.h |   14 ++++++++++++++
 include/linux/poison.h   |    4 ++++
 lib/Kconfig              |    3 +++
 4 files changed, 22 insertions(+), 0 deletions(-)
 create mode 100644 include/asm-x86/poison.h

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index fe361ae..0eb34a9 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -25,6 +25,7 @@ config X86
 	select HAVE_KRETPROBES
 	select HAVE_KVM if ((X86_32 && !X86_VOYAGER && !X86_VISWS && !X86_NUMAQ) || X86_64)
 	select HAVE_ARCH_KGDB if !X86_VOYAGER
+	select HAVE_ARCH_POISON
 
 config DEFCONFIG_LIST
 	string
diff --git a/include/asm-x86/poison.h b/include/asm-x86/poison.h
new file mode 100644
index 0000000..26b95dd
--- /dev/null
+++ b/include/asm-x86/poison.h
@@ -0,0 +1,14 @@
+#ifndef _ASM_X86_POISON_H
+#define _ASM_X86_POISON_H
+
+/*
+ * Define LIST_POISON[12] as pointers that cannot be dereferenced.
+ */
+#ifdef CONFIG_X86_84
+#  undef LIST_POISON1
+#  undef LIST_POISON2
+#  define LIST_POISON1 ((void *)0x8001000100010001L)
+#  define LIST_POISON2 ((void *)0x8002000200020002L)
+#endif
+
+#endif
diff --git a/include/linux/poison.h b/include/linux/poison.h
index 9f31683..8bb6ad9 100644
--- a/include/linux/poison.h
+++ b/include/linux/poison.h
@@ -68,4 +68,8 @@
 /********** sound/oss/ **********/
 #define OSS_POISON_FREE		0xAB
 
+#ifdef CONFIG_HAVE_ARCH_POISON
+#include <asm/poison.h>
+#endif
+
 #endif
diff --git a/lib/Kconfig b/lib/Kconfig
index 8cc8e87..31b9457 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -150,4 +150,7 @@ config CHECK_SIGNATURE
 config HAVE_LMB
 	boolean
 
+config HAVE_ARCH_POISON
+	boolean
+
 endmenu
-- 
1.5.5.1

Re: [PATCH] Make LIST_POISON less deadly

[Ingo Molnar]

* Avi Kivity <avi@qumranet.com> wrote:

> The list macros use LIST_POISON1 and LIST_POISON2 as undereferencable 
> pointers in order to trap erronous use of freed list_heads.  
> Unfortunately userspace can arrange for those pointers to actually be 
> dereferencable, potentially turning an oops to an expolit.
> 
> To avoid this allow architectures (currently x86_64 only) to override 
> the default values for these pointers with truly-undereferncable 
> values. This is easy on x86_64 as the virtual address space is smaller 
> than the range spanned by pointer values.

nice idea!

i'd suggest a slightly different solution:

> +/*
> + * Define LIST_POISON[12] as pointers that cannot be dereferenced.
> + */
> +#ifdef CONFIG_X86_84
> +#  undef LIST_POISON1
> +#  undef LIST_POISON2
> +#  define LIST_POISON1 ((void *)0x8001000100010001L)
> +#  define LIST_POISON2 ((void *)0x8002000200020002L)
> +#endif

i'd suggest to add an ARCH_ILLEGAL_POINTER define instead, which 
defaults to zero and gets added to pointer-ish poison values. That makes 
it both simpler and also it does not need any include/asm changes 
because ARCH_ILLEGAL_POINTER can be set from the Kconfig space.

hm?

	Ingo

Re: [PATCH] Make LIST_POISON less deadly

[Avi Kivity]

Ingo Molnar wrote:
>   
>> +/*
>> + * Define LIST_POISON[12] as pointers that cannot be dereferenced.
>> + */
>> +#ifdef CONFIG_X86_84
>> +#  undef LIST_POISON1
>> +#  undef LIST_POISON2
>> +#  define LIST_POISON1 ((void *)0x8001000100010001L)
>> +#  define LIST_POISON2 ((void *)0x8002000200020002L)
>> +#endif
>>     
>
> i'd suggest to add an ARCH_ILLEGAL_POINTER define instead, which 
> defaults to zero and gets added to pointer-ish poison values. That makes 
> it both simpler and also it does not need any include/asm changes 
> because ARCH_ILLEGAL_POINTER can be set from the Kconfig space.
>   

Right.

How much unmapped space do we have on i386 or other archs?  The deltas 
added to ARCH_ILLEGAL_POINTER will have to be limited to that.

Maybe have ARCH_ILLEGAL_POINTER_BASE and ARCH_ILLEGAL_POINTER_RANGE, 
defaulting to 0 and (say) 4MB, and use the range to mask the deltas to 
avoid overflows into legal pointer range.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] Make LIST_POISON less deadly

[Ingo Molnar]

* Avi Kivity <avi@qumranet.com> wrote:

>> i'd suggest to add an ARCH_ILLEGAL_POINTER define instead, which 
>> defaults to zero and gets added to pointer-ish poison values. That 
>> makes it both simpler and also it does not need any include/asm 
>> changes because ARCH_ILLEGAL_POINTER can be set from the Kconfig 
>> space.
>
> Right.
>
> How much unmapped space do we have on i386 or other archs?  The deltas 
> added to ARCH_ILLEGAL_POINTER will have to be limited to that.

can we get away with having the offset default to zero, and all poison 
values remain unchanged if that? Then 64-bit x86 could just set the 
offset to 2^63 and we wont have to worry about 32-bit x86 at all ...

i.e. start small with this and only have an effect on 64-bit x86.

	Ingo

Re: [PATCH] Make LIST_POISON less deadly

[Avi Kivity]

Ingo Molnar wrote:
> * Avi Kivity <avi@qumranet.com> wrote:
>
>   
>>> i'd suggest to add an ARCH_ILLEGAL_POINTER define instead, which 
>>> defaults to zero and gets added to pointer-ish poison values. That 
>>> makes it both simpler and also it does not need any include/asm 
>>> changes because ARCH_ILLEGAL_POINTER can be set from the Kconfig 
>>> space.
>>>       
>> Right.
>>
>> How much unmapped space do we have on i386 or other archs?  The deltas 
>> added to ARCH_ILLEGAL_POINTER will have to be limited to that.
>>     
>
> can we get away with having the offset default to zero, and all poison 
> values remain unchanged if that? Then 64-bit x86 could just set the 
> offset to 2^63 and we wont have to worry about 32-bit x86 at all ...
>
> i.e. start small with this and only have an effect on 64-bit x86.
>   

Sure, will post an updated patch.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] Make LIST_POISON less deadly

[Avi Kivity]

Ingo Molnar wrote:
> i'd suggest to add an ARCH_ILLEGAL_POINTER define instead, which 
> defaults to zero and gets added to pointer-ish poison values. That makes 
> it both simpler and also it does not need any include/asm changes 
> because ARCH_ILLEGAL_POINTER can be set from the Kconfig space.
>   

Any idea how to override a value in Kconfig?  We can do it for bool 
values but not strings.

e.g.

lib/Kconfig:
  config ILLEGAL_POINTER_VALUE
      hex
      default 0

arch/x86/Kconfig:
  config X86
    select ILLEGAL_POINTER_VALUE=0x80000000000000000000000000 if X86_64

?

I could do it with a boolean signifying existence and a hex for the 
value, but I'd rather not.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] Make LIST_POISON less deadly

[Sam Ravnborg]

On Mon, May 19, 2008 at 04:45:03PM +0300, Avi Kivity wrote:
> Ingo Molnar wrote:
> >i'd suggest to add an ARCH_ILLEGAL_POINTER define instead, which 
> >defaults to zero and gets added to pointer-ish poison values. That makes 
> >it both simpler and also it does not need any include/asm changes 
> >because ARCH_ILLEGAL_POINTER can be set from the Kconfig space.
> >  
> 
> Any idea how to override a value in Kconfig?  We can do it for bool 
> values but not strings.
> 
> e.g.
> 
> lib/Kconfig:
>  config ILLEGAL_POINTER_VALUE
>      hex
>      default 0
> 
> arch/x86/Kconfig:
>  config X86
>    select ILLEGAL_POINTER_VALUE=0x80000000000000000000000000 if X86_64
> 
> ?
> 
> I could do it with a boolean signifying existence and a hex for the 
> value, but I'd rather not.

Sorry - no help here. I have not tried to work that much with int in
Kconfig.

That said I really think stuff like this belongs in a header file.
I do not agree to hide this as part of the configuration.

	Sam

Re: [PATCH] Make LIST_POISON less deadly

[Avi Kivity]

Sam Ravnborg wrote:
>> Any idea how to override a value in Kconfig?  We can do it for bool 
>> values but not strings.
>>
>> e.g.
>>
>> lib/Kconfig:
>>  config ILLEGAL_POINTER_VALUE
>>      hex
>>      default 0
>>
>> arch/x86/Kconfig:
>>  config X86
>>    select ILLEGAL_POINTER_VALUE=0x80000000000000000000000000 if X86_64
>>
>> ?
>>
>> I could do it with a boolean signifying existence and a hex for the 
>> value, but I'd rather not.
>>     
>
> Sorry - no help here. I have not tried to work that much with int in
> Kconfig.
>
>   

I found some workaround (define the variable only on archs that want it, 
use #ifdef to check).

> That said I really think stuff like this belongs in a header file.
> I do not agree to hide this as part of the configuration.
>   

While I agree with you, I defer to Ingo on this.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] Make LIST_POISON less deadly

[Andi Kleen]

Avi Kivity <avi@qumranet.com> writes:

> The list macros use LIST_POISON1 and LIST_POISON2 as undereferencable
> pointers in order to trap erronous use of freed list_heads.  Unfortunately
> userspace can arrange for those pointers to actually be dereferencable,
> potentially turning an oops to an expolit.
>
> To avoid this allow architectures (currently x86_64 only) to override
> the default values for these pointers with truly-undereferncable values.
> This is easy on x86_64 as the virtual address space is smaller than
> the range spanned by pointer values.

Hmm, thought I had sent a reply earlier, but don't see it so again.
My apologies if you see it twice.

The problem with your address values is that they're non canonical
and will result in a #GP, not #PF and oops handler cannot display
the address which will make them much less obvious.

I would rather use a guaranteed to be unmapped but canonical
address like in the ffffc10000000000 - ffffc1ffffffffff range
so that you still get page faults.

-Andi

Re: [PATCH] Make LIST_POISON less deadly

[Avi Kivity]

Andi Kleen wrote:
> Avi Kivity <avi@qumranet.com> writes:
>
>   
>> The list macros use LIST_POISON1 and LIST_POISON2 as undereferencable
>> pointers in order to trap erronous use of freed list_heads.  Unfortunately
>> userspace can arrange for those pointers to actually be dereferencable,
>> potentially turning an oops to an expolit.
>>
>> To avoid this allow architectures (currently x86_64 only) to override
>> the default values for these pointers with truly-undereferncable values.
>> This is easy on x86_64 as the virtual address space is smaller than
>> the range spanned by pointer values.
>>     
>
> Hmm, thought I had sent a reply earlier, but don't see it so again.
> My apologies if you see it twice.
>   

No, this is the first one I see.

> The problem with your address values is that they're non canonical
> and will result in a #GP, not #PF and oops handler cannot display
> the address which will make them much less obvious.
>
> I would rather use a guaranteed to be unmapped but canonical
> address like in the ffffc10000000000 - ffffc1ffffffffff range
> so that you still get page faults.
>   

Makes sense.  I'll send out v3.

Is there a similar range on i386?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] Make LIST_POISON less deadly

[Andi Kleen]

> Makes sense.  I'll send out v3.
> 
> Is there a similar range on i386?

Don't think so. Address space is too precious here.

What you could probably do is to unmap one fixed page in some range
that is always split to 4K pages anyways and use that. Perhaps somewhere
in the 640k-1MB range. But if you're unlucky this might require a
variable, not a constant.

-Andi

Re: [PATCH] Make LIST_POISON less deadly

[Avi Kivity]

Andi Kleen wrote:
>> Makes sense.  I'll send out v3.
>>
>> Is there a similar range on i386?
>>     
>
> Don't think so. Address space is too precious here.
>
> What you could probably do is to unmap one fixed page in some range
> that is always split to 4K pages anyways and use that. Perhaps somewhere
> in the 640k-1MB range. But if you're unlucky this might require a
> variable, not a constant.
>   

I guess a fixmap would work for this.  But then the offsets added to 
that page would need to be limited to 4K.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] Make LIST_POISON less deadly

[Andi Kleen]

Avi Kivity wrote:
> Andi Kleen wrote:
>>> Makes sense.  I'll send out v3.
>>>
>>> Is there a similar range on i386?
>>>     
>>
>> Don't think so. Address space is too precious here.
>>
>> What you could probably do is to unmap one fixed page in some range
>> that is always split to 4K pages anyways and use that. Perhaps somewhere
>> in the 640k-1MB range. But if you're unlucky this might require a
>> variable, not a constant.
>>   
> 
> I guess a fixmap would work for this.  But then the offsets added to
> that page would need to be limited to 4K.

You could make a 3 page fixmap and point to the middle page. 3 because
negative offsets are also especially importants for list_heads due to
list_entry

-Andi

Re: [PATCH] Make LIST_POISON less deadly

[Ingo Molnar]

* Avi Kivity <avi@qumranet.com> wrote:

> I guess a fixmap would work for this.  But then the offsets added to 
> that page would need to be limited to 4K.

i dont think it's worth going for 32-bit here. On 32-bit the poison 
value gets skewed into hard to recognize values which might make oops 
analysis harder. Lets start small with 64-bit-only - there it's a quite 
plausible change, besides the exponentially larger address space on 
64-bit it might realistically happen that an attacker can control a 
32-bit-is offset but not a full 64-bit offset.

	Ingo

Re: [PATCH] Make LIST_POISON less deadly

[Pavel Machek]

On Sun 2008-05-18 18:38:14, Avi Kivity wrote:
> The list macros use LIST_POISON1 and LIST_POISON2 as undereferencable
> pointers in order to trap erronous use of freed list_heads.  Unfortunately
> userspace can arrange for those pointers to actually be dereferencable,
> potentially turning an oops to an expolit.
> 
> To avoid this allow architectures (currently x86_64 only) to override
> the default values for these pointers with truly-undereferncable values.
> This is easy on x86_64 as the virtual address space is smaller than
> the range spanned by pointer values.

"Security hole unless arch maintainer does _foo_" sounds
scary. Especially when i386 is hard to fix...

(And very nice catch, btw).
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] Make LIST_POISON less deadly

[Avi Kivity]

Pavel Machek wrote:
> On Sun 2008-05-18 18:38:14, Avi Kivity wrote:
>   
>> The list macros use LIST_POISON1 and LIST_POISON2 as undereferencable
>> pointers in order to trap erronous use of freed list_heads.  Unfortunately
>> userspace can arrange for those pointers to actually be dereferencable,
>> potentially turning an oops to an expolit.
>>
>> To avoid this allow architectures (currently x86_64 only) to override
>> the default values for these pointers with truly-undereferncable values.
>> This is easy on x86_64 as the virtual address space is smaller than
>> the range spanned by pointer values.
>>     
>
> "Security hole unless arch maintainer does _foo_" sounds
> scary. Especially when i386 is hard to fix...
>   

It's a potential security hole.  You need to find a list corruption 
first.  The patch prevents escalation of the oops into a code injection.

i386 is fixable, though it will take more work than x86_64.

-- 
error compiling committee.c: too many arguments to function