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 3608BC3DA4A for ; Fri, 2 Aug 2024 09:57:35 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id AA29C6B0089; Fri, 2 Aug 2024 05:57:34 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A79086B008A; Fri, 2 Aug 2024 05:57:34 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 940F86B008C; Fri, 2 Aug 2024 05:57:34 -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 70B806B0089 for ; Fri, 2 Aug 2024 05:57:34 -0400 (EDT) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 121F8141135 for ; Fri, 2 Aug 2024 09:57:34 +0000 (UTC) X-FDA: 82406853228.29.598291A Received: from mail-pl1-f172.google.com (mail-pl1-f172.google.com [209.85.214.172]) by imf20.hostedemail.com (Postfix) with ESMTP id 2DFAE1C0008 for ; Fri, 2 Aug 2024 09:57:30 +0000 (UTC) Authentication-Results: imf20.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=LT8M5cMj; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf20.hostedemail.com: domain of jannh@google.com designates 209.85.214.172 as permitted sender) smtp.mailfrom=jannh@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1722592586; 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=X0BnywGd6SV2Ld2s9hvxt+PjYwo9DGOXawX5efNb35U=; b=gL27yksObWF95spLZx1qAd1AEqMq8ys3XXeB6mSqLiwIAY9x2VhCrmUm+dxnsJ50S4tlC/ uttzakclfK/KMDus13ITJYIaDNcYMEQX3yStJBgFUX0qg6Q6CKdFrO3Q5NrI6Ler1jQoa6 On2tDIj/JwL73piXLk8xUayoFlen2fg= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1722592586; a=rsa-sha256; cv=none; b=xGjrsDCoJVZQRCnw4FoF8H2MjEcYOedkHVxdcNaWz+TAtlznTyV7gK5B6a/dpUwk2SXIZS Eg44x6wuNf1aOK6PYROqX57nbPNqvaGzZFQuqTDDXKdGiyLyr0QrZcI/DHbPPgCtaXBTXK De3unshJMS402p/86hNTSgrvy+OUgZc= ARC-Authentication-Results: i=1; imf20.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=LT8M5cMj; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf20.hostedemail.com: domain of jannh@google.com designates 209.85.214.172 as permitted sender) smtp.mailfrom=jannh@google.com Received: by mail-pl1-f172.google.com with SMTP id d9443c01a7336-1fd7509397bso531435ad.0 for ; Fri, 02 Aug 2024 02:57:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1722592650; x=1723197450; 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=X0BnywGd6SV2Ld2s9hvxt+PjYwo9DGOXawX5efNb35U=; b=LT8M5cMjGOOiRxOCxAnnFAUvwZnwO+W//cg+NWMeunKJce7qFdrRzABBsySHNnzuxo p6Y+2O4gxTtmP1BOr0GwXI9aQsisNmJNYtpj1vdy8+x9mEeoh7B3ERlSJwElu1tMdAhC +OmMup7IRsGHDxvh9b91J1ypGt+ZtggwNIHfM7RftT9TUnL98pra2u+RS8YOl23rlmJ5 TAG+FBPUgXLzHARLcjQ1EW9eHmQXzIJ5dMbsBRjRxjtccbdKwoUBs3jHnHF08D+NOP9t sq0fr+thKXYBJOFB+Fg/T1UZGxafNabaaU3007JYXGArCALd+cf/bTgLhaNvkj8kZgBZ 4SKA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1722592650; x=1723197450; 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=X0BnywGd6SV2Ld2s9hvxt+PjYwo9DGOXawX5efNb35U=; b=jSpmwdPvRhiXZqOpAx/thVtOrJbQuiA3LvAqUlTPWMlRcYdJg5YKw17d8v/SW2FNEF 1ZtK2xxi22bXt4tminKP30jBSl7YNtWuZS5+miGoEk5zhLMnw6ZLpU6qdl6Vmc/E7W7G FWEgv8PaQB4+yjTfj0i1f43sb8miQGBeM1wscVKNaD+ak5nBhJNARjDP9fBbdYCuGJHT PlsEViiNzMaBLwxO3xPvBLyp6LJa5a3vRGpG+R5y+u6cSmjpIXfKgxpRj8pyYImdCdvl Buga6oQZVV1RjoBP7v3VX0jaFj/9AI9dMqyy1Xa2m3/mSG1U2YBkHT9VbinKlV33uVL8 s08Q== X-Forwarded-Encrypted: i=1; AJvYcCWR69WYLx+u7F74JTA6LcACdLt0zXSKRF4WcRgDlU1bFGL9s5TJqTKEGX0gdl8bkm1+cyVPXZHeOrh/c9bmGkBS/vQ= X-Gm-Message-State: AOJu0Yz2JxX9vWEwdAyqi3zXNTueWWpq2wMaXRKgBfnBNuyjAqHbOEqV Vm3u70IzPidNxFr+n+Vg9V8NCoAXmd4elGNUoN8pmP1BzUP7m7LUATuwuYgJD0NO6v/uoyWKQE5 E0Zp7u5VmYpQiSHLrxV5pS3eWxsjnQFcZSXxz X-Google-Smtp-Source: AGHT+IFlUjJDdyS1eLzxkU2trpY0BKCJLsfCCbCLusbYdoOVCdXuf1gFzo6ObDZkLnHI4AYmtGhzt+maiiNTOQLWG+Q= X-Received: by 2002:a17:902:ec90:b0:1f7:3764:1e19 with SMTP id d9443c01a7336-1ff5cc4a18cmr1463485ad.20.1722592649125; Fri, 02 Aug 2024 02:57:29 -0700 (PDT) MIME-Version: 1.0 References: <20240730-kasan-tsbrcu-v5-0-48d3cbdfccc5@google.com> <20240730-kasan-tsbrcu-v5-1-48d3cbdfccc5@google.com> In-Reply-To: From: Jann Horn Date: Fri, 2 Aug 2024 11:56:50 +0200 Message-ID: Subject: Re: [PATCH v5 1/2] kasan: catch invalid free before SLUB reinitializes the object To: Andrey Konovalov 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: rspam07 X-Rspamd-Queue-Id: 2DFAE1C0008 X-Stat-Signature: d34gfmbypixjyz6m9rwh1z4mw8k1pqy3 X-Rspam-User: X-HE-Tag: 1722592650-792718 X-HE-Meta: U2FsdGVkX18qVjjn0PVH0UvZ8IOAUFq1lQbHUxwRyem8ttzjEEAW7Y+mT2YinKfVh0am6cd3rM7urzaavqrusiLk7sNpBJsCCaNDyjvJcOG/g6lR34uVbOShhT9aVSA8sNme0n4V53ydG9jX2umbQwcFjH8krwD10eojPzruzT1Q2kRuHBfczCx3tDznDW54crza3vfcuIF41807ItMCoyhl9uT/7NBRLUKfk7cm7VcheEsuPexyYomVCks59LuUzbbKF2lX4smN4WpG0Mc8ZelTiUBa/lpoxZphxFWFDRHzrgVwq56MgQ15NC6p340YRHB3QJ8xz/f2uL1F3bSUBenchSDi6R1e375rdYWMLfZj3R2Je5ZTIGI0LXisfq31+E9BEgegEULuJ6v0PQktzYO6qc9LSLdx737JFOK2hxvT1pQ3orUoa8tHWIczZ4NCi4Ak7vPz1r+OsAbeyzc3dzsx2Z122Qc9y0AjT4oBT4hrpHs9lgCufCDBQEjgTgkZzBMO387qlHVi0JyRe8z9T6bqCPf0xMbFkBkIK9Kv0dNzfchy/BMHehtr2Hq/IG4Mw02MiFHgBENu9mWQN5F1kK2bqCygWQGs2xkHNBpWVmAliF29PZebG6fyb4eUKW6cHPLYwDbprCkcUDSHXDJQGgiEo83ZnHeY23kCWdxrmcdoQNBmoYLTVpWrNfN8G4xpIvTvmr9GYf8AbS5pS2fS9mGkLvK/CTzYmpvVD9H/ygjlV1LnLkORLMOoxEvkRRhHATqysSLCKk5eJD0ZWUFIeV3ZJ2pZcwUDJrTy88ru+SK29pX/yPGzcGMvj0E77k59UNsTKyuwvyC4GjdkJ8ktjsSPtsdNcW9JwgVOLRtNOMjB6E6O8mUBRRtvZr4ePJfPVRZ7cY5gax2RodWIADT74zzonB/mPj7F28Qo7MD0noq0EnruylFFZrZk4qs+yiunZImTJMKQpUWpxasRfmV AQ2OyEb6 rUAnqBee3lZPWAyljqFASr88bmqAxzrCzzta0CUB8i5MFPkBbfp1OGUG/weB8DC/01ELY5/vG6c80GvNmTlJus3riv+t6fdnZXC1GnAoG/py6llWSBkyWBZpSOgngp1tLrZTtCVsMRodv2Uk1fOAsx9hb4bijyByqE1CRCih8V/acotVS7b/ljNHjGJgrNSk9kRHmntkeaGT4GWzGklYvKwhg5hSS9+WKMEso9LgQe3Vlp9v3D/VsKceMtd7m1yC3LrGprXWVhKIcLNaYc/ZH/3MCGPnK+C1XlgpHD/6BhWH2+KGknLmammwaOscAT/BU+Npi 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, Aug 1, 2024 at 2:23=E2=80=AFAM Andrey Konovalov wrote: > On Tue, Jul 30, 2024 at 1:06=E2=80=AFPM Jann Horn wrot= e: > > > > 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 RCU-delay the actual > > SLUB freeing of an object, and we'd like KASAN to still validate > > synchronously that freeing the object is permitted. (Otherwise this > > change will make the existing testcase kmem_cache_invalid_free fail.) > > > > 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. > > A few more minor comments below. With that: > > Reviewed-by: Andrey Konovalov > > Thank you! > > > Inside KASAN, this: > > > > - moves checks from poison_slab_object() into check_slab_free() > > - moves kasan_arch_is_ready() up into callers of poison_slab_object() > > - removes "ip" argument of poison_slab_object() and __kasan_slab_free(= ) > > (since those functions no longer do any reporting) > > > - renames check_slab_free() to check_slab_allocation() > > check_slab_allocation() is introduced in this patch, so technically > you don't rename anything. Right, I'll fix the commit message. > > Acked-by: Vlastimil Babka #slub > > Signed-off-by: Jann Horn > > --- > > include/linux/kasan.h | 43 ++++++++++++++++++++++++++++++++++--- > > mm/kasan/common.c | 59 +++++++++++++++++++++++++++++++------------= -------- > > mm/slub.c | 7 ++++++ > > 3 files changed, 83 insertions(+), 26 deletions(-) > > > > diff --git a/include/linux/kasan.h b/include/linux/kasan.h > > index 70d6a8f6e25d..34cb7a25aacb 100644 > > --- a/include/linux/kasan.h > > +++ b/include/linux/kasan.h > > @@ -172,19 +172,50 @@ static __always_inline void * __must_check kasan_= init_slab_obj( > > { > > if (kasan_enabled()) > > return __kasan_init_slab_obj(cache, object); > > return (void *)object; > > } > > > > -bool __kasan_slab_free(struct kmem_cache *s, void *object, > > - unsigned long ip, bool init); > > +bool __kasan_slab_pre_free(struct kmem_cache *s, void *object, > > + unsigned long ip); > > +/** > > + * kasan_slab_pre_free - Validate a slab object freeing request. > > + * @object: Object to free. > > + * > > + * This function checks whether freeing the given object might be perm= itted; it > > + * checks things like whether the given object is properly aligned and= not > > + * already freed. > > + * > > + * This function is only intended for use by the slab allocator. > > + * > > + * @Return true if freeing the object is known to be invalid; false ot= herwise. > > + */ > > Let's reword this to: > > kasan_slab_pre_free - Check whether freeing a slab object is safe. > @object: Object to be freed. > > This function checks whether freeing the given object is safe. It > performs checks to detect double-free and invalid-free bugs and > reports them. > > This function is intended only for use by the slab allocator. > > @Return true if freeing the object is not safe; false otherwise. Ack, will apply this for v6. But I'll replace "not safe" with "unsafe", and change "It performs checks to detect double-free and invalid-free bugs and reports them" to "It may check for double-free and invalid-free bugs and report them.", since KASAN only sometimes performs such checks (depending on CONFIG_KASAN, kasan_enabled(), kasan_arch_is_ready(), and so on). > > +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; > > +} > > + > > +bool __kasan_slab_free(struct kmem_cache *s, void *object, bool init); > > +/** > > + * kasan_slab_free - Possibly handle slab object freeing. > > + * @object: Object to free. > > + * > > + * This hook is called from the slab allocator to give KASAN a chance = to take > > + * ownership of the object and handle its freeing. > > + * kasan_slab_pre_free() must have already been called on the same obj= ect. > > + * > > + * @Return true if KASAN took ownership of the object; false otherwise= . > > + */ > > kasan_slab_free - Poison, initialize, and quarantine a slab object. > @object: Object to be freed. > @init: Whether to initialize the object. > > This function poisons a slab object and saves a free stack trace for > it, except for SLAB_TYPESAFE_BY_RCU caches. > > For KASAN modes that have integrated memory initialization > (kasan_has_integrated_init() =3D=3D true), this function also initializes > the object's memory. For other modes, the @init argument is ignored. As an aside: Is this actually reliably true? It would be false for kfence objects, but luckily we can't actually get kfence objects passed to this function (which I guess maybe we should maybe document here as part of the API). It would also be wrong if __kasan_slab_free() can be reached while kasan_arch_is_ready() is false, which I guess would happen if you ran a CONFIG_KASAN=3Dy kernel on a powerpc machine without radix or something like that? (And similarly I wonder if the check of kasan_has_integrated_init() in slab_post_alloc_hook() is racy, but I haven't checked in which phase of boot KASAN is enabled for HWASAN.) But I guess that's out of scope for this series. > For the Generic mode, this function might also quarantine the object. > When this happens, KASAN will defer freeing the object to a later > stage and handle it internally then. The return value indicates > whether the object was quarantined. > > This function is intended only for use by the slab allocator. > > @Return true if KASAN quarantined the object; false otherwise. Same thing as I wrote on patch 2/2: To me this seems like too much implementation detail for the documentation of an API between components of the kernel? I agree that the meaning of the "init" argument is important to document here, and it should be documented that the hook can take ownership of the object (and I guess it's fine to mention that this is for quarantine purposes), but I would leave out details about differences in behavior between KASAN modes. Basically my heuristic here is that in my opinion, this header comment should mostly describe as much of the function as SLUB has to know to properly use it. So I'd do something like: <<< kasan_slab_free - Poison, initialize, and quarantine a slab object. @object: Object to be freed. @init: Whether to initialize the object. This function informs that a slab object has been freed and is not supposed to be accessed anymore, except for objects in SLAB_TYPESAFE_BY_RCU caches. For KASAN modes that have integrated memory initialization (kasan_has_integrated_init() =3D=3D true), this function also initializes the object's memory. For other modes, the @init argument is ignored. This function might also take ownership of the object to quarantine it. When this happens, KASAN will defer freeing the object to a later stage and handle it internally until then. The return value indicates whether KASAN took ownership of the object. This function is intended only for use by the slab allocator. @Return true if KASAN took ownership of the object; false otherwise. >>> But if you disagree, I'll add your full comment as you suggested. [...] > > diff --git a/mm/kasan/common.c b/mm/kasan/common.c > > index 85e7c6b4575c..8cede1ce00e1 100644 > > --- a/mm/kasan/common.c > > +++ b/mm/kasan/common.c > > @@ -205,59 +205,65 @@ void * __must_check __kasan_init_slab_obj(struct = kmem_cache *cache, > > /* Tag is ignored in set_tag() without CONFIG_KASAN_SW/HW_TAGS = */ > > object =3D set_tag(object, assign_tag(cache, object, true)); > > > > return (void *)object; > > } > > > > -static inline bool poison_slab_object(struct kmem_cache *cache, void *= object, > > - unsigned long ip, bool init) > > +/* returns true for invalid request */ > > "Returns true when freeing the object is not safe." ack, applied