On Tue, Nov 12, 2019 at 08:17:57AM +0100, Jiri Slaby wrote:

On 11. 01. 18, 3:02, Kees Cook wrote:

quoted

From: David Windsor <redacted>

Mark the kmalloc slab caches as entirely whitelisted. These caches
are frequently used to fulfill kernel allocations that contain data
to be copied to/from userspace. Internal-only uses are also common,
but are scattered in the kernel. For now, mark all the kmalloc caches
as whitelisted.

This patch is modified from Brad Spengler/PaX Team's PAX_USERCOPY
whitelisting code in the last public patch of grsecurity/PaX based on my
understanding of the code. Changes or omissions from the original code are
mine and don't reflect the original grsecurity/PaX code.

Signed-off-by: David Windsor <redacted>
[kees: merged in moved kmalloc hunks, adjust commit log]
Cc: Pekka Enberg <penberg@kernel.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Joonsoo Kim <redacted>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mm@kvack.org
Cc: linux-xfs@vger.kernel.org
Signed-off-by: Kees Cook <redacted>
Acked-by: Christoph Lameter <redacted>
---
 mm/slab.c        |  3 ++-
 mm/slab.h        |  3 ++-
 mm/slab_common.c | 10 ++++++----
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index b9b0df620bb9..dd367fe17a4e 100644
--- a/mm/slab.c
+++ b/mm/slab.c

...

quoted

@@ -1098,7 +1099,8 @@ void __init setup_kmalloc_cache_index_table(void)
 static void __init new_kmalloc_cache(int idx, slab_flags_t flags)
 {
 	kmalloc_caches[idx] = create_kmalloc_cache(kmalloc_info[idx].name,
-					kmalloc_info[idx].size, flags);
+					kmalloc_info[idx].size, flags, 0,
+					kmalloc_info[idx].size);
 }
 
 /*

@@ -1139,7 +1141,7 @@ void __init create_kmalloc_caches(slab_flags_t flags)
 
 			BUG_ON(!n);
 			kmalloc_dma_caches[i] = create_kmalloc_cache(n,
-				size, SLAB_CACHE_DMA | flags);
+				size, SLAB_CACHE_DMA | flags, 0, 0);

Hi,

was there any (undocumented) reason NOT to mark DMA caches as usercopy?

We are seeing this on s390x:

quoted

usercopy: Kernel memory overwrite attempt detected to SLUB object

'dma-kmalloc-1k' (offset 0, size 11)!

quoted

------------[ cut here ]------------
kernel BUG at mm/usercopy.c:99!

Interesting! I believe the rationale was that if the region is used for
DMA, allowing direct access to it from userspace could be prone to
races.

See:
https://bugzilla.suse.com/show_bug.cgi?id=1156053

For context from the bug, the trace is:

(<0000000000386c5a> usercopy_abort+0xa2/0xa8) 
 <000000000036097a> __check_heap_object+0x11a/0x120  
 <0000000000386b3a> __check_object_size+0x18a/0x208  
 <000000000079b4ba> skb_copy_datagram_from_iter+0x62/0x240  
 <000003ff804edd5c> iucv_sock_sendmsg+0x1fc/0x858 Ýaf_iucv¨  
 <0000000000785894> sock_sendmsg+0x54/0x90  
 <0000000000785944> sock_write_iter+0x74/0xa0  
 <000000000038a3f0> new_sync_write+0x110/0x180  
 <000000000038d42e> vfs_write+0xa6/0x1d0  
 <000000000038d748> ksys_write+0x60/0xe8  
 <000000000096a660> system_call+0xdc/0x2e0  

I know Al worked on fixing up usercopy checking for iters. I wonder if
there is redundant checking happening here? i.e. haven't iters already
done object size verifications, so they're not needed during iter copy
helpers?

How is iucv the only network protocol that has run into this? Do others
use a bounce buffer?

-- 
Kees Cook