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=-11.4 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SIGNED_OFF_BY,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable 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 B195EC43457 for ; Thu, 15 Oct 2020 02:36:12 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 0512A2227E for ; Thu, 15 Oct 2020 02:36:11 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="KBsabh+m" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0512A2227E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 65B7F6B0062; Wed, 14 Oct 2020 22:36:11 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 61127900002; Wed, 14 Oct 2020 22:36:11 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 54ABC6B006E; Wed, 14 Oct 2020 22:36:11 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0121.hostedemail.com [216.40.44.121]) by kanga.kvack.org (Postfix) with ESMTP id 27E956B0062 for ; Wed, 14 Oct 2020 22:36:11 -0400 (EDT) Received: from smtpin22.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id AC4901EF1 for ; Thu, 15 Oct 2020 02:36:10 +0000 (UTC) X-FDA: 77372595300.22.space26_5207aab27210 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin22.hostedemail.com (Postfix) with ESMTP id 8A53A18038E60 for ; Thu, 15 Oct 2020 02:36:10 +0000 (UTC) X-HE-Tag: space26_5207aab27210 X-Filterd-Recvd-Size: 6927 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by imf45.hostedemail.com (Postfix) with ESMTP for ; Thu, 15 Oct 2020 02:36:10 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1602729369; h=from:from: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; bh=A4nV7WKozM0AOIQ1QdjvaOOj/choWDPKz6rzFA68jkM=; b=KBsabh+mxtvHRrpLIo5n/JoNUYIfr1Snz4erb8h/Y4F7syWk66rJsfdRV+JC/NWXzGXi3F GJKsfz4PRFQhH94ntIo+HvPeLRMnArEsOtLouFRA3Acfnr7btshYMPmtEAMwjm4Qiyq4VI BUjLSxQTo4AYdT6hSBOOqLwu0XcXpwg= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-301-aAUQ-D-8MkOG-RPN0FIymw-1; Wed, 14 Oct 2020 22:36:05 -0400 X-MC-Unique: aAUQ-D-8MkOG-RPN0FIymw-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id ADE69803620; Thu, 15 Oct 2020 02:36:03 +0000 (UTC) Received: from llong.remote.csb (ovpn-120-0.rdu2.redhat.com [10.10.120.0]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5EB8C27CC1; Thu, 15 Oct 2020 02:36:02 +0000 (UTC) Subject: Re: [PATCH] slub: Actually fix freelist pointer vs redzoning To: Kees Cook , Andrew Morton Cc: Marco Elver , stable@vger.kernel.org, Pekka Enberg , Christoph Lameter , David Rientjes , Joonsoo Kim , linux-kernel@vger.kernel.org, linux-mm@kvack.org References: <20201008233443.3335464-1-keescook@chromium.org> From: Waiman Long Organization: Red Hat Message-ID: Date: Wed, 14 Oct 2020 22:36:01 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <20201008233443.3335464-1-keescook@chromium.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 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/8/20 7:34 PM, Kees Cook wrote: > It turns out that SLUB redzoning ("slub_debug=Z") checks from > s->object_size rather than from s->inuse (which is normally bumped to > make room for the freelist pointer), so a cache created with an object > size less than 24 would have their freelist pointer written beyond > s->object_size, causing the redzone to corrupt the freelist pointer. > This was very visible with "slub_debug=ZF": > > BUG test (Tainted: G B ): Redzone overwritten > ----------------------------------------------------------------------------- > > INFO: 0xffff957ead1c05de-0xffff957ead1c05df @offset=1502. First byte 0x1a instead of 0xbb > INFO: Slab 0xffffef3950b47000 objects=170 used=170 fp=0x0000000000000000 flags=0x8000000000000200 > INFO: Object 0xffff957ead1c05d8 @offset=1496 fp=0xffff957ead1c0620 > > Redzone (____ptrval____): bb bb bb bb bb bb bb bb ........ > Object (____ptrval____): 00 00 00 00 00 f6 f4 a5 ........ > Redzone (____ptrval____): 40 1d e8 1a aa @.... > Padding (____ptrval____): 00 00 00 00 00 00 00 00 ........ > > Adjust the offset to stay within s->object_size. > > (Note that there appear to be no such small-sized caches in the kernel > currently.) > > Reported-by: Marco Elver > Link: https://lore.kernel.org/linux-mm/20200807160627.GA1420741@elver.google.com/ > Fixes: 89b83f282d8b (slub: avoid redzone when choosing freepointer location) > Cc: stable@vger.kernel.org > Signed-off-by: Kees Cook > --- > mm/slub.c | 17 +++++------------ > 1 file changed, 5 insertions(+), 12 deletions(-) > > diff --git a/mm/slub.c b/mm/slub.c > index 68c02b2eecd9..979f5da26992 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -3641,7 +3641,6 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order) > { > slab_flags_t flags = s->flags; > unsigned int size = s->object_size; > - unsigned int freepointer_area; > unsigned int order; > > /* > @@ -3650,13 +3649,6 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order) > * the possible location of the free pointer. > */ > size = ALIGN(size, sizeof(void *)); > - /* > - * This is the area of the object where a freepointer can be > - * safely written. If redzoning adds more to the inuse size, we > - * can't use that portion for writing the freepointer, so > - * s->offset must be limited within this for the general case. > - */ > - freepointer_area = size; > > #ifdef CONFIG_SLUB_DEBUG > /* > @@ -3682,7 +3674,7 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order) > > /* > * With that we have determined the number of bytes in actual use > - * by the object. This is the potential offset to the free pointer. > + * by the object and redzoning. > */ > s->inuse = size; > > @@ -3694,7 +3686,8 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order) > * kmem_cache_free. > * > * This is the case if we do RCU, have a constructor or > - * destructor or are poisoning the objects. > + * destructor, are poisoning the objects, or are > + * redzoning an object smaller than sizeof(void *). > * > * The assumption that s->offset >= s->inuse means free > * pointer is outside of the object is used in the There is no check to go into this if condition to put free pointer after object when redzoning an object smaller than sizeof(void *). In that sense, the additional comment isn't correct. Should we add that check even though we don't have slab objects with such a small size in the kernel? > @@ -3703,13 +3696,13 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order) > */ > s->offset = size; > size += sizeof(void *); > - } else if (freepointer_area > sizeof(void *)) { > + } else { > /* > * Store freelist pointer near middle of object to keep > * it away from the edges of the object to avoid small > * sized over/underflows from neighboring allocations. > */ > - s->offset = ALIGN(freepointer_area / 2, sizeof(void *)); > + s->offset = ALIGN_DOWN(s->object_size / 2, sizeof(void *)); > } > > #ifdef CONFIG_SLUB_DEBUG Other than the above, the patch looks good. Cheers, Longman