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

[Frank Heimes]

Well, I'm sure this is a question for experienced kernel engineers - so I
just stay tuned.

But as a side note:
My approach was only to get the single commit 49f2d2419d60 in as smooth as
possible,
means with as few changes as possible (incl. context),
so that potential further cherry-picks or backports in that area are not
harmed (or at least as little as possible).


On Tue, Oct 5, 2021 at 4:10 PM Kleber Souza <kleber.souza at canonical.com>
wrote:

> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20211005/9e72e48c/attachment-0001.html>