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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0C511C3DA49 for ; Fri, 26 Jul 2024 00:43:59 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 9615D6B009C; Thu, 25 Jul 2024 20:43:58 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 911996B009E; Thu, 25 Jul 2024 20:43:58 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7D8AA6B009F; Thu, 25 Jul 2024 20:43:58 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 5EB9A6B009C for ; Thu, 25 Jul 2024 20:43:58 -0400 (EDT) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 065F01414F3 for ; Fri, 26 Jul 2024 00:43:58 +0000 (UTC) X-FDA: 82380056556.14.009B64E Received: from mail-wm1-f50.google.com (mail-wm1-f50.google.com [209.85.128.50]) by imf13.hostedemail.com (Postfix) with ESMTP id 3C4BC20011 for ; Fri, 26 Jul 2024 00:43:56 +0000 (UTC) Authentication-Results: imf13.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=IDc78Uyh; spf=pass (imf13.hostedemail.com: domain of andreyknvl@gmail.com designates 209.85.128.50 as permitted sender) smtp.mailfrom=andreyknvl@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1721954570; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=xYO5SO4GDvuXPDiqp7OtLZoWNem0w1AFsK/JqDBRgRw=; b=gs/SHDaoAjgot4FaJD4LBD3fhkQdtIC/et9tzenrRAVGIsTFOs1a3vDNLEL8+llzZTXB+S nRHx4mHmJ8ZBKD2H4awQMMwivRgUM+ZVxqtRSFTniHQ0u1mDbMj0/p5MrM6r/tg0lAwXQ5 ZarYu1nljaAONjlbnTFw2OdBNq0ZN1Q= ARC-Authentication-Results: i=1; imf13.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=IDc78Uyh; spf=pass (imf13.hostedemail.com: domain of andreyknvl@gmail.com designates 209.85.128.50 as permitted sender) smtp.mailfrom=andreyknvl@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1721954570; a=rsa-sha256; cv=none; b=ue/M3CfVJhLCfuOntleoCinRChbRAMDTxXB4Y0Xkbp9mk47UTyNHDKlf3zuel4EiUCEaXt 3RyqwaUFYHz/RLiZQrNGDCCwF5ahic5Itp48ImdGTqKf7S9wVDnZyTQK7KxUA+A7/NF51T AGoHHGlLXZhvL9l3xkzahRQLW1E5m54= Received: by mail-wm1-f50.google.com with SMTP id 5b1f17b1804b1-427cede1e86so11832875e9.0 for ; Thu, 25 Jul 2024 17:43:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1721954635; x=1722559435; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=xYO5SO4GDvuXPDiqp7OtLZoWNem0w1AFsK/JqDBRgRw=; b=IDc78UyhcJ3bRbKeodSkMb51hiFWBPUENqKwQhBeRwrLdsyfS/lrHoY1YR7BZ+R8rj ni568nTQTDkvkg9ZJHBAnp64ajLmDopWAnQihWbp0Onrx3gu3bgCjZ+WIKMUSW7cRDqX rA8iofXVNmdxsej7am9ChdnaQPlCYrK5OSX8+WTLloRSBFLrAaif7Xhma+DNDZolmeNT mzWC/6IQxncBDKrci1t5afua0mmUhasM53QgaPAqZw94OETqwPDb/7IxIhnA1sYSg96H du8UataVSEifr5j4Ss4Odf2DAQ2swewQgUKQrMdX8p06ifXCC84e9hbIqIiEf/I9SvXv Uzjw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1721954635; x=1722559435; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=xYO5SO4GDvuXPDiqp7OtLZoWNem0w1AFsK/JqDBRgRw=; b=gkNyTbDjrqXkYvwTb34PPLRDKMu6AzbcolfCaWAjNmnMAmrpdoCSlHhE/UjLGrTI/t B3WWZMMmmHZQh2dsbhoTMlcF3B75RWLW/82XVUJvSS2y6YaTdaTHASokjOK5e7EQyCI2 lURtm20CEB7QH3VnP6JvGAJuNQnS5JkJritkUUNz1j+dvJ5+2ug4Id0r+grWy/9MiQKd 7PY8SCGIemY3hiQaJcOQWAyzjbcLiqGzyI2YeGy+BorH71KZMZe2GscOTULSX2LPCh2I GFXKZ4z6PMAEyBMM8+TpnL4MkPiTtpNLuWWIQYyP0OjpGqT95uy4T4Jw9kmy/CUio6pm vbWw== X-Forwarded-Encrypted: i=1; AJvYcCXEUF5smvPvH7gUqfzCfd2Ds+AquPHVWUKJRMiipClTovK6e/08ErJTyAaHPqG5y/Diya3HwIcW+Ojft/1077F8ccI= X-Gm-Message-State: AOJu0Yz5hVRpOA6vs3PeduBrI0BFz1mKUE1apeVmJWIvLIIWRnJgOUki I6TCqXb6WOXkJd/uD3WGwiuZncWlSXyBHgBi6YQzXjMeiVTSomuAhWg7+Q77gHFZ/GqA01AKnGE MgBBGPdRlxQyAjx925favHZOsCvE= X-Google-Smtp-Source: AGHT+IHXZA7vxZZe9vudcFJu5wQEoaTE/BEGgNP5tTIiap5Lqc5xK2XD+4YyiKwDFyG9zQzigP2NzwAoTFjmLpndzKk= X-Received: by 2002:a05:6000:112:b0:367:8a72:b8b4 with SMTP id ffacd0b85a97d-36b363a30b9mr2518002f8f.33.1721954634536; Thu, 25 Jul 2024 17:43:54 -0700 (PDT) MIME-Version: 1.0 References: <20240725-kasan-tsbrcu-v3-0-51c92f8f1101@google.com> <20240725-kasan-tsbrcu-v3-1-51c92f8f1101@google.com> In-Reply-To: <20240725-kasan-tsbrcu-v3-1-51c92f8f1101@google.com> From: Andrey Konovalov Date: Fri, 26 Jul 2024 02:43:43 +0200 Message-ID: Subject: Re: [PATCH v3 1/2] kasan: catch invalid free before SLUB reinitializes the object To: Jann Horn Cc: Andrey Ryabinin , Alexander Potapenko , Dmitry Vyukov , Vincenzo Frascino , Andrew Morton , Christoph Lameter , Pekka Enberg , David Rientjes , Joonsoo Kim , Vlastimil Babka , Roman Gushchin , Hyeonggon Yoo <42.hyeyoo@gmail.com>, Marco Elver , kasan-dev@googlegroups.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 3C4BC20011 X-Stat-Signature: i8dh91y6w3s8xe5idshu5rpcoksmrg1c X-Rspam-User: X-HE-Tag: 1721954636-273683 X-HE-Meta: U2FsdGVkX18Qem2YHXik+1yzATCXAGZViu2ciJwyS+JY9G9glVYZiip8DbEwgQlxhqW5zSHW7q/k8vMSIu5n74csVELYohbcSnsPqdM4wEIdwFOv4ucTuoNTkJOD5E4168Pw0KbYuXULlz3Mkem9MgRkuFsqxq3Oisn5HTA4j4tiq9J41EHjjRPpRgawtPzlbr6Cd13XsHksw1ZnC3XVYwFPT88+2SebzmwOuUnYdWeAhyNXjelnmJ1WkPRsjIVSwMBVmAZOb/C19dOaNupxZbIC7A4KInOX5tLw+EO9O8c8B9d+/IJjZPydF9jJZlDzwX7Jv76glHrer/1wUhQJQ+WyHpiypGWHKdM8Pp6YNkfsEpjTRjkqMqHIWsSFCSD5CqLZ3Q3zVDhY2S0B1VvDh1QXURwiGYP3p1lG7VWr87POHZoIYnRR8q1F0/FmK5m7cnme5nxO5mLZ5/T6+boFnZ56kgVX1AT4ODSJaPQ8T75P9IU9vSCO+9dPYF8aVrKkOxozot46yrTcfImxMZRqAUWlWoW0xRsFUIfV0pSy8fSkwa1PXOqwwTFpSJ0Fir97LzBJRj4oy9J3U6JM/Wk+PoBY+9yZ4vQx1psqBMeMsFxaHU518mYp+HuarZjeWjiEKAA79pcxszNYPYvJWsf32k0UMBQpsEFRxGsxyVOzvOzHlRmO2NvHhuDaQcUUFXhf6LDu4DYPS5GuMqVJ2/aEhG58Vm8F0h1MX/zMqTJIn0arzLM3fLBgSWp9rxUTukoTqZZxji+fQpc8ryF5B59DMedOY/7aIuNXREKdcAxctJCVPpHWkJnffy7jCnKrgsMv2cqJwsj4uVtdijJ4j/6//tO1701/jEQkXXnUW3y1NFx+B+te59/GDSgSqARzWwbGSovEcfnWmNpBQOO4hnKUr3BAhsfoTcU2b8jzCXAzApZDKrJGrfLO9K4sG9VgEi/BBWbZg0ahhA5BZMFx9Ch gm/eYHJN +QUXasgqaa9mEtk6oSSrX7cSOOaItWUDUjpWQiuD9q3m6bYmAHNMGdNL/c6TTLhKBobqGpzxWmPzAjtmr+XbpVR0bcnCKPz2cn5df0rY8hs88FUxjZNgsPPBxhd9kg+ETfLyMjju9vtH//k6qcbkL0TnZ2vKueOh41vi7C99qZJzAfxWyq6npLOU3w8yhuaHfxbpTOBxUF7IW8D4pVIZyH7YrWWwqSpEasXa2qXYNysppFZWjTt2raZdTZpaTxWWgPew6bNjNyMKsGaWZwVKiaR22C/MPZPeeaAFSSmlpNFDWtF5rf/vSbBF8YTtE28pQUW0QRqtcSgMVnnYO2NpkTzPBElqGJ6tTttOoy9Y+mC7GlLpSL1GZkViaDw1Jdq56x74g9OsD7PUvAtg= 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: List-Subscribe: List-Unsubscribe: On Thu, Jul 25, 2024 at 5:32=E2=80=AFPM Jann Horn wrote: > > Currently, when KASAN is combined with init-on-free behavior, the > initialization happens before KASAN's "invalid free" checks. > > More importantly, a subsequent commit will want to use the object metadat= a > region to store an rcu_head, and we should let KASAN check that the objec= t > pointer is valid before that. (Otherwise that change will make the existi= ng > testcase kmem_cache_invalid_free fail.) This is not the case since v3, right? Do we still need this patch? If it's still needed, see the comment below. Thank you! > So add a new KASAN hook that allows KASAN to pre-validate a > kmem_cache_free() operation before SLUB actually starts modifying the > object or its metadata. > > Acked-by: Vlastimil Babka #slub > Signed-off-by: Jann Horn > --- > include/linux/kasan.h | 16 ++++++++++++++++ > mm/kasan/common.c | 51 +++++++++++++++++++++++++++++++++++++++------= ------ > mm/slub.c | 7 +++++++ > 3 files changed, 62 insertions(+), 12 deletions(-) > > diff --git a/include/linux/kasan.h b/include/linux/kasan.h > index 70d6a8f6e25d..ebd93c843e78 100644 > --- a/include/linux/kasan.h > +++ b/include/linux/kasan.h > @@ -175,6 +175,16 @@ static __always_inline void * __must_check kasan_ini= t_slab_obj( > return (void *)object; > } > > +bool __kasan_slab_pre_free(struct kmem_cache *s, void *object, > + unsigned long ip); > +static __always_inline bool kasan_slab_pre_free(struct kmem_cache *s, > + void *object) > +{ > + if (kasan_enabled()) > + return __kasan_slab_pre_free(s, object, _RET_IP_); > + return false; > +} Please add a documentation comment for this new hook; something like what we have for kasan_mempool_poison_pages() and some of the others. (I've been meaning to add them for all of them, but still didn't get around to that.) > + > bool __kasan_slab_free(struct kmem_cache *s, void *object, > unsigned long ip, bool init); > static __always_inline bool kasan_slab_free(struct kmem_cache *s, > @@ -371,6 +381,12 @@ static inline void *kasan_init_slab_obj(struct kmem_= cache *cache, > { > return (void *)object; > } > + > +static inline bool kasan_slab_pre_free(struct kmem_cache *s, void *objec= t) > +{ > + return false; > +} > + > static inline bool kasan_slab_free(struct kmem_cache *s, void *object, b= ool init) > { > return false; > diff --git a/mm/kasan/common.c b/mm/kasan/common.c > index 85e7c6b4575c..7c7fc6ce7eb7 100644 > --- a/mm/kasan/common.c > +++ b/mm/kasan/common.c > @@ -208,31 +208,52 @@ void * __must_check __kasan_init_slab_obj(struct km= em_cache *cache, > return (void *)object; > } > > -static inline bool poison_slab_object(struct kmem_cache *cache, void *ob= ject, > - unsigned long ip, bool init) > +enum free_validation_result { > + KASAN_FREE_IS_IGNORED, > + KASAN_FREE_IS_VALID, > + KASAN_FREE_IS_INVALID > +}; > + > +static enum free_validation_result check_slab_free(struct kmem_cache *ca= che, > + void *object, unsigned lo= ng ip) > { > - void *tagged_object; > + void *tagged_object =3D object; > > - if (!kasan_arch_is_ready()) > - return false; > + if (is_kfence_address(object) || !kasan_arch_is_ready()) > + return KASAN_FREE_IS_IGNORED; > > - tagged_object =3D object; > object =3D kasan_reset_tag(object); > > if (unlikely(nearest_obj(cache, virt_to_slab(object), object) != =3D object)) { > kasan_report_invalid_free(tagged_object, ip, KASAN_REPORT= _INVALID_FREE); > - return true; > + return KASAN_FREE_IS_INVALID; > } > > - /* RCU slabs could be legally used after free within the RCU peri= od. */ > - if (unlikely(cache->flags & SLAB_TYPESAFE_BY_RCU)) > - return false; > - > if (!kasan_byte_accessible(tagged_object)) { > kasan_report_invalid_free(tagged_object, ip, KASAN_REPORT= _DOUBLE_FREE); > - return true; > + return KASAN_FREE_IS_INVALID; > } > > + return KASAN_FREE_IS_VALID; > +} > + > +static inline bool poison_slab_object(struct kmem_cache *cache, void *ob= ject, > + unsigned long ip, bool init) > +{ > + void *tagged_object =3D object; > + enum free_validation_result valid =3D check_slab_free(cache, obje= ct, ip); I believe we don't need check_slab_free() here, as it was already done in kasan_slab_pre_free()? Checking just kasan_arch_is_ready() and is_kfence_address() should save a bit on performance impact. Though if we remove check_slab_free() from here, we do need to add it to __kasan_mempool_poison_object(). > + > + if (valid =3D=3D KASAN_FREE_IS_IGNORED) > + return false; > + if (valid =3D=3D KASAN_FREE_IS_INVALID) > + return true; > + > + object =3D kasan_reset_tag(object); > + > + /* RCU slabs could be legally used after free within the RCU peri= od. */ > + if (unlikely(cache->flags & SLAB_TYPESAFE_BY_RCU)) > + return false; I vaguely recall there was some reason why this check was done before the kasan_byte_accessible() check, but I might be wrong. Could you try booting the kernel with only this patch applied to see if anything breaks? > + > kasan_poison(object, round_up(cache->object_size, KASAN_GRANULE_S= IZE), > KASAN_SLAB_FREE, init); > > @@ -242,6 +263,12 @@ static inline bool poison_slab_object(struct kmem_ca= che *cache, void *object, > return false; > } > > +bool __kasan_slab_pre_free(struct kmem_cache *cache, void *object, > + unsigned long ip) > +{ > + return check_slab_free(cache, object, ip) =3D=3D KASAN_FREE_IS_IN= VALID; > +} > + > bool __kasan_slab_free(struct kmem_cache *cache, void *object, > unsigned long ip, bool init) > { > diff --git a/mm/slub.c b/mm/slub.c > index 4927edec6a8c..34724704c52d 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -2170,6 +2170,13 @@ bool slab_free_hook(struct kmem_cache *s, void *x,= bool init) > if (kfence_free(x)) > return false; > > + /* > + * Give KASAN a chance to notice an invalid free operation before= we > + * modify the object. > + */ > + if (kasan_slab_pre_free(s, x)) > + return false; > + > /* > * As memory initialization might be integrated into KASAN, > * kasan_slab_free and initialization memset's must be > > -- > 2.45.2.1089.g2a221341d9-goog >