| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 26 Feb 2016 14:21:03 -0800
From: Laura Abbott <labbott@...hat.com>
To: Jianyu Zhan <nasa4836@...il.com>
Cc: Kees Cook <keescook@...omium.org>,
Laura Abbott <labbott@...oraproject.org>,
Andrew Morton <akpm@...ux-foundation.org>,
"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
Vlastimil Babka <vbabka@...e.cz>,
Michal Hocko <mhocko@...e.com>,
Mathias Krause <minipli@...glemail.com>,
Dave Hansen <dave.hansen@...el.com>,
"linux-mm@...ck.org" <linux-mm@...ck.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC][PATCH v3 1/2] mm/page_poison.c: Enable PAGE_POISONING as a
separate option
On 02/25/2016 09:34 PM, Jianyu Zhan wrote:
> On Fri, Feb 26, 2016 at 12:45 PM, Laura Abbott <labbott@...hat.com> wrote:
>> Do you have some suggestion on wording here? I'm not sure what else to
>> say besides poison patterns to differentiate from hardware poison.
>>
>
>
> Is the below wording OK?
>
>
> config PAGE_POISONING
> bool
> bool "Poison pages after freeing"
> select PAGE_EXTENSION
> select PAGE_POISONING_NO_SANITY if HIBERNATION
> ---help---
> Fill the pages with poison patterns after free_pages() and verify
> the patterns before alloc_pages. The filling of the memory helps
> reduce the risk of information leaks from freed data. This does
> have a potential performance impact.
>
> Note that "poison" here is not the same thing as that in "HWPoison"
> for CONFIG_MEMORY_FAILURE, in which "poison" is just a nomenclature
> borrowed from Intel , for the processor support for
> "poisoned" memory, an
> adaptive method for flagging and recovering from memory errors
>
Okay, I see what you are getting at here. This sounds okay.
>>
>>>>
>>>> +config PAGE_POISONING_NO_SANITY
>>>> + depends on PAGE_POISONING
>>>> + bool "Only poison, don't sanity check"
>>>> + ---help---
>>>> + Skip the sanity checking on alloc, only fill the pages with
>>>> + poison on free. This reduces some of the overhead of the
>>>> + poisoning feature.
>>>> +
>>>> + If you are only interested in sanitization, say Y. Otherwise
>>>> + say N.
>>>> diff --git a/mm/Makefile b/mm/Makefile
>>>> index fb1a7948c107..ec59c071b4f9 100644
>>>> --- a/mm/Makefile
>>>> +++ b/mm/Makefile
>>>> @@ -13,7 +13,6 @@ KCOV_INSTRUMENT_slob.o := n
>>>> KCOV_INSTRUMENT_slab.o := n
>>>> KCOV_INSTRUMENT_slub.o := n
>>>> KCOV_INSTRUMENT_page_alloc.o := n
>>>> -KCOV_INSTRUMENT_debug-pagealloc.o := n
>>>> KCOV_INSTRUMENT_kmemleak.o := n
>>>> KCOV_INSTRUMENT_kmemcheck.o := n
>>>> KCOV_INSTRUMENT_memcontrol.o := n
>>>> @@ -63,9 +62,6 @@ obj-$(CONFIG_SPARSEMEM_VMEMMAP) += sparse-vmemmap.o
>>>> obj-$(CONFIG_SLOB) += slob.o
>>>> obj-$(CONFIG_MMU_NOTIFIER) += mmu_notifier.o
>>>> obj-$(CONFIG_KSM) += ksm.o
>>>> -ifndef CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC
>>>> - obj-$(CONFIG_DEBUG_PAGEALLOC) += debug-pagealloc.o
>>>> -endif
>>>> obj-$(CONFIG_PAGE_POISONING) += page_poison.o
>>>> obj-$(CONFIG_SLAB) += slab.o
>>>> obj-$(CONFIG_SLUB) += slub.o
>>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>>> index a34c359d8e81..0bdb3cfd83b5 100644
>>>> --- a/mm/page_alloc.c
>>>> +++ b/mm/page_alloc.c
>>>> @@ -1026,6 +1026,7 @@ static bool free_pages_prepare(struct page *page,
>>>> unsigned int order)
>>>> PAGE_SIZE << order);
>>>> }
>>>> arch_free_page(page, order);
>>>> + kernel_poison_pages(page, 1 << order, 0);
>>>> kernel_map_pages(page, 1 << order, 0);
>>>>
>>>> return true;
>>>> @@ -1497,6 +1498,7 @@ static int prep_new_page(struct page *page,
>>>> unsigned int order, gfp_t gfp_flags,
>>>>
>>>> arch_alloc_page(page, order);
>>>> kernel_map_pages(page, 1 << order, 1);
>>>> + kernel_poison_pages(page, 1 << order, 1);
>>>> kasan_alloc_pages(page, order);
>>>>
>>>> if (gfp_flags & __GFP_ZERO)
>>>> diff --git a/mm/page_poison.c b/mm/page_poison.c
>>>> index 92ead727b8f0..884a6f854432 100644
>>>> --- a/mm/page_poison.c
>>>> +++ b/mm/page_poison.c
>>>> @@ -80,7 +80,7 @@ static void poison_page(struct page *page)
>>>> kunmap_atomic(addr);
>>>> }
>>>>
>>>> -void poison_pages(struct page *page, int n)
>>>> +static void poison_pages(struct page *page, int n)
>>>> {
>>>> int i;
>>>>
>>>> @@ -101,6 +101,9 @@ static void check_poison_mem(unsigned char *mem,
>>>> size_t bytes)
>>>> unsigned char *start;
>>>> unsigned char *end;
>>>>
>>>> + if (IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY))
>>>> + return;
>>>> +
>>>> start = memchr_inv(mem, PAGE_POISON, bytes);
>>>> if (!start)
>>>> return;
>>>> @@ -113,9 +116,9 @@ static void check_poison_mem(unsigned char *mem,
>>>> size_t bytes)
>>>> if (!__ratelimit(&ratelimit))
>>>> return;
>>>> else if (start == end && single_bit_flip(*start, PAGE_POISON))
>>>> - printk(KERN_ERR "pagealloc: single bit error\n");
>>>> + pr_err("pagealloc: single bit error\n");
>>>> else
>>>> - printk(KERN_ERR "pagealloc: memory corruption\n");
>>>> + pr_err("pagealloc: memory corruption\n");
>>>>
>>>> print_hex_dump(KERN_ERR, "", DUMP_PREFIX_ADDRESS, 16, 1, start,
>>>> end - start + 1, 1);
>>>> @@ -135,10 +138,28 @@ static void unpoison_page(struct page *page)
>>>> kunmap_atomic(addr);
>>>> }
>>>>
>>>> -void unpoison_pages(struct page *page, int n)
>>>> +static void unpoison_pages(struct page *page, int n)
>>>> {
>>>> int i;
>>>>
>>>> for (i = 0; i < n; i++)
>>>> unpoison_page(page + i);
>>>> }
>>>> +
>>>> +void kernel_poison_pages(struct page *page, int numpages, int enable)
>>>> +{
>>>> + if (!page_poisoning_enabled())
>>>> + return;
>>>> +
>>>> + if (enable)
>>>> + unpoison_pages(page, numpages);
>>>> + else
>>>> + poison_pages(page, numpages);
>>>> +}
>>>> +
>>>> +#ifndef CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC
>>>> +void __kernel_map_pages(struct page *page, int numpages, int enable)
>>>> +{
>>>> + /* This function does nothing, all work is done via poison pages
>>>> */
>>>> +}
>>>> +#endif
>>>
>>>
>>> IMHO, kernel_map_pages is originally incorporated for debugging page
>>> allocation.
>>> And latter for archs that do not support arch-specific page poisoning,
>>> a software poisoning
>>> method was used.
>>>
>>> So I think it is not appropriate to use two interfaces in the alloc/free
>>> hooks.
>>>
>>> The kernel_poison_pages actually should be an implementation detail
>>> and should be hided
>>> in the kernel_map_pages interface.
>>>
>>
>> We want to have the poisoning independent of anything that kernel_map_pages
>> does. It was originally added for software poisoning for arches that
>> didn't have the full ARCH_SUPPORTS_DEBUG_PAGEALLOC support but there's
>> nothing that specifically ties it to mapping. It's beneficial even when
>> we aren't mapping/unmapping the pages so putting it in kernel_map_pages
>> would defeat what we're trying to accomplish here.
>>
>
> Ok, fair enough. If so, I suggest you add this clarification into the
> code, or as least, in
> the changelog.
Sounds fine.
>
>
> Thanks,
> Jianyu Zhan
>
Thanks,
Laura
Powered by blists - more mailing lists