Hi,
I'm trying revive an old debate here[1], though with a simpler approach than
was previously tried. This patch series implements a new option to sanitize
freed pages, a (very) small subset of what is done in PaX/grsecurity[3],
inspired by a previous submission [4].
The first patch is fairly independent, and could be taken as-is. The second is
the meat and should be straight-forward to review.
There are a few different uses that this can cover:
- some cases of use-after-free could be detected (crashes), although this not
as efficient as KAsan/kmemcheck
- it can help with long-term memory consumption in an environment with
multiple VMs and Kernel Same-page Merging on the host. [2]
- finally, it can reduce infoleaks, although this is hard to measure.
The approach is voluntarily kept as simple as possible. A single configuration
option, no command line option, no sysctl nob. It can of course be changed,
although I'd be wary of runtime-configuration options that could be used for
races.
I haven't been able to measure a meaningful performance difference when
compiling a (in-cache) kernel; I'd be interested to see what difference it
makes with your particular workload/hardware (I suspect mine is CPU-bound on
this small laptop).
[1] https://lwn.net/Articles/334747/
[2] https://staff.aist.go.jp/k.suzaki/EuroSec12-SUZAKI-revised2.pdf
[3] http://en.wikibooks.org/wiki/Grsecurity/Appendix/Grsecurity_and_PaX_Configuration_Options#Sanitize_all_freed_memory
[4] http://article.gmane.org/gmane.linux.kernel.mm/34398
Anisse Astier (2):
mm/page_alloc.c: cleanup obsolete KM_USER*
mm/page_alloc.c: add config option to sanitize freed pages
mm/Kconfig | 12 ++++++++++++
mm/page_alloc.c | 15 +++++++--------
2 files changed, 19 insertions(+), 8 deletions(-)
--
1.9.3
It's been five years now that KM_* kmap flags have been removed and
that we can call clear_highpage from any context. So we simplify
prep_zero_pages accordingly and rename it in the process.
Signed-off-by: Anisse Astier <[email protected]>
---
mm/page_alloc.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index ebffa0e..05fcec9 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -380,16 +380,10 @@ void prep_compound_page(struct page *page, unsigned long order)
}
}
-static inline void prep_zero_page(struct page *page, unsigned int order,
- gfp_t gfp_flags)
+static inline void zero_pages(struct page *page, unsigned int order)
{
int i;
- /*
- * clear_highpage() will use KM_USER0, so it's a bug to use __GFP_ZERO
- * and __GFP_HIGHMEM from hard or soft interrupt context.
- */
- VM_BUG_ON((gfp_flags & __GFP_HIGHMEM) && in_interrupt());
for (i = 0; i < (1 << order); i++)
clear_highpage(page + i);
}
@@ -975,7 +969,7 @@ static int prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags,
kasan_alloc_pages(page, order);
if (gfp_flags & __GFP_ZERO)
- prep_zero_page(page, order, gfp_flags);
+ zero_pages(page, order);
if (order && (gfp_flags & __GFP_COMP))
prep_compound_page(page, order);
--
1.9.3
This new config option will sanitize all freed pages. This is a pretty
low-level change useful to track some cases of use-after-free, help
kernel same-page merging in VM environments, and counter a few info
leaks.
Signed-off-by: Anisse Astier <[email protected]>
---
mm/Kconfig | 12 ++++++++++++
mm/page_alloc.c | 5 +++++
2 files changed, 17 insertions(+)
diff --git a/mm/Kconfig b/mm/Kconfig
index 390214d..cb2df5f 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -635,3 +635,15 @@ config MAX_STACK_SIZE_MB
changed to a smaller value in which case that is used.
A sane initial value is 80 MB.
+
+config SANITIZE_FREED_PAGES
+ bool "Sanitize memory pages after free"
+ default n
+ help
+ This option is used to make sure all pages freed are zeroed. This is
+ quite low-level and doesn't handle your slab buffers.
+ It has various applications, from preventing some info leaks to
+ helping kernel same-page merging in virtualised environments.
+ Depending on your workload, it will reduce performance of about 3%.
+
+ If unsure, say N.
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 05fcec9..c71440a 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -803,6 +803,11 @@ static bool free_pages_prepare(struct page *page, unsigned int order)
debug_check_no_obj_freed(page_address(page),
PAGE_SIZE << order);
}
+
+#ifdef CONFIG_SANITIZE_FREED_PAGES
+ zero_pages(page, order);
+#endif
+
arch_free_page(page, order);
kernel_map_pages(page, 1 << order, 0);
--
1.9.3
On Fri, 24 Apr 2015, Anisse Astier wrote:
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index ebffa0e..05fcec9 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -380,16 +380,10 @@ void prep_compound_page(struct page *page, unsigned long order)
> }
> }
>
> -static inline void prep_zero_page(struct page *page, unsigned int order,
> - gfp_t gfp_flags)
> +static inline void zero_pages(struct page *page, unsigned int order)
> {
> int i;
>
> - /*
> - * clear_highpage() will use KM_USER0, so it's a bug to use __GFP_ZERO
> - * and __GFP_HIGHMEM from hard or soft interrupt context.
> - */
> - VM_BUG_ON((gfp_flags & __GFP_HIGHMEM) && in_interrupt());
> for (i = 0; i < (1 << order); i++)
> clear_highpage(page + i);
> }
> @@ -975,7 +969,7 @@ static int prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags,
> kasan_alloc_pages(page, order);
>
> if (gfp_flags & __GFP_ZERO)
> - prep_zero_page(page, order, gfp_flags);
> + zero_pages(page, order);
>
> if (order && (gfp_flags & __GFP_COMP))
> prep_compound_page(page, order);
No objection to removing the VM_BUG_ON() here, but I'm not sure that we
need an inline function to do this and to add additional callers in your
next patch. Why can't we just remove the helper entirely and do the
iteration in prep_new_page()? We iterate pages all the time.
On Fri, 24 Apr 2015, Anisse Astier wrote:
> diff --git a/mm/Kconfig b/mm/Kconfig
> index 390214d..cb2df5f 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -635,3 +635,15 @@ config MAX_STACK_SIZE_MB
> changed to a smaller value in which case that is used.
>
> A sane initial value is 80 MB.
> +
> +config SANITIZE_FREED_PAGES
> + bool "Sanitize memory pages after free"
> + default n
> + help
> + This option is used to make sure all pages freed are zeroed. This is
> + quite low-level and doesn't handle your slab buffers.
> + It has various applications, from preventing some info leaks to
> + helping kernel same-page merging in virtualised environments.
> + Depending on your workload, it will reduce performance of about 3%.
> +
> + If unsure, say N.
Objection to allowing this without first enabling some other DEBUG config
option, it should never be a standalone option, but also to pretending to
have any insight into what the performance degredation of it will be. On
my systems, this would be _massive_.
Hi David,
First of all, thanks a lot for your time reviewing this series.
On Fri, Apr 24, 2015 at 11:36 PM, David Rientjes <[email protected]> wrote:
> On Fri, 24 Apr 2015, Anisse Astier wrote:
>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index ebffa0e..05fcec9 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -380,16 +380,10 @@ void prep_compound_page(struct page *page, unsigned long order)
>> }
>> }
>>
>> -static inline void prep_zero_page(struct page *page, unsigned int order,
>> - gfp_t gfp_flags)
>> +static inline void zero_pages(struct page *page, unsigned int order)
>> {
>> int i;
>>
>> - /*
>> - * clear_highpage() will use KM_USER0, so it's a bug to use __GFP_ZERO
>> - * and __GFP_HIGHMEM from hard or soft interrupt context.
>> - */
>> - VM_BUG_ON((gfp_flags & __GFP_HIGHMEM) && in_interrupt());
>> for (i = 0; i < (1 << order); i++)
>> clear_highpage(page + i);
>> }
>> @@ -975,7 +969,7 @@ static int prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags,
>> kasan_alloc_pages(page, order);
>>
>> if (gfp_flags & __GFP_ZERO)
>> - prep_zero_page(page, order, gfp_flags);
>> + zero_pages(page, order);
>>
>> if (order && (gfp_flags & __GFP_COMP))
>> prep_compound_page(page, order);
>
> No objection to removing the VM_BUG_ON() here, but I'm not sure that we
> need an inline function to do this and to add additional callers in your
> next patch. Why can't we just remove the helper entirely and do the
> iteration in prep_new_page()? We iterate pages all the time.
I just felt it was easier to read as a whole; unless anyone else
objects, I think I'll keep it as-is in the next iteration.
Anisse
On Fri, Apr 24, 2015 at 11:38 PM, David Rientjes <[email protected]> wrote:
> On Fri, 24 Apr 2015, Anisse Astier wrote:
>
>> diff --git a/mm/Kconfig b/mm/Kconfig
>> index 390214d..cb2df5f 100644
>> --- a/mm/Kconfig
>> +++ b/mm/Kconfig
>> @@ -635,3 +635,15 @@ config MAX_STACK_SIZE_MB
>> changed to a smaller value in which case that is used.
>>
>> A sane initial value is 80 MB.
>> +
>> +config SANITIZE_FREED_PAGES
>> + bool "Sanitize memory pages after free"
>> + default n
>> + help
>> + This option is used to make sure all pages freed are zeroed. This is
>> + quite low-level and doesn't handle your slab buffers.
>> + It has various applications, from preventing some info leaks to
>> + helping kernel same-page merging in virtualised environments.
>> + Depending on your workload, it will reduce performance of about 3%.
>> +
>> + If unsure, say N.
>
> Objection to allowing this without first enabling some other DEBUG config
> option, it should never be a standalone option, but also to pretending to
I'm not sure I understand the rationale here. Is it to protect the
innocent ? The performance warning and "N" recommendation ought to be
enough.
I'm not sure depending on DEBUG will help anyone; it will just hinder
those who want to use this on a hardened system (where you might not
want to have DEBUG enabled).
> have any insight into what the performance degredation of it will be. On
I fully agree I shouldn't have let the 3% ballpark estimate slip, I'll
remove it.
> my systems, this would be _massive_.
I'm interested in what you mean by "massive". Have you conducted
experiments on the impact or is just your gut feeling ? Anyway, I'd be
curious to see numbers showing what it looks like on big hardware.
Anisse
Anisse Astier <[email protected]> writes:
> + If unsure, say N.
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 05fcec9..c71440a 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -803,6 +803,11 @@ static bool free_pages_prepare(struct page *page, unsigned int order)
> debug_check_no_obj_freed(page_address(page),
> PAGE_SIZE << order);
> }
> +
> +#ifdef CONFIG_SANITIZE_FREED_PAGES
> + zero_pages(page, order);
> +#endif
And not removing the clear on __GFP_ZERO by remembering that?
That means all clears would be done twice.
That patch is far too simple. Clearing is commonly the most
expensive kernel operation.
-Andi
--
[email protected] -- Speaking for myself only
Hi Andi,
Thinks for taking the time to review this.
On Sun, Apr 26, 2015 at 10:12 PM, Andi Kleen <[email protected]> wrote:
> Anisse Astier <[email protected]> writes:
>> + If unsure, say N.
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 05fcec9..c71440a 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -803,6 +803,11 @@ static bool free_pages_prepare(struct page *page, unsigned int order)
>> debug_check_no_obj_freed(page_address(page),
>> PAGE_SIZE << order);
>> }
>> +
>> +#ifdef CONFIG_SANITIZE_FREED_PAGES
>> + zero_pages(page, order);
>> +#endif
>
> And not removing the clear on __GFP_ZERO by remembering that?
>
> That means all clears would be done twice.
>
> That patch is far too simple. Clearing is commonly the most
> expensive kernel operation.
>
I thought about this, but if you unconditionally remove the clear on
__GFP_ZERO, you wouldn't be guaranteed to have a zeroed page when
memory is first used (you would protect the kernel from its own info
leaks though); you'd need to clear memory on boot for example.
If you try to remember that a page it's cleared, it means using a page
flag, which is was previously deemed too precious for this kind of
operation.
Regarding the expensive operation, I don't think this is an option
you'd enable on your systems if you care about performance.
Regards,
Anisse
On 27 Apr 2015 at 10:11, Anisse Astier wrote:
> >> +#ifdef CONFIG_SANITIZE_FREED_PAGES
> >> + zero_pages(page, order);
> >> +#endif
> >
> > And not removing the clear on __GFP_ZERO by remembering that?
> >
> > That means all clears would be done twice.
> >
> > That patch is far too simple. Clearing is commonly the most
> > expensive kernel operation.
> >
>
> I thought about this, but if you unconditionally remove the clear on
> __GFP_ZERO, you wouldn't be guaranteed to have a zeroed page when
> memory is first used (you would protect the kernel from its own info
> leaks though);
the PaX SANITIZE feature does exactly this in mm/page_alloc.c:prep_new_page:
#ifndef CONFIG_PAX_MEMORY_SANITIZE
if (gfp_flags & __GFP_ZERO)
prep_zero_page(page, order, gfp_flags);
#endif
> you'd need to clear memory on boot for example.
it happens automagically because on boot during the transition from the
boot allocator to the buddy one each page gets freed which will then go
through the page clearing path.
however there's a known problem/conflict with HIBERNATION (see
http://marc.info/?l=linux-pm&m=132871433416256&w=2) which i think would
have to be resolved before upstream acceptance.
cheers,
PaX Team
On Mon, Apr 27, 2015 at 11:25 AM, PaX Team <[email protected]> wrote:
>
> the PaX SANITIZE feature does exactly this in mm/page_alloc.c:prep_new_page:
>
> #ifndef CONFIG_PAX_MEMORY_SANITIZE
> if (gfp_flags & __GFP_ZERO)
> prep_zero_page(page, order, gfp_flags);
> #endif
>
Thanks, I'll do that in the next iteration.
>> you'd need to clear memory on boot for example.
>
> it happens automagically because on boot during the transition from the
> boot allocator to the buddy one each page gets freed which will then go
> through the page clearing path.
Interesting, I'll see how it works.
>
> however there's a known problem/conflict with HIBERNATION (see
> http://marc.info/?l=linux-pm&m=132871433416256&w=2) which i think would
> have to be resolved before upstream acceptance.
I don't use hibernation, but I'll see if I can create a swap partition
to test that.
Regards,
Anisse