[SRU][F][PATCH 1/1] usercopy: mark dma-kmalloc caches as usercopy caches

[Kleber Souza]

On 05.10.21 08:32, frank.heimes at canonical.com wrote:
> From: Vlastimil Babka <vbabka at suse.cz>
> 
> BugLink: https://bugs.launchpad.net/bugs/1913442
> 
> Upstream commit : 49f2d2419d60a103752e5fbaf158cf8d07c0d884
> 
> We have seen a "usercopy: Kernel memory overwrite attempt detected to
> SLUB object 'dma-kmalloc-1 k' (offset 0, size 11)!" error on s390x, as
> IUCV uses kmalloc() with __GFP_DMA because of memory address
> restrictions.  The issue has been discussed [2] and it has been noted
> that if all the kmalloc caches are marked as usercopy, there's little
> reason not to mark dma-kmalloc caches too.  The 'dma' part merely means
> that __GFP_DMA is used to restrict memory address range.
> 
> As Jann Horn put it [3]:
>   "I think dma-kmalloc slabs should be handled the same way as normal
>    kmalloc slabs. When a dma-kmalloc allocation is freshly created, it is
>    just normal kernel memory - even if it might later be used for DMA -,
>    and it should be perfectly fine to copy_from_user() into such
>    allocations at that point, and to copy_to_user() out of them at the
>    end. If you look at the places where such allocations are created, you
>    can see things like kmemdup(), memcpy() and so on - all normal
>    operations that shouldn't conceptually be different from usercopy in
>    any relevant way."
> 
> Thus this patch marks the dma-kmalloc-* caches as usercopy.
> 
> [1] https://bugzilla.suse.com/show_bug.cgi?id=1156053
> [2] https://lore.kernel.org/kernel-hardening/bfca96db-bbd0-d958-7732-76e36c667c68@suse.cz/
> [3] https://lore.kernel.org/kernel-hardening/CAG48ez1a4waGk9kB0WLaSbs4muSoK0AYAVk8=XYaKj4_+6e6Hg@mail.gmail.com/
> 
> Signed-off-by: Vlastimil Babka <vbabka at suse.cz>
> Signed-off-by: Andrew Morton <akpm at linux-foundation.org>
> Acked-by: Christian Borntraeger <borntraeger at de.ibm.com>
> Acked-by: Jiri Slaby <jslaby at suse.cz>
> Cc: Jann Horn <jannh at google.com>
> Cc: Christoph Hellwig <hch at infradead.org>
> Cc: Christopher Lameter <cl at linux.com>
> Cc: Julian Wiedmann <jwi at linux.ibm.com>
> Cc: Ursula Braun <ubraun at linux.ibm.com>
> Cc: Alexander Viro <viro at zeniv.linux.org.uk>
> Cc: David Windsor <dave at nullcore.net>
> Cc: Pekka Enberg <penberg at kernel.org>
> Cc: David Rientjes <rientjes at google.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim at lge.com>
> Cc: Andy Lutomirski <luto at kernel.org>
> Cc: "David S. Miller" <davem at davemloft.net>
> Cc: Laura Abbott <labbott at redhat.com>
> Cc: Mark Rutland <mark.rutland at arm.com>
> Cc: "Martin K. Petersen" <martin.petersen at oracle.com>
> Cc: Paolo Bonzini <pbonzini at redhat.com>
> Cc: Christoffer Dall <christoffer.dall at linaro.org>
> Cc: Dave Kleikamp <dave.kleikamp at oracle.com>
> Cc: Jan Kara <jack at suse.cz>
> Cc: Luis de Bethencourt <luisbg at kernel.org>
> Cc: Marc Zyngier <marc.zyngier at arm.com>
> Cc: Rik van Riel <riel at surriel.com>
> Cc: Matthew Garrett <mjg59 at google.com>
> Cc: Michal Kubecek <mkubecek at suse.cz>
> Link: http://lkml.kernel.org/r/7d810f6d-8085-ea2f-7805-47ba3842dc50@suse.cz
> Signed-off-by: Linus Torvalds <torvalds at linux-foundation.org>
> (backported from commit 49f2d2419d60a103752e5fbaf158cf8d07c0d884)
> Signed-off-by: Frank Heimes <frank.heimes at canonical.com>
> ---
>   mm/slab_common.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 8c1ffbf7de45..76433e2187d0 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -1296,7 +1296,7 @@ void __init create_kmalloc_caches(slab_flags_t flags)
>   
>   			BUG_ON(!n);
>   			kmalloc_caches[KMALLOC_DMA][i] = create_kmalloc_cache(
> -				n, size, SLAB_CACHE_DMA | flags, 0, 0);
> +				n, size, SLAB_CACHE_DMA | flags, 0, kmalloc_info[i].size);

Although this is probably functionally the same, I'm wondering if we should
keep both "size" and "usersize" parameters passed to create_kmalloc_cache()
consistent.

In the original patch (49f2d2419d60), the "size" is not calculated anymore using
"kmalloc_size(i)", which was removed by dc0a7f7558dd (mm, slab: remove unused
kmalloc_size()), but passing "kmalloc_info[i].size" instead. The same is passed
as the "usersize" then.

Both ways of calculating "usersize" seem equivalent (using either "kmalloc_size(i)"
or "kmalloc_info[i].size"), but I didn't look very deeply to check if any patch
changed any of them in the meantime. So in my opinion we should also use "size" for
the backport to be on the safe side and do something like:

   			kmalloc_caches[KMALLOC_DMA][i] = create_kmalloc_cache(
-				n, size, SLAB_CACHE_DMA | flags, 0, 0);
+				n, size, SLAB_CACHE_DMA | flags, 0, size);


I'll not ACK nor NACK for now and wait for someone else to weight in.


Kleber