From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.4 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D5027CA9EA0 for ; Fri, 18 Oct 2019 15:30:59 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 761C32089C for ; Fri, 18 Oct 2019 15:30:59 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="EkWgFFxK" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 761C32089C Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 0E8248E0005; Fri, 18 Oct 2019 11:30:59 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0985D8E0003; Fri, 18 Oct 2019 11:30:59 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EC9FD8E0005; Fri, 18 Oct 2019 11:30:58 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0228.hostedemail.com [216.40.44.228]) by kanga.kvack.org (Postfix) with ESMTP id C265A8E0003 for ; Fri, 18 Oct 2019 11:30:58 -0400 (EDT) Received: from smtpin08.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with SMTP id 629E682DDCA5 for ; Fri, 18 Oct 2019 15:30:58 +0000 (UTC) X-FDA: 76057293396.08.mom53_5fb2b6fabdc3e X-HE-Tag: mom53_5fb2b6fabdc3e X-Filterd-Recvd-Size: 16949 Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) by imf18.hostedemail.com (Postfix) with ESMTP for ; Fri, 18 Oct 2019 15:30:57 +0000 (UTC) Received: by mail-wm1-f65.google.com with SMTP id 5so6652943wmg.0 for ; Fri, 18 Oct 2019 08:30:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=lAXZcKInKZO/fUzCvBqiPgvJmui5H4sAW2k7r8uLPmE=; b=EkWgFFxK8zp07d3h+m6iFRl5tdjez+t9FYFmt0UQDUYwrU/5rR0P3LVR0bY/dWU67S 0EgpY+xyzHpgLiABe7b+uG3Rm4Zbt7Uvd1nIk7BCutNsNXyLxhJ1dOPntmZ7abL5czHe tOrPaUwkhzNIl8MHDRAatoWwptpUBYhxzQYhXAn/B9np+oJGDxr2oym7iy3kFK7JChMP lFpJwKDD3jRK2Dn/W9lSZYdobbY7D9nWl3Db2CiUmqUALC9We7LCHCEPwEwXronVPIgZ UpEBGYvGWOZh1pN6Ry1Tch+iAPn9aDA3y1oYANLNP5BWC/VHcKFJRc8/CwJ658riYcgB dbIw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=lAXZcKInKZO/fUzCvBqiPgvJmui5H4sAW2k7r8uLPmE=; b=Bvf7sU0eH8qdBplJSCJGm+fe1QvEKmhn8CyAJs36ioWKSlUgXftw4DfRE1QyQDTEd/ 5NmGLdmU2mgVTbExgWuTZBZK+LEQDLo671x/Wd1DoMzjbdCbHzF84dXJgp/KQLwjBB5W uifW4mvV8cgYG4Yxda9YHSLh4QbnJzc2SJ1M+QikqboAg0ihel2zneVrrTd6XrIq3HlC slGLb3b3ttxLJZ6gm9132/tw1ID1EKkki3MrdCETSB6C+lGLnuWlKNjPq81+Lg7caBFy 37uir6jbNC1/RE6nUittmdlAMMp/YvczoAI5UWChq7MakFbaffQe8RKODasWJebtLLtN c0Zw== X-Gm-Message-State: APjAAAXy2BIPyi01cywvrVm2/aSyGJ0zvGMBgthiR7Ouuu1DECc1VI8Z 79y/vXs2TAAmPpxTv/p19gZ8n/lcZSuRWSmbmoUpxQ== X-Google-Smtp-Source: APXvYqynJw+93Lo3lzC96VTMTLWgibbdt3QZqiceDQpJhEqA+D6Ky9o/fSXH6ufdpIGUVb/Xus4moCqWVj5u3W0bYLM= X-Received: by 2002:a05:600c:2196:: with SMTP id e22mr2321941wme.79.1571412655690; Fri, 18 Oct 2019 08:30:55 -0700 (PDT) MIME-Version: 1.0 References: <20191018094304.37056-1-glider@google.com> <20191018094304.37056-19-glider@google.com> <1571404943.5937.71.camel@lca.pw> <1571406117.5937.73.camel@lca.pw> <1571409732.5937.76.camel@lca.pw> <1571411587.5937.80.camel@lca.pw> In-Reply-To: <1571411587.5937.80.camel@lca.pw> From: Alexander Potapenko Date: Fri, 18 Oct 2019 17:30:44 +0200 Message-ID: Subject: Re: [PATCH RFC v1 18/26] kmsan: mm: call KMSAN hooks from SLUB code To: Qian Cai Cc: Andrew Morton , Vegard Nossum , Dmitry Vyukov , Linux Memory Management List Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Fri, Oct 18, 2019 at 5:13 PM Qian Cai wrote: > > On Fri, 2019-10-18 at 16:54 +0200, Alexander Potapenko wrote: > > On Fri, Oct 18, 2019 at 4:42 PM Qian Cai wrote: > > > > > > On Fri, 2019-10-18 at 15:55 +0200, Alexander Potapenko wrote: > > > > On Fri, Oct 18, 2019 at 3:42 PM Qian Cai wrote: > > > > > > > > > > On Fri, 2019-10-18 at 15:33 +0200, Alexander Potapenko wrote: > > > > > > On Fri, Oct 18, 2019 at 3:22 PM Qian Cai wrote: > > > > > > > > > > > > > > On Fri, 2019-10-18 at 11:42 +0200, glider@google.com wrote: > > > > > > > > In order to report uninitialized memory coming from heap al= locations > > > > > > > > KMSAN has to poison them unless they're created with __GFP_= ZERO. > > > > > > > > > > > > > > > > It's handy that we need KMSAN hooks in the places where > > > > > > > > init_on_alloc/init_on_free initialization is performed. > > > > > > > > > > > > > > Well, there is SLUB debug which has red zoning and poisoning = checks. What's > > > > > > > value of this patch? > > > > > > > > > > > > Sorry, are you talking about the whole patchset or just this pa= tch? > > > > > > > > > > Just this patch. > > > > > > > > > > > Note that SLUB debug is unable to detect uninitialized values w= ith > > > > > > bit-to-bit precision, neither have I heard of anyone using it f= or > > > > > > detecting uses of uninitialized memory in the kernel at all. > > > > > > The purpose of SLUB debug is to detect corruptions of freed mem= ory. > > > > > > > > > > The point is if developers have SLUB debug enabled, all the free = objects will be > > > > > poisoned, so what's the point of checking uninitialized memory th= ere? > > > > > > > > You are right, SLUB debug has to be handled separately. If I'm > > > > understanding correctly, right now KMSAN poisons freed memory befor= e > > > > SLUB debug wipes it, therefore the memory will count as initialized= . > > > > On the other hand, newly allocated memory is still marked as > > > > uninitialized, so a lot of bugs still remain detectable. > > > > > > Yes, but newly allocated slub objects will be poisoned as well. > > > > As far as I can tell, KMSAN hook marking newly allocated objects as > > uninitialized is called after slub poisoning. > > Therefore these allocations will be treated by KMSAN as uninitialized. > > > > TBH, I haven't tested KMSAN with SLUB debug good enough. Note that > > > > it's anyway a separate build that requires Clang, so right now it > > > > doesn't make much sense to combine KMSAN and SLUB debug together. > > > > > > Can't you just build a debug kernel with SLUB debug enabled but dropp= ing this > > > patch? If there is an uninitialized memory here leading to data corru= ption, SLUB > > > debug should be detected as well as this patch. If not, it needs to u= nderstand > > > why. > > > > Sorry, there might be some misunderstanding here. > > KMSAN keeps the state of heap objects separately by keeping exactly > > the same amount of initialized/uninitialized bits for every > > allocation. > > A call to kmsan_slab_alloc()/kmsan_slab_free() will mark an allocation > > as uninitialized for KMSAN. Not doing so will result in false reports. > > A call to memset(object, POISON_FREE, object_size) performed by SLAB > > debug will actually mark this allocation as initialized from KMSAN > > point of view, because we're memsetting a range with initialized data. > > Note that use of uninitialized data doesn't necessarily lead to > > immediate data corruption, so there might be nothing to detect for > > SLUB debug. > > Well, SLUB debug would mark any access of uninitialized memory as data > corruption with the help of poisoning and red zoning. > > It is true that KMSAN might be doing a bit more than SLUB debug here, but= the > question is if it is worth the maintenance burden? Do you have any existi= ng bugs > to show that KMSAN would find by this patch but SLUB debug can't? Consider the following case: int *a =3D kmalloc(sizeof(int), GFP_KERNEL); if (*a) { // do something, e.g. become root } IIUC SLUB poisoning will result in the branch being taken, but as long as there are no invalid accesses the kernel won't crash and no errors will be reported. KMSAN however will report an error, because the contents of the buffer pointed to by |a| are poisoned by kmsan_slab_alloc(). The compiler instrumentation will retrieve the shadow value for *a, compare it to zero and report a use of uninitialized memory. Another example would be copying the contents of newly allocated buffer to the userspace. > > > > > > > > > > > > > > > > > > > Signed-off-by: Alexander Potapenko > > > > > > > > To: Alexander Potapenko > > > > > > > > Cc: Andrew Morton > > > > > > > > Cc: Vegard Nossum > > > > > > > > Cc: Dmitry Vyukov > > > > > > > > Cc: linux-mm@kvack.org > > > > > > > > --- > > > > > > > > > > > > > > > > Change-Id: I51103b7981d3aabed747d0c85cbdc85568665871 > > > > > > > > --- > > > > > > > > mm/slub.c | 37 +++++++++++++++++++++++++++++++------ > > > > > > > > 1 file changed, 31 insertions(+), 6 deletions(-) > > > > > > > > > > > > > > > > diff --git a/mm/slub.c b/mm/slub.c > > > > > > > > index 3d63ae320d31..3d6d4c63446e 100644 > > > > > > > > --- a/mm/slub.c > > > > > > > > +++ b/mm/slub.c > > > > > > > > @@ -21,6 +21,8 @@ > > > > > > > > #include > > > > > > > > #include > > > > > > > > #include > > > > > > > > +#include > > > > > > > > +#include /* KMSAN_INIT_VALUE */ > > > > > > > > #include > > > > > > > > #include > > > > > > > > #include > > > > > > > > @@ -285,17 +287,27 @@ static void prefetch_freepointer(cons= t struct kmem_cache *s, void *object) > > > > > > > > prefetch(object + s->offset); > > > > > > > > } > > > > > > > > > > > > > > > > +/* > > > > > > > > + * When running under KMSAN, get_freepointer_safe() may re= turn an uninitialized > > > > > > > > + * pointer value in the case the current thread loses the = race for the next > > > > > > > > + * memory chunk in the freelist. In that case this_cpu_cmp= xchg_double() in > > > > > > > > + * slab_alloc_node() will fail, so the uninitialized value= won't be used, but > > > > > > > > + * KMSAN will still check all arguments of cmpxchg because= of imperfect > > > > > > > > + * handling of inline assembly. > > > > > > > > + * To work around this problem, use KMSAN_INIT_VALUE() to = force initialize the > > > > > > > > + * return value of get_freepointer_safe(). > > > > > > > > + */ > > > > > > > > static inline void *get_freepointer_safe(struct kmem_cache= *s, void *object) > > > > > > > > { > > > > > > > > unsigned long freepointer_addr; > > > > > > > > void *p; > > > > > > > > > > > > > > > > if (!debug_pagealloc_enabled()) > > > > > > > > - return get_freepointer(s, object); > > > > > > > > + return KMSAN_INIT_VALUE(get_freepointer(s, ob= ject)); > > > > > > > > > > > > > > > > freepointer_addr =3D (unsigned long)object + s->offse= t; > > > > > > > > probe_kernel_read(&p, (void **)freepointer_addr, size= of(p)); > > > > > > > > - return freelist_ptr(s, p, freepointer_addr); > > > > > > > > + return KMSAN_INIT_VALUE(freelist_ptr(s, p, freepointe= r_addr)); > > > > > > > > } > > > > > > > > > > > > > > > > static inline void set_freepointer(struct kmem_cache *s, v= oid *object, void *fp) > > > > > > > > @@ -1390,6 +1402,7 @@ static inline void *kmalloc_large_nod= e_hook(void *ptr, size_t size, gfp_t flags) > > > > > > > > ptr =3D kasan_kmalloc_large(ptr, size, flags); > > > > > > > > /* As ptr might get tagged, call kmemleak hook after = KASAN. */ > > > > > > > > kmemleak_alloc(ptr, size, 1, flags); > > > > > > > > + kmsan_kmalloc_large(ptr, size, flags); > > > > > > > > return ptr; > > > > > > > > } > > > > > > > > > > > > > > > > @@ -1397,6 +1410,7 @@ static __always_inline void kfree_hoo= k(void *x) > > > > > > > > { > > > > > > > > kmemleak_free(x); > > > > > > > > kasan_kfree_large(x, _RET_IP_); > > > > > > > > + kmsan_kfree_large(x); > > > > > > > > } > > > > > > > > > > > > > > > > static __always_inline bool slab_free_hook(struct kmem_cac= he *s, void *x) > > > > > > > > @@ -1453,6 +1467,12 @@ static inline bool slab_free_freelis= t_hook(struct kmem_cache *s, > > > > > > > > } while (object !=3D old_tail); > > > > > > > > } > > > > > > > > > > > > > > > > + do { > > > > > > > > + object =3D next; > > > > > > > > + next =3D get_freepointer(s, object); > > > > > > > > + kmsan_slab_free(s, object); > > > > > > > > + } while (object !=3D old_tail); > > > > > > > > + > > > > > > > > /* > > > > > > > > * Compiler cannot detect this function can be removed if = slab_free_hook() > > > > > > > > * evaluates to nothing. Thus, catch all relevant config = debug options here. > > > > > > > > @@ -2769,6 +2789,7 @@ static __always_inline void *slab_all= oc_node(struct kmem_cache *s, > > > > > > > > if (unlikely(slab_want_init_on_alloc(gfpflags, s)) &&= object) > > > > > > > > memset(object, 0, s->object_size); > > > > > > > > > > > > > > > > + kmsan_slab_alloc(s, object, gfpflags); > > > > > > > > slab_post_alloc_hook(s, gfpflags, 1, &object); > > > > > > > > > > > > > > > > return object; > > > > > > > > @@ -2797,6 +2818,7 @@ void *kmem_cache_alloc_trace(struct k= mem_cache *s, gfp_t gfpflags, size_t size) > > > > > > > > void *ret =3D slab_alloc(s, gfpflags, _RET_IP_); > > > > > > > > trace_kmalloc(_RET_IP_, ret, size, s->size, gfpflags)= ; > > > > > > > > ret =3D kasan_kmalloc(s, ret, size, gfpflags); > > > > > > > > + > > > > > > > > return ret; > > > > > > > > } > > > > > > > > EXPORT_SYMBOL(kmem_cache_alloc_trace); > > > > > > > > @@ -2809,7 +2831,6 @@ void *kmem_cache_alloc_node(struct km= em_cache *s, gfp_t gfpflags, int node) > > > > > > > > > > > > > > > > trace_kmem_cache_alloc_node(_RET_IP_, ret, > > > > > > > > s->object_size, s->size, = gfpflags, node); > > > > > > > > - > > > > > > > > return ret; > > > > > > > > } > > > > > > > > EXPORT_SYMBOL(kmem_cache_alloc_node); > > > > > > > > @@ -2825,6 +2846,7 @@ void *kmem_cache_alloc_node_trace(str= uct kmem_cache *s, > > > > > > > > size, s->size, gfpflags, node); > > > > > > > > > > > > > > > > ret =3D kasan_kmalloc(s, ret, size, gfpflags); > > > > > > > > + > > > > > > > > return ret; > > > > > > > > } > > > > > > > > EXPORT_SYMBOL(kmem_cache_alloc_node_trace); > > > > > > > > @@ -3150,7 +3172,7 @@ int kmem_cache_alloc_bulk(struct kmem= _cache *s, gfp_t flags, size_t size, > > > > > > > > void **p) > > > > > > > > { > > > > > > > > struct kmem_cache_cpu *c; > > > > > > > > - int i; > > > > > > > > + int i, j; > > > > > > > > > > > > > > > > /* memcg and kmem_cache debug support */ > > > > > > > > s =3D slab_pre_alloc_hook(s, flags); > > > > > > > > @@ -3188,11 +3210,11 @@ int kmem_cache_alloc_bulk(struct km= em_cache *s, gfp_t flags, size_t size, > > > > > > > > > > > > > > > > /* Clear memory outside IRQ disabled fastpath loop */ > > > > > > > > if (unlikely(slab_want_init_on_alloc(flags, s))) { > > > > > > > > - int j; > > > > > > > > - > > > > > > > > for (j =3D 0; j < i; j++) > > > > > > > > memset(p[j], 0, s->object_size); > > > > > > > > } > > > > > > > > + for (j =3D 0; j < i; j++) > > > > > > > > + kmsan_slab_alloc(s, p[j], flags); > > > > > > > > > > > > > > > > /* memcg and kmem_cache debug support */ > > > > > > > > slab_post_alloc_hook(s, flags, size, p); > > > > > > > > @@ -3793,6 +3815,7 @@ static int __init setup_slub_min_obje= cts(char *str) > > > > > > > > > > > > > > > > __setup("slub_min_objects=3D", setup_slub_min_objects); > > > > > > > > > > > > > > > > +__no_sanitize_memory > > > > > > > > void *__kmalloc(size_t size, gfp_t flags) > > > > > > > > { > > > > > > > > struct kmem_cache *s; > > > > > > > > @@ -5698,6 +5721,7 @@ static char *create_unique_id(struct = kmem_cache *s) > > > > > > > > p +=3D sprintf(p, "%07u", s->size); > > > > > > > > > > > > > > > > BUG_ON(p > name + ID_STR_LENGTH - 1); > > > > > > > > + kmsan_unpoison_shadow(name, p - name); > > > > > > > > return name; > > > > > > > > } > > > > > > > > > > > > > > > > @@ -5847,6 +5871,7 @@ static int sysfs_slab_alias(struct km= em_cache *s, const char *name) > > > > > > > > al->name =3D name; > > > > > > > > al->next =3D alias_list; > > > > > > > > alias_list =3D al; > > > > > > > > + kmsan_unpoison_shadow(al, sizeof(struct saved_alias))= ; > > > > > > > > return 0; > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --=20 Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Stra=C3=9Fe, 33 80636 M=C3=BCnchen Gesch=C3=A4ftsf=C3=BChrer: Paul Manicle, Halimah DeLaine Prado Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg