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

Groups > linux.kernel > #1690606 > unrolled thread

Re: [PATCH 1/1] mm/slub.c: add a naive detection of double free or corruption

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: [PATCH 1/1] mm/slub.c: add a naive detection of double free or  corruption Alexander Popov <alex.popov@linux.com> - 2017-07-18 22:00 +0200
    Re: [PATCH 1/1] mm/slub.c: add a naive detection of double free or corruption Kees Cook <keescook@chromium.org> - 2017-07-18 22:10 +0200
      Re: [PATCH 1/1] mm/slub.c: add a naive detection of double free or  corruption Alexander Popov <alex.popov@linux.com> - 2017-07-19 10:40 +0200
      Re: [PATCH 1/1] mm/slub.c: add a naive detection of double free or  corruption Christopher Lameter <cl@linux.com> - 2017-07-19 16:10 +0200

#1690606 — Re: [PATCH 1/1] mm/slub.c: add a naive detection of double free or corruption

On 17.07.2017 22:11, Kees Cook wrote:
> On Mon, Jul 17, 2017 at 12:01 PM, Alexander Popov <alex.popov@linux.com> wrote:
>> Hello Christopher,
>>
>> Thanks for your reply.
>>
>> On 17.07.2017 21:04, Christopher Lameter wrote:
>>> On Mon, 17 Jul 2017, Matthew Wilcox wrote:
>>>
>>>> On Mon, Jul 17, 2017 at 07:45:07PM +0300, Alexander Popov wrote:
>>>>> Add an assertion similar to "fasttop" check in GNU C Library allocator:
>>>>> an object added to a singly linked freelist should not point to itself.
>>>>> That helps to detect some double free errors (e.g. CVE-2017-2636) without
>>>>> slub_debug and KASAN. Testing with hackbench doesn't show any noticeable
>>>>> performance penalty.
>>>>
>>>>>  {
>>>>> +   BUG_ON(object == fp); /* naive detection of double free or corruption */
>>>>>     *(void **)(object + s->offset) = fp;
>>>>>  }
>>>>
>>>> Is BUG() the best response to this situation?  If it's a corruption, then
>>>> yes, but if we spot a double-free, then surely we should WARN() and return
>>>> without doing anything?
>>>
>>> The double free debug checking already does the same thing in a more
>>> thourough way (this one only checks if the last free was the same
>>> address). So its duplicating a check that already exists.
>>
>> Yes, absolutely. Enabled slub_debug (or KASAN with its quarantine) can detect
>> more double-free errors. But it introduces much bigger performance penalty and
>> it's disabled by default.
>>
>>> However, this one is always on.
>>
>> Yes, I would propose to have this relatively cheap check enabled by default. I
>> think it will block a good share of double-free errors. Currently it's really
>> easy to turn such a double-free into use-after-free and exploit it, since, as I
>> wrote, next two kmalloc() calls return the same address. So we could make
>> exploiting harder for a relatively low price.
>>
>> Christopher, if I change BUG_ON() to VM_BUG_ON(), it will be disabled by default
>> again, right?
> 
> Let's merge this with the proposed CONFIG_FREELIST_HARDENED, then the
> performance change is behind a config, and we gain the rest of the
> freelist protections at the same time:
> 
> http://www.openwall.com/lists/kernel-hardening/2017/07/06/1

Hello Kees,

If I change BUG_ON() to VM_BUG_ON(), this check will work at least on Fedora
since it has CONFIG_DEBUG_VM enabled. Debian based distros have this option
disabled. Do you like that more than having this check under
CONFIG_FREELIST_HARDENED?

If you insist on putting this check under CONFIG_FREELIST_HARDENED, should I
rebase onto your patch and send again?

Best regards,
Alexander

[toc] | [next] | [standalone]

#1690615 — Re: [PATCH 1/1] mm/slub.c: add a naive detection of double free or corruption

On Tue, Jul 18, 2017 at 12:56 PM, Alexander Popov <alex.popov@linux.com> wrote:
> On 17.07.2017 22:11, Kees Cook wrote:
>> On Mon, Jul 17, 2017 at 12:01 PM, Alexander Popov <alex.popov@linux.com> wrote:
>>> Hello Christopher,
>>>
>>> Thanks for your reply.
>>>
>>> On 17.07.2017 21:04, Christopher Lameter wrote:
>>>> On Mon, 17 Jul 2017, Matthew Wilcox wrote:
>>>>
>>>>> On Mon, Jul 17, 2017 at 07:45:07PM +0300, Alexander Popov wrote:
>>>>>> Add an assertion similar to "fasttop" check in GNU C Library allocator:
>>>>>> an object added to a singly linked freelist should not point to itself.
>>>>>> That helps to detect some double free errors (e.g. CVE-2017-2636) without
>>>>>> slub_debug and KASAN. Testing with hackbench doesn't show any noticeable
>>>>>> performance penalty.
>>>>>
>>>>>>  {
>>>>>> +   BUG_ON(object == fp); /* naive detection of double free or corruption */
>>>>>>     *(void **)(object + s->offset) = fp;
>>>>>>  }
>>>>>
>>>>> Is BUG() the best response to this situation?  If it's a corruption, then
>>>>> yes, but if we spot a double-free, then surely we should WARN() and return
>>>>> without doing anything?
>>>>
>>>> The double free debug checking already does the same thing in a more
>>>> thourough way (this one only checks if the last free was the same
>>>> address). So its duplicating a check that already exists.
>>>
>>> Yes, absolutely. Enabled slub_debug (or KASAN with its quarantine) can detect
>>> more double-free errors. But it introduces much bigger performance penalty and
>>> it's disabled by default.
>>>
>>>> However, this one is always on.
>>>
>>> Yes, I would propose to have this relatively cheap check enabled by default. I
>>> think it will block a good share of double-free errors. Currently it's really
>>> easy to turn such a double-free into use-after-free and exploit it, since, as I
>>> wrote, next two kmalloc() calls return the same address. So we could make
>>> exploiting harder for a relatively low price.
>>>
>>> Christopher, if I change BUG_ON() to VM_BUG_ON(), it will be disabled by default
>>> again, right?
>>
>> Let's merge this with the proposed CONFIG_FREELIST_HARDENED, then the
>> performance change is behind a config, and we gain the rest of the
>> freelist protections at the same time:
>>
>> http://www.openwall.com/lists/kernel-hardening/2017/07/06/1
>
> Hello Kees,
>
> If I change BUG_ON() to VM_BUG_ON(), this check will work at least on Fedora
> since it has CONFIG_DEBUG_VM enabled. Debian based distros have this option
> disabled. Do you like that more than having this check under
> CONFIG_FREELIST_HARDENED?

I think there are two issues: first, this should likely be under
CONFIG_FREELIST_HARDENED since Christoph hasn't wanted to make these
changes enabled by default (if I'm understanding his earlier review
comments to me). The second issue is what to DO when a double-free is
discovered. Is there any way to make it safe/survivable? If not, I
think it should just be BUG_ON(). If it can be made safe, then likely
a WARN_ONCE and do whatever is needed to prevent the double-free.

> If you insist on putting this check under CONFIG_FREELIST_HARDENED, should I
> rebase onto your patch and send again?

That would be preferred for me -- I'd like to have both checks. :)

-Kees

-- 
Kees Cook
Pixel Security

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

#1691112

On 18.07.2017 23:04, Kees Cook wrote:
> On Tue, Jul 18, 2017 at 12:56 PM, Alexander Popov <alex.popov@linux.com> wrote:
>> On 17.07.2017 22:11, Kees Cook wrote:
>>> Let's merge this with the proposed CONFIG_FREELIST_HARDENED, then the
>>> performance change is behind a config, and we gain the rest of the
>>> freelist protections at the same time:
>>>
>>> http://www.openwall.com/lists/kernel-hardening/2017/07/06/1
>>
>> Hello Kees,
>>
>> If I change BUG_ON() to VM_BUG_ON(), this check will work at least on Fedora
>> since it has CONFIG_DEBUG_VM enabled. Debian based distros have this option
>> disabled. Do you like that more than having this check under
>> CONFIG_FREELIST_HARDENED?
> 
> I think there are two issues: first, this should likely be under
> CONFIG_FREELIST_HARDENED since Christoph hasn't wanted to make these
> changes enabled by default (if I'm understanding his earlier review
> comments to me).

Ok, I'll rebase onto FREELIST_HARDENED and test it all together.

> The second issue is what to DO when a double-free is
> discovered. Is there any way to make it safe/survivable? If not, I
> think it should just be BUG_ON(). If it can be made safe, then likely
> a WARN_ONCE and do whatever is needed to prevent the double-free.

Please correct me if I'm wrong. It seems to me that double-free is a dangerous
situation that indicates some serious kernel bug (which might be maliciously
exploited). So I would not trust / rely on the process which experiences a
double-free error in the kernel mode.

But I guess the reaction to it should depend on the Linux kernel policy of
handling faults. Is it defined explicitly?

Anyway, if we try to mitigate the effect from a double-free error _here_ (for
example, skip putting the duplicated object to the freelist), I think we should
do the same for other cases of double-free and memory corruptions.

>> If you insist on putting this check under CONFIG_FREELIST_HARDENED, should I
>> rebase onto your patch and send again?
> 
> That would be preferred for me -- I'd like to have both checks. :)

Ok.

Best regards,
Alexander

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

#1691706

On Tue, 18 Jul 2017, Kees Cook wrote:

> I think there are two issues: first, this should likely be under
> CONFIG_FREELIST_HARDENED since Christoph hasn't wanted to make these
> changes enabled by default (if I'm understanding his earlier review
> comments to me). The second issue is what to DO when a double-free is
> discovered. Is there any way to make it safe/survivable? If not, I

The simple thing is to not free the object when you discover this. That is
what the existing debugging code does. But you do not have an easy way to
fail at the point in the code that is patched here.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web