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 5252BC433FE for ; Thu, 10 Nov 2022 15:48:39 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 80AD36B0071; Thu, 10 Nov 2022 10:48:38 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 7938B6B0072; Thu, 10 Nov 2022 10:48:38 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 60CFB6B0074; Thu, 10 Nov 2022 10:48:38 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 4BE266B0071 for ; Thu, 10 Nov 2022 10:48:38 -0500 (EST) Received: from smtpin19.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 2327840AD4 for ; Thu, 10 Nov 2022 15:48:38 +0000 (UTC) X-FDA: 80117965116.19.0F034B6 Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by imf14.hostedemail.com (Postfix) with ESMTP id 8A0EE100003 for ; Thu, 10 Nov 2022 15:48:37 +0000 (UTC) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id 43F6222999; Thu, 10 Nov 2022 15:48:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1668095316; h=from:from:reply-to: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; bh=J3QJBtM4MnGOZMLT0ADfXmiVyB/zENfV6p3Drb9JBbs=; b=zskpez+6BQephEy8Wiz2FE8+VfHE1YRC6JFOsQBrpgPQLxjLzEyyNfD4Qb/lumHY7QY1mG H5J5YxQhfYbZKzWbK42esgKfIYy6gP1/FLU7/R4mGjTeakgOmouJJHt7Xo8WZW4gd8o29E 4BAwpaejFVK06fWHta3gQmw9Z5zuHpQ= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1668095316; h=from:from:reply-to: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; bh=J3QJBtM4MnGOZMLT0ADfXmiVyB/zENfV6p3Drb9JBbs=; b=zwMcF9Q1r+FQdJIrJ4Nnhm384FgU1dof6X0sojrpKvcnpH0y+ET6p18GKsfQf0rTDs7J4u CP++s1rqSCnW+hDA== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 047A813B58; Thu, 10 Nov 2022 15:48:36 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id yQlpAFQdbWPjUgAAMHmgww (envelope-from ); Thu, 10 Nov 2022 15:48:36 +0000 Message-ID: Date: Thu, 10 Nov 2022 16:48:35 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.1 Subject: Re: [PATCH v7 3/3] mm/slub: extend redzone check to extra allocated kmalloc space than requested Content-Language: en-US To: Feng Tang , Andrew Morton , Christoph Lameter , Pekka Enberg , David Rientjes , Joonsoo Kim , Roman Gushchin , Hyeonggon Yoo <42.hyeyoo@gmail.com>, Dmitry Vyukov , Andrey Konovalov , Kees Cook Cc: Dave Hansen , linux-mm@kvack.org, linux-kernel@vger.kernel.org, kasan-dev@googlegroups.com References: <20221021032405.1825078-1-feng.tang@intel.com> <20221021032405.1825078-4-feng.tang@intel.com> From: Vlastimil Babka In-Reply-To: <20221021032405.1825078-4-feng.tang@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1668095317; a=rsa-sha256; cv=none; b=JLIi5JRtQ2oKTiiNpTAjeE9xJCODeb9ovxS9uFi0MhgZEqBezVe9qABfaT710TfCjukG/0 9zzbQQv6y4hX6upaOLk8J7AoyygPbQnffG09p14914QZnSSB1LTtsS4+Kxy6WwdfFNHWVf YIbwJbzVe5lUTxhFpS8Ur8TaEtjnbUE= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=zskpez+6; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=zwMcF9Q1; dmarc=none; spf=pass (imf14.hostedemail.com: domain of vbabka@suse.cz designates 195.135.220.28 as permitted sender) smtp.mailfrom=vbabka@suse.cz ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1668095317; 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=J3QJBtM4MnGOZMLT0ADfXmiVyB/zENfV6p3Drb9JBbs=; b=I2Mbe415yB0Zy4pZpB/wrlxLbpShYlcdpm0ddM7gYi3SvVvB4oXB6isfTxk5aSpO1sUfgG ll4Go+vPZa/VB2MpRhvm3ln9tAyhbqHWyo1WaPXAcn4+upUOpMsg+j/qb5C9La3/m3+ssX wR5x0OZZkPg7kbSqss+hPqlz9aqKMwI= X-Rspamd-Queue-Id: 8A0EE100003 Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=zskpez+6; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=zwMcF9Q1; dmarc=none; spf=pass (imf14.hostedemail.com: domain of vbabka@suse.cz designates 195.135.220.28 as permitted sender) smtp.mailfrom=vbabka@suse.cz X-Rspam-User: X-Rspamd-Server: rspam01 X-Stat-Signature: fapsb1kx9q9uidnpe35w7c9pfkwtx67w X-HE-Tag: 1668095317-524242 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 10/21/22 05:24, Feng Tang wrote: > kmalloc will round up the request size to a fixed size (mostly power > of 2), so there could be a extra space than what is requested, whose > size is the actual buffer size minus original request size. > > To better detect out of bound access or abuse of this space, add > redzone sanity check for it. > > In current kernel, some kmalloc user already knows the existence of > the space and utilizes it after calling 'ksize()' to know the real > size of the allocated buffer. So we skip the sanity check for objects > which have been called with ksize(), as treating them as legitimate > users. Hm so once Kees's effort is finished and all ksize() users behave correctly, we can drop all that skip_orig_size_check() code, right? > In some cases, the free pointer could be saved inside the latter > part of object data area, which may overlap the redzone part(for > small sizes of kmalloc objects). As suggested by Hyeonggon Yoo, > force the free pointer to be in meta data area when kmalloc redzone > debug is enabled, to make all kmalloc objects covered by redzone > check. > > Suggested-by: Vlastimil Babka > Signed-off-by: Feng Tang > Acked-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> Looks fine, but a suggestion below: > --- > mm/slab.h | 4 ++++ > mm/slab_common.c | 4 ++++ > mm/slub.c | 51 ++++++++++++++++++++++++++++++++++++++++++++---- > 3 files changed, 55 insertions(+), 4 deletions(-) > > diff --git a/mm/slab.h b/mm/slab.h > index 8b4ee02fc14a..1dd773afd0c4 100644 > --- a/mm/slab.h > +++ b/mm/slab.h > @@ -885,4 +885,8 @@ void __check_heap_object(const void *ptr, unsigned long n, > } > #endif > > +#ifdef CONFIG_SLUB_DEBUG > +void skip_orig_size_check(struct kmem_cache *s, const void *object); > +#endif > + > #endif /* MM_SLAB_H */ > diff --git a/mm/slab_common.c b/mm/slab_common.c > index 33b1886b06eb..0bb4625f10a2 100644 > --- a/mm/slab_common.c > +++ b/mm/slab_common.c > @@ -1037,6 +1037,10 @@ size_t __ksize(const void *object) > return folio_size(folio); > } > > +#ifdef CONFIG_SLUB_DEBUG > + skip_orig_size_check(folio_slab(folio)->slab_cache, object); > +#endif > + > return slab_ksize(folio_slab(folio)->slab_cache); > } > > diff --git a/mm/slub.c b/mm/slub.c > index adff7553b54e..76581da6b9df 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -829,6 +829,17 @@ static inline void set_orig_size(struct kmem_cache *s, > if (!slub_debug_orig_size(s)) > return; > > +#ifdef CONFIG_KASAN_GENERIC > + /* > + * KASAN could save its free meta data in object's data area at > + * offset 0, if the size is larger than 'orig_size', it will > + * overlap the data redzone in [orig_size+1, object_size], and > + * the check should be skipped. > + */ > + if (kasan_metadata_size(s, true) > orig_size) > + orig_size = s->object_size; > +#endif > + > p += get_info_end(s); > p += sizeof(struct track) * 2; > > @@ -848,6 +859,11 @@ static inline unsigned int get_orig_size(struct kmem_cache *s, void *object) > return *(unsigned int *)p; > } > > +void skip_orig_size_check(struct kmem_cache *s, const void *object) > +{ > + set_orig_size(s, (void *)object, s->object_size); > +} > + > static void slab_bug(struct kmem_cache *s, char *fmt, ...) > { > struct va_format vaf; > @@ -966,13 +982,27 @@ static __printf(3, 4) void slab_err(struct kmem_cache *s, struct slab *slab, > static void init_object(struct kmem_cache *s, void *object, u8 val) > { > u8 *p = kasan_reset_tag(object); > + unsigned int orig_size = s->object_size; > > - if (s->flags & SLAB_RED_ZONE) > + if (s->flags & SLAB_RED_ZONE) { > memset(p - s->red_left_pad, val, s->red_left_pad); > > + if (slub_debug_orig_size(s) && val == SLUB_RED_ACTIVE) { > + orig_size = get_orig_size(s, object); > + > + /* > + * Redzone the extra allocated space by kmalloc > + * than requested. > + */ > + if (orig_size < s->object_size) > + memset(p + orig_size, val, > + s->object_size - orig_size); Wondering if we can remove this if - memset and instead below: > + } > + } > + > if (s->flags & __OBJECT_POISON) { > - memset(p, POISON_FREE, s->object_size - 1); > - p[s->object_size - 1] = POISON_END; > + memset(p, POISON_FREE, orig_size - 1); > + p[orig_size - 1] = POISON_END; > } > > if (s->flags & SLAB_RED_ZONE) This continues by: memset(p + s->object_size, val, s->inuse - s->object_size); Instead we could do this, no? memset(p + orig_size, val, s->inuse - orig_size); > @@ -1120,6 +1150,7 @@ static int check_object(struct kmem_cache *s, struct slab *slab, > { > u8 *p = object; > u8 *endobject = object + s->object_size; > + unsigned int orig_size; > > if (s->flags & SLAB_RED_ZONE) { > if (!check_bytes_and_report(s, slab, object, "Left Redzone", > @@ -1129,6 +1160,17 @@ static int check_object(struct kmem_cache *s, struct slab *slab, > if (!check_bytes_and_report(s, slab, object, "Right Redzone", > endobject, val, s->inuse - s->object_size)) > return 0; > + > + if (slub_debug_orig_size(s) && val == SLUB_RED_ACTIVE) { > + orig_size = get_orig_size(s, object); > + > + if (s->object_size > orig_size && > + !check_bytes_and_report(s, slab, object, > + "kmalloc Redzone", p + orig_size, > + val, s->object_size - orig_size)) { > + return 0; > + } > + } > } else { > if ((s->flags & SLAB_POISON) && s->object_size < s->inuse) { > check_bytes_and_report(s, slab, p, "Alignment padding", > @@ -4206,7 +4248,8 @@ static int calculate_sizes(struct kmem_cache *s) > */ > s->inuse = size; > > - if ((flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON)) || > + if (slub_debug_orig_size(s) || > + (flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON)) || > ((flags & SLAB_RED_ZONE) && s->object_size < sizeof(void *)) || > s->ctor) { > /*