All of lore.kernel.org
 help / color / mirror / Atom feed
* [kernel-hardening] Merge in PAX_MEMORY_SANITIZE work from grsec to linux-next
@ 2017-01-18  4:21 Kaiwan N Billimoria
  2017-01-18 19:44 ` Laura Abbott
  2017-01-19  0:54 ` Kees Cook
  0 siblings, 2 replies; 20+ messages in thread
From: Kaiwan N Billimoria @ 2017-01-18  4:21 UTC (permalink / raw)
  To: keescook, kernel-hardening

Hi Kees,

So, following up on the previous discussion, 
>I'd like to see the mentioned excluded slab caches also done in the
>kernel, along with similar kernel command line options. Additionally,
>getting all the upstream stuff behind a single CONFIG (similar to
>CONFIG_PAX_MEMORY_SANITIZE) would be great, instead of having to set 3
>CONFIGs and 2 kernel parameters. :)

Basically what I've done so far is:
- pulled in the linux-next tree, and setup my own branch

- taken the grsecurity patch (for 4.8.17) and merged those portions of
the code encompassing the  CONFIG_PAX_MEMORY_SANITIZE directive

- There were some compile issues, which seem to be there mostly because of other
grsec infra that hasn't been merged in (yet). 
For now, I've just done small fixups where required:
(see code in ptach below):

[1. fs/dcache.c
  kmem_cache_create_usercopy() unavailable (for now at least).
Probably part of other grsec infrastructure..
 So am just adding the SLAB_NO_SANITIZE flag to the usual
kmem_cache_create() call.

2. mm/slab_common.c
Compile failure:
enum pax_sanitize_mode pax_sanitize_slab __read_only = PAX_SANITIZE_SLAB_FAST;

?? -above orig statement from the grsec-4.8.17 patch fails compile with:
mm/slab_common.c:34:37: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or
‘__attribute__’ before ‘__read_only’

So I've just removed the "__read_only" attribute for now.
What's the actual approach?

3. mm/slub.c
Compile failure:
#ifdef CONFIG_PAX_MEMORY_SANITIZE
 &sanitize_attr.attr,
 * ?? -above statement from the grsec-4.8.17 patch fails compile with:
 mm/slub.c:5337:3: error: ‘sanitize_attr’ undeclared here (not in a function)
  &sanitize_attr.attr,

Just commented this out for now.
]



===
diff --git a/fs/buffer.c b/fs/buffer.c
index 28484b3..b524eda 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -3511,7 +3511,7 @@ void __init buffer_init(void)
 	bh_cachep = kmem_cache_create("buffer_head",
 			sizeof(struct buffer_head), 0,
 				(SLAB_RECLAIM_ACCOUNT|SLAB_PANIC|
-				SLAB_MEM_SPREAD),
+				SLAB_MEM_SPREAD|SLAB_NO_SANITIZE),
 				NULL);
 
 	/*
diff --git a/fs/dcache.c b/fs/dcache.c
index 95d71ed..95ed43c 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -3616,8 +3616,16 @@ void __init vfs_caches_init_early(void)
 
 void __init vfs_caches_init(void)
 {
+/**
+ *	names_cachep = kmem_cache_create_usercopy("names_cache", PATH_MAX, 0,
+ *			SLAB_HWCACHE_ALIGN|SLAB_PANIC| SLAB_NO_SANITIZE,
+ *			0, PATH_MAX, NULL);
+ *	kmem_cache_create_usercopy() unavailable for now at least.
+ *	So just adding the SLAB_NO_SANITIZE flag to the usual call below..
+ */
 	names_cachep = kmem_cache_create("names_cache", PATH_MAX, 0,
-			SLAB_HWCACHE_ALIGN|SLAB_PANIC, NULL);
+			SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_NO_SANITIZE, NULL);
+
 
 	dcache_init();
 	inode_init();
diff --git a/include/linux/highmem.h b/include/linux/highmem.h
index bb3f329..9daed55 100644
--- a/include/linux/highmem.h
+++ b/include/linux/highmem.h
@@ -190,6 +190,18 @@ static inline void clear_highpage(struct page *page)
 	kunmap_atomic(kaddr);
 }
 
+static inline void sanitize_highpage(struct page *page)
+{
+	void *kaddr;
+	unsigned long flags;
+
+	local_irq_save(flags);
+	kaddr = kmap_atomic(page);
+	clear_page(kaddr);
+	kunmap_atomic(kaddr);
+	local_irq_restore(flags);
+}
+
 static inline void zero_user_segments(struct page *page,
 	unsigned start1, unsigned end1,
 	unsigned start2, unsigned end2)
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 4c53635..334bd89 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -23,6 +23,13 @@
 #define SLAB_CONSISTENCY_CHECKS	0x00000100UL	/* DEBUG: Perform (expensive) checks on alloc/free */
 #define SLAB_RED_ZONE		0x00000400UL	/* DEBUG: Red zone objs in a cache */
 #define SLAB_POISON		0x00000800UL	/* DEBUG: Poison objects */
+
+#ifdef CONFIG_PAX_MEMORY_SANITIZE
+#define SLAB_NO_SANITIZE	0x00001000UL	/* PaX: Do not sanitize objs on free */
+#else
+#define SLAB_NO_SANITIZE	0x00000000UL
+#endif
+
 #define SLAB_HWCACHE_ALIGN	0x00002000UL	/* Align objs on cache lines */
 #define SLAB_CACHE_DMA		0x00004000UL	/* Use GFP_DMA memory */
 #define SLAB_STORE_USER		0x00010000UL	/* DEBUG: Store the last owner for bug hunting */
diff --git a/include/linux/slab_def.h b/include/linux/slab_def.h
index 4ad2c5a..a30bbd2 100644
--- a/include/linux/slab_def.h
+++ b/include/linux/slab_def.h
@@ -60,6 +60,10 @@ struct kmem_cache {
 	atomic_t allocmiss;
 	atomic_t freehit;
 	atomic_t freemiss;
+#ifdef CONFIG_PAX_MEMORY_SANITIZE
+	atomic_unchecked_t sanitized;
+	atomic_unchecked_t not_sanitized;
+#endif
 #ifdef CONFIG_DEBUG_SLAB_LEAK
 	atomic_t store_user_clean;
 #endif
diff --git a/kernel/fork.c b/kernel/fork.c
index 7da33cb..42646d4 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2098,7 +2098,8 @@ void __init proc_caches_init(void)
 			sizeof(struct mm_struct), ARCH_MIN_MMSTRUCT_ALIGN,
 			SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_NOTRACK|SLAB_ACCOUNT,
 			NULL);
-	vm_area_cachep = KMEM_CACHE(vm_area_struct, SLAB_PANIC|SLAB_ACCOUNT);
+	vm_area_cachep = KMEM_CACHE(vm_area_struct, SLAB_PANIC|SLAB_ACCOUNT|
+			 SLAB_NO_SANITIZE);
 	mmap_init();
 	nsproxy_cache_init();
 }
diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug
index afcc550..ed3f097 100644
--- a/mm/Kconfig.debug
+++ b/mm/Kconfig.debug
@@ -10,6 +10,7 @@ config PAGE_EXTENSION
 config DEBUG_PAGEALLOC
 	bool "Debug page memory allocations"
 	depends on DEBUG_KERNEL
+	depends on !PAX_MEMORY_SANITIZE
 	depends on !HIBERNATION || ARCH_SUPPORTS_DEBUG_PAGEALLOC && !PPC && !SPARC
 	depends on !KMEMCHECK
 	select PAGE_EXTENSION
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 21ea508..3e899fa 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -994,6 +994,10 @@ static __always_inline bool free_pages_prepare(struct page *page,
 {
 	int bad = 0;
 
+#ifdef CONFIG_PAX_MEMORY_SANITIZE
+	unsigned long index = 1UL << order;
+#endif
+
 	VM_BUG_ON_PAGE(PageTail(page), page);
 
 	trace_mm_page_free(page, order);
@@ -1040,6 +1044,12 @@ static __always_inline bool free_pages_prepare(struct page *page,
 		debug_check_no_obj_freed(page_address(page),
 					   PAGE_SIZE << order);
 	}
+
+#ifdef CONFIG_PAX_MEMORY_SANITIZE
+	for (; index; --index)
+		sanitize_highpage(page + index - 1);
+#endif
+
 	arch_free_page(page, order);
 	kernel_poison_pages(page, 1 << order, 0);
 	kernel_map_pages(page, 1 << order, 0);
@@ -1696,8 +1706,10 @@ static inline int check_new_page(struct page *page)
 
 static inline bool free_pages_prezeroed(bool poisoned)
 {
-	return IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) &&
-		page_poisoning_enabled() && poisoned;
+	return IS_ENABLED(CONFIG_PAX_MEMORY_SANITIZE) ||
+		(IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) &&
+		 page_poisoning_enabled() && poisoned);
+
 }
 
 #ifdef CONFIG_DEBUG_VM
@@ -1753,11 +1765,13 @@ static void prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags
 	int i;
 	bool poisoned = true;
 
+#ifndef CONFIG_PAX_MEMORY_SANITIZE
 	for (i = 0; i < (1 << order); i++) {
 		struct page *p = page + i;
 		if (poisoned)
 			poisoned &= page_is_poisoned(p);
 	}
+#endif
 
 	post_alloc_hook(page, order, gfp_flags);
 
diff --git a/mm/rmap.c b/mm/rmap.c
index 91619fd..e209ff6 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -428,10 +428,10 @@ static void anon_vma_ctor(void *data)
 void __init anon_vma_init(void)
 {
 	anon_vma_cachep = kmem_cache_create("anon_vma", sizeof(struct anon_vma),
-			0, SLAB_DESTROY_BY_RCU|SLAB_PANIC|SLAB_ACCOUNT,
-			anon_vma_ctor);
+			0, SLAB_DESTROY_BY_RCU|SLAB_PANIC|SLAB_ACCOUNT|
+			SLAB_NO_SANITIZE, anon_vma_ctor);
 	anon_vma_chain_cachep = KMEM_CACHE(anon_vma_chain,
-			SLAB_PANIC|SLAB_ACCOUNT);
+			SLAB_PANIC|SLAB_ACCOUNT|SLAB_NO_SANITIZE);
 }
 
 /*
diff --git a/mm/slab.c b/mm/slab.c
index 4f2ec6b..6dc9f4a 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3511,6 +3511,20 @@ void ___cache_free(struct kmem_cache *cachep, void *objp,
 	struct array_cache *ac = cpu_cache_get(cachep);
 
 	check_irq_off();
+
+#ifdef CONFIG_PAX_MEMORY_SANITIZE
+	if (cachep->flags & (SLAB_POISON | SLAB_NO_SANITIZE))
+		STATS_INC_NOT_SANITIZED(cachep);
+	else {
+		memset(objp, PAX_MEMORY_SANITIZE_VALUE, cachep->object_size);
+
+		if (cachep->ctor)
+			cachep->ctor(objp);
+
+		STATS_INC_SANITIZED(cachep);
+	}
+#endif
+
 	kmemleak_free_recursive(objp, cachep->flags);
 	objp = cache_free_debugcheck(cachep, objp, caller);
 
@@ -4157,6 +4171,14 @@ void slabinfo_show_stats(struct seq_file *m, struct kmem_cache *cachep)
 		seq_printf(m, " : cpustat %6lu %6lu %6lu %6lu",
 			   allochit, allocmiss, freehit, freemiss);
 	}
+#ifdef CONFIG_PAX_MEMORY_SANITIZE
+	{
+		unsigned long sanitized = atomic_read_unchecked(&cachep->sanitized);
+		unsigned long not_sanitized = atomic_read_unchecked(&cachep->not_sanitized);
+
+		seq_printf(m, " : pax %6lu %6lu", sanitized, not_sanitized);
+	}
+#endif
 #endif
 }
 
diff --git a/mm/slab.h b/mm/slab.h
index de6579d..2965ebe 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -71,6 +71,36 @@ extern struct list_head slab_caches;
 /* The slab cache that manages slab cache information */
 extern struct kmem_cache *kmem_cache;
 
+#ifdef CONFIG_PAX_MEMORY_SANITIZE
+#ifdef CONFIG_X86_64
+#define PAX_MEMORY_SANITIZE_VALUE	'\xfe'
+#else
+#define PAX_MEMORY_SANITIZE_VALUE	'\xff'
+#endif
+enum pax_sanitize_mode {
+	PAX_SANITIZE_SLAB_OFF = 0,
+	PAX_SANITIZE_SLAB_FAST,
+	PAX_SANITIZE_SLAB_FULL,
+};
+
+extern enum pax_sanitize_mode pax_sanitize_slab;
+
+static inline unsigned long pax_sanitize_slab_flags(unsigned long flags)
+{
+	if (pax_sanitize_slab == PAX_SANITIZE_SLAB_OFF ||
+	    (flags & SLAB_DESTROY_BY_RCU))
+		flags |= SLAB_NO_SANITIZE;
+	else if (pax_sanitize_slab == PAX_SANITIZE_SLAB_FULL)
+		flags &= ~SLAB_NO_SANITIZE;
+	return flags;
+}
+#else
+static inline unsigned long pax_sanitize_slab_flags(unsigned long flags)
+{
+	return flags;
+}
+#endif
+
 unsigned long calculate_alignment(unsigned long flags,
 		unsigned long align, unsigned long size);
 
@@ -120,7 +150,8 @@ static inline unsigned long kmem_cache_flags(unsigned long object_size,
 
 /* Legal flag mask for kmem_cache_create(), for various configurations */
 #define SLAB_CORE_FLAGS (SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA | SLAB_PANIC | \
-			 SLAB_DESTROY_BY_RCU | SLAB_DEBUG_OBJECTS )
+			 SLAB_DESTROY_BY_RCU | SLAB_DEBUG_OBJECTS | \
+			 SLAB_NO_SANITIZE)
 
 #if defined(CONFIG_DEBUG_SLAB)
 #define SLAB_DEBUG_FLAGS (SLAB_RED_ZONE | SLAB_POISON | SLAB_STORE_USER)
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 1dfc209..0a0851f 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -30,6 +30,37 @@ LIST_HEAD(slab_caches);
 DEFINE_MUTEX(slab_mutex);
 struct kmem_cache *kmem_cache;
 
+#ifdef CONFIG_PAX_MEMORY_SANITIZE
+/**
+ * enum pax_sanitize_mode pax_sanitize_slab __read_only = PAX_SANITIZE_SLAB_FAST;
+ *
+ * ?? -above orig statement from the grsec-4.8.17 patch fails compile with:
+ * mm/slab_common.c:34:37: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘__read_only’
+ *  pax_sanitize_mode pax_sanitize_slab __read_only = PAX_SANITIZE_SLAB_FAST;
+ */
+enum pax_sanitize_mode pax_sanitize_slab = PAX_SANITIZE_SLAB_FAST;
+static int __init pax_sanitize_slab_setup(char *str)
+{
+	if (!str)
+		return 0;
+
+	if (!strcmp(str, "0") || !strcmp(str, "off")) {
+		pr_info("PaX slab sanitization: %s\n", "disabled");
+		pax_sanitize_slab = PAX_SANITIZE_SLAB_OFF;
+	} else if (!strcmp(str, "1") || !strcmp(str, "fast")) {
+		pr_info("PaX slab sanitization: %s\n", "fast");
+		pax_sanitize_slab = PAX_SANITIZE_SLAB_FAST;
+	} else if (!strcmp(str, "full")) {
+		pr_info("PaX slab sanitization: %s\n", "full");
+		pax_sanitize_slab = PAX_SANITIZE_SLAB_FULL;
+	} else
+		pr_err("PaX slab sanitization: unsupported option '%s'\n", str);
+
+	return 0;
+}
+early_param("pax_sanitize_slab", pax_sanitize_slab_setup);
+#endif
+
 /*
  * Set of flags that will prevent slab merging
  */
@@ -1136,6 +1167,9 @@ static void print_slabinfo_header(struct seq_file *m)
 #ifdef CONFIG_DEBUG_SLAB
 	seq_puts(m, " : globalstat <listallocs> <maxobjs> <grown> <reaped> <error> <maxfreeable> <nodeallocs> <remotefrees> <alienoverflow>");
 	seq_puts(m, " : cpustat <allochit> <allocmiss> <freehit> <freemiss>");
+#ifdef CONFIG_PAX_MEMORY_SANITIZE
+	seq_puts(m, " : pax <sanitized> <not_sanitized>");
+#endif
 #endif
 	seq_putc(m, '\n');
 }
diff --git a/mm/slob.c b/mm/slob.c
index eac04d43..f455845 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -365,6 +365,11 @@ static void slob_free(void *block, int size)
 		return;
 	}
 
+#ifdef CONFIG_PAX_MEMORY_SANITIZE
+	if (pax_sanitize_slab && !(c && (c->flags & SLAB_NO_SANITIZE)))
+		memset(block, PAX_MEMORY_SANITIZE_VALUE, size);
+#endif
+
 	if (!slob_page_free(sp)) {
 		/* This slob page is about to become partially free. Easy! */
 		sp->units = units;
diff --git a/mm/slub.c b/mm/slub.c
index 067598a..cead7ee 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2911,6 +2911,24 @@ static __always_inline void do_slab_free(struct kmem_cache *s,
 	void *tail_obj = tail ? : head;
 	struct kmem_cache_cpu *c;
 	unsigned long tid;
+
+#ifdef CONFIG_PAX_MEMORY_SANITIZE
+	if (!(s->flags & SLAB_NO_SANITIZE)) {
+		int offset = s->offset ? 0 : sizeof(void *);
+		void *x = head;
+
+		while (1) {
+			memset(x + offset, PAX_MEMORY_SANITIZE_VALUE,
+					s->object_size - offset);
+			if (s->ctor)
+				s->ctor(x);
+			if (x == tail_obj)
+				break;
+			x = get_freepointer(s, x);
+		}
+	}
+#endif
+
 redo:
 	/*
 	 * Determine the currently cpus per cpu slab.
@@ -5316,6 +5334,15 @@ static struct attribute *slab_attrs[] = {
 #ifdef CONFIG_ZONE_DMA
 	&cache_dma_attr.attr,
 #endif
+/**
+ * #ifdef CONFIG_PAX_MEMORY_SANITIZE
+ *	&sanitize_attr.attr,
+ * ?? -above statement from the grsec-4.8.17 patch fails compile with:
+ * mm/slub.c:5337:3: error: ‘sanitize_attr’ undeclared here (not in a function)
+ *   &sanitize_attr.attr,
+ *  ...
+ * #endif
+ */
 #ifdef CONFIG_NUMA
 	&remote_node_defrag_ratio_attr.attr,
 #endif
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 7ad67d7..2e60366 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3453,12 +3453,14 @@ void __init skb_init(void)
 	skbuff_head_cache = kmem_cache_create("skbuff_head_cache",
 					      sizeof(struct sk_buff),
 					      0,
-					      SLAB_HWCACHE_ALIGN|SLAB_PANIC,
+					      SLAB_HWCACHE_ALIGN | SLAB_PANIC |
+					      SLAB_NO_SANITIZE,
 					      NULL);
 	skbuff_fclone_cache = kmem_cache_create("skbuff_fclone_cache",
 						sizeof(struct sk_buff_fclones),
 						0,
-						SLAB_HWCACHE_ALIGN|SLAB_PANIC,
+						SLAB_HWCACHE_ALIGN | SLAB_PANIC |
+						SLAB_NO_SANITIZE,
 						NULL);
 }
 
diff --git a/security/Kconfig b/security/Kconfig
index 118f454..3ad5110 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -4,6 +4,46 @@
 
 menu "Security options"
 
+menu "Miscellaneous hardening features"
+
+config PAX_MEMORY_SANITIZE
+	bool "Sanitize all freed memory"
+	default y if (GRKERNSEC_CONFIG_AUTO && GRKERNSEC_CONFIG_PRIORITY_SECURITY)
+	help
+	  By saying Y here the kernel will erase memory pages and slab objects
+	  as soon as they are freed.  This in turn reduces the lifetime of data
+	  stored in them, making it less likely that sensitive information such
+	  as passwords, cryptographic secrets, etc stay in memory for too long.
+
+	  This is especially useful for programs whose runtime is short, long
+	  lived processes and the kernel itself benefit from this as long as
+	  they ensure timely freeing of memory that may hold sensitive
+	  information.
+
+	  A nice side effect of the sanitization of slab objects is the
+	  reduction of possible info leaks caused by padding bytes within the
+	  leaky structures.  Use-after-free bugs for structures containing
+	  pointers can also be detected as dereferencing the sanitized pointer
+	  will generate an access violation.
+
+	  The tradeoff is performance impact, on a single CPU system kernel
+	  compilation sees a 3% slowdown, other systems and workloads may vary
+	  and you are advised to test this feature on your expected workload
+	  before deploying it.
+
+	  The slab sanitization feature excludes a few slab caches per default
+	  for performance reasons.  To extend the feature to cover those as
+	  well, pass "pax_sanitize_slab=full" as kernel command line parameter.
+
+	  To reduce the performance penalty by sanitizing pages only, albeit
+	  limiting the effectiveness of this feature at the same time, slab
+	  sanitization can be disabled with the kernel command line parameter
+	  "pax_sanitize_slab=off".
+
+	  Note that this feature does not protect data stored in live pages,
+	  e.g., process memory swapped to disk may stay there for a long time.
+endmenu
+
 source security/keys/Kconfig
 
 config SECURITY_DMESG_RESTRICT

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [kernel-hardening] Merge in PAX_MEMORY_SANITIZE work from grsec to linux-next
  2017-01-18  4:21 [kernel-hardening] Merge in PAX_MEMORY_SANITIZE work from grsec to linux-next Kaiwan N Billimoria
@ 2017-01-18 19:44 ` Laura Abbott
  2017-01-31  1:49   ` Kaiwan N Billimoria
  2017-02-03  4:49   ` Kaiwan N Billimoria
  2017-01-19  0:54 ` Kees Cook
  1 sibling, 2 replies; 20+ messages in thread
From: Laura Abbott @ 2017-01-18 19:44 UTC (permalink / raw)
  To: kernel-hardening, keescook

On 01/17/2017 08:21 PM, Kaiwan N Billimoria wrote:
> Hi Kees,
> 
> So, following up on the previous discussion, 
>> I'd like to see the mentioned excluded slab caches also done in the
>> kernel, along with similar kernel command line options. Additionally,
>> getting all the upstream stuff behind a single CONFIG (similar to
>> CONFIG_PAX_MEMORY_SANITIZE) would be great, instead of having to set 3
>> CONFIGs and 2 kernel parameters. :)
> 
> Basically what I've done so far is:
> - pulled in the linux-next tree, and setup my own branch
> 
> - taken the grsecurity patch (for 4.8.17) and merged those portions of
> the code encompassing the  CONFIG_PAX_MEMORY_SANITIZE directive
> 
> - There were some compile issues, which seem to be there mostly because of other
> grsec infra that hasn't been merged in (yet). 
> For now, I've just done small fixups where required:
> (see code in ptach below):
> 
> [1. fs/dcache.c
>   kmem_cache_create_usercopy() unavailable (for now at least).
> Probably part of other grsec infrastructure..
>  So am just adding the SLAB_NO_SANITIZE flag to the usual
> kmem_cache_create() call.
> 
> 2. mm/slab_common.c
> Compile failure:
> enum pax_sanitize_mode pax_sanitize_slab __read_only = PAX_SANITIZE_SLAB_FAST;
> 
> ?? -above orig statement from the grsec-4.8.17 patch fails compile with:
> mm/slab_common.c:34:37: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or
> ‘__attribute__’ before ‘__read_only’
> 
> So I've just removed the "__read_only" attribute for now.
> What's the actual approach?
> 
> 3. mm/slub.c
> Compile failure:
> #ifdef CONFIG_PAX_MEMORY_SANITIZE
>  &sanitize_attr.attr,
>  * ?? -above statement from the grsec-4.8.17 patch fails compile with:
>  mm/slub.c:5337:3: error: ‘sanitize_attr’ undeclared here (not in a function)
>   &sanitize_attr.attr,
> 
> Just commented this out for now.
> ]

This is roughly the work I did before (http://www.openwall.com/lists/kernel-hardening/2015/12/22/1)

>From that discussion, the conclusion is that we need to
use the existing slab_debug infrastructure to do sanitization.
The part in mm/page_alloc.c has been turned into a separate
Kconfig.

As Kees mentioned, a good task would be to create a new Kconfig
(CONFIG_MEMORY_SANITIZE for example) that will turn on both
CONFIG_DEBUG_PAGEALLOC (the equivalent of CONFIG_PAX_MEMORY_SANITIZE)
and also turn on slab poisoning.

There's other stuff here you can do but hopefully working on
that will give you a chance to look and explore what's already
present vs. grsecurity.

Thanks,
Laura

> 
> 
> 
> ===
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 28484b3..b524eda 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -3511,7 +3511,7 @@ void __init buffer_init(void)
>  	bh_cachep = kmem_cache_create("buffer_head",
>  			sizeof(struct buffer_head), 0,
>  				(SLAB_RECLAIM_ACCOUNT|SLAB_PANIC|
> -				SLAB_MEM_SPREAD),
> +				SLAB_MEM_SPREAD|SLAB_NO_SANITIZE),
>  				NULL);
>  
>  	/*
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 95d71ed..95ed43c 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -3616,8 +3616,16 @@ void __init vfs_caches_init_early(void)
>  
>  void __init vfs_caches_init(void)
>  {
> +/**
> + *	names_cachep = kmem_cache_create_usercopy("names_cache", PATH_MAX, 0,
> + *			SLAB_HWCACHE_ALIGN|SLAB_PANIC| SLAB_NO_SANITIZE,
> + *			0, PATH_MAX, NULL);
> + *	kmem_cache_create_usercopy() unavailable for now at least.
> + *	So just adding the SLAB_NO_SANITIZE flag to the usual call below..
> + */
>  	names_cachep = kmem_cache_create("names_cache", PATH_MAX, 0,
> -			SLAB_HWCACHE_ALIGN|SLAB_PANIC, NULL);
> +			SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_NO_SANITIZE, NULL);
> +
>  
>  	dcache_init();
>  	inode_init();
> diff --git a/include/linux/highmem.h b/include/linux/highmem.h
> index bb3f329..9daed55 100644
> --- a/include/linux/highmem.h
> +++ b/include/linux/highmem.h
> @@ -190,6 +190,18 @@ static inline void clear_highpage(struct page *page)
>  	kunmap_atomic(kaddr);
>  }
>  
> +static inline void sanitize_highpage(struct page *page)
> +{
> +	void *kaddr;
> +	unsigned long flags;
> +
> +	local_irq_save(flags);
> +	kaddr = kmap_atomic(page);
> +	clear_page(kaddr);
> +	kunmap_atomic(kaddr);
> +	local_irq_restore(flags);
> +}
> +
>  static inline void zero_user_segments(struct page *page,
>  	unsigned start1, unsigned end1,
>  	unsigned start2, unsigned end2)
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 4c53635..334bd89 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -23,6 +23,13 @@
>  #define SLAB_CONSISTENCY_CHECKS	0x00000100UL	/* DEBUG: Perform (expensive) checks on alloc/free */
>  #define SLAB_RED_ZONE		0x00000400UL	/* DEBUG: Red zone objs in a cache */
>  #define SLAB_POISON		0x00000800UL	/* DEBUG: Poison objects */
> +
> +#ifdef CONFIG_PAX_MEMORY_SANITIZE
> +#define SLAB_NO_SANITIZE	0x00001000UL	/* PaX: Do not sanitize objs on free */
> +#else
> +#define SLAB_NO_SANITIZE	0x00000000UL
> +#endif
> +
>  #define SLAB_HWCACHE_ALIGN	0x00002000UL	/* Align objs on cache lines */
>  #define SLAB_CACHE_DMA		0x00004000UL	/* Use GFP_DMA memory */
>  #define SLAB_STORE_USER		0x00010000UL	/* DEBUG: Store the last owner for bug hunting */
> diff --git a/include/linux/slab_def.h b/include/linux/slab_def.h
> index 4ad2c5a..a30bbd2 100644
> --- a/include/linux/slab_def.h
> +++ b/include/linux/slab_def.h
> @@ -60,6 +60,10 @@ struct kmem_cache {
>  	atomic_t allocmiss;
>  	atomic_t freehit;
>  	atomic_t freemiss;
> +#ifdef CONFIG_PAX_MEMORY_SANITIZE
> +	atomic_unchecked_t sanitized;
> +	atomic_unchecked_t not_sanitized;
> +#endif
>  #ifdef CONFIG_DEBUG_SLAB_LEAK
>  	atomic_t store_user_clean;
>  #endif
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 7da33cb..42646d4 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2098,7 +2098,8 @@ void __init proc_caches_init(void)
>  			sizeof(struct mm_struct), ARCH_MIN_MMSTRUCT_ALIGN,
>  			SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_NOTRACK|SLAB_ACCOUNT,
>  			NULL);
> -	vm_area_cachep = KMEM_CACHE(vm_area_struct, SLAB_PANIC|SLAB_ACCOUNT);
> +	vm_area_cachep = KMEM_CACHE(vm_area_struct, SLAB_PANIC|SLAB_ACCOUNT|
> +			 SLAB_NO_SANITIZE);
>  	mmap_init();
>  	nsproxy_cache_init();
>  }
> diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug
> index afcc550..ed3f097 100644
> --- a/mm/Kconfig.debug
> +++ b/mm/Kconfig.debug
> @@ -10,6 +10,7 @@ config PAGE_EXTENSION
>  config DEBUG_PAGEALLOC
>  	bool "Debug page memory allocations"
>  	depends on DEBUG_KERNEL
> +	depends on !PAX_MEMORY_SANITIZE
>  	depends on !HIBERNATION || ARCH_SUPPORTS_DEBUG_PAGEALLOC && !PPC && !SPARC
>  	depends on !KMEMCHECK
>  	select PAGE_EXTENSION
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 21ea508..3e899fa 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -994,6 +994,10 @@ static __always_inline bool free_pages_prepare(struct page *page,
>  {
>  	int bad = 0;
>  
> +#ifdef CONFIG_PAX_MEMORY_SANITIZE
> +	unsigned long index = 1UL << order;
> +#endif
> +
>  	VM_BUG_ON_PAGE(PageTail(page), page);
>  
>  	trace_mm_page_free(page, order);
> @@ -1040,6 +1044,12 @@ static __always_inline bool free_pages_prepare(struct page *page,
>  		debug_check_no_obj_freed(page_address(page),
>  					   PAGE_SIZE << order);
>  	}
> +
> +#ifdef CONFIG_PAX_MEMORY_SANITIZE
> +	for (; index; --index)
> +		sanitize_highpage(page + index - 1);
> +#endif
> +
>  	arch_free_page(page, order);
>  	kernel_poison_pages(page, 1 << order, 0);
>  	kernel_map_pages(page, 1 << order, 0);
> @@ -1696,8 +1706,10 @@ static inline int check_new_page(struct page *page)
>  
>  static inline bool free_pages_prezeroed(bool poisoned)
>  {
> -	return IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) &&
> -		page_poisoning_enabled() && poisoned;
> +	return IS_ENABLED(CONFIG_PAX_MEMORY_SANITIZE) ||
> +		(IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) &&
> +		 page_poisoning_enabled() && poisoned);
> +
>  }
>  
>  #ifdef CONFIG_DEBUG_VM
> @@ -1753,11 +1765,13 @@ static void prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags
>  	int i;
>  	bool poisoned = true;
>  
> +#ifndef CONFIG_PAX_MEMORY_SANITIZE
>  	for (i = 0; i < (1 << order); i++) {
>  		struct page *p = page + i;
>  		if (poisoned)
>  			poisoned &= page_is_poisoned(p);
>  	}
> +#endif
>  
>  	post_alloc_hook(page, order, gfp_flags);
>  
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 91619fd..e209ff6 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -428,10 +428,10 @@ static void anon_vma_ctor(void *data)
>  void __init anon_vma_init(void)
>  {
>  	anon_vma_cachep = kmem_cache_create("anon_vma", sizeof(struct anon_vma),
> -			0, SLAB_DESTROY_BY_RCU|SLAB_PANIC|SLAB_ACCOUNT,
> -			anon_vma_ctor);
> +			0, SLAB_DESTROY_BY_RCU|SLAB_PANIC|SLAB_ACCOUNT|
> +			SLAB_NO_SANITIZE, anon_vma_ctor);
>  	anon_vma_chain_cachep = KMEM_CACHE(anon_vma_chain,
> -			SLAB_PANIC|SLAB_ACCOUNT);
> +			SLAB_PANIC|SLAB_ACCOUNT|SLAB_NO_SANITIZE);
>  }
>  
>  /*
> diff --git a/mm/slab.c b/mm/slab.c
> index 4f2ec6b..6dc9f4a 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -3511,6 +3511,20 @@ void ___cache_free(struct kmem_cache *cachep, void *objp,
>  	struct array_cache *ac = cpu_cache_get(cachep);
>  
>  	check_irq_off();
> +
> +#ifdef CONFIG_PAX_MEMORY_SANITIZE
> +	if (cachep->flags & (SLAB_POISON | SLAB_NO_SANITIZE))
> +		STATS_INC_NOT_SANITIZED(cachep);
> +	else {
> +		memset(objp, PAX_MEMORY_SANITIZE_VALUE, cachep->object_size);
> +
> +		if (cachep->ctor)
> +			cachep->ctor(objp);
> +
> +		STATS_INC_SANITIZED(cachep);
> +	}
> +#endif
> +
>  	kmemleak_free_recursive(objp, cachep->flags);
>  	objp = cache_free_debugcheck(cachep, objp, caller);
>  
> @@ -4157,6 +4171,14 @@ void slabinfo_show_stats(struct seq_file *m, struct kmem_cache *cachep)
>  		seq_printf(m, " : cpustat %6lu %6lu %6lu %6lu",
>  			   allochit, allocmiss, freehit, freemiss);
>  	}
> +#ifdef CONFIG_PAX_MEMORY_SANITIZE
> +	{
> +		unsigned long sanitized = atomic_read_unchecked(&cachep->sanitized);
> +		unsigned long not_sanitized = atomic_read_unchecked(&cachep->not_sanitized);
> +
> +		seq_printf(m, " : pax %6lu %6lu", sanitized, not_sanitized);
> +	}
> +#endif
>  #endif
>  }
>  
> diff --git a/mm/slab.h b/mm/slab.h
> index de6579d..2965ebe 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -71,6 +71,36 @@ extern struct list_head slab_caches;
>  /* The slab cache that manages slab cache information */
>  extern struct kmem_cache *kmem_cache;
>  
> +#ifdef CONFIG_PAX_MEMORY_SANITIZE
> +#ifdef CONFIG_X86_64
> +#define PAX_MEMORY_SANITIZE_VALUE	'\xfe'
> +#else
> +#define PAX_MEMORY_SANITIZE_VALUE	'\xff'
> +#endif
> +enum pax_sanitize_mode {
> +	PAX_SANITIZE_SLAB_OFF = 0,
> +	PAX_SANITIZE_SLAB_FAST,
> +	PAX_SANITIZE_SLAB_FULL,
> +};
> +
> +extern enum pax_sanitize_mode pax_sanitize_slab;
> +
> +static inline unsigned long pax_sanitize_slab_flags(unsigned long flags)
> +{
> +	if (pax_sanitize_slab == PAX_SANITIZE_SLAB_OFF ||
> +	    (flags & SLAB_DESTROY_BY_RCU))
> +		flags |= SLAB_NO_SANITIZE;
> +	else if (pax_sanitize_slab == PAX_SANITIZE_SLAB_FULL)
> +		flags &= ~SLAB_NO_SANITIZE;
> +	return flags;
> +}
> +#else
> +static inline unsigned long pax_sanitize_slab_flags(unsigned long flags)
> +{
> +	return flags;
> +}
> +#endif
> +
>  unsigned long calculate_alignment(unsigned long flags,
>  		unsigned long align, unsigned long size);
>  
> @@ -120,7 +150,8 @@ static inline unsigned long kmem_cache_flags(unsigned long object_size,
>  
>  /* Legal flag mask for kmem_cache_create(), for various configurations */
>  #define SLAB_CORE_FLAGS (SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA | SLAB_PANIC | \
> -			 SLAB_DESTROY_BY_RCU | SLAB_DEBUG_OBJECTS )
> +			 SLAB_DESTROY_BY_RCU | SLAB_DEBUG_OBJECTS | \
> +			 SLAB_NO_SANITIZE)
>  
>  #if defined(CONFIG_DEBUG_SLAB)
>  #define SLAB_DEBUG_FLAGS (SLAB_RED_ZONE | SLAB_POISON | SLAB_STORE_USER)
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 1dfc209..0a0851f 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -30,6 +30,37 @@ LIST_HEAD(slab_caches);
>  DEFINE_MUTEX(slab_mutex);
>  struct kmem_cache *kmem_cache;
>  
> +#ifdef CONFIG_PAX_MEMORY_SANITIZE
> +/**
> + * enum pax_sanitize_mode pax_sanitize_slab __read_only = PAX_SANITIZE_SLAB_FAST;
> + *
> + * ?? -above orig statement from the grsec-4.8.17 patch fails compile with:
> + * mm/slab_common.c:34:37: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘__read_only’
> + *  pax_sanitize_mode pax_sanitize_slab __read_only = PAX_SANITIZE_SLAB_FAST;
> + */
> +enum pax_sanitize_mode pax_sanitize_slab = PAX_SANITIZE_SLAB_FAST;
> +static int __init pax_sanitize_slab_setup(char *str)
> +{
> +	if (!str)
> +		return 0;
> +
> +	if (!strcmp(str, "0") || !strcmp(str, "off")) {
> +		pr_info("PaX slab sanitization: %s\n", "disabled");
> +		pax_sanitize_slab = PAX_SANITIZE_SLAB_OFF;
> +	} else if (!strcmp(str, "1") || !strcmp(str, "fast")) {
> +		pr_info("PaX slab sanitization: %s\n", "fast");
> +		pax_sanitize_slab = PAX_SANITIZE_SLAB_FAST;
> +	} else if (!strcmp(str, "full")) {
> +		pr_info("PaX slab sanitization: %s\n", "full");
> +		pax_sanitize_slab = PAX_SANITIZE_SLAB_FULL;
> +	} else
> +		pr_err("PaX slab sanitization: unsupported option '%s'\n", str);
> +
> +	return 0;
> +}
> +early_param("pax_sanitize_slab", pax_sanitize_slab_setup);
> +#endif
> +
>  /*
>   * Set of flags that will prevent slab merging
>   */
> @@ -1136,6 +1167,9 @@ static void print_slabinfo_header(struct seq_file *m)
>  #ifdef CONFIG_DEBUG_SLAB
>  	seq_puts(m, " : globalstat <listallocs> <maxobjs> <grown> <reaped> <error> <maxfreeable> <nodeallocs> <remotefrees> <alienoverflow>");
>  	seq_puts(m, " : cpustat <allochit> <allocmiss> <freehit> <freemiss>");
> +#ifdef CONFIG_PAX_MEMORY_SANITIZE
> +	seq_puts(m, " : pax <sanitized> <not_sanitized>");
> +#endif
>  #endif
>  	seq_putc(m, '\n');
>  }
> diff --git a/mm/slob.c b/mm/slob.c
> index eac04d43..f455845 100644
> --- a/mm/slob.c
> +++ b/mm/slob.c
> @@ -365,6 +365,11 @@ static void slob_free(void *block, int size)
>  		return;
>  	}
>  
> +#ifdef CONFIG_PAX_MEMORY_SANITIZE
> +	if (pax_sanitize_slab && !(c && (c->flags & SLAB_NO_SANITIZE)))
> +		memset(block, PAX_MEMORY_SANITIZE_VALUE, size);
> +#endif
> +
>  	if (!slob_page_free(sp)) {
>  		/* This slob page is about to become partially free. Easy! */
>  		sp->units = units;
> diff --git a/mm/slub.c b/mm/slub.c
> index 067598a..cead7ee 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2911,6 +2911,24 @@ static __always_inline void do_slab_free(struct kmem_cache *s,
>  	void *tail_obj = tail ? : head;
>  	struct kmem_cache_cpu *c;
>  	unsigned long tid;
> +
> +#ifdef CONFIG_PAX_MEMORY_SANITIZE
> +	if (!(s->flags & SLAB_NO_SANITIZE)) {
> +		int offset = s->offset ? 0 : sizeof(void *);
> +		void *x = head;
> +
> +		while (1) {
> +			memset(x + offset, PAX_MEMORY_SANITIZE_VALUE,
> +					s->object_size - offset);
> +			if (s->ctor)
> +				s->ctor(x);
> +			if (x == tail_obj)
> +				break;
> +			x = get_freepointer(s, x);
> +		}
> +	}
> +#endif
> +
>  redo:
>  	/*
>  	 * Determine the currently cpus per cpu slab.
> @@ -5316,6 +5334,15 @@ static struct attribute *slab_attrs[] = {
>  #ifdef CONFIG_ZONE_DMA
>  	&cache_dma_attr.attr,
>  #endif
> +/**
> + * #ifdef CONFIG_PAX_MEMORY_SANITIZE
> + *	&sanitize_attr.attr,
> + * ?? -above statement from the grsec-4.8.17 patch fails compile with:
> + * mm/slub.c:5337:3: error: ‘sanitize_attr’ undeclared here (not in a function)
> + *   &sanitize_attr.attr,
> + *  ...
> + * #endif
> + */
>  #ifdef CONFIG_NUMA
>  	&remote_node_defrag_ratio_attr.attr,
>  #endif
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 7ad67d7..2e60366 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3453,12 +3453,14 @@ void __init skb_init(void)
>  	skbuff_head_cache = kmem_cache_create("skbuff_head_cache",
>  					      sizeof(struct sk_buff),
>  					      0,
> -					      SLAB_HWCACHE_ALIGN|SLAB_PANIC,
> +					      SLAB_HWCACHE_ALIGN | SLAB_PANIC |
> +					      SLAB_NO_SANITIZE,
>  					      NULL);
>  	skbuff_fclone_cache = kmem_cache_create("skbuff_fclone_cache",
>  						sizeof(struct sk_buff_fclones),
>  						0,
> -						SLAB_HWCACHE_ALIGN|SLAB_PANIC,
> +						SLAB_HWCACHE_ALIGN | SLAB_PANIC |
> +						SLAB_NO_SANITIZE,
>  						NULL);
>  }
>  
> diff --git a/security/Kconfig b/security/Kconfig
> index 118f454..3ad5110 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -4,6 +4,46 @@
>  
>  menu "Security options"
>  
> +menu "Miscellaneous hardening features"
> +
> +config PAX_MEMORY_SANITIZE
> +	bool "Sanitize all freed memory"
> +	default y if (GRKERNSEC_CONFIG_AUTO && GRKERNSEC_CONFIG_PRIORITY_SECURITY)
> +	help
> +	  By saying Y here the kernel will erase memory pages and slab objects
> +	  as soon as they are freed.  This in turn reduces the lifetime of data
> +	  stored in them, making it less likely that sensitive information such
> +	  as passwords, cryptographic secrets, etc stay in memory for too long.
> +
> +	  This is especially useful for programs whose runtime is short, long
> +	  lived processes and the kernel itself benefit from this as long as
> +	  they ensure timely freeing of memory that may hold sensitive
> +	  information.
> +
> +	  A nice side effect of the sanitization of slab objects is the
> +	  reduction of possible info leaks caused by padding bytes within the
> +	  leaky structures.  Use-after-free bugs for structures containing
> +	  pointers can also be detected as dereferencing the sanitized pointer
> +	  will generate an access violation.
> +
> +	  The tradeoff is performance impact, on a single CPU system kernel
> +	  compilation sees a 3% slowdown, other systems and workloads may vary
> +	  and you are advised to test this feature on your expected workload
> +	  before deploying it.
> +
> +	  The slab sanitization feature excludes a few slab caches per default
> +	  for performance reasons.  To extend the feature to cover those as
> +	  well, pass "pax_sanitize_slab=full" as kernel command line parameter.
> +
> +	  To reduce the performance penalty by sanitizing pages only, albeit
> +	  limiting the effectiveness of this feature at the same time, slab
> +	  sanitization can be disabled with the kernel command line parameter
> +	  "pax_sanitize_slab=off".
> +
> +	  Note that this feature does not protect data stored in live pages,
> +	  e.g., process memory swapped to disk may stay there for a long time.
> +endmenu
> +
>  source security/keys/Kconfig
>  
>  config SECURITY_DMESG_RESTRICT
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [kernel-hardening] Merge in PAX_MEMORY_SANITIZE work from grsec to linux-next
  2017-01-18  4:21 [kernel-hardening] Merge in PAX_MEMORY_SANITIZE work from grsec to linux-next Kaiwan N Billimoria
  2017-01-18 19:44 ` Laura Abbott
@ 2017-01-19  0:54 ` Kees Cook
  1 sibling, 0 replies; 20+ messages in thread
From: Kees Cook @ 2017-01-19  0:54 UTC (permalink / raw)
  To: Kaiwan N Billimoria; +Cc: kernel-hardening

On Tue, Jan 17, 2017 at 8:21 PM, Kaiwan N Billimoria
<kaiwan@kaiwantech.com> wrote:
> So, following up on the previous discussion,

Thanks for working on this!

>>I'd like to see the mentioned excluded slab caches also done in the
>>kernel, along with similar kernel command line options. Additionally,
>>getting all the upstream stuff behind a single CONFIG (similar to
>>CONFIG_PAX_MEMORY_SANITIZE) would be great, instead of having to set 3
>>CONFIGs and 2 kernel parameters. :)
>
> Basically what I've done so far is:
> - pulled in the linux-next tree, and setup my own branch
>
> - taken the grsecurity patch (for 4.8.17) and merged those portions of
> the code encompassing the  CONFIG_PAX_MEMORY_SANITIZE directive

This is likely good for reference, but it's not going to work for
upstreaming. (And most of what I'll say below echoes Laura's email.)

> - There were some compile issues, which seem to be there mostly because of other
> grsec infra that hasn't been merged in (yet).
> For now, I've just done small fixups where required:
> (see code in ptach below):
>
> [1. fs/dcache.c
>   kmem_cache_create_usercopy() unavailable (for now at least).
> Probably part of other grsec infrastructure..
>  So am just adding the SLAB_NO_SANITIZE flag to the usual
> kmem_cache_create() call.

These SLAB_NO_SANITIZE uses are one of the things I want to see added
to upstream. This flag would be used to mark the performance-sensitive
(and low security risk) slabs, and then they could be excluded from
the upstream poisoning (rather than using the PaX sanitization).

> 2. mm/slab_common.c
> Compile failure:
> enum pax_sanitize_mode pax_sanitize_slab __read_only = PAX_SANITIZE_SLAB_FAST;
>
> ?? -above orig statement from the grsec-4.8.17 patch fails compile with:
> mm/slab_common.c:34:37: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or
> ‘__attribute__’ before ‘__read_only’
>
> So I've just removed the "__read_only" attribute for now.
> What's the actual approach?

Right, this is a marking for a separate PaX feature.

> 3. mm/slub.c
> Compile failure:
> #ifdef CONFIG_PAX_MEMORY_SANITIZE
>  &sanitize_attr.attr,
>  * ?? -above statement from the grsec-4.8.17 patch fails compile with:
>  mm/slub.c:5337:3: error: ‘sanitize_attr’ undeclared here (not in a function)
>   &sanitize_attr.attr,
>
> Just commented this out for now.
> ]

As Laura mentioned, the path for upstream sanitization is to use the
existing upstream poisoning code instead of adding new sanitization.

> ===
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 28484b3..b524eda 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -3511,7 +3511,7 @@ void __init buffer_init(void)
>         bh_cachep = kmem_cache_create("buffer_head",
>                         sizeof(struct buffer_head), 0,
>                                 (SLAB_RECLAIM_ACCOUNT|SLAB_PANIC|
> -                               SLAB_MEM_SPREAD),
> +                               SLAB_MEM_SPREAD|SLAB_NO_SANITIZE),
>                                 NULL);

Another part of this is to better understand why these slabs were
chosen to have their sanitization disabled. That needs to be expressed
to help others guide any future application of the SLAB_NO_SANITIZE
flag.

> diff --git a/mm/slab.h b/mm/slab.h
> index de6579d..2965ebe 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -71,6 +71,36 @@ extern struct list_head slab_caches;
>  /* The slab cache that manages slab cache information */
>  extern struct kmem_cache *kmem_cache;
>
> +#ifdef CONFIG_PAX_MEMORY_SANITIZE
> +#ifdef CONFIG_X86_64
> +#define PAX_MEMORY_SANITIZE_VALUE      '\xfe'
> +#else
> +#define PAX_MEMORY_SANITIZE_VALUE      '\xff'
> +#endif

This is another element of the sanitization I'd like to bring to
upstream: changing the poison value. IIUC, these values were chosen so
that on 64-bit, if a pointer were referenced it would land at address
0xfefefe.... which is in the non-canonical memory space. For 32-bit,
the it would be at 0xffffffff so it would be at the top of memory.
This would frustrate attempts to use the existing poison value as an
actual address.

(The new upstream CONFIG would include switching this poison value...)

> +enum pax_sanitize_mode {
> +       PAX_SANITIZE_SLAB_OFF = 0,
> +       PAX_SANITIZE_SLAB_FAST,
> +       PAX_SANITIZE_SLAB_FULL,
> +};
> +
> +extern enum pax_sanitize_mode pax_sanitize_slab;
> +
> +static inline unsigned long pax_sanitize_slab_flags(unsigned long flags)
> +{
> +       if (pax_sanitize_slab == PAX_SANITIZE_SLAB_OFF ||
> +           (flags & SLAB_DESTROY_BY_RCU))
> +               flags |= SLAB_NO_SANITIZE;
> +       else if (pax_sanitize_slab == PAX_SANITIZE_SLAB_FULL)
> +               flags &= ~SLAB_NO_SANITIZE;
> +       return flags;
> +}
> +#else
> +static inline unsigned long pax_sanitize_slab_flags(unsigned long flags)
> +{
> +       return flags;
> +}
> +#endif

This contains an interesting effect: RCU slabs aren't sanitized. It's
unclear to me if that's due to the PaX sanitization implementation or
if upstream poisoning also must avoid RCU slabs.

> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 1dfc209..0a0851f 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -30,6 +30,37 @@ LIST_HEAD(slab_caches);
>  DEFINE_MUTEX(slab_mutex);
>  struct kmem_cache *kmem_cache;
>
> +#ifdef CONFIG_PAX_MEMORY_SANITIZE
> +/**
> + * enum pax_sanitize_mode pax_sanitize_slab __read_only = PAX_SANITIZE_SLAB_FAST;
> + *
> + * ?? -above orig statement from the grsec-4.8.17 patch fails compile with:
> + * mm/slab_common.c:34:37: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘__read_only’
> + *  pax_sanitize_mode pax_sanitize_slab __read_only = PAX_SANITIZE_SLAB_FAST;
> + */
> +enum pax_sanitize_mode pax_sanitize_slab = PAX_SANITIZE_SLAB_FAST;
> +static int __init pax_sanitize_slab_setup(char *str)
> +{
> +       if (!str)
> +               return 0;
> +
> +       if (!strcmp(str, "0") || !strcmp(str, "off")) {
> +               pr_info("PaX slab sanitization: %s\n", "disabled");
> +               pax_sanitize_slab = PAX_SANITIZE_SLAB_OFF;
> +       } else if (!strcmp(str, "1") || !strcmp(str, "fast")) {
> +               pr_info("PaX slab sanitization: %s\n", "fast");
> +               pax_sanitize_slab = PAX_SANITIZE_SLAB_FAST;
> +       } else if (!strcmp(str, "full")) {
> +               pr_info("PaX slab sanitization: %s\n", "full");
> +               pax_sanitize_slab = PAX_SANITIZE_SLAB_FULL;
> +       } else
> +               pr_err("PaX slab sanitization: unsupported option '%s'\n", str);
> +
> +       return 0;
> +}
> +early_param("pax_sanitize_slab", pax_sanitize_slab_setup);
> +#endif

Getting the naming right matter too. Some kind of common language for
the new CONFIG and the resulting kernel cmdline option would be nice.
I like "sanitize", though everything else in upstream currently uses
"poison". Due to the behavioral changes (poison values changing and
optional poisoning), perhaps it shouldn't be called "poison". I'm open
to ideas! :)

> diff --git a/security/Kconfig b/security/Kconfig
> index 118f454..3ad5110 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -4,6 +4,46 @@
>
>  menu "Security options"
>
> +menu "Miscellaneous hardening features"
> +
> +config PAX_MEMORY_SANITIZE
> +       bool "Sanitize all freed memory"
> +       default y if (GRKERNSEC_CONFIG_AUTO && GRKERNSEC_CONFIG_PRIORITY_SECURITY)
> +       help
> +         By saying Y here the kernel will erase memory pages and slab objects
> +         as soon as they are freed.  This in turn reduces the lifetime of data
> +         stored in them, making it less likely that sensitive information such
> +         as passwords, cryptographic secrets, etc stay in memory for too long.
> +
> +         This is especially useful for programs whose runtime is short, long
> +         lived processes and the kernel itself benefit from this as long as
> +         they ensure timely freeing of memory that may hold sensitive
> +         information.
> +
> +         A nice side effect of the sanitization of slab objects is the
> +         reduction of possible info leaks caused by padding bytes within the
> +         leaky structures.  Use-after-free bugs for structures containing
> +         pointers can also be detected as dereferencing the sanitized pointer
> +         will generate an access violation.

It may be worth noting that new kernel stacks may be filled with the
poison value (since they were at some time freed and reallocated as a
kernel stack), though this needs some further examination, especially
since stacks have moved to vmap.

> +         The tradeoff is performance impact, on a single CPU system kernel
> +         compilation sees a 3% slowdown, other systems and workloads may vary
> +         and you are advised to test this feature on your expected workload
> +         before deploying it.

I'd like to see benchmarks against upstream's implementation (I linked
to my earlier efforts at this before).

> +         The slab sanitization feature excludes a few slab caches per default
> +         for performance reasons.  To extend the feature to cover those as
> +         well, pass "pax_sanitize_slab=full" as kernel command line parameter.
> +
> +         To reduce the performance penalty by sanitizing pages only, albeit
> +         limiting the effectiveness of this feature at the same time, slab
> +         sanitization can be disabled with the kernel command line parameter
> +         "pax_sanitize_slab=off".
> +
> +         Note that this feature does not protect data stored in live pages,
> +         e.g., process memory swapped to disk may stay there for a long time.
> +endmenu
> +
>  source security/keys/Kconfig
>
>  config SECURITY_DMESG_RESTRICT

This likely needs to live near the other POISON configs, and, as Laura
mentioned, needs to select the various parts from upstream needed to
enable poisoning. Also, the kernel command line options needed for
upstream poisoning to be enabled need to check against the new CONFIG.

For testing, you can try out the LKDTM tests that cover poisoning,
namely READ_AFTER_FREE and READ_BUDDY_AFTER_FREE.

-Kees

-- 
Kees Cook
Nexus Security

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [kernel-hardening] Merge in PAX_MEMORY_SANITIZE work from grsec to linux-next
  2017-01-18 19:44 ` Laura Abbott
@ 2017-01-31  1:49   ` Kaiwan N Billimoria
  2017-02-03  4:49   ` Kaiwan N Billimoria
  1 sibling, 0 replies; 20+ messages in thread
From: Kaiwan N Billimoria @ 2017-01-31  1:49 UTC (permalink / raw)
  To: Laura Abbott, kernel-hardening; +Cc: keescook

Apologies for the delayed response!

>On Wed, 18 Jan 2017 11:44:47 -0800
>Laura Abbott <labbott@redhat.com> wrote:
> This is roughly the work I did before
> (http://www.openwall.com/lists/kernel-hardening/2015/12/22/1)

​Yes Laura, indeed it _is_ your code that I merged into linux-next, just
attempting to move forward. Thanks :)
​
> From that discussion, the conclusion is that we need to
> use the existing slab_debug infrastructure to do sanitization.
> The part in mm/page_alloc.c has been turned into a separate
> Kconfig.

Ok, so, I'll attempt working on going down this path, also taking into
account what you mentioned Kees.. will take it a step at a time though!​
 
> As Kees mentioned, a good task would be to create a new Kconfig
> (CONFIG_MEMORY_SANITIZE for example) that will turn on both
> CONFIG_DEBUG_PAGEALLOC (the equivalent of CONFIG_PAX_MEMORY_SANITIZE)
> and also turn on slab poisoning.
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [kernel-hardening] Merge in PAX_MEMORY_SANITIZE work from grsec to linux-next
  2017-01-18 19:44 ` Laura Abbott
  2017-01-31  1:49   ` Kaiwan N Billimoria
@ 2017-02-03  4:49   ` Kaiwan N Billimoria
  2017-02-04  0:23     ` Kees Cook
  1 sibling, 1 reply; 20+ messages in thread
From: Kaiwan N Billimoria @ 2017-02-03  4:49 UTC (permalink / raw)
  To: Laura Abbott; +Cc: kernel-hardening, keescook

Pl bear with me, am trying to understand this correctly.
From what I gather, we look to create a new config directive, say,
CONFIG_MEMORY_SANITIZE .

It will turn ON :
 CONFIG_DEBUG_PAGEALLOC 
   (which internally turns ON CONFIG_PAGE_EXTENSION), and
 CONFIG_PAGE_POISONING
   (similar internal turn-ons :).

The new config now enables the CONFIG_PAX_MEMORY_SANITIZE code (my prev 
email patch), correct? (leaving out the stuff we cannot get without the 
full grsec implementation).
-Would include the 'new' poison / sanitize values of 0xfe and 0xff for slab
(64 and 32-bit resp), etc etc.


Even if this is (somewhat) correct, my thinking is - correct me I'm totally 
wrong here - that the whole purpose of the exercise is to improve _security_;
hence, tying this code into the "debug features" of the kernel isn't really 
what we want, yes?
Most folks would only use debug during development, if at all - given all the 
concerns regarding performance. Here, the objective is to enable a powerful 
security feature set. Hence, the config directives should come under the 
'Security Options' menu.

Regards,
Kaiwan.

On Wed, 18 Jan 2017 11:44:47 -0800
Laura Abbott <labbott@redhat.com> wrote:

> On 01/17/2017 08:21 PM, Kaiwan N Billimoria wrote:
> > Hi Kees,
> > 
> > So, following up on the previous discussion,   
> >> I'd like to see the mentioned excluded slab caches also done in the
> >> kernel, along with similar kernel command line options.
> >> Additionally, getting all the upstream stuff behind a single
> >> CONFIG (similar to CONFIG_PAX_MEMORY_SANITIZE) would be great,
> >> instead of having to set 3 CONFIGs and 2 kernel parameters. :)  
> > 
> > Basically what I've done so far is:
> > - pulled in the linux-next tree, and setup my own branch
> > 
> > - taken the grsecurity patch (for 4.8.17) and merged those portions
> > of the code encompassing the  CONFIG_PAX_MEMORY_SANITIZE directive
> > 
> > - There were some compile issues, which seem to be there mostly
> > because of other grsec infra that hasn't been merged in (yet). 
> > For now, I've just done small fixups where required:
> > (see code in ptach below):
> > 
> > [1. fs/dcache.c
> >   kmem_cache_create_usercopy() unavailable (for now at least).
> > Probably part of other grsec infrastructure..
> >  So am just adding the SLAB_NO_SANITIZE flag to the usual
> > kmem_cache_create() call.
> > 
> > 2. mm/slab_common.c
> > Compile failure:
> > enum pax_sanitize_mode pax_sanitize_slab __read_only =
> > PAX_SANITIZE_SLAB_FAST;
> > 
> > ?? -above orig statement from the grsec-4.8.17 patch fails compile
> > with: mm/slab_common.c:34:37: error: expected ‘=’, ‘,’, ‘;’, ‘asm’
> > or ‘__attribute__’ before ‘__read_only’
> > 
> > So I've just removed the "__read_only" attribute for now.
> > What's the actual approach?
> > 
> > 3. mm/slub.c
> > Compile failure:
> > #ifdef CONFIG_PAX_MEMORY_SANITIZE
> >  &sanitize_attr.attr,
> >  * ?? -above statement from the grsec-4.8.17 patch fails compile
> > with: mm/slub.c:5337:3: error: ‘sanitize_attr’ undeclared here (not
> > in a function) &sanitize_attr.attr,
> > 
> > Just commented this out for now.
> > ]  
> 
> This is roughly the work I did before
> (http://www.openwall.com/lists/kernel-hardening/2015/12/22/1)
> 
> From that discussion, the conclusion is that we need to
> use the existing slab_debug infrastructure to do sanitization.
> The part in mm/page_alloc.c has been turned into a separate
> Kconfig.
> 
> As Kees mentioned, a good task would be to create a new Kconfig
> (CONFIG_MEMORY_SANITIZE for example) that will turn on both
> CONFIG_DEBUG_PAGEALLOC (the equivalent of CONFIG_PAX_MEMORY_SANITIZE)
> and also turn on slab poisoning.
> 
> There's other stuff here you can do but hopefully working on
> that will give you a chance to look and explore what's already
> present vs. grsecurity.
> 
> Thanks,
> Laura
> 
> > 
> > 
> > 
> > ===
> > diff --git a/fs/buffer.c b/fs/buffer.c
> > index 28484b3..b524eda 100644
> > --- a/fs/buffer.c
> > +++ b/fs/buffer.c
> > @@ -3511,7 +3511,7 @@ void __init buffer_init(void)
> >  	bh_cachep = kmem_cache_create("buffer_head",
> >  			sizeof(struct buffer_head), 0,
> >  				(SLAB_RECLAIM_ACCOUNT|SLAB_PANIC|
> > -				SLAB_MEM_SPREAD),
> > +				SLAB_MEM_SPREAD|SLAB_NO_SANITIZE),
> >  				NULL);
> >  
> >  	/*
> > diff --git a/fs/dcache.c b/fs/dcache.c
> > index 95d71ed..95ed43c 100644
> > --- a/fs/dcache.c
> > +++ b/fs/dcache.c
> > @@ -3616,8 +3616,16 @@ void __init vfs_caches_init_early(void)
> >  
> >  void __init vfs_caches_init(void)
> >  {
> > +/**
> > + *	names_cachep = kmem_cache_create_usercopy("names_cache",
> > PATH_MAX, 0,
> > + *			SLAB_HWCACHE_ALIGN|SLAB_PANIC|
> > SLAB_NO_SANITIZE,
> > + *			0, PATH_MAX, NULL);
> > + *	kmem_cache_create_usercopy() unavailable for now at
> > least.
> > + *	So just adding the SLAB_NO_SANITIZE flag to the usual
> > call below..
> > + */
> >  	names_cachep = kmem_cache_create("names_cache", PATH_MAX,
> > 0,
> > -			SLAB_HWCACHE_ALIGN|SLAB_PANIC, NULL);
> > +
> > SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_NO_SANITIZE, NULL); +
> >  
> >  	dcache_init();
> >  	inode_init();
> > diff --git a/include/linux/highmem.h b/include/linux/highmem.h
> > index bb3f329..9daed55 100644
> > --- a/include/linux/highmem.h
> > +++ b/include/linux/highmem.h
> > @@ -190,6 +190,18 @@ static inline void clear_highpage(struct page
> > *page) kunmap_atomic(kaddr);
> >  }
> >  
> > +static inline void sanitize_highpage(struct page *page)
> > +{
> > +	void *kaddr;
> > +	unsigned long flags;
> > +
> > +	local_irq_save(flags);
> > +	kaddr = kmap_atomic(page);
> > +	clear_page(kaddr);
> > +	kunmap_atomic(kaddr);
> > +	local_irq_restore(flags);
> > +}
> > +
> >  static inline void zero_user_segments(struct page *page,
> >  	unsigned start1, unsigned end1,
> >  	unsigned start2, unsigned end2)
> > diff --git a/include/linux/slab.h b/include/linux/slab.h
> > index 4c53635..334bd89 100644
> > --- a/include/linux/slab.h
> > +++ b/include/linux/slab.h
> > @@ -23,6 +23,13 @@
> >  #define SLAB_CONSISTENCY_CHECKS	0x00000100UL	/*
> > DEBUG: Perform (expensive) checks on alloc/free */ #define
> > SLAB_RED_ZONE		0x00000400UL	/* DEBUG: Red zone
> > objs in a cache */ #define SLAB_POISON
> > 0x00000800UL	/* DEBUG: Poison objects */ + +#ifdef
> > CONFIG_PAX_MEMORY_SANITIZE +#define SLAB_NO_SANITIZE
> > 0x00001000UL	/* PaX: Do not sanitize objs on free */ +#else
> > +#define SLAB_NO_SANITIZE	0x00000000UL
> > +#endif
> > +
> >  #define SLAB_HWCACHE_ALIGN	0x00002000UL	/* Align
> > objs on cache lines */ #define SLAB_CACHE_DMA
> > 0x00004000UL	/* Use GFP_DMA memory */ #define
> > SLAB_STORE_USER		0x00010000UL	/* DEBUG: Store
> > the last owner for bug hunting */ diff --git
> > a/include/linux/slab_def.h b/include/linux/slab_def.h index
> > 4ad2c5a..a30bbd2 100644 --- a/include/linux/slab_def.h +++
> > b/include/linux/slab_def.h @@ -60,6 +60,10 @@ struct kmem_cache {
> >  	atomic_t allocmiss;
> >  	atomic_t freehit;
> >  	atomic_t freemiss;
> > +#ifdef CONFIG_PAX_MEMORY_SANITIZE
> > +	atomic_unchecked_t sanitized;
> > +	atomic_unchecked_t not_sanitized;
> > +#endif
> >  #ifdef CONFIG_DEBUG_SLAB_LEAK
> >  	atomic_t store_user_clean;
> >  #endif
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 7da33cb..42646d4 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -2098,7 +2098,8 @@ void __init proc_caches_init(void)
> >  			sizeof(struct mm_struct),
> > ARCH_MIN_MMSTRUCT_ALIGN,
> > SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_NOTRACK|SLAB_ACCOUNT, NULL);
> > -	vm_area_cachep = KMEM_CACHE(vm_area_struct,
> > SLAB_PANIC|SLAB_ACCOUNT);
> > +	vm_area_cachep = KMEM_CACHE(vm_area_struct,
> > SLAB_PANIC|SLAB_ACCOUNT|
> > +			 SLAB_NO_SANITIZE);
> >  	mmap_init();
> >  	nsproxy_cache_init();
> >  }
> > diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug
> > index afcc550..ed3f097 100644
> > --- a/mm/Kconfig.debug
> > +++ b/mm/Kconfig.debug
> > @@ -10,6 +10,7 @@ config PAGE_EXTENSION
> >  config DEBUG_PAGEALLOC
> >  	bool "Debug page memory allocations"
> >  	depends on DEBUG_KERNEL
> > +	depends on !PAX_MEMORY_SANITIZE
> >  	depends on !HIBERNATION || ARCH_SUPPORTS_DEBUG_PAGEALLOC
> > && !PPC && !SPARC depends on !KMEMCHECK
> >  	select PAGE_EXTENSION
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 21ea508..3e899fa 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -994,6 +994,10 @@ static __always_inline bool
> > free_pages_prepare(struct page *page, {
> >  	int bad = 0;
> >  
> > +#ifdef CONFIG_PAX_MEMORY_SANITIZE
> > +	unsigned long index = 1UL << order;
> > +#endif
> > +
> >  	VM_BUG_ON_PAGE(PageTail(page), page);
> >  
> >  	trace_mm_page_free(page, order);
> > @@ -1040,6 +1044,12 @@ static __always_inline bool
> > free_pages_prepare(struct page *page,
> > debug_check_no_obj_freed(page_address(page), PAGE_SIZE << order);
> >  	}
> > +
> > +#ifdef CONFIG_PAX_MEMORY_SANITIZE
> > +	for (; index; --index)
> > +		sanitize_highpage(page + index - 1);
> > +#endif
> > +
> >  	arch_free_page(page, order);
> >  	kernel_poison_pages(page, 1 << order, 0);
> >  	kernel_map_pages(page, 1 << order, 0);
> > @@ -1696,8 +1706,10 @@ static inline int check_new_page(struct page
> > *page) 
> >  static inline bool free_pages_prezeroed(bool poisoned)
> >  {
> > -	return IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) &&
> > -		page_poisoning_enabled() && poisoned;
> > +	return IS_ENABLED(CONFIG_PAX_MEMORY_SANITIZE) ||
> > +		(IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) &&
> > +		 page_poisoning_enabled() && poisoned);
> > +
> >  }
> >  
> >  #ifdef CONFIG_DEBUG_VM
> > @@ -1753,11 +1765,13 @@ static void prep_new_page(struct page
> > *page, unsigned int order, gfp_t gfp_flags int i;
> >  	bool poisoned = true;
> >  
> > +#ifndef CONFIG_PAX_MEMORY_SANITIZE
> >  	for (i = 0; i < (1 << order); i++) {
> >  		struct page *p = page + i;
> >  		if (poisoned)
> >  			poisoned &= page_is_poisoned(p);
> >  	}
> > +#endif
> >  
> >  	post_alloc_hook(page, order, gfp_flags);
> >  
> > diff --git a/mm/rmap.c b/mm/rmap.c
> > index 91619fd..e209ff6 100644
> > --- a/mm/rmap.c
> > +++ b/mm/rmap.c
> > @@ -428,10 +428,10 @@ static void anon_vma_ctor(void *data)
> >  void __init anon_vma_init(void)
> >  {
> >  	anon_vma_cachep = kmem_cache_create("anon_vma",
> > sizeof(struct anon_vma),
> > -			0,
> > SLAB_DESTROY_BY_RCU|SLAB_PANIC|SLAB_ACCOUNT,
> > -			anon_vma_ctor);
> > +			0,
> > SLAB_DESTROY_BY_RCU|SLAB_PANIC|SLAB_ACCOUNT|
> > +			SLAB_NO_SANITIZE, anon_vma_ctor);
> >  	anon_vma_chain_cachep = KMEM_CACHE(anon_vma_chain,
> > -			SLAB_PANIC|SLAB_ACCOUNT);
> > +			SLAB_PANIC|SLAB_ACCOUNT|SLAB_NO_SANITIZE);
> >  }
> >  
> >  /*
> > diff --git a/mm/slab.c b/mm/slab.c
> > index 4f2ec6b..6dc9f4a 100644
> > --- a/mm/slab.c
> > +++ b/mm/slab.c
> > @@ -3511,6 +3511,20 @@ void ___cache_free(struct kmem_cache
> > *cachep, void *objp, struct array_cache *ac = cpu_cache_get(cachep);
> >  
> >  	check_irq_off();
> > +
> > +#ifdef CONFIG_PAX_MEMORY_SANITIZE
> > +	if (cachep->flags & (SLAB_POISON | SLAB_NO_SANITIZE))
> > +		STATS_INC_NOT_SANITIZED(cachep);
> > +	else {
> > +		memset(objp, PAX_MEMORY_SANITIZE_VALUE,
> > cachep->object_size); +
> > +		if (cachep->ctor)
> > +			cachep->ctor(objp);
> > +
> > +		STATS_INC_SANITIZED(cachep);
> > +	}
> > +#endif
> > +
> >  	kmemleak_free_recursive(objp, cachep->flags);
> >  	objp = cache_free_debugcheck(cachep, objp, caller);
> >  
> > @@ -4157,6 +4171,14 @@ void slabinfo_show_stats(struct seq_file *m,
> > struct kmem_cache *cachep) seq_printf(m, " : cpustat %6lu %6lu %6lu
> > %6lu", allochit, allocmiss, freehit, freemiss);
> >  	}
> > +#ifdef CONFIG_PAX_MEMORY_SANITIZE
> > +	{
> > +		unsigned long sanitized =
> > atomic_read_unchecked(&cachep->sanitized);
> > +		unsigned long not_sanitized =
> > atomic_read_unchecked(&cachep->not_sanitized); +
> > +		seq_printf(m, " : pax %6lu %6lu", sanitized,
> > not_sanitized);
> > +	}
> > +#endif
> >  #endif
> >  }
> >  
> > diff --git a/mm/slab.h b/mm/slab.h
> > index de6579d..2965ebe 100644
> > --- a/mm/slab.h
> > +++ b/mm/slab.h
> > @@ -71,6 +71,36 @@ extern struct list_head slab_caches;
> >  /* The slab cache that manages slab cache information */
> >  extern struct kmem_cache *kmem_cache;
> >  
> > +#ifdef CONFIG_PAX_MEMORY_SANITIZE
> > +#ifdef CONFIG_X86_64
> > +#define PAX_MEMORY_SANITIZE_VALUE	'\xfe'
> > +#else
> > +#define PAX_MEMORY_SANITIZE_VALUE	'\xff'
> > +#endif
> > +enum pax_sanitize_mode {
> > +	PAX_SANITIZE_SLAB_OFF = 0,
> > +	PAX_SANITIZE_SLAB_FAST,
> > +	PAX_SANITIZE_SLAB_FULL,
> > +};
> > +
> > +extern enum pax_sanitize_mode pax_sanitize_slab;
> > +
> > +static inline unsigned long pax_sanitize_slab_flags(unsigned long
> > flags) +{
> > +	if (pax_sanitize_slab == PAX_SANITIZE_SLAB_OFF ||
> > +	    (flags & SLAB_DESTROY_BY_RCU))
> > +		flags |= SLAB_NO_SANITIZE;
> > +	else if (pax_sanitize_slab == PAX_SANITIZE_SLAB_FULL)
> > +		flags &= ~SLAB_NO_SANITIZE;
> > +	return flags;
> > +}
> > +#else
> > +static inline unsigned long pax_sanitize_slab_flags(unsigned long
> > flags) +{
> > +	return flags;
> > +}
> > +#endif
> > +
> >  unsigned long calculate_alignment(unsigned long flags,
> >  		unsigned long align, unsigned long size);
> >  
> > @@ -120,7 +150,8 @@ static inline unsigned long
> > kmem_cache_flags(unsigned long object_size, 
> >  /* Legal flag mask for kmem_cache_create(), for various
> > configurations */ #define SLAB_CORE_FLAGS (SLAB_HWCACHE_ALIGN |
> > SLAB_CACHE_DMA | SLAB_PANIC | \
> > -			 SLAB_DESTROY_BY_RCU | SLAB_DEBUG_OBJECTS )
> > +			 SLAB_DESTROY_BY_RCU | SLAB_DEBUG_OBJECTS
> > | \
> > +			 SLAB_NO_SANITIZE)
> >  
> >  #if defined(CONFIG_DEBUG_SLAB)
> >  #define SLAB_DEBUG_FLAGS (SLAB_RED_ZONE | SLAB_POISON |
> > SLAB_STORE_USER) diff --git a/mm/slab_common.c b/mm/slab_common.c
> > index 1dfc209..0a0851f 100644
> > --- a/mm/slab_common.c
> > +++ b/mm/slab_common.c
> > @@ -30,6 +30,37 @@ LIST_HEAD(slab_caches);
> >  DEFINE_MUTEX(slab_mutex);
> >  struct kmem_cache *kmem_cache;
> >  
> > +#ifdef CONFIG_PAX_MEMORY_SANITIZE
> > +/**
> > + * enum pax_sanitize_mode pax_sanitize_slab __read_only =
> > PAX_SANITIZE_SLAB_FAST;
> > + *
> > + * ?? -above orig statement from the grsec-4.8.17 patch fails
> > compile with:
> > + * mm/slab_common.c:34:37: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or
> > ‘__attribute__’ before ‘__read_only’
> > + *  pax_sanitize_mode pax_sanitize_slab __read_only =
> > PAX_SANITIZE_SLAB_FAST;
> > + */
> > +enum pax_sanitize_mode pax_sanitize_slab = PAX_SANITIZE_SLAB_FAST;
> > +static int __init pax_sanitize_slab_setup(char *str)
> > +{
> > +	if (!str)
> > +		return 0;
> > +
> > +	if (!strcmp(str, "0") || !strcmp(str, "off")) {
> > +		pr_info("PaX slab sanitization: %s\n", "disabled");
> > +		pax_sanitize_slab = PAX_SANITIZE_SLAB_OFF;
> > +	} else if (!strcmp(str, "1") || !strcmp(str, "fast")) {
> > +		pr_info("PaX slab sanitization: %s\n", "fast");
> > +		pax_sanitize_slab = PAX_SANITIZE_SLAB_FAST;
> > +	} else if (!strcmp(str, "full")) {
> > +		pr_info("PaX slab sanitization: %s\n", "full");
> > +		pax_sanitize_slab = PAX_SANITIZE_SLAB_FULL;
> > +	} else
> > +		pr_err("PaX slab sanitization: unsupported option
> > '%s'\n", str); +
> > +	return 0;
> > +}
> > +early_param("pax_sanitize_slab", pax_sanitize_slab_setup);
> > +#endif
> > +
> >  /*
> >   * Set of flags that will prevent slab merging
> >   */
> > @@ -1136,6 +1167,9 @@ static void print_slabinfo_header(struct
> > seq_file *m) #ifdef CONFIG_DEBUG_SLAB
> >  	seq_puts(m, " : globalstat <listallocs> <maxobjs> <grown>
> > <reaped> <error> <maxfreeable> <nodeallocs> <remotefrees>
> > <alienoverflow>"); seq_puts(m, " : cpustat <allochit> <allocmiss>
> > <freehit> <freemiss>"); +#ifdef CONFIG_PAX_MEMORY_SANITIZE
> > +	seq_puts(m, " : pax <sanitized> <not_sanitized>");
> > +#endif
> >  #endif
> >  	seq_putc(m, '\n');
> >  }
> > diff --git a/mm/slob.c b/mm/slob.c
> > index eac04d43..f455845 100644
> > --- a/mm/slob.c
> > +++ b/mm/slob.c
> > @@ -365,6 +365,11 @@ static void slob_free(void *block, int size)
> >  		return;
> >  	}
> >  
> > +#ifdef CONFIG_PAX_MEMORY_SANITIZE
> > +	if (pax_sanitize_slab && !(c && (c->flags &
> > SLAB_NO_SANITIZE)))
> > +		memset(block, PAX_MEMORY_SANITIZE_VALUE, size);
> > +#endif
> > +
> >  	if (!slob_page_free(sp)) {
> >  		/* This slob page is about to become partially
> > free. Easy! */ sp->units = units;
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 067598a..cead7ee 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -2911,6 +2911,24 @@ static __always_inline void
> > do_slab_free(struct kmem_cache *s, void *tail_obj = tail ? : head;
> >  	struct kmem_cache_cpu *c;
> >  	unsigned long tid;
> > +
> > +#ifdef CONFIG_PAX_MEMORY_SANITIZE
> > +	if (!(s->flags & SLAB_NO_SANITIZE)) {
> > +		int offset = s->offset ? 0 : sizeof(void *);
> > +		void *x = head;
> > +
> > +		while (1) {
> > +			memset(x + offset,
> > PAX_MEMORY_SANITIZE_VALUE,
> > +					s->object_size - offset);
> > +			if (s->ctor)
> > +				s->ctor(x);
> > +			if (x == tail_obj)
> > +				break;
> > +			x = get_freepointer(s, x);
> > +		}
> > +	}
> > +#endif
> > +
> >  redo:
> >  	/*
> >  	 * Determine the currently cpus per cpu slab.
> > @@ -5316,6 +5334,15 @@ static struct attribute *slab_attrs[] = {
> >  #ifdef CONFIG_ZONE_DMA
> >  	&cache_dma_attr.attr,
> >  #endif
> > +/**
> > + * #ifdef CONFIG_PAX_MEMORY_SANITIZE
> > + *	&sanitize_attr.attr,
> > + * ?? -above statement from the grsec-4.8.17 patch fails compile
> > with:
> > + * mm/slub.c:5337:3: error: ‘sanitize_attr’ undeclared here (not
> > in a function)
> > + *   &sanitize_attr.attr,
> > + *  ...
> > + * #endif
> > + */
> >  #ifdef CONFIG_NUMA
> >  	&remote_node_defrag_ratio_attr.attr,
> >  #endif
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 7ad67d7..2e60366 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -3453,12 +3453,14 @@ void __init skb_init(void)
> >  	skbuff_head_cache = kmem_cache_create("skbuff_head_cache",
> >  					      sizeof(struct
> > sk_buff), 0,
> > -
> > SLAB_HWCACHE_ALIGN|SLAB_PANIC,
> > +					      SLAB_HWCACHE_ALIGN |
> > SLAB_PANIC |
> > +					      SLAB_NO_SANITIZE,
> >  					      NULL);
> >  	skbuff_fclone_cache =
> > kmem_cache_create("skbuff_fclone_cache", sizeof(struct
> > sk_buff_fclones), 0,
> > -
> > SLAB_HWCACHE_ALIGN|SLAB_PANIC,
> > +						SLAB_HWCACHE_ALIGN
> > | SLAB_PANIC |
> > +						SLAB_NO_SANITIZE,
> >  						NULL);
> >  }
> >  
> > diff --git a/security/Kconfig b/security/Kconfig
> > index 118f454..3ad5110 100644
> > --- a/security/Kconfig
> > +++ b/security/Kconfig
> > @@ -4,6 +4,46 @@
> >  
> >  menu "Security options"
> >  
> > +menu "Miscellaneous hardening features"
> > +
> > +config PAX_MEMORY_SANITIZE
> > +	bool "Sanitize all freed memory"
> > +	default y if (GRKERNSEC_CONFIG_AUTO &&
> > GRKERNSEC_CONFIG_PRIORITY_SECURITY)
> > +	help
> > +	  By saying Y here the kernel will erase memory pages and
> > slab objects
> > +	  as soon as they are freed.  This in turn reduces the
> > lifetime of data
> > +	  stored in them, making it less likely that sensitive
> > information such
> > +	  as passwords, cryptographic secrets, etc stay in memory
> > for too long. +
> > +	  This is especially useful for programs whose runtime is
> > short, long
> > +	  lived processes and the kernel itself benefit from this
> > as long as
> > +	  they ensure timely freeing of memory that may hold
> > sensitive
> > +	  information.
> > +
> > +	  A nice side effect of the sanitization of slab objects
> > is the
> > +	  reduction of possible info leaks caused by padding bytes
> > within the
> > +	  leaky structures.  Use-after-free bugs for structures
> > containing
> > +	  pointers can also be detected as dereferencing the
> > sanitized pointer
> > +	  will generate an access violation.
> > +
> > +	  The tradeoff is performance impact, on a single CPU
> > system kernel
> > +	  compilation sees a 3% slowdown, other systems and
> > workloads may vary
> > +	  and you are advised to test this feature on your
> > expected workload
> > +	  before deploying it.
> > +
> > +	  The slab sanitization feature excludes a few slab caches
> > per default
> > +	  for performance reasons.  To extend the feature to cover
> > those as
> > +	  well, pass "pax_sanitize_slab=full" as kernel command
> > line parameter. +
> > +	  To reduce the performance penalty by sanitizing pages
> > only, albeit
> > +	  limiting the effectiveness of this feature at the same
> > time, slab
> > +	  sanitization can be disabled with the kernel command
> > line parameter
> > +	  "pax_sanitize_slab=off".
> > +
> > +	  Note that this feature does not protect data stored in
> > live pages,
> > +	  e.g., process memory swapped to disk may stay there for
> > a long time. +endmenu
> > +
> >  source security/keys/Kconfig
> >  
> >  config SECURITY_DMESG_RESTRICT
> >   
> 
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [kernel-hardening] Merge in PAX_MEMORY_SANITIZE work from grsec to linux-next
  2017-02-03  4:49   ` Kaiwan N Billimoria
@ 2017-02-04  0:23     ` Kees Cook
  2017-02-04  3:12       ` Kaiwan N Billimoria
  2017-02-09  3:37       ` Kaiwan N Billimoria
  0 siblings, 2 replies; 20+ messages in thread
From: Kees Cook @ 2017-02-04  0:23 UTC (permalink / raw)
  To: Kaiwan N Billimoria; +Cc: Laura Abbott, kernel-hardening

On Thu, Feb 2, 2017 at 8:49 PM, Kaiwan N Billimoria
<kaiwan@kaiwantech.com> wrote:
> Pl bear with me, am trying to understand this correctly.

No problem at all. Thanks for continuing to poke at this. :)

> From what I gather, we look to create a new config directive, say,
> CONFIG_MEMORY_SANITIZE .
>
> It will turn ON :
>  CONFIG_DEBUG_PAGEALLOC
>    (which internally turns ON CONFIG_PAGE_EXTENSION), and
>  CONFIG_PAGE_POISONING
>    (similar internal turn-ons :).

I think CONFIG_MEMORY_SANITIZE would enable:

CONFIG_SLUB_DEBUG=y
CONFIG_PAGE_POISONING=y
CONFIG_PAGE_POISONING_NO_SANITY=y

but it would _also_ need to set these kernel command-line variables as
if they had been set:

page_poison=1
slub_debug=P

> The new config now enables the CONFIG_PAX_MEMORY_SANITIZE code (my prev
> email patch), correct? (leaving out the stuff we cannot get without the
> full grsec implementation).

No, the first step would be for the config to only provide the above changes.

Then, we'd want to add the poison value defaults as you mention:

> -Would include the 'new' poison / sanitize values of 0xfe and 0xff for slab
> (64 and 32-bit resp), etc etc.

And then finally add the exceptions for the "frequently freed" slub
caches, as identified by PaX already. This would need the flag
defined, the poisoning logic adjusted to check the flag, and for the
new kernel command-line options for changing the whether or not the
flag is respected (like PaX's "pax_sanitize_slab").

> Even if this is (somewhat) correct, my thinking is - correct me I'm totally
> wrong here - that the whole purpose of the exercise is to improve _security_;
> hence, tying this code into the "debug features" of the kernel isn't really
> what we want, yes?

In the discussions Laura had with the mm folks, the only realistic
path to landing this in the upstream kernel is through these debug
features.

> Most folks would only use debug during development, if at all - given all the
> concerns regarding performance. Here, the objective is to enable a powerful
> security feature set. Hence, the config directives should come under the
> 'Security Options' menu.

We're not close to having a "Kernel Security" menu, so for now, I've
wanted to focus on getting the features, and then making the Kconfig
menus pretty later.

Hopefully that all makes sense! :)

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [kernel-hardening] Merge in PAX_MEMORY_SANITIZE work from grsec to linux-next
  2017-02-04  0:23     ` Kees Cook
@ 2017-02-04  3:12       ` Kaiwan N Billimoria
  2017-02-09  3:37       ` Kaiwan N Billimoria
  1 sibling, 0 replies; 20+ messages in thread
From: Kaiwan N Billimoria @ 2017-02-04  3:12 UTC (permalink / raw)
  To: Kees Cook; +Cc: Laura Abbott, kernel-hardening

[-- Attachment #1: Type: text/plain, Size: 1925 bytes --]

>
>
> I think CONFIG_MEMORY_SANITIZE would enable:
>
> CONFIG_SLUB_DEBUG=y
> CONFIG_PAGE_POISONING=y
> CONFIG_PAGE_POISONING_NO_SANITY=y
>
> but it would _also_ need to set these kernel command-line variables as
> if they had been set:
>
> page_poison=1
> slub_debug=P
>
> ​Okay got it.
> > The new config now enables the CONFIG_PAX_MEMORY_SANITIZE code (my prev
> > email patch), correct? (leaving out the stuff we cannot get without the
> > full grsec implementation).
>
> No, the first step would be for the config to only provide the above
> changes.
>
> Then, we'd want to add the poison value defaults as you mention:
>
> ​Right.

>
> And then finally add the exceptions for the "frequently freed" slub
> caches, as identified by PaX already. This would need the flag
> defined, the poisoning logic adjusted to check the flag, and for the
> new kernel command-line options for changing the whether or not the
> flag is respected (like PaX's "pax_sanitize_slab").
>
> Yup..​​

>
> In the discussions Laura had with the mm folks, the only realistic
> path to landing this in the upstream kernel is through these debug
> features.
>
> ​Hmm, ok.. naivete on my part :-)
> > Most folks would only use debug during development, if at all - given
> all the
> > concerns regarding performance. Here, the objective is to enable a
> powerful
> > security feature set. Hence, the config directives should come under the
> > 'Security Options' menu.
>
> We're not close to having a "Kernel Security" menu, so for now, I've
> wanted to focus on getting the features, and then making the Kconfig
> menus pretty later.
>
​Yeah; I meant under the existing 'Security options' menu actually. But
still..​

>
> Hopefully that all makes sense! :)
>
​Indeed! Thanks very much..
-Kaiwan.​

>
> -Kees
>
> --
> Kees Cook
> Pixel Security
>
>

[-- Attachment #2: Type: text/html, Size: 3706 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [kernel-hardening] Merge in PAX_MEMORY_SANITIZE work from grsec to linux-next
  2017-02-04  0:23     ` Kees Cook
  2017-02-04  3:12       ` Kaiwan N Billimoria
@ 2017-02-09  3:37       ` Kaiwan N Billimoria
  2017-02-13 11:42         ` Kaiwan N Billimoria
  2017-02-13 17:28         ` Kees Cook
  1 sibling, 2 replies; 20+ messages in thread
From: Kaiwan N Billimoria @ 2017-02-09  3:37 UTC (permalink / raw)
  To: Kees Cook; +Cc: Laura Abbott, kernel-hardening

Hi Kees,

> I think CONFIG_MEMORY_SANITIZE would enable:
> 
> CONFIG_SLUB_DEBUG=y
> CONFIG_PAGE_POISONING=y
> CONFIG_PAGE_POISONING_NO_SANITY=y
> 
> but it would _also_ need to set these kernel command-line variables as
> if they had been set:
> 
> page_poison=1
> slub_debug=P
> 
> No, the first step would be for the config to only provide the above
> changes.

Have made a first cut (below). Pl take a look and let me know if okay 
and how I should proceed.

Note-
 - the printks' are just for checking that options are enabled as required 
(will get rid of them later)
 - this is on linux-next (x86_64)
 - dmesg filtered output when booted with this kernel (no page_posion or slub_debug 
cmdline options passed):

$ dmesg |grep "MEMORY_SANITIZE"
kern  :info  : [  +0.000387] [CONFIG_MEMORY_SANITIZE]: slub_debug = P? yes [0x800]
kern  :debug : [  +0.000000] [CONFIG_MEMORY_SANITIZE]: page_poisoning_enabled? yes
$

===
diff --git a/init/main.c b/init/main.c
index ef47035..ba44574 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1028,6 +1028,11 @@ static noinline void __init kernel_init_freeable(void)
 
	 do_basic_setup();
 
+#ifdef CONFIG_MEMORY_SANITIZE
+    pr_debug("[CONFIG_MEMORY_SANITIZE]: page_poisoning_enabled? %s\n",
+   	 page_poisoning_enabled() ? "yes" : "no");
+#endif
+
	 /* Open the /dev/console on the rootfs, this should never fail */
	 if (sys_open((const char __user *) "/dev/console", O_RDWR, 0) < 0)
		 pr_err("Warning: unable to open an initial console.\n");
diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug
index 3e5eada..8eed6ca 100644
--- a/mm/Kconfig.debug
+++ b/mm/Kconfig.debug
@@ -97,3 +97,11 @@ config DEBUG_RODATA_TEST
 	---help---
   	This option enables a testcase for the setting rodata read-only.
 
+config MEMORY_SANITIZE
+	bool "Enable memory sanitization features"
+	select SLUB_DEBUG
+	select PAGE_POISONING
+	select PAGE_POISONING_NO_SANITY if HIBERNATION
+	---help---
+   	This option enables ...
+
diff --git a/mm/page_poison.c b/mm/page_poison.c
index 2e647c6..b45bc0a 100644
--- a/mm/page_poison.c
+++ b/mm/page_poison.c
@@ -49,6 +49,19 @@ struct page_ext_operations page_poisoning_ops = {
	 .init = init_page_poisoning,
 };
 
+#ifdef CONFIG_MEMORY_SANITIZE
+static int __init memory_sanitize_init(void)
+{
+    /* With 'memory sanitize' On, page poisoning Must be turned on */
+    if (IS_ENABLED(CONFIG_MEMORY_SANITIZE)) {
+   	 want_page_poisoning = true;
+   	 __page_poisoning_enabled = true;
+    }
+    return 0;
+}
+early_initcall(memory_sanitize_init);
+#endif
+
 static inline void set_page_poison(struct page *page)
 {
	 struct page_ext *page_ext;
diff --git a/mm/slub.c b/mm/slub.c
index d24e1ce..ed26b10 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -449,10 +449,15 @@ static inline void *restore_red_left(struct kmem_cache *s, void *p)
  * Debug settings:
  */
 #if defined(CONFIG_SLUB_DEBUG_ON)
+#if defined(CONFIG_MEMORY_SANITIZE)
+/* With 'memory sanitize' On, slub_debug should be 'P' */
+static int slub_debug = SLAB_POISON;
+#else
 static int slub_debug = DEBUG_DEFAULT_FLAGS;
+#endif /* CONFIG_MEMORY_SANITIZE */
 #else
 static int slub_debug;
-#endif
+#endif /* CONFIG_SLUB_DEBUG_ON */
 
 static char *slub_debug_slabs;
 static int disable_higher_order_debug;
@@ -5675,6 +5680,11 @@ static int __init slab_sysfs_init(void)
	 struct kmem_cache *s;
	 int err;
 
+#ifdef CONFIG_MEMORY_SANITIZE
+    pr_info("[CONFIG_MEMORY_SANITIZE]: slub_debug = P? %s [0x%x]\n",
+   	 slub_debug & SLAB_POISON ? "yes" : "no", slub_debug);
+#endif
+
	 mutex_lock(&slab_mutex);
 
	 slab_kset = kset_create_and_add("slab", &slab_uevent_ops, kernel_kobj);
===

Regards,
Kaiwan.

> Then, we'd want to add the poison value defaults as you mention:
> 
> > -Would include the 'new' poison / sanitize values of 0xfe and 0xff
> > for slab (64 and 32-bit resp), etc etc.  
> 
> And then finally add the exceptions for the "frequently freed" slub
> caches, as identified by PaX already. This would need the flag
> defined, the poisoning logic adjusted to check the flag, and for the
> new kernel command-line options for changing the whether or not the
> flag is respected (like PaX's "pax_sanitize_slab").
> 
> > Even if this is (somewhat) correct, my thinking is - correct me I'm
> > totally wrong here - that the whole purpose of the exercise is to
> > improve _security_; hence, tying this code into the "debug
> > features" of the kernel isn't really what we want, yes?  
> 
> In the discussions Laura had with the mm folks, the only realistic
> path to landing this in the upstream kernel is through these debug
> features.
> 
> > Most folks would only use debug during development, if at all -
> > given all the concerns regarding performance. Here, the objective
> > is to enable a powerful security feature set. Hence, the config
> > directives should come under the 'Security Options' menu.  
> 
> We're not close to having a "Kernel Security" menu, so for now, I've
> wanted to focus on getting the features, and then making the Kconfig
> menus pretty later.
> 
> Hopefully that all makes sense! :)
> 
> -Kees
> 

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [kernel-hardening] Merge in PAX_MEMORY_SANITIZE work from grsec to linux-next
  2017-02-09  3:37       ` Kaiwan N Billimoria
@ 2017-02-13 11:42         ` Kaiwan N Billimoria
  2017-02-13 17:28         ` Kees Cook
  1 sibling, 0 replies; 20+ messages in thread
From: Kaiwan N Billimoria @ 2017-02-13 11:42 UTC (permalink / raw)
  To: Kees Cook; +Cc: Laura Abbott, kernel-hardening

[-- Attachment #1: Type: text/plain, Size: 6361 bytes --]

<Nudge>, kindly check out the patch, thanks.


Regards,
Kaiwan.

Kaiwan N Billimoria
✉ kaiwan@kaiwantech.com
✉ kaiwan.billimoria@gmail.com

kaiwanTECH
http://kaiwantech.in
Do visit our enhanced website.

4931, 11th Floor, Highpoint IV, 45 Palace Road, Bangalore 560 001.
☎ +91.9845016788 (M)
☎ TeleFax: +91.80.22389396 | Alt. Tel: +91.80.64500257

LinkedIn: https://in.linkedin.com/in/kaiwanbillimoria

I blog here:
Tech : http://kaiwantech.wordpress.com  |
Running : http://kaiwanbill.blogspot.in/

"Don't be afraid that your life will end,
be afraid that it will never begin."

On Thu, Feb 9, 2017 at 9:07 AM, Kaiwan N Billimoria <kaiwan@kaiwantech.com>
wrote:

> Hi Kees,
>
> > I think CONFIG_MEMORY_SANITIZE would enable:
> >
> > CONFIG_SLUB_DEBUG=y
> > CONFIG_PAGE_POISONING=y
> > CONFIG_PAGE_POISONING_NO_SANITY=y
> >
> > but it would _also_ need to set these kernel command-line variables as
> > if they had been set:
> >
> > page_poison=1
> > slub_debug=P
> >
> > No, the first step would be for the config to only provide the above
> > changes.
>
> Have made a first cut (below). Pl take a look and let me know if okay
> and how I should proceed.
>
> Note-
>  - the printks' are just for checking that options are enabled as required
> (will get rid of them later)
>  - this is on linux-next (x86_64)
>  - dmesg filtered output when booted with this kernel (no page_posion or
> slub_debug
> cmdline options passed):
>
> $ dmesg |grep "MEMORY_SANITIZE"
> kern  :info  : [  +0.000387] [CONFIG_MEMORY_SANITIZE]: slub_debug = P? yes
> [0x800]
> kern  :debug : [  +0.000000] [CONFIG_MEMORY_SANITIZE]:
> page_poisoning_enabled? yes
> $
>
> ===
> diff --git a/init/main.c b/init/main.c
> index ef47035..ba44574 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1028,6 +1028,11 @@ static noinline void __init
> kernel_init_freeable(void)
>
>          do_basic_setup();
>
> +#ifdef CONFIG_MEMORY_SANITIZE
> +    pr_debug("[CONFIG_MEMORY_SANITIZE]: page_poisoning_enabled? %s\n",
> +        page_poisoning_enabled() ? "yes" : "no");
> +#endif
> +
>          /* Open the /dev/console on the rootfs, this should never fail */
>          if (sys_open((const char __user *) "/dev/console", O_RDWR, 0) < 0)
>                  pr_err("Warning: unable to open an initial console.\n");
> diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug
> index 3e5eada..8eed6ca 100644
> --- a/mm/Kconfig.debug
> +++ b/mm/Kconfig.debug
> @@ -97,3 +97,11 @@ config DEBUG_RODATA_TEST
>         ---help---
>         This option enables a testcase for the setting rodata read-only.
>
> +config MEMORY_SANITIZE
> +       bool "Enable memory sanitization features"
> +       select SLUB_DEBUG
> +       select PAGE_POISONING
> +       select PAGE_POISONING_NO_SANITY if HIBERNATION
> +       ---help---
> +       This option enables ...
> +
> diff --git a/mm/page_poison.c b/mm/page_poison.c
> index 2e647c6..b45bc0a 100644
> --- a/mm/page_poison.c
> +++ b/mm/page_poison.c
> @@ -49,6 +49,19 @@ struct page_ext_operations page_poisoning_ops = {
>          .init = init_page_poisoning,
>  };
>
> +#ifdef CONFIG_MEMORY_SANITIZE
> +static int __init memory_sanitize_init(void)
> +{
> +    /* With 'memory sanitize' On, page poisoning Must be turned on */
> +    if (IS_ENABLED(CONFIG_MEMORY_SANITIZE)) {
> +        want_page_poisoning = true;
> +        __page_poisoning_enabled = true;
> +    }
> +    return 0;
> +}
> +early_initcall(memory_sanitize_init);
> +#endif
> +
>  static inline void set_page_poison(struct page *page)
>  {
>          struct page_ext *page_ext;
> diff --git a/mm/slub.c b/mm/slub.c
> index d24e1ce..ed26b10 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -449,10 +449,15 @@ static inline void *restore_red_left(struct
> kmem_cache *s, void *p)
>   * Debug settings:
>   */
>  #if defined(CONFIG_SLUB_DEBUG_ON)
> +#if defined(CONFIG_MEMORY_SANITIZE)
> +/* With 'memory sanitize' On, slub_debug should be 'P' */
> +static int slub_debug = SLAB_POISON;
> +#else
>  static int slub_debug = DEBUG_DEFAULT_FLAGS;
> +#endif /* CONFIG_MEMORY_SANITIZE */
>  #else
>  static int slub_debug;
> -#endif
> +#endif /* CONFIG_SLUB_DEBUG_ON */
>
>  static char *slub_debug_slabs;
>  static int disable_higher_order_debug;
> @@ -5675,6 +5680,11 @@ static int __init slab_sysfs_init(void)
>          struct kmem_cache *s;
>          int err;
>
> +#ifdef CONFIG_MEMORY_SANITIZE
> +    pr_info("[CONFIG_MEMORY_SANITIZE]: slub_debug = P? %s [0x%x]\n",
> +        slub_debug & SLAB_POISON ? "yes" : "no", slub_debug);
> +#endif
> +
>          mutex_lock(&slab_mutex);
>
>          slab_kset = kset_create_and_add("slab", &slab_uevent_ops,
> kernel_kobj);
> ===
>
> Regards,
> Kaiwan.
>
> > Then, we'd want to add the poison value defaults as you mention:
> >
> > > -Would include the 'new' poison / sanitize values of 0xfe and 0xff
> > > for slab (64 and 32-bit resp), etc etc.
> >
> > And then finally add the exceptions for the "frequently freed" slub
> > caches, as identified by PaX already. This would need the flag
> > defined, the poisoning logic adjusted to check the flag, and for the
> > new kernel command-line options for changing the whether or not the
> > flag is respected (like PaX's "pax_sanitize_slab").
> >
> > > Even if this is (somewhat) correct, my thinking is - correct me I'm
> > > totally wrong here - that the whole purpose of the exercise is to
> > > improve _security_; hence, tying this code into the "debug
> > > features" of the kernel isn't really what we want, yes?
> >
> > In the discussions Laura had with the mm folks, the only realistic
> > path to landing this in the upstream kernel is through these debug
> > features.
> >
> > > Most folks would only use debug during development, if at all -
> > > given all the concerns regarding performance. Here, the objective
> > > is to enable a powerful security feature set. Hence, the config
> > > directives should come under the 'Security Options' menu.
> >
> > We're not close to having a "Kernel Security" menu, so for now, I've
> > wanted to focus on getting the features, and then making the Kconfig
> > menus pretty later.
> >
> > Hopefully that all makes sense! :)
> >
> > -Kees
> >
>
>
>

[-- Attachment #2: Type: text/html, Size: 8303 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [kernel-hardening] Merge in PAX_MEMORY_SANITIZE work from grsec to linux-next
  2017-02-09  3:37       ` Kaiwan N Billimoria
  2017-02-13 11:42         ` Kaiwan N Billimoria
@ 2017-02-13 17:28         ` Kees Cook
  2017-02-14  3:01           ` Kaiwan N Billimoria
  2017-02-14  8:04           ` Kaiwan N Billimoria
  1 sibling, 2 replies; 20+ messages in thread
From: Kees Cook @ 2017-02-13 17:28 UTC (permalink / raw)
  To: Kaiwan N Billimoria; +Cc: Laura Abbott, kernel-hardening

On Wed, Feb 8, 2017 at 7:37 PM, Kaiwan N Billimoria
<kaiwan@kaiwantech.com> wrote:
> Hi Kees,
>
>> I think CONFIG_MEMORY_SANITIZE would enable:
>>
>> CONFIG_SLUB_DEBUG=y
>> CONFIG_PAGE_POISONING=y
>> CONFIG_PAGE_POISONING_NO_SANITY=y
>>
>> but it would _also_ need to set these kernel command-line variables as
>> if they had been set:
>>
>> page_poison=1
>> slub_debug=P
>>
>> No, the first step would be for the config to only provide the above
>> changes.
>
> Have made a first cut (below). Pl take a look and let me know if okay
> and how I should proceed.
>
> Note-
>  - the printks' are just for checking that options are enabled as required
> (will get rid of them later)
>  - this is on linux-next (x86_64)
>  - dmesg filtered output when booted with this kernel (no page_posion or slub_debug
> cmdline options passed):
>
> $ dmesg |grep "MEMORY_SANITIZE"
> kern  :info  : [  +0.000387] [CONFIG_MEMORY_SANITIZE]: slub_debug = P? yes [0x800]
> kern  :debug : [  +0.000000] [CONFIG_MEMORY_SANITIZE]: page_poisoning_enabled? yes
> $
>
> ===
> diff --git a/init/main.c b/init/main.c
> index ef47035..ba44574 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1028,6 +1028,11 @@ static noinline void __init kernel_init_freeable(void)
>
>          do_basic_setup();
>
> +#ifdef CONFIG_MEMORY_SANITIZE
> +    pr_debug("[CONFIG_MEMORY_SANITIZE]: page_poisoning_enabled? %s\n",
> +        page_poisoning_enabled() ? "yes" : "no");
> +#endif
> +
>          /* Open the /dev/console on the rootfs, this should never fail */
>          if (sys_open((const char __user *) "/dev/console", O_RDWR, 0) < 0)
>                  pr_err("Warning: unable to open an initial console.\n");
> diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug
> index 3e5eada..8eed6ca 100644
> --- a/mm/Kconfig.debug
> +++ b/mm/Kconfig.debug
> @@ -97,3 +97,11 @@ config DEBUG_RODATA_TEST
>         ---help---
>         This option enables a testcase for the setting rodata read-only.
>
> +config MEMORY_SANITIZE
> +       bool "Enable memory sanitization features"
> +       select SLUB_DEBUG
> +       select PAGE_POISONING
> +       select PAGE_POISONING_NO_SANITY if HIBERNATION
> +       ---help---
> +       This option enables ...

Good start! Why the "if HIBERNATION" bit? It seems like sanity checks
are very expensive, so we'd not want it as part of this config?

> +
> diff --git a/mm/page_poison.c b/mm/page_poison.c
> index 2e647c6..b45bc0a 100644
> --- a/mm/page_poison.c
> +++ b/mm/page_poison.c
> @@ -49,6 +49,19 @@ struct page_ext_operations page_poisoning_ops = {
>          .init = init_page_poisoning,
>  };
>
> +#ifdef CONFIG_MEMORY_SANITIZE
> +static int __init memory_sanitize_init(void)
> +{
> +    /* With 'memory sanitize' On, page poisoning Must be turned on */
> +    if (IS_ENABLED(CONFIG_MEMORY_SANITIZE)) {
> +        want_page_poisoning = true;
> +        __page_poisoning_enabled = true;
> +    }
> +    return 0;
> +}
> +early_initcall(memory_sanitize_init);
> +#endif

The ifdef and the IS_ENABLED seem redundant to me. I'd drop the ifdef.

> +
>  static inline void set_page_poison(struct page *page)
>  {
>          struct page_ext *page_ext;
> diff --git a/mm/slub.c b/mm/slub.c
> index d24e1ce..ed26b10 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -449,10 +449,15 @@ static inline void *restore_red_left(struct kmem_cache *s, void *p)
>   * Debug settings:
>   */
>  #if defined(CONFIG_SLUB_DEBUG_ON)
> +#if defined(CONFIG_MEMORY_SANITIZE)
> +/* With 'memory sanitize' On, slub_debug should be 'P' */
> +static int slub_debug = SLAB_POISON;
> +#else
>  static int slub_debug = DEBUG_DEFAULT_FLAGS;
> +#endif /* CONFIG_MEMORY_SANITIZE */
>  #else
>  static int slub_debug;
> -#endif
> +#endif /* CONFIG_SLUB_DEBUG_ON */

Could the definition of DEBUG_DEFAULT_FLAGS be adjusted instead of
doing the ifdefs here in the .c file? Or, perhaps do a slub_debug |=
SLAB_POISON in memory_sanitize_init()?

>
>  static char *slub_debug_slabs;
>  static int disable_higher_order_debug;
> @@ -5675,6 +5680,11 @@ static int __init slab_sysfs_init(void)
>          struct kmem_cache *s;
>          int err;
>
> +#ifdef CONFIG_MEMORY_SANITIZE
> +    pr_info("[CONFIG_MEMORY_SANITIZE]: slub_debug = P? %s [0x%x]\n",
> +        slub_debug & SLAB_POISON ? "yes" : "no", slub_debug);
> +#endif
> +
>          mutex_lock(&slab_mutex);
>
>          slab_kset = kset_create_and_add("slab", &slab_uevent_ops, kernel_kobj);

Good first step to getting the CONFIG to do something meaningful, thanks!

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [kernel-hardening] Merge in PAX_MEMORY_SANITIZE work from grsec to linux-next
  2017-02-13 17:28         ` Kees Cook
@ 2017-02-14  3:01           ` Kaiwan N Billimoria
  2017-02-14 17:19             ` Kees Cook
  2017-02-14  8:04           ` Kaiwan N Billimoria
  1 sibling, 1 reply; 20+ messages in thread
From: Kaiwan N Billimoria @ 2017-02-14  3:01 UTC (permalink / raw)
  To: Kees Cook; +Cc: Laura Abbott, kernel-hardening

[-- Attachment #1: Type: text/plain, Size: 2386 bytes --]

​Thanks for your response...​

>
>
> +config MEMORY_SANITIZE
> > +       bool "Enable memory sanitization features"
> > +       select SLUB_DEBUG
> > +       select PAGE_POISONING
> > +       select PAGE_POISONING_NO_SANITY if HIBERNATION
> > +       ---help---
> > +       This option enables ...
>
> Good start! Why the "if HIBERNATION" bit? It seems like sanity checks
> are very expensive, so we'd not want it as part of this config?
>
> ​Okay, I wasn't sure. So would it be (more) correct to retain the first
two configs plus
​PAGE_POISONING_NO_SANITY (without the if)?

>
> > +#ifdef CONFIG_MEMORY_SANITIZE
> > +static int __init memory_sanitize_init(void)
> > +{
> > +    /* With 'memory sanitize' On, page poisoning Must be turned on */
> > +    if (IS_ENABLED(CONFIG_MEMORY_SANITIZE)) {
> > +        want_page_poisoning = true;
> > +        __page_poisoning_enabled = true;
> > +    }
> > +    return 0;
> > +}
> > +early_initcall(memory_sanitize_init);
> > +#endif
>
> The ifdef and the IS_ENABLED seem redundant to me. I'd drop the ifdef.
>
> ​Agree on the redundancy, will do.


> >  #if defined(CONFIG_SLUB_DEBUG_ON)
> > +#if defined(CONFIG_MEMORY_SANITIZE)
> > +/* With 'memory sanitize' On, slub_debug should be 'P' */
> > +static int slub_debug = SLAB_POISON;
> > +#else
> >  static int slub_debug = DEBUG_DEFAULT_FLAGS;
> > +#endif /* CONFIG_MEMORY_SANITIZE */
> >  #else
> >  static int slub_debug;
> > -#endif
> > +#endif /* CONFIG_SLUB_DEBUG_ON */
>
> Could the definition of DEBUG_DEFAULT_FLAGS be adjusted instead of
> doing the ifdefs here in the .c file? Or, perhaps do a slub_debug |=
> SLAB_POISON in memory_sanitize_init()?
>
> Yes, the latter sounds good but the init function is in mm/page_poison.c
and the slub_debug var is a static in mm/slub.c . Any suggestions?
​

>
> > +#ifdef CONFIG_MEMORY_SANITIZE
> > +    pr_info("[CONFIG_MEMORY_SANITIZE]: slub_debug = P? %s [0x%x]\n",
> > +        slub_debug & SLAB_POISON ? "yes" : "no", slub_debug);
> > +#endif
> > +
> >          mutex_lock(&slab_mutex);
> >
> >          slab_kset = kset_create_and_add("slab", &slab_uevent_ops,
> kernel_kobj);
>
> Good first step to getting the CONFIG to do something meaningful, thanks!
>
> -Kees
>
> ​Thanks, happy to help!
- Kaiwan.​

> --
> Kees Cook
> Pixel Security
>
>

[-- Attachment #2: Type: text/html, Size: 4655 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [kernel-hardening] Merge in PAX_MEMORY_SANITIZE work from grsec to linux-next
  2017-02-13 17:28         ` Kees Cook
  2017-02-14  3:01           ` Kaiwan N Billimoria
@ 2017-02-14  8:04           ` Kaiwan N Billimoria
  2017-02-14 16:19             ` Mark Rutland
  1 sibling, 1 reply; 20+ messages in thread
From: Kaiwan N Billimoria @ 2017-02-14  8:04 UTC (permalink / raw)
  To: Kees Cook; +Cc: Laura Abbott, kernel-hardening

>> diff --git a/mm/page_poison.c b/mm/page_poison.c
>> index 2e647c6..b45bc0a 100644
>> --- a/mm/page_poison.c
>> +++ b/mm/page_poison.c
>> @@ -49,6 +49,19 @@ struct page_ext_operations page_poisoning_ops = {
>>          .init = init_page_poisoning,
>>  };
>>
>> +#ifdef CONFIG_MEMORY_SANITIZE
>> +static int __init memory_sanitize_init(void)
>> +{
>> +    /* With 'memory sanitize' On, page poisoning Must be turned on */
>> +    if (IS_ENABLED(CONFIG_MEMORY_SANITIZE)) {
>> +        want_page_poisoning = true;
>> +        __page_poisoning_enabled = true;
>> +    }
>> +    return 0;
>> +}
>> +early_initcall(memory_sanitize_init);
>> +#endif
>
> The ifdef and the IS_ENABLED seem redundant to me. I'd drop the ifdef.

An academic question perhaps- am not clear as to why the IS_ENABLED()
is preferable to an #ifdef. I thought eliminating a runtime 'if'
condition (by using an ifdef) is more optimal than the "if
IS_ENABLED(foo)"? Or (probably) does the compiler optimize it out
regardless (assuming that CONFIG_MEMORY_SANITIZE is not selected of
course)?

-Thanks, kaiwan.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [kernel-hardening] Merge in PAX_MEMORY_SANITIZE work from grsec to linux-next
  2017-02-14  8:04           ` Kaiwan N Billimoria
@ 2017-02-14 16:19             ` Mark Rutland
  2017-02-14 16:33               ` Kaiwan N Billimoria
  0 siblings, 1 reply; 20+ messages in thread
From: Mark Rutland @ 2017-02-14 16:19 UTC (permalink / raw)
  To: Kaiwan N Billimoria; +Cc: Kees Cook, Laura Abbott, kernel-hardening, nd

On Tue, Feb 14, 2017 at 01:34:38PM +0530, Kaiwan N Billimoria wrote:
> >> diff --git a/mm/page_poison.c b/mm/page_poison.c
> >> index 2e647c6..b45bc0a 100644
> >> --- a/mm/page_poison.c
> >> +++ b/mm/page_poison.c
> >> @@ -49,6 +49,19 @@ struct page_ext_operations page_poisoning_ops = {
> >>          .init = init_page_poisoning,
> >>  };
> >>
> >> +#ifdef CONFIG_MEMORY_SANITIZE
> >> +static int __init memory_sanitize_init(void)
> >> +{
> >> +    /* With 'memory sanitize' On, page poisoning Must be turned on */
> >> +    if (IS_ENABLED(CONFIG_MEMORY_SANITIZE)) {
> >> +        want_page_poisoning = true;
> >> +        __page_poisoning_enabled = true;
> >> +    }
> >> +    return 0;
> >> +}
> >> +early_initcall(memory_sanitize_init);
> >> +#endif
> >
> > The ifdef and the IS_ENABLED seem redundant to me. I'd drop the ifdef.
> 
> An academic question perhaps- am not clear as to why the IS_ENABLED()
> is preferable to an #ifdef. I thought eliminating a runtime 'if'
> condition (by using an ifdef) is more optimal than the "if
> IS_ENABLED(foo)"? Or (probably) does the compiler optimize it out
> regardless (assuming that CONFIG_MEMORY_SANITIZE is not selected of
> course)?

We expect the compiler to optimize the code out, since IS_ENABLED(x) is
a compile-time constant.

As for why it's preferable, it has several benefits.

For one thing, using if (IS_ENABLED(x)) means that we always get build
coverage of the code predicated on the config option, so it's harder to
accidentally break some build configurations.

For another, it composes more nicely with other conditionals, which is
useful when you have a condition that depends on several config options,
or config options and runtime expressions.

e.g. something like:

	if (IS_ENABLED(FOO) && IS_ENABLED(BAR) && runtime_option)
		do_combination_thing();
	else if (IS_ENABLED(BAR))
		do_bar_only_thing();
	else
		do_other_thing();

... is much more painful to write using ifdefs.

Generally, it's nicer for people to deal with than ifdeffery. It's
easier for people to match braces in well-formatted code than it is to
match #ifdef #endif pairs.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [kernel-hardening] Merge in PAX_MEMORY_SANITIZE work from grsec to linux-next
  2017-02-14 16:19             ` Mark Rutland
@ 2017-02-14 16:33               ` Kaiwan N Billimoria
  0 siblings, 0 replies; 20+ messages in thread
From: Kaiwan N Billimoria @ 2017-02-14 16:33 UTC (permalink / raw)
  To: Mark Rutland; +Cc: Kees Cook, Laura Abbott, kernel-hardening

[-- Attachment #1: Type: text/plain, Size: 2479 bytes --]

Thanks for your succinct explanation Mark! Makes sense.
Will incorporate it, as Kees suggests..

On Tue, 14 Feb 2017, 9:49 pm Mark Rutland, <mark.rutland@arm.com> wrote:

> On Tue, Feb 14, 2017 at 01:34:38PM +0530, Kaiwan N Billimoria wrote:
> > >> diff --git a/mm/page_poison.c b/mm/page_poison.c
> > >> index 2e647c6..b45bc0a 100644
> > >> --- a/mm/page_poison.c
> > >> +++ b/mm/page_poison.c
> > >> @@ -49,6 +49,19 @@ struct page_ext_operations page_poisoning_ops = {
> > >>          .init = init_page_poisoning,
> > >>  };
> > >>
> > >> +#ifdef CONFIG_MEMORY_SANITIZE
> > >> +static int __init memory_sanitize_init(void)
> > >> +{
> > >> +    /* With 'memory sanitize' On, page poisoning Must be turned on */
> > >> +    if (IS_ENABLED(CONFIG_MEMORY_SANITIZE)) {
> > >> +        want_page_poisoning = true;
> > >> +        __page_poisoning_enabled = true;
> > >> +    }
> > >> +    return 0;
> > >> +}
> > >> +early_initcall(memory_sanitize_init);
> > >> +#endif
> > >
> > > The ifdef and the IS_ENABLED seem redundant to me. I'd drop the ifdef.
> >
> > An academic question perhaps- am not clear as to why the IS_ENABLED()
> > is preferable to an #ifdef. I thought eliminating a runtime 'if'
> > condition (by using an ifdef) is more optimal than the "if
> > IS_ENABLED(foo)"? Or (probably) does the compiler optimize it out
> > regardless (assuming that CONFIG_MEMORY_SANITIZE is not selected of
> > course)?
>
> We expect the compiler to optimize the code out, since IS_ENABLED(x) is
> a compile-time constant.
>
> As for why it's preferable, it has several benefits.
>
> For one thing, using if (IS_ENABLED(x)) means that we always get build
> coverage of the code predicated on the config option, so it's harder to
> accidentally break some build configurations.
>
> For another, it composes more nicely with other conditionals, which is
> useful when you have a condition that depends on several config options,
> or config options and runtime expressions.
>
> e.g. something like:
>
>         if (IS_ENABLED(FOO) && IS_ENABLED(BAR) && runtime_option)
>                 do_combination_thing();
>         else if (IS_ENABLED(BAR))
>                 do_bar_only_thing();
>         else
>                 do_other_thing();
>
> ... is much more painful to write using ifdefs.
>
> Generally, it's nicer for people to deal with than ifdeffery. It's
> easier for people to match braces in well-formatted code than it is to
> match #ifdef #endif pairs.
>
> Thanks,
> Mark.
>
>

[-- Attachment #2: Type: text/html, Size: 4369 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [kernel-hardening] Merge in PAX_MEMORY_SANITIZE work from grsec to linux-next
  2017-02-14  3:01           ` Kaiwan N Billimoria
@ 2017-02-14 17:19             ` Kees Cook
  2017-02-15  7:27               ` Kaiwan N Billimoria
  0 siblings, 1 reply; 20+ messages in thread
From: Kees Cook @ 2017-02-14 17:19 UTC (permalink / raw)
  To: Kaiwan N Billimoria; +Cc: Laura Abbott, kernel-hardening

On Mon, Feb 13, 2017 at 7:01 PM, Kaiwan N Billimoria
<kaiwan@kaiwantech.com> wrote:
> Thanks for your response...
>>
>>
>>
>> > +config MEMORY_SANITIZE
>> > +       bool "Enable memory sanitization features"
>> > +       select SLUB_DEBUG
>> > +       select PAGE_POISONING
>> > +       select PAGE_POISONING_NO_SANITY if HIBERNATION
>> > +       ---help---
>> > +       This option enables ...
>>
>> Good start! Why the "if HIBERNATION" bit? It seems like sanity checks
>> are very expensive, so we'd not want it as part of this config?
>>
> Okay, I wasn't sure. So would it be (more) correct to retain the first two
> configs plus
> PAGE_POISONING_NO_SANITY (without the if)?

I think so, yes. We may need to tweak it in the future, but I think
that's the correct config for now.

>> >  #if defined(CONFIG_SLUB_DEBUG_ON)
>> > +#if defined(CONFIG_MEMORY_SANITIZE)
>> > +/* With 'memory sanitize' On, slub_debug should be 'P' */
>> > +static int slub_debug = SLAB_POISON;
>> > +#else
>> >  static int slub_debug = DEBUG_DEFAULT_FLAGS;
>> > +#endif /* CONFIG_MEMORY_SANITIZE */
>> >  #else
>> >  static int slub_debug;
>> > -#endif
>> > +#endif /* CONFIG_SLUB_DEBUG_ON */
>>
>> Could the definition of DEBUG_DEFAULT_FLAGS be adjusted instead of
>> doing the ifdefs here in the .c file? Or, perhaps do a slub_debug |=
>> SLAB_POISON in memory_sanitize_init()?
>>
> Yes, the latter sounds good but the init function is in mm/page_poison.c and
> the slub_debug var is a static in mm/slub.c . Any suggestions?

Perhaps add another early_init like you did the page_poison.c?

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [kernel-hardening] Merge in PAX_MEMORY_SANITIZE work from grsec to linux-next
  2017-02-14 17:19             ` Kees Cook
@ 2017-02-15  7:27               ` Kaiwan N Billimoria
  2017-02-15 20:30                 ` Kees Cook
  0 siblings, 1 reply; 20+ messages in thread
From: Kaiwan N Billimoria @ 2017-02-15  7:27 UTC (permalink / raw)
  To: Kees Cook; +Cc: Laura Abbott, kernel-hardening

Okay, I've incorporated your suggestions. 
Of course, the printk's are temporary. 

Pl see:
- the updated patch, and
- a 'truth table' enumerating some basic testing with these configs,
below:

---
diff --git a/init/main.c b/init/main.c
index ef47035..ba44574 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1028,6 +1028,11 @@ static noinline void __init kernel_init_freeable(void)
 
 	do_basic_setup();
 
+#ifdef CONFIG_MEMORY_SANITIZE
+	pr_debug("[CONFIG_MEMORY_SANITIZE]: page_poisoning_enabled? %s\n",
+		page_poisoning_enabled() ? "yes" : "no");
+#endif
+
 	/* Open the /dev/console on the rootfs, this should never fail */
 	if (sys_open((const char __user *) "/dev/console", O_RDWR, 0) < 0)
 		pr_err("Warning: unable to open an initial console.\n");
diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug
index 3e5eada..fbb4290 100644
--- a/mm/Kconfig.debug
+++ b/mm/Kconfig.debug
@@ -97,3 +97,24 @@ config DEBUG_RODATA_TEST
     ---help---
       This option enables a testcase for the setting rodata read-only.
 
+config MEMORY_SANITIZE
+    bool "Enable memory sanitization features"
+    select SLUB_DEBUG
+    select PAGE_POISONING
+    select PAGE_POISONING_NO_SANITY
+    ---help---
+       This option enables memory sanitization features. Particularly,
+       when you turn on this option, it auto-enables:
+       - SLUB debug
+       - page poisoning
+       - page poisoning no sanity.
+
+       Implication: turning this option on _will_ implicitly enable:
+       - the SLUB_DEBUG switch to the equivalent of the kernel command-line
+         'slub_debug=p' ; (where p=PAGE_POISON),
+       - page poisoning, equivalent to passing the kernel command-line option
+         'page_poison=on',
+       irrespective of whether the options are explicitly passed or not.
+
+       If unsure, say N.
+
diff --git a/mm/page_poison.c b/mm/page_poison.c
index 2e647c6..8d1e883 100644
--- a/mm/page_poison.c
+++ b/mm/page_poison.c
@@ -49,6 +49,17 @@ struct page_ext_operations page_poisoning_ops = {
 	.init = init_page_poisoning,
 };
 
+static int __init memory_sanitize_pagepoison_init(void)
+{
+	/* With 'memory sanitize' On, page poisoning Must be turned on */
+	if (IS_ENABLED(CONFIG_MEMORY_SANITIZE)) {
+		want_page_poisoning = true;
+		__page_poisoning_enabled = true;
+	}
+	return 0;
+}
+early_initcall(memory_sanitize_pagepoison_init);
+
 static inline void set_page_poison(struct page *page)
 {
 	struct page_ext *page_ext;
diff --git a/mm/slub.c b/mm/slub.c
index d24e1ce..62de543 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -457,6 +457,17 @@ static int slub_debug;
 static char *slub_debug_slabs;
 static int disable_higher_order_debug;
 
+static int __init memory_sanitize_slubdebug_init(void)
+{
+/* With 'memory sanitize' On, slub_debug Must be set to 'p' */
+	if (IS_ENABLED(CONFIG_SLUB_DEBUG_ON) &&
+	     IS_ENABLED(CONFIG_MEMORY_SANITIZE)) {
+		slub_debug |= SLAB_POISON;
+	}
+	return 0;
+}
+early_initcall(memory_sanitize_slubdebug_init);
+
 /*
  * slub is about to manipulate internal object metadata.  This memory lies
  * outside the range of the allocated object, so accessing it would normally
@@ -5675,6 +5686,11 @@ static int __init slab_sysfs_init(void)
 	struct kmem_cache *s;
 	int err;
 
+#ifdef CONFIG_MEMORY_SANITIZE
+	pr_info("[CONFIG_MEMORY_SANITIZE]: slub_debug = P? %s [0x%x]\n",
+		slub_debug & SLAB_POISON ? "yes" : "no", slub_debug);
+#endif
+
 	mutex_lock(&slab_mutex);
 
 	slab_kset = kset_create_and_add("slab", &slab_uevent_ops, kernel_kobj);

---
(Minimal) Testing done so far:
-------------------------------+----------------------+-----------------------
|         Kconfig              |     kernel cmdline   |        Result*       |
|--------------+---------------+----------------------+----------------------|
SLUB_DEBUG[_ON]|MEMORY_SANITIZE|page_poison|slub_debug|page_poison|slub_debug|
|--------------+---------------+----------------------+----------------------|
      Y        |         Y     |     np[1] |    np    |     on    |     p    |
      Y        |         Y     |     np[1] |    =p    |     on    |     p    |
      Y        |         Y     |     np[1] |    =fzu  |     on    |     p    |
      Y        |         Y     |     np[1] |    =fzup |     on    |     p    |
      Y        |         Y     |     np[1] |    =     |     on    |     p    |
|--------------+---------------+----------------------+----------------------|
      Y        |         Y     |     =off  |    np    |     on    |     p    |
      Y        |         Y     |     =off  |    =p    |     on    |     p    |
      Y        |         Y     |     =off  |    =fzu  |     on    |     p    |
      Y        |         Y     |     =off  |    =     |     on    |     p    |
|--------------+---------------+----------------------+----------------------|
      Y        |         Y     |     =on   |    np    |     on    |     p    |
      Y        |         Y     |     =on   |    =p    |     on    |     p    |
      Y        |         Y     |     =on   |    =fzu  |     on    |     p    |
      Y        |         Y     |     =on   |    =     |     on    |     p    |
|--------------+---------------+----------------------+----------------------|
[1] np = not passed
* Result: MEMORY_SANITIZE='y' => page_poison should be 'on' and slub_debug='p'
  (implying SLAB_POISON bit set).
So far, all tests passed..

Thanks,
Kaiwan.

On Tue, 14 Feb 2017 09:19:27 -0800
Kees Cook <keescook@chromium.org> wrote:

> On Mon, Feb 13, 2017 at 7:01 PM, Kaiwan N Billimoria
> <kaiwan@kaiwantech.com> wrote:
> > Thanks for your response...  
> >>
> >>
> >>  
> >> > +config MEMORY_SANITIZE
> >> > +       bool "Enable memory sanitization features"
> >> > +       select SLUB_DEBUG
> >> > +       select PAGE_POISONING
> >> > +       select PAGE_POISONING_NO_SANITY if HIBERNATION
> >> > +       ---help---
> >> > +       This option enables ...  
> >>
> >> Good start! Why the "if HIBERNATION" bit? It seems like sanity
> >> checks are very expensive, so we'd not want it as part of this
> >> config? 
> > Okay, I wasn't sure. So would it be (more) correct to retain the
> > first two configs plus
> > PAGE_POISONING_NO_SANITY (without the if)?  
> 
> I think so, yes. We may need to tweak it in the future, but I think
> that's the correct config for now.
> 
> >> >  #if defined(CONFIG_SLUB_DEBUG_ON)
> >> > +#if defined(CONFIG_MEMORY_SANITIZE)
> >> > +/* With 'memory sanitize' On, slub_debug should be 'P' */
> >> > +static int slub_debug = SLAB_POISON;
> >> > +#else
> >> >  static int slub_debug = DEBUG_DEFAULT_FLAGS;
> >> > +#endif /* CONFIG_MEMORY_SANITIZE */
> >> >  #else
> >> >  static int slub_debug;
> >> > -#endif
> >> > +#endif /* CONFIG_SLUB_DEBUG_ON */  
> >>
> >> Could the definition of DEBUG_DEFAULT_FLAGS be adjusted instead of
> >> doing the ifdefs here in the .c file? Or, perhaps do a slub_debug
> >> |= SLAB_POISON in memory_sanitize_init()?
> >>  
> > Yes, the latter sounds good but the init function is in
> > mm/page_poison.c and the slub_debug var is a static in mm/slub.c .
> > Any suggestions?  
> 
> Perhaps add another early_init like you did the page_poison.c?
> 
> -Kees
> 

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [kernel-hardening] Merge in PAX_MEMORY_SANITIZE work from grsec to linux-next
  2017-02-15  7:27               ` Kaiwan N Billimoria
@ 2017-02-15 20:30                 ` Kees Cook
  2017-02-17  3:18                   ` Kaiwan N Billimoria
  0 siblings, 1 reply; 20+ messages in thread
From: Kees Cook @ 2017-02-15 20:30 UTC (permalink / raw)
  To: Kaiwan N Billimoria, Christoph Lameter; +Cc: Laura Abbott, kernel-hardening

On Tue, Feb 14, 2017 at 11:27 PM, Kaiwan N Billimoria
<kaiwan@kaiwantech.com> wrote:
> Okay, I've incorporated your suggestions.
> Of course, the printk's are temporary.
>
> Pl see:
> - the updated patch, and
> - a 'truth table' enumerating some basic testing with these configs,
> below:
>
> ---
> diff --git a/init/main.c b/init/main.c
> index ef47035..ba44574 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1028,6 +1028,11 @@ static noinline void __init kernel_init_freeable(void)
>
>         do_basic_setup();
>
> +#ifdef CONFIG_MEMORY_SANITIZE
> +       pr_debug("[CONFIG_MEMORY_SANITIZE]: page_poisoning_enabled? %s\n",
> +               page_poisoning_enabled() ? "yes" : "no");
> +#endif
> +
>         /* Open the /dev/console on the rootfs, this should never fail */
>         if (sys_open((const char __user *) "/dev/console", O_RDWR, 0) < 0)
>                 pr_err("Warning: unable to open an initial console.\n");
> diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug
> index 3e5eada..fbb4290 100644
> --- a/mm/Kconfig.debug
> +++ b/mm/Kconfig.debug
> @@ -97,3 +97,24 @@ config DEBUG_RODATA_TEST
>      ---help---
>        This option enables a testcase for the setting rodata read-only.
>
> +config MEMORY_SANITIZE
> +    bool "Enable memory sanitization features"
> +    select SLUB_DEBUG
> +    select PAGE_POISONING
> +    select PAGE_POISONING_NO_SANITY
> +    ---help---
> +       This option enables memory sanitization features. Particularly,
> +       when you turn on this option, it auto-enables:
> +       - SLUB debug
> +       - page poisoning
> +       - page poisoning no sanity.
> +
> +       Implication: turning this option on _will_ implicitly enable:
> +       - the SLUB_DEBUG switch to the equivalent of the kernel command-line
> +         'slub_debug=p' ; (where p=PAGE_POISON),
> +       - page poisoning, equivalent to passing the kernel command-line option
> +         'page_poison=on',
> +       irrespective of whether the options are explicitly passed or not.

Hm, we don't want to force this on against other command line
settings, so how about what I suggest below...

> +
> +       If unsure, say N.
> +
> diff --git a/mm/page_poison.c b/mm/page_poison.c
> index 2e647c6..8d1e883 100644
> --- a/mm/page_poison.c
> +++ b/mm/page_poison.c
> @@ -49,6 +49,17 @@ struct page_ext_operations page_poisoning_ops = {
>         .init = init_page_poisoning,
>  };
>
> +static int __init memory_sanitize_pagepoison_init(void)
> +{
> +       /* With 'memory sanitize' On, page poisoning Must be turned on */
> +       if (IS_ENABLED(CONFIG_MEMORY_SANITIZE)) {
> +               want_page_poisoning = true;
> +               __page_poisoning_enabled = true;
> +       }
> +       return 0;
> +}
> +early_initcall(memory_sanitize_pagepoison_init);

Can this be changed to interact better with the command line
arguments, in case someone selects CONFIG_MEMORY_SANITIZE, but boots
with page_poison=off?

e.g. instead of your init code, do this:

static bool want_page_poisoning __read_mostly =
IS_ENABLED(CONFIG_MEMORY_SANITIZE);

That way the logic for __page_poisoning_enabled is retained, etc.

> +
>  static inline void set_page_poison(struct page *page)
>  {
>         struct page_ext *page_ext;
> diff --git a/mm/slub.c b/mm/slub.c
> index d24e1ce..62de543 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -457,6 +457,17 @@ static int slub_debug;
>  static char *slub_debug_slabs;
>  static int disable_higher_order_debug;
>
> +static int __init memory_sanitize_slubdebug_init(void)
> +{
> +/* With 'memory sanitize' On, slub_debug Must be set to 'p' */
> +       if (IS_ENABLED(CONFIG_SLUB_DEBUG_ON) &&
> +            IS_ENABLED(CONFIG_MEMORY_SANITIZE)) {
> +               slub_debug |= SLAB_POISON;
> +       }
> +       return 0;
> +}
> +early_initcall(memory_sanitize_slubdebug_init);

And instead of this, use:

#if defined(CONFIG_SLUB_DEBUG_ON)
# define SLUB_DEBUG_INITIAL_FLAGS       DEBUG_DEFAULT_FLAGS
#elif defined(CONFIG_MEMORY_SANITIZE)
# define SLUB_DEBUG_INITIAL_FLAGS       SLAB_POISON
#else
# define SLUB_DEBUG_INITIAL_FLAGS       0
#endif

static int slub_debug = SLUB_DEBUG_INITIAL_FLAGS;

That way we're just setting the boot-time defaults and it'll safely
interact with everything else.

(I've added Christoph to CC to see what he thinks of this.)

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [kernel-hardening] Merge in PAX_MEMORY_SANITIZE work from grsec to linux-next
  2017-02-15 20:30                 ` Kees Cook
@ 2017-02-17  3:18                   ` Kaiwan N Billimoria
  2017-02-21 23:26                     ` Kees Cook
  0 siblings, 1 reply; 20+ messages in thread
From: Kaiwan N Billimoria @ 2017-02-17  3:18 UTC (permalink / raw)
  To: Kees Cook; +Cc: Christoph Lameter, Laura Abbott, kernel-hardening

>
> Hm, we don't want to force this on against other command line
> settings, so how about what I suggest below...
>

Ok, can see the logic in that..

>
> Can this be changed to interact better with the command line
> arguments, in case someone selects CONFIG_MEMORY_SANITIZE, but boots
> with page_poison=off?
>
> e.g. instead of your init code, do this:
>
> static bool want_page_poisoning __read_mostly =
> IS_ENABLED(CONFIG_MEMORY_SANITIZE);
>
> That way the logic for __page_poisoning_enabled is retained, etc.
>
>> +
>>  static inline void set_page_poison(struct page *page)
>>  {
>>         struct page_ext *page_ext;
>> diff --git a/mm/slub.c b/mm/slub.c
>> index d24e1ce..62de543 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -457,6 +457,17 @@ static int slub_debug;
>>  static char *slub_debug_slabs;
>>  static int disable_higher_order_debug;
>>
>> +static int __init memory_sanitize_slubdebug_init(void)
>> +{
>> +/* With 'memory sanitize' On, slub_debug Must be set to 'p' */
>> +       if (IS_ENABLED(CONFIG_SLUB_DEBUG_ON) &&
>> +            IS_ENABLED(CONFIG_MEMORY_SANITIZE)) {
>> +               slub_debug |= SLAB_POISON;
>> +       }
>> +       return 0;
>> +}
>> +early_initcall(memory_sanitize_slubdebug_init);
>
> And instead of this, use:
>
> #if defined(CONFIG_SLUB_DEBUG_ON)
> # define SLUB_DEBUG_INITIAL_FLAGS       DEBUG_DEFAULT_FLAGS
> #elif defined(CONFIG_MEMORY_SANITIZE)
> # define SLUB_DEBUG_INITIAL_FLAGS       SLAB_POISON
> #else
> # define SLUB_DEBUG_INITIAL_FLAGS       0
> #endif
>
> static int slub_debug = SLUB_DEBUG_INITIAL_FLAGS;
>
> That way we're just setting the boot-time defaults and it'll safely
> interact with everything else.
>
> (I've added Christoph to CC to see what he thinks of this.)
>

What about a kernel command-line param - called 'mem_sanitize'?
If set to 'on', it will enforce the forced behaviour (as per the curr
implementation).

I can think of this (but...):
--------------------+--------------------------+---------------------------
mem_sanitize   |   page_poisoning   |    slub_debug
--------------------+--------------------------+---------------------------
      strict           |            on                |    |= SLAB_POISON
      normal       |                      <current default>
      off              |              off               |            off
--------------------+--------------------------+---------------------------

But... if you think about it, I guess only the "strict" option is useful in any
meaningful way.
'normal' is what we'll get when I re-implement the code
as you suggested above.
Do we even want an 'off' case? As again, we'd be poking into the page_poisoning
and slub_debug values.

Of course we can (and will) document all this...
Thoughts?

-Kaiwan.

> -Kees
>
> --
> Kees Cook
> Pixel Security
>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [kernel-hardening] Merge in PAX_MEMORY_SANITIZE work from grsec to linux-next
  2017-02-17  3:18                   ` Kaiwan N Billimoria
@ 2017-02-21 23:26                     ` Kees Cook
  2017-02-24  9:38                       ` Kaiwan N Billimoria
  0 siblings, 1 reply; 20+ messages in thread
From: Kees Cook @ 2017-02-21 23:26 UTC (permalink / raw)
  To: Kaiwan N Billimoria; +Cc: Christoph Lameter, Laura Abbott, kernel-hardening

On Thu, Feb 16, 2017 at 7:18 PM, Kaiwan N Billimoria
<kaiwan@kaiwantech.com> wrote:
>> Hm, we don't want to force this on against other command line
>> settings, so how about what I suggest below...
>>
>
> Ok, can see the logic in that..
>
>>
>> Can this be changed to interact better with the command line
>> arguments, in case someone selects CONFIG_MEMORY_SANITIZE, but boots
>> with page_poison=off?
>>
>> e.g. instead of your init code, do this:
>>
>> static bool want_page_poisoning __read_mostly =
>> IS_ENABLED(CONFIG_MEMORY_SANITIZE);
>>
>> That way the logic for __page_poisoning_enabled is retained, etc.
>>
>>> +
>>>  static inline void set_page_poison(struct page *page)
>>>  {
>>>         struct page_ext *page_ext;
>>> diff --git a/mm/slub.c b/mm/slub.c
>>> index d24e1ce..62de543 100644
>>> --- a/mm/slub.c
>>> +++ b/mm/slub.c
>>> @@ -457,6 +457,17 @@ static int slub_debug;
>>>  static char *slub_debug_slabs;
>>>  static int disable_higher_order_debug;
>>>
>>> +static int __init memory_sanitize_slubdebug_init(void)
>>> +{
>>> +/* With 'memory sanitize' On, slub_debug Must be set to 'p' */
>>> +       if (IS_ENABLED(CONFIG_SLUB_DEBUG_ON) &&
>>> +            IS_ENABLED(CONFIG_MEMORY_SANITIZE)) {
>>> +               slub_debug |= SLAB_POISON;
>>> +       }
>>> +       return 0;
>>> +}
>>> +early_initcall(memory_sanitize_slubdebug_init);
>>
>> And instead of this, use:
>>
>> #if defined(CONFIG_SLUB_DEBUG_ON)
>> # define SLUB_DEBUG_INITIAL_FLAGS       DEBUG_DEFAULT_FLAGS
>> #elif defined(CONFIG_MEMORY_SANITIZE)
>> # define SLUB_DEBUG_INITIAL_FLAGS       SLAB_POISON
>> #else
>> # define SLUB_DEBUG_INITIAL_FLAGS       0
>> #endif
>>
>> static int slub_debug = SLUB_DEBUG_INITIAL_FLAGS;
>>
>> That way we're just setting the boot-time defaults and it'll safely
>> interact with everything else.
>>
>> (I've added Christoph to CC to see what he thinks of this.)
>>
>
> What about a kernel command-line param - called 'mem_sanitize'?
> If set to 'on', it will enforce the forced behaviour (as per the curr
> implementation).
>
> I can think of this (but...):
> --------------------+--------------------------+---------------------------
> mem_sanitize   |   page_poisoning   |    slub_debug
> --------------------+--------------------------+---------------------------
>       strict           |            on                |    |= SLAB_POISON
>       normal       |                      <current default>
>       off              |              off               |            off
> --------------------+--------------------------+---------------------------
>
> But... if you think about it, I guess only the "strict" option is useful in any
> meaningful way.
> 'normal' is what we'll get when I re-implement the code
> as you suggested above.
> Do we even want an 'off' case? As again, we'd be poking into the page_poisoning
> and slub_debug values.
>
> Of course we can (and will) document all this...
> Thoughts?

I think mem_sanitize should likely follow the logic used by
pax_sanitize_slab. i.e CONFIG_MEMORIZE_SANITIZE as suggested above,
then a mem_sanitize= option for "off" and "full". (i.e.
CONFIG_MEMORIZE_SANITIZE implies "on")

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [kernel-hardening] Merge in PAX_MEMORY_SANITIZE work from grsec to linux-next
  2017-02-21 23:26                     ` Kees Cook
@ 2017-02-24  9:38                       ` Kaiwan N Billimoria
  0 siblings, 0 replies; 20+ messages in thread
From: Kaiwan N Billimoria @ 2017-02-24  9:38 UTC (permalink / raw)
  To: Kees Cook; +Cc: Christoph Lameter, Laura Abbott, kernel-hardening

On Tue, 21 Feb 2017 15:26:03 -0800
Kees Cook <keescook@chromium.org> wrote:

> I think mem_sanitize should likely follow the logic used by
> pax_sanitize_slab. i.e CONFIG_MEMORIZE_SANITIZE as suggested above,
> then a mem_sanitize= option for "off" and "full". (i.e.
> CONFIG_MEMORIZE_SANITIZE implies "on")
> 
> -Kees
> 

Okay. So, while I found that the (pseudo)code you quickly mentioned did not 
exactly work out, I have (hopefully) implemented the idea. 

A qs: currently, the slub_debug option gets set to 'p' as long as MEMORY_SANITIZE 
is enabled, even if the user passes 'slub_debug=-' on the command line. Is this ok?

Pl review the patch below:

diff --git a/init/main.c b/init/main.c
index 63d640e..4397a11 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1027,6 +1027,11 @@ static noinline void __init kernel_init_freeable(void)
 
	 do_basic_setup();
 
+#ifdef CONFIG_MEMORY_SANITIZE
+    pr_debug("[CONFIG_MEMORY_SANITIZE]: page_poisoning_enabled? %s\n",
+   	 page_poisoning_enabled() ? "yes" : "no");
+#endif
+
	 /* Open the /dev/console on the rootfs, this should never fail */
	 if (sys_open((const char __user *) "/dev/console", O_RDWR, 0) < 0)
		 pr_err("Warning: unable to open an initial console.\n");
diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug
index 3c88b7e..e8aa61d 100644
--- a/mm/Kconfig.debug
+++ b/mm/Kconfig.debug
@@ -97,3 +97,25 @@ config DEBUG_RODATA_TEST
 	---help---
   	This option enables a testcase for the setting rodata read-only.
 
+config MEMORY_SANITIZE
+	bool "Enable memory sanitization features"
+	select SLUB_DEBUG
+	select PAGE_POISONING
+	select PAGE_POISONING_NO_SANITY
+	---help---
+   	This option enables memory sanitization features. Particularly,
+   	when you turn on this option, it auto-enables:
+   	- SLUB debug
+   	- page poisoning
+   	- page poisoning no sanity.
+
+   	Implication: turning this option on _will_ implicitly enable:
+   	- the SLUB_DEBUG switch to the equivalent of the kernel command-line
+     	'slub_debug=p' ; (where p=SLAB_POISON),
+   	- page poisoning, equivalent to passing the kernel command-line option
+     	'page_poison=on'.
+    Of course, kernel command-line options 'page_poison' and 'slub_debug'
+    are still honoured (except that slub-debug will always have the 'p' set).
+
+   	If unsure, say N.
+
diff --git a/mm/page_poison.c b/mm/page_poison.c
index 2e647c6..0ca3bc5 100644
--- a/mm/page_poison.c
+++ b/mm/page_poison.c
@@ -7,7 +7,8 @@
 #include <linux/ratelimit.h>
 
 static bool __page_poisoning_enabled __read_mostly;
-static bool want_page_poisoning __read_mostly;
+static bool want_page_poisoning __read_mostly =
+   	 IS_ENABLED(CONFIG_MEMORY_SANITIZE);
 
 static int early_page_poison_param(char *buf)
 {
@@ -49,6 +50,17 @@ struct page_ext_operations page_poisoning_ops = {
	 .init = init_page_poisoning,
 };
 
+static int __init memory_sanitize_pagepoison_init(void)
+{
+     */
+    if (IS_ENABLED(CONFIG_MEMORY_SANITIZE) && !want_page_poisoning)
+   	 __page_poisoning_enabled = false;
+    return 0;
+}
+early_initcall(memory_sanitize_pagepoison_init);
+
 static inline void set_page_poison(struct page *page)
 {
	 struct page_ext *page_ext;
diff --git a/mm/slub.c b/mm/slub.c
index 7f4bc70..76abcec 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -450,15 +450,30 @@ static inline void *restore_red_left(struct kmem_cache *s, void *p)
 /*
  * Debug settings:
  */
-#if defined(CONFIG_SLUB_DEBUG_ON)
-static int slub_debug = DEBUG_DEFAULT_FLAGS;
-#else
 static int slub_debug;
-#endif
 
 static char *slub_debug_slabs;
 static int disable_higher_order_debug;
 
+static int __init memory_sanitize_slubdebug_init(void)
+{
+/* With MEMORY_SANITIZE On, slub_debug Must be set to 'p' */
+    if (IS_ENABLED(CONFIG_SLUB_DEBUG_ON) &&
+     	IS_ENABLED(CONFIG_MEMORY_SANITIZE)) {
+   	 slub_debug |= SLAB_POISON;
+    } else if (!IS_ENABLED(CONFIG_SLUB_DEBUG_ON) &&
+     	IS_ENABLED(CONFIG_MEMORY_SANITIZE)) {
+   	 slub_debug = SLAB_POISON;
+    } else if (IS_ENABLED(CONFIG_SLUB_DEBUG_ON) &&
+     	!IS_ENABLED(CONFIG_MEMORY_SANITIZE)) {
+   	 slub_debug = DEBUG_DEFAULT_FLAGS;
+    } else { /* both disabled */
+   	 slub_debug = 0;
+    }
+    return 0;
+}
+early_initcall(memory_sanitize_slubdebug_init);
+
 /*
  * slub is about to manipulate internal object metadata.  This memory lies
  * outside the range of the allocated object, so accessing it would normally
@@ -5755,6 +5770,11 @@ static int __init slab_sysfs_init(void)
	 struct kmem_cache *s;
	 int err;
 
+#ifdef CONFIG_MEMORY_SANITIZE
+    pr_info("[CONFIG_MEMORY_SANITIZE]: slub_debug = P? %s [0x%x]\n",
+   	 slub_debug & SLAB_POISON ? "yes" : "no", slub_debug);
+#endif
+
	 mutex_lock(&slab_mutex);
 
	 slab_kset = kset_create_and_add("slab", &slab_uevent_ops, kernel_kobj);


Thanks,
Kaiwan.

^ permalink raw reply related	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2017-02-24  9:38 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-18  4:21 [kernel-hardening] Merge in PAX_MEMORY_SANITIZE work from grsec to linux-next Kaiwan N Billimoria
2017-01-18 19:44 ` Laura Abbott
2017-01-31  1:49   ` Kaiwan N Billimoria
2017-02-03  4:49   ` Kaiwan N Billimoria
2017-02-04  0:23     ` Kees Cook
2017-02-04  3:12       ` Kaiwan N Billimoria
2017-02-09  3:37       ` Kaiwan N Billimoria
2017-02-13 11:42         ` Kaiwan N Billimoria
2017-02-13 17:28         ` Kees Cook
2017-02-14  3:01           ` Kaiwan N Billimoria
2017-02-14 17:19             ` Kees Cook
2017-02-15  7:27               ` Kaiwan N Billimoria
2017-02-15 20:30                 ` Kees Cook
2017-02-17  3:18                   ` Kaiwan N Billimoria
2017-02-21 23:26                     ` Kees Cook
2017-02-24  9:38                       ` Kaiwan N Billimoria
2017-02-14  8:04           ` Kaiwan N Billimoria
2017-02-14 16:19             ` Mark Rutland
2017-02-14 16:33               ` Kaiwan N Billimoria
2017-01-19  0:54 ` Kees Cook

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.