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).
Changes since v1:
- fix some issues raised by David Rientjes, Andi Kleen and PaX Team.
- add hibernate fix (third patch)
- add debug code, this is "just in case" someone has an issue with this
feature. Not sure if it should be merged.
[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 (4):
mm/page_alloc.c: cleanup obsolete KM_USER*
mm/page_alloc.c: add config option to sanitize freed pages
PM / Hibernate: fix SANITIZE_FREED_PAGES
mm: Add debug code for SANITIZE_FREED_PAGES
kernel/power/hibernate.c | 7 ++++++-
kernel/power/power.h | 4 ++++
kernel/power/snapshot.c | 24 ++++++++++++++++++++++
mm/Kconfig | 22 ++++++++++++++++++++
mm/page_alloc.c | 52 ++++++++++++++++++++++++++++++++++--------------
5 files changed, 93 insertions(+), 16 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 remove
prep_zero_pages accordingly.
Signed-off-by: Anisse Astier <[email protected]>
---
mm/page_alloc.c | 17 ++---------------
1 file changed, 2 insertions(+), 15 deletions(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index ebffa0e..4d5ce6e 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -380,20 +380,6 @@ 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)
-{
- 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);
-}
-
#ifdef CONFIG_DEBUG_PAGEALLOC
unsigned int _debug_guardpage_minorder;
bool _debug_pagealloc_enabled __read_mostly;
@@ -975,7 +961,8 @@ 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);
+ for (i = 0; i < (1 << order); i++)
+ clear_highpage(page + i);
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 | 12 ++++++++++++
2 files changed, 24 insertions(+)
diff --git a/mm/Kconfig b/mm/Kconfig
index 390214d..e9fb3bd 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 greatly reduce performance.
+
+ If unsure, say N.
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 4d5ce6e..c29e3a0 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -795,6 +795,12 @@ 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
+ for (i = 0; i < (1 << order); i++)
+ clear_highpage(page + i);
+#endif
+
arch_free_page(page, order);
kernel_map_pages(page, 1 << order, 0);
@@ -960,9 +966,15 @@ static int prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags,
kernel_map_pages(page, 1 << order, 1);
kasan_alloc_pages(page, order);
+#ifndef CONFIG_SANITIZE_FREED_PAGES
+ /* SANITIZE_FREED_PAGES relies implicitly on the fact that pages are
+ * cleared before use, so we don't need gfp zero in the default case
+ * because all pages go through the free_pages_prepare code path when
+ * switching from bootmem to the default allocator */
if (gfp_flags & __GFP_ZERO)
for (i = 0; i < (1 << order); i++)
clear_highpage(page + i);
+#endif
if (order && (gfp_flags & __GFP_COMP))
prep_compound_page(page, order);
--
1.9.3
SANITIZE_FREED_PAGES feature relies on having all pages going through
the free_pages_prepare path in order to be cleared before being used. In
the hibernate use case, pages will automagically appear in the system
without being cleared.
This fix will make sure free pages are cleared on resume.
Signed-off-by: Anisse Astier <[email protected]>
---
kernel/power/hibernate.c | 7 ++++++-
kernel/power/power.h | 4 ++++
kernel/power/snapshot.c | 21 +++++++++++++++++++++
3 files changed, 31 insertions(+), 1 deletion(-)
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 2329daa..3193b9a 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -305,9 +305,14 @@ static int create_image(int platform_mode)
error);
/* Restore control flow magically appears here */
restore_processor_state();
- if (!in_suspend)
+ if (!in_suspend) {
events_check_enabled = false;
+#ifdef CONFIG_SANITIZE_FREED_PAGES
+ clear_free_pages();
+ printk(KERN_INFO "PM: free pages cleared after restore\n");
+#endif
+ }
platform_leave(platform_mode);
Power_up:
diff --git a/kernel/power/power.h b/kernel/power/power.h
index ce9b832..26b2101 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -92,6 +92,10 @@ extern int create_basic_memory_bitmaps(void);
extern void free_basic_memory_bitmaps(void);
extern int hibernate_preallocate_memory(void);
+#ifdef CONFIG_SANITIZE_FREED_PAGES
+extern void clear_free_pages(void);
+#endif
+
/**
* Auxiliary structure used for reading the snapshot image data and
* metadata from and writing them to the list of page backup entries
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 5235dd4..673ade1 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -1032,6 +1032,27 @@ void free_basic_memory_bitmaps(void)
pr_debug("PM: Basic memory bitmaps freed\n");
}
+#ifdef CONFIG_SANITIZE_FREED_PAGES
+void clear_free_pages(void)
+{
+ struct memory_bitmap *bm = free_pages_map;
+ unsigned long pfn;
+
+ if (WARN_ON(!(free_pages_map)))
+ return;
+
+ memory_bm_position_reset(bm);
+ pfn = memory_bm_next_pfn(bm);
+ while (pfn != BM_END_OF_MAP) {
+ if (pfn_valid(pfn))
+ clear_highpage(pfn_to_page(pfn));
+
+ pfn = memory_bm_next_pfn(bm);
+ }
+ memory_bm_position_reset(bm);
+}
+#endif /* SANITIZE_FREED_PAGES */
+
/**
* snapshot_additional_pages - estimate the number of additional pages
* be needed for setting up the suspend image data structures for given
--
1.9.3
Add debug code for sanitize freed pages to print status and verify pages
at alloc to make sure they're clean. It can be useful if you have
crashes when using SANITIZE_FREED_PAGES.
Signed-off-by: Anisse Astier <[email protected]>
---
kernel/power/snapshot.c | 8 ++++++--
mm/Kconfig | 10 ++++++++++
mm/page_alloc.c | 25 +++++++++++++++++++++++++
3 files changed, 41 insertions(+), 2 deletions(-)
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 673ade1..dfbfb5f 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -1044,9 +1044,13 @@ void clear_free_pages(void)
memory_bm_position_reset(bm);
pfn = memory_bm_next_pfn(bm);
while (pfn != BM_END_OF_MAP) {
- if (pfn_valid(pfn))
+ if (pfn_valid(pfn)) {
+#ifdef CONFIG_SANITIZE_FREED_PAGES_DEBUG
+ printk(KERN_INFO "Clearing page %p\n",
+ page_address(pfn_to_page(pfn)));
+#endif
clear_highpage(pfn_to_page(pfn));
-
+ }
pfn = memory_bm_next_pfn(bm);
}
memory_bm_position_reset(bm);
diff --git a/mm/Kconfig b/mm/Kconfig
index e9fb3bd..95364f2 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -647,3 +647,13 @@ config SANITIZE_FREED_PAGES
Depending on your workload it will greatly reduce performance.
If unsure, say N.
+
+config SANITIZE_FREED_PAGES_DEBUG
+ bool "Debug sanitize pages feature"
+ default n
+ depends on SANITIZE_FREED_PAGES && DEBUG_KERNEL
+ help
+ This option adds some debugging code for the SANITIZE_FREED_PAGES
+ option, as well as verification code to ensure pages are really
+ zeroed. Don't enable unless you want to debug this feature.
+ If unsure, say N.
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index c29e3a0..ba8aa25 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -975,6 +975,31 @@ static int prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags,
for (i = 0; i < (1 << order); i++)
clear_highpage(page + i);
#endif
+#ifdef CONFIG_SANITIZE_FREED_PAGES_DEBUG
+ for (i = 0; i < (1 << order); i++) {
+ struct page *p = page + i;
+ int j;
+ bool err = false;
+ void *kaddr = kmap_atomic(p);
+
+ for (j = 0; j < PAGE_SIZE; j++) {
+ if (((char *)kaddr)[j] != 0) {
+ pr_err("page %p is not zero on alloc! %s\n",
+ page_address(p), (gfp_flags &
+ __GFP_ZERO) ?
+ "fixing." : "");
+ if (gfp_flags & __GFP_ZERO) {
+ err = true;
+ kunmap_atomic(kaddr);
+ clear_highpage(p);
+ }
+ break;
+ }
+ }
+ if (!err)
+ kunmap_atomic(kaddr);
+ }
+#endif
if (order && (gfp_flags & __GFP_COMP))
prep_compound_page(page, order);
--
1.9.3
On 4 May 2015 at 23:16, Anisse Astier wrote:
> SANITIZE_FREED_PAGES feature relies on having all pages going through
> the free_pages_prepare path in order to be cleared before being used. In
> the hibernate use case, pages will automagically appear in the system
> without being cleared.
is this based on debugging/code reading/discussions with hibernation folks
(i see none of them on CC, added them now) or is it just a brute force attempt
to fix the symptoms? if the former, it'd be nice to share some more details
and have Acks from the code owners.
> This fix will make sure free pages are cleared on resume.
>
> Signed-off-by: Anisse Astier <[email protected]>
> ---
> kernel/power/hibernate.c | 7 ++++++-
> kernel/power/power.h | 4 ++++
> kernel/power/snapshot.c | 21 +++++++++++++++++++++
> 3 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 2329daa..3193b9a 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -305,9 +305,14 @@ static int create_image(int platform_mode)
> error);
> /* Restore control flow magically appears here */
> restore_processor_state();
> - if (!in_suspend)
> + if (!in_suspend) {
> events_check_enabled = false;
>
> +#ifdef CONFIG_SANITIZE_FREED_PAGES
> + clear_free_pages();
> + printk(KERN_INFO "PM: free pages cleared after restore\n");
> +#endif
> + }
> platform_leave(platform_mode);
>
> Power_up:
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index ce9b832..26b2101 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -92,6 +92,10 @@ extern int create_basic_memory_bitmaps(void);
> extern void free_basic_memory_bitmaps(void);
> extern int hibernate_preallocate_memory(void);
>
> +#ifdef CONFIG_SANITIZE_FREED_PAGES
> +extern void clear_free_pages(void);
> +#endif
> +
> /**
> * Auxiliary structure used for reading the snapshot image data and
> * metadata from and writing them to the list of page backup entries
> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> index 5235dd4..673ade1 100644
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -1032,6 +1032,27 @@ void free_basic_memory_bitmaps(void)
> pr_debug("PM: Basic memory bitmaps freed\n");
> }
>
> +#ifdef CONFIG_SANITIZE_FREED_PAGES
> +void clear_free_pages(void)
> +{
> + struct memory_bitmap *bm = free_pages_map;
> + unsigned long pfn;
> +
> + if (WARN_ON(!(free_pages_map)))
> + return;
> +
> + memory_bm_position_reset(bm);
> + pfn = memory_bm_next_pfn(bm);
> + while (pfn != BM_END_OF_MAP) {
> + if (pfn_valid(pfn))
> + clear_highpage(pfn_to_page(pfn));
> +
> + pfn = memory_bm_next_pfn(bm);
> + }
> + memory_bm_position_reset(bm);
> +}
> +#endif /* SANITIZE_FREED_PAGES */
> +
> /**
> * snapshot_additional_pages - estimate the number of additional pages
> * be needed for setting up the suspend image data structures for given
> --
> 1.9.3
>
>
On 4 May 2015 at 23:16, Anisse Astier wrote:
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index c29e3a0..ba8aa25 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -975,6 +975,31 @@ static int prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags,
> for (i = 0; i < (1 << order); i++)
> clear_highpage(page + i);
> #endif
> +#ifdef CONFIG_SANITIZE_FREED_PAGES_DEBUG
> + for (i = 0; i < (1 << order); i++) {
> + struct page *p = page + i;
> + int j;
> + bool err = false;
> + void *kaddr = kmap_atomic(p);
> +
> + for (j = 0; j < PAGE_SIZE; j++) {
did you mean to use memchr_inv(kaddr, 0, PAGE_SIZE) instead? ;)
> + if (((char *)kaddr)[j] != 0) {
> + pr_err("page %p is not zero on alloc! %s\n",
> + page_address(p), (gfp_flags &
> + __GFP_ZERO) ?
> + "fixing." : "");
> + if (gfp_flags & __GFP_ZERO) {
> + err = true;
> + kunmap_atomic(kaddr);
> + clear_highpage(p);
> + }
> + break;
> + }
> + }
> + if (!err)
> + kunmap_atomic(kaddr);
> + }
> +#endif
>
> if (order && (gfp_flags & __GFP_COMP))
> prep_compound_page(page, order);
> --
> 1.9.3
>
>
On 4 May 2015 at 23:16, Anisse Astier wrote:
> @@ -960,9 +966,15 @@ static int prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags,
> kernel_map_pages(page, 1 << order, 1);
> kasan_alloc_pages(page, order);
>
> +#ifndef CONFIG_SANITIZE_FREED_PAGES
> + /* SANITIZE_FREED_PAGES relies implicitly on the fact that pages are
> + * cleared before use, so we don't need gfp zero in the default case
> + * because all pages go through the free_pages_prepare code path when
> + * switching from bootmem to the default allocator */
> if (gfp_flags & __GFP_ZERO)
> for (i = 0; i < (1 << order); i++)
> clear_highpage(page + i);
> +#endif
this hunk should not be applied before the hibernation fix otherwise
bisect will break.
On Monday, May 04, 2015 11:50:13 PM PaX Team wrote:
> On 4 May 2015 at 23:16, Anisse Astier wrote:
>
> > SANITIZE_FREED_PAGES feature relies on having all pages going through
> > the free_pages_prepare path in order to be cleared before being used. In
> > the hibernate use case, pages will automagically appear in the system
> > without being cleared.
>
> is this based on debugging/code reading/discussions with hibernation folks
> (i see none of them on CC, added them now) or is it just a brute force attempt
> to fix the symptoms? if the former, it'd be nice to share some more details
> and have Acks from the code owners.
I haven't seen it, for one, and I'm wondering why the "clearing" cannot be done
at the swsusp_free() time?
In any case, please CC hibernation-related patches and discussions thereof to
[email protected].
Thanks!
On Tue, May 5, 2015 at 12:29 AM, Rafael J. Wysocki <[email protected]> wrote:
> I haven't seen it, for one, and I'm wondering why the "clearing" cannot be done
> at the swsusp_free() time?
Because the validity of the free pages bitmap is short-lived since
device resume code might do some allocations.
>
> In any case, please CC hibernation-related patches and discussions thereof to
> [email protected].
Thanks, I had to forget something when sending this series :-/ ; I'm
preparing v3 that will be sent to linux-pm too.
Regards,
Anisse
On Mon, May 4, 2015 at 11:50 PM, PaX Team <[email protected]> wrote:
> On 4 May 2015 at 23:16, Anisse Astier wrote:
>
>> @@ -960,9 +966,15 @@ static int prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags,
>> kernel_map_pages(page, 1 << order, 1);
>> kasan_alloc_pages(page, order);
>>
>> +#ifndef CONFIG_SANITIZE_FREED_PAGES
>> + /* SANITIZE_FREED_PAGES relies implicitly on the fact that pages are
>> + * cleared before use, so we don't need gfp zero in the default case
>> + * because all pages go through the free_pages_prepare code path when
>> + * switching from bootmem to the default allocator */
>> if (gfp_flags & __GFP_ZERO)
>> for (i = 0; i < (1 << order); i++)
>> clear_highpage(page + i);
>> +#endif
>
> this hunk should not be applied before the hibernation fix otherwise
> bisect will break.
>
It will be re-ordered in v3, thanks.
On Mon, May 4, 2015 at 11:50 PM, PaX Team <[email protected]> wrote:
> On 4 May 2015 at 23:16, Anisse Astier wrote:
>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index c29e3a0..ba8aa25 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -975,6 +975,31 @@ static int prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags,
>> for (i = 0; i < (1 << order); i++)
>> clear_highpage(page + i);
>> #endif
>> +#ifdef CONFIG_SANITIZE_FREED_PAGES_DEBUG
>> + for (i = 0; i < (1 << order); i++) {
>> + struct page *p = page + i;
>> + int j;
>> + bool err = false;
>> + void *kaddr = kmap_atomic(p);
>> +
>> + for (j = 0; j < PAGE_SIZE; j++) {
>
> did you mean to use memchr_inv(kaddr, 0, PAGE_SIZE) instead? ;)
Will be fixed in v3, although as I said I'm not sure if this debug
code should go in or not.
Thanks for you time.
Anisse