Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1302283 > unrolled thread

Re: [RFC][PATCH 0/7] Sanitization of slabs based on grsecurity/PaX

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC][PATCH 0/7] Sanitization of slabs based on grsecurity/PaX Kees Cook <keescook@chromium.org> - 2016-01-06 01:10 +0100
    Re: [RFC][PATCH 0/7] Sanitization of slabs based on grsecurity/PaX Laura Abbott <laura@labbott.name> - 2016-01-06 04:20 +0100
      Re: [RFC][PATCH 0/7] Sanitization of slabs based on grsecurity/PaX Christoph Lameter <cl@linux.com> - 2016-01-07 17:30 +0100
        Re: [RFC][PATCH 0/7] Sanitization of slabs based on grsecurity/PaX Laura Abbott <laura@labbott.name> - 2016-01-08 02:30 +0100
          Re: [RFC][PATCH 0/7] Sanitization of slabs based on grsecurity/PaX Christoph Lameter <cl@linux.com> - 2016-01-08 15:10 +0100
            Re: [RFC][PATCH 0/7] Sanitization of slabs based on grsecurity/PaX Laura Abbott <laura@labbott.name> - 2016-01-14 05:00 +0100

#1302283 — Re: [RFC][PATCH 0/7] Sanitization of slabs based on grsecurity/PaX

On Tue, Dec 22, 2015 at 12:04 PM, Laura Abbott <laura@labbott.name> wrote:
> On 12/22/15 8:08 AM, Christoph Lameter wrote:
>>
>> On Mon, 21 Dec 2015, Laura Abbott wrote:
>>
>>> The biggest change from PAX_MEMORY_SANTIIZE is that this feature
>>> sanitizes
>>> the SL[AOU]B allocators only. My plan is to work on the buddy allocator
>>> santization after this series gets picked up. A side effect of this is
>>> that allocations which go directly to the buddy allocator (i.e. large
>>> allocations) aren't sanitized. I'd like feedback about whether it's worth
>>> it to add sanitization on that path directly or just use the page
>>> allocator sanitization when that comes in.

This looks great! I love the added lkdtm tests, too. Very cool.

>> I am not sure what the point of this patchset is. We have a similar effect
>> to sanitization already in the allocators through two mechanisms:
>>
>> 1. Slab poisoning
>> 2. Allocation with GFP_ZERO
>>
>> I do not think we need a third one. You could accomplish your goals much
>> easier without this code churn by either
>>
>> 1. Improve the existing poisoning mechanism. Ensure that there are no
>>     gaps. Security sensitive kernel slab caches can then be created with
>>     the  POISONING flag set. Maybe add a Kconfig flag that enables
>>     POISONING for each cache? What was the issue when you tried using
>>     posining for sanitization?
>
> The existing poisoning does work for sanitization but it's still a debug
> feature. It seemed more appropriate to keep debug features and non-debug
> features separate hence the separate option and configuration.

What stuff is intertwined in the existing poisoning that makes it
incompatible/orthogonal?

>> 2. Add a mechanism that ensures that GFP_ZERO is set for each allocation.
>>     That way every object you retrieve is zeroed and thus you have implied
>>     sanitization. This also can be done in a rather simple way by changing
>>     the  GFP_KERNEL etc constants to include __GFP_ZERO depending on a
>>     Kconfig option. Or add some runtime setting of the gfp flags
>> somewhere.
>>
>
> That's good for allocation but sanitization is done on free. The goal
> is to reduce any leftover data that might be around while on an unallocated
> slab.

Right -- we want this on free. I wonder if we could also add the
always-zero option as an additional improvement. A separate config,
since I suspect the overhead would be ugly.

>> Generally I would favor option #2 if you must have sanitization because
>> that is the only option to really give you a deterministic content of
>> object on each allocation. Any half way measures would not work I think.
>>
>> Note also that most allocations are already either allocations that zero
>> the content or they are immediately initializing the content of the
>> allocated object. After all the object is not really usable if the
>> content is random. You may be able to avoid this whole endeavor by
>> auditing the kernel for locations where the object is not initialized
>> after allocation.
>>
>> Once one recognizes the above it seems that sanitization is pretty
>> useless. Its just another pass of writing zeroes before the allocator or
>> uer of the allocated object sets up deterministic content of the object or
>> -- in most cases -- zeroes it again.
>>
>
> The sanitization is going towards kernel hardening which is designed to
> help keep the kernel secure even when programmers screwed up. Auditing
> still won't catch everything. sanitization is going towards the idea
> of kernel self-protection which is what Grsecurity is known for
> and Kees Cook is trying to promote for mainline
> (http://lwn.net/Articles/662219/)

Yup, well said. Auditing is important, and we're already doing it, but
we want to catch the mistakes during runtime, since we'll never be
free of bugs, bug lifetime is measured in years, and end users are
frequently forced to run Linux with additional non-upstream code, so
we want to protect them from those mistakes as well.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1302359

On 1/5/16 4:09 PM, Kees Cook wrote:
> On Tue, Dec 22, 2015 at 12:04 PM, Laura Abbott <laura@labbott.name> wrote:
>> On 12/22/15 8:08 AM, Christoph Lameter wrote:
>>>
>>> On Mon, 21 Dec 2015, Laura Abbott wrote:
>>>
>>>> The biggest change from PAX_MEMORY_SANTIIZE is that this feature
>>>> sanitizes
>>>> the SL[AOU]B allocators only. My plan is to work on the buddy allocator
>>>> santization after this series gets picked up. A side effect of this is
>>>> that allocations which go directly to the buddy allocator (i.e. large
>>>> allocations) aren't sanitized. I'd like feedback about whether it's worth
>>>> it to add sanitization on that path directly or just use the page
>>>> allocator sanitization when that comes in.
>
> This looks great! I love the added lkdtm tests, too. Very cool.
>
>>> I am not sure what the point of this patchset is. We have a similar effect
>>> to sanitization already in the allocators through two mechanisms:
>>>
>>> 1. Slab poisoning
>>> 2. Allocation with GFP_ZERO
>>>
>>> I do not think we need a third one. You could accomplish your goals much
>>> easier without this code churn by either
>>>
>>> 1. Improve the existing poisoning mechanism. Ensure that there are no
>>>      gaps. Security sensitive kernel slab caches can then be created with
>>>      the  POISONING flag set. Maybe add a Kconfig flag that enables
>>>      POISONING for each cache? What was the issue when you tried using
>>>      posining for sanitization?
>>
>> The existing poisoning does work for sanitization but it's still a debug
>> feature. It seemed more appropriate to keep debug features and non-debug
>> features separate hence the separate option and configuration.
>
> What stuff is intertwined in the existing poisoning that makes it
> incompatible/orthogonal?
>

It's not the poisoning per se that's incompatible, it's how the poisoning is
set up. At least for slub, the current poisoning is part of SLUB_DEBUG which
enables other consistency checks on the allocator. Trying to pull out just
the poisoning for use when SLUB_DEBUG isn't on would result in roughly what
would be here anyway. I looked at trying to reuse some of the existing poisoning
and came to the conclusion it was less intrusive to the allocator to keep it
separate.

Thanks,
Laura
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1303723

On Tue, 5 Jan 2016, Laura Abbott wrote:

> It's not the poisoning per se that's incompatible, it's how the poisoning is
> set up. At least for slub, the current poisoning is part of SLUB_DEBUG which
> enables other consistency checks on the allocator. Trying to pull out just
> the poisoning for use when SLUB_DEBUG isn't on would result in roughly what
> would be here anyway. I looked at trying to reuse some of the existing
> poisoning
> and came to the conclusion it was less intrusive to the allocator to keep it
> separate.

SLUB_DEBUG does *not* enable any debugging features. It builds the logic
for debugging into the kernel but does not activate it. CONFIG_SLUB_DEBUG
is set for production kernels. The poisoning is build in by default into
any recent linux kernel out there. You can enable poisoning selectively
(and no other debug feature) by specifying slub_debug=P on the Linux
kernel command line right now.

There is a SLAB_POISON flag for each kmem_cache that can be set to
*only* enable poisoning and nothing else from code.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1304092

On 1/7/16 8:26 AM, Christoph Lameter wrote:
> On Tue, 5 Jan 2016, Laura Abbott wrote:
>
>> It's not the poisoning per se that's incompatible, it's how the poisoning is
>> set up. At least for slub, the current poisoning is part of SLUB_DEBUG which
>> enables other consistency checks on the allocator. Trying to pull out just
>> the poisoning for use when SLUB_DEBUG isn't on would result in roughly what
>> would be here anyway. I looked at trying to reuse some of the existing
>> poisoning
>> and came to the conclusion it was less intrusive to the allocator to keep it
>> separate.
>
> SLUB_DEBUG does *not* enable any debugging features. It builds the logic
> for debugging into the kernel but does not activate it. CONFIG_SLUB_DEBUG
> is set for production kernels. The poisoning is build in by default into
> any recent linux kernel out there. You can enable poisoning selectively
> (and no other debug feature) by specifying slub_debug=P on the Linux
> kernel command line right now.
>
> There is a SLAB_POISON flag for each kmem_cache that can be set to
> *only* enable poisoning and nothing else from code.
>
>

The slub_debug=P not only poisons it enables other consistency checks on the
slab as well, assuming my understanding of what check_object does is correct.
My hope was to have the poison part only and none of the consistency checks in
an attempt to mitigate performance issues. I misunderstood when the checks
actually run and how SLUB_DEBUG was used.

Another option would be to have a flag like SLAB_NO_SANITY_CHECK.
sanitization enablement would just be that and SLAB_POISON
in the debug options. The disadvantage to this approach would be losing
the sanitization for ->ctor caches (the grsecurity version works around this
by re-initializing with ->ctor, I haven't heard any feedback if this actually
acceptable) and not having some of the fast paths enabled
(assuming I'm understanding the code path correctly.) which would also
be a performance penalty

Thanks,
Laura

[toc] | [prev] | [next] | [standalone]

#1304569

On Thu, 7 Jan 2016, Laura Abbott wrote:

> The slub_debug=P not only poisons it enables other consistency checks on the
> slab as well, assuming my understanding of what check_object does is correct.
> My hope was to have the poison part only and none of the consistency checks in
> an attempt to mitigate performance issues. I misunderstood when the checks
> actually run and how SLUB_DEBUG was used.

Ok I see that there pointer check is done without checking the
corresponding debug flag. Patch attached thar fixes it.

> Another option would be to have a flag like SLAB_NO_SANITY_CHECK.
> sanitization enablement would just be that and SLAB_POISON
> in the debug options. The disadvantage to this approach would be losing
> the sanitization for ->ctor caches (the grsecurity version works around this
> by re-initializing with ->ctor, I haven't heard any feedback if this actually
> acceptable) and not having some of the fast paths enabled
> (assuming I'm understanding the code path correctly.) which would also
> be a performance penalty

I think we simply need to fix the missing check there. There is already a
flag SLAB_DEBUG_FREE for the pointer checks.



Subject: slub: Only perform pointer checks in check_object when SLAB_DEBUG_FREE is set

Seems that check_object() always checks for pointer issues currently.

Signed-off-by: Christoph Lameter <cl@linux.com>

Index: linux/mm/slub.c
===================================================================
--- linux.orig/mm/slub.c
+++ linux/mm/slub.c
@@ -848,6 +848,9 @@ static int check_object(struct kmem_cach
 		 */
 		return 1;

+	if (!(s->flags & SLAB_DEBUG_FREE))
+		return 1;
+
 	/* Check free pointer validity */
 	if (!check_valid_pointer(s, page, get_freepointer(s, p))) {
 		object_err(s, page, p, "Freepointer corrupt");

[toc] | [prev] | [next] | [standalone]

#1308984

On 1/8/16 6:07 AM, Christoph Lameter wrote:
> On Thu, 7 Jan 2016, Laura Abbott wrote:
>
>> The slub_debug=P not only poisons it enables other consistency checks on the
>> slab as well, assuming my understanding of what check_object does is correct.
>> My hope was to have the poison part only and none of the consistency checks in
>> an attempt to mitigate performance issues. I misunderstood when the checks
>> actually run and how SLUB_DEBUG was used.
>
> Ok I see that there pointer check is done without checking the
> corresponding debug flag. Patch attached thar fixes it.
>
>> Another option would be to have a flag like SLAB_NO_SANITY_CHECK.
>> sanitization enablement would just be that and SLAB_POISON
>> in the debug options. The disadvantage to this approach would be losing
>> the sanitization for ->ctor caches (the grsecurity version works around this
>> by re-initializing with ->ctor, I haven't heard any feedback if this actually
>> acceptable) and not having some of the fast paths enabled
>> (assuming I'm understanding the code path correctly.) which would also
>> be a performance penalty
>
> I think we simply need to fix the missing check there. There is already a
> flag SLAB_DEBUG_FREE for the pointer checks.
>
>

The patch improves performance but the overall performance of these full
sanitization patches is still significantly better than slub_debug=P. I'll
put some effort into seeing if I can figure out where the slow down is
coming from.

Thanks,
Laura

>
> Subject: slub: Only perform pointer checks in check_object when SLAB_DEBUG_FREE is set
>
> Seems that check_object() always checks for pointer issues currently.
>
> Signed-off-by: Christoph Lameter <cl@linux.com>
>
> Index: linux/mm/slub.c
> ===================================================================
> --- linux.orig/mm/slub.c
> +++ linux/mm/slub.c
> @@ -848,6 +848,9 @@ static int check_object(struct kmem_cach
>   		 */
>   		return 1;
>
> +	if (!(s->flags & SLAB_DEBUG_FREE))
> +		return 1;
> +
>   	/* Check free pointer validity */
>   	if (!check_valid_pointer(s, page, get_freepointer(s, p))) {
>   		object_err(s, page, p, "Freepointer corrupt");
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web