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.2 required=3.0 tests=BAYES_00, 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=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 31835C433E7 for ; Tue, 13 Oct 2020 16:44:20 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 6D277252C9 for ; Tue, 13 Oct 2020 16:44:19 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6D277252C9 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id A6AEB940008; Tue, 13 Oct 2020 12:44:18 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A424C900002; Tue, 13 Oct 2020 12:44:18 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 97E9A940008; Tue, 13 Oct 2020 12:44:18 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0109.hostedemail.com [216.40.44.109]) by kanga.kvack.org (Postfix) with ESMTP id 8CE99900002 for ; Tue, 13 Oct 2020 12:44:18 -0400 (EDT) Received: from smtpin12.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 5F56F180AD817 for ; Tue, 13 Oct 2020 16:44:18 +0000 (UTC) X-FDA: 77367474996.12.curve36_4c1007a27204 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin12.hostedemail.com (Postfix) with ESMTP id 36310180555D0 for ; Tue, 13 Oct 2020 16:44:18 +0000 (UTC) X-HE-Tag: curve36_4c1007a27204 X-Filterd-Recvd-Size: 5556 Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by imf08.hostedemail.com (Postfix) with ESMTP for ; Tue, 13 Oct 2020 16:44:17 +0000 (UTC) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 694EBAB0E; Tue, 13 Oct 2020 16:44:16 +0000 (UTC) To: Kees Cook , Andrew Morton Cc: Marco Elver , Jonathan Corbet , Christoph Lameter , Pekka Enberg , David Rientjes , Joonsoo Kim , Roman Gushchin , linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, linux-mm@kvack.org References: <20201009195411.4018141-1-keescook@chromium.org> <20201009195411.4018141-4-keescook@chromium.org> From: Vlastimil Babka Subject: Re: [PATCH v2 3/3] mm/slub: Actually fix freelist pointer vs redzoning Message-ID: <0f7dd7b2-7496-5e2d-9488-2ec9f8e90441@suse.cz> Date: Tue, 13 Oct 2020 18:44:15 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.3.2 MIME-Version: 1.0 In-Reply-To: <20201009195411.4018141-4-keescook@chromium.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US 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 10/9/20 9:54 PM, Kees Cook wrote: > It turns out that SLUB redzoning ("slub_debug=3DZ") 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. Is this accurate? Seems to me that redzone is (re)initialized only when=20 freepointer is not active. So it is actually freelist pointer corrupting = the=20 redzone... > This was very visible with "slub_debug=3DZF": ... as this report shows :) >=20 > BUG test (Tainted: G B ): Right Redzone overwritten > -----------------------------------------------------------------------= ------ >=20 > INFO: 0xffff957ead1c05de-0xffff957ead1c05df @offset=3D1502. First byte = 0x1a instead of 0xbb > INFO: Slab 0xffffef3950b47000 objects=3D170 used=3D170 fp=3D0x000000000= 0000000 flags=3D0x8000000000000200 > INFO: Object 0xffff957ead1c05d8 @offset=3D1496 fp=3D0xffff957ead1c0620 >=20 > 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 ......= .. >=20 > Adjust the offset to stay within s->object_size. >=20 > Reported-by: Marco Elver > Link: https://lore.kernel.org/linux-mm/20200807160627.GA1420741@elver.g= oogle.com/ > Fixes: 89b83f282d8b (slub: avoid redzone when choosing freepointer loca= tion) > Tested-by: Marco Elver > Link: https://lore.kernel.org/lkml/CANpmjNOwZ5VpKQn+SYWovTkFB4VsT-RPwyE= NBmaK0dLcpqStkA@mail.gmail.com > Signed-off-by: Kees Cook Acked-by: Vlastimil Babka This struggle to get it right perhaps calls for some selftests of all=20 combinations of flags that affect object layout, e.g. of=20 redzone/poison/store_user, on sizes from sizeof(void *) to e.g. 3*sizeof(= void=20 *), with sanity_checks enabled. Shouldn't be too many tests... > --- > mm/slub.c | 14 +++----------- > 1 file changed, 3 insertions(+), 11 deletions(-) >=20 > diff --git a/mm/slub.c b/mm/slub.c > index 752fad36522c..6f115e56c5d0 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -3637,7 +3637,6 @@ static int calculate_sizes(struct kmem_cache *s, = int forced_order) > { > slab_flags_t flags =3D s->flags; > unsigned int size =3D s->object_size; > - unsigned int freepointer_area; > unsigned int order; > =20 > /* > @@ -3646,13 +3645,6 @@ static int calculate_sizes(struct kmem_cache *s,= int forced_order) > * the possible location of the free pointer. > */ > size =3D 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 =3D size; > =20 > #ifdef CONFIG_SLUB_DEBUG > /* > @@ -3678,7 +3670,7 @@ static int calculate_sizes(struct kmem_cache *s, = int forced_order) > =20 > /* > * 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 =3D size; > =20 > @@ -3701,13 +3693,13 @@ static int calculate_sizes(struct kmem_cache *s= , int forced_order) > */ > s->offset =3D size; > size +=3D 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 =3D ALIGN(freepointer_area / 2, sizeof(void *)); > + s->offset =3D ALIGN_DOWN(s->object_size / 2, sizeof(void *)); > } > =20 > #ifdef CONFIG_SLUB_DEBUG >=20