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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9133CC433F5 for ; Mon, 28 Feb 2022 11:25:44 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235832AbiB1L0V (ORCPT ); Mon, 28 Feb 2022 06:26:21 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51482 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235791AbiB1L0J (ORCPT ); Mon, 28 Feb 2022 06:26:09 -0500 Received: from mail-yw1-x1132.google.com (mail-yw1-x1132.google.com [IPv6:2607:f8b0:4864:20::1132]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0530D710CC for ; Mon, 28 Feb 2022 03:25:29 -0800 (PST) Received: by mail-yw1-x1132.google.com with SMTP id 00721157ae682-2d6923bca1aso104049157b3.9 for ; Mon, 28 Feb 2022 03:25:28 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=p96IvP9fUv4h1PakADfoqUYPHYxVnqzVwq2/kj5L+eU=; b=KeqMjQGcDStWNcUS49dVbtvAJpxlSiOVErm61ZtH3VI5hAlAj0vQ1zcHquzneX3cK7 neNjKRdIJoiS6BtbKzb2YPS5nMEe8m5YxMTC5lR8kyZFI3AO2veXAPK8lwntA20ztYMd KwUOA9e03fmOZ7HBoLWjb7t05sZeU/W4/KDzfTVTyKgA+MnDe1LSwmaphusTfs1ef6fK of3KNhlrrVLJq9O7wpEVW9QSv8MmTwUXBvSrPyyIb375h4xyL7mvnPWA5QyNw68S+Ep9 h8+rf2NLd5Ip6RKVm7BOKl3zCDQHJWQQ+5s3ZGb3VbTOTSOB/sLncqcMr94bCvJDt0gD xv+A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=p96IvP9fUv4h1PakADfoqUYPHYxVnqzVwq2/kj5L+eU=; b=P/WuvcaZhqh4dL5EHMM/JLOMPeu5yRbo04XXyJdqGzmGJcZIBiwMG0KKxeuwII/Ygq 654lRxd0d3jSUxclnMsoKFSZTVA2cgHe5Ctf28Dfv1Sfv5iVMplcWy57D64S+8OE6s5E HpEi6djclg4pl9d9bD96PmYzQ9Taref5SHnAlX0R8Yq0pOAI+kJPLNkRPoWeyvb6o6tQ r6EFa+NmsRZKOZtCmTV50WWScK1RB7IMAip8C3lxZx/aI2kaIG493dKMs0LkrY/WX4N4 JFmfZ2SN+Z+jSv8kiG0hxeJ7C/6WdQh6NbOd5n9otPzvyjvJ+GjQ87GVWiL9i0+49Eyi nBSg== X-Gm-Message-State: AOAM531mxoONZIz68xlOLV/3HbkS8OkUQMyhLGvrJwdU2h15FpeNuQsr jG3E4GOtlcsBjDoE09QV9Nt2uNtBmiBkYvaxqw2x/Q== X-Google-Smtp-Source: ABdhPJz54dXk6AW8cnKh1rdgLlHMk+YG/fbrfpJzpZACyNSQ2YmjarYCbN3mfTc15EcGKVu9pa7aHfMqg5A3HN7vpFE= X-Received: by 2002:a81:7943:0:b0:2d0:c8bb:a45d with SMTP id u64-20020a817943000000b002d0c8bba45dmr18980716ywc.264.1646047527866; Mon, 28 Feb 2022 03:25:27 -0800 (PST) MIME-Version: 1.0 References: <20220225221625.3531852-1-keescook@chromium.org> In-Reply-To: <20220225221625.3531852-1-keescook@chromium.org> From: Marco Elver Date: Mon, 28 Feb 2022 12:24:51 +0100 Message-ID: Subject: Re: [PATCH] mm: Handle ksize() vs __alloc_size by forgetting size To: Kees Cook Cc: llvm@lists.linux.dev, Pekka Enberg , David Rientjes , Joonsoo Kim , Andrew Morton , Vlastimil Babka , linux-mm@kvack.org, stable@vger.kernel.org, Greg Kroah-Hartman , "Rafael J. Wysocki" , Christoph Lameter , Nathan Chancellor , Nick Desaulniers , Daniel Micay , linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 25 Feb 2022 at 23:16, Kees Cook wrote: > > If ksize() is used on an allocation, the compiler cannot make any > assumptions about its size any more (as hinted by __alloc_size). Force > it to forget. > > One caller was using a container_of() construction that needed to be > worked around. > > Cc: Marco Elver > Cc: Pekka Enberg > Cc: David Rientjes > Cc: Joonsoo Kim > Cc: Andrew Morton > Cc: Vlastimil Babka > Cc: linux-mm@kvack.org > Link: https://github.com/ClangBuiltLinux/linux/issues/1599 > Fixes: c37495d6254c ("slab: add __alloc_size attributes for better bounds checking") > Cc: stable@vger.kernel.org > Signed-off-by: Kees Cook > --- > This appears to work for me, but I'm waiting for more feedback on > the specific instance got tripped over in Android. > --- > drivers/base/devres.c | 4 +++- > include/linux/slab.h | 26 +++++++++++++++++++++++++- > mm/slab_common.c | 19 +++---------------- > 3 files changed, 31 insertions(+), 18 deletions(-) > > diff --git a/drivers/base/devres.c b/drivers/base/devres.c > index eaa9a5cd1db9..1a2645bd7234 100644 > --- a/drivers/base/devres.c > +++ b/drivers/base/devres.c > @@ -855,6 +855,7 @@ void *devm_krealloc(struct device *dev, void *ptr, size_t new_size, gfp_t gfp) > size_t total_new_size, total_old_size; > struct devres *old_dr, *new_dr; > unsigned long flags; > + void *allocation; > > if (unlikely(!new_size)) { > devm_kfree(dev, ptr); > @@ -874,7 +875,8 @@ void *devm_krealloc(struct device *dev, void *ptr, size_t new_size, gfp_t gfp) > if (!check_dr_size(new_size, &total_new_size)) > return NULL; > > - total_old_size = ksize(container_of(ptr, struct devres, data)); > + allocation = container_of(ptr, struct devres, data); > + total_old_size = ksize(allocation); > if (total_old_size == 0) { > WARN(1, "Pointer doesn't point to dynamically allocated memory."); > return NULL; > diff --git a/include/linux/slab.h b/include/linux/slab.h > index 37bde99b74af..a14f3bfa2f44 100644 > --- a/include/linux/slab.h > +++ b/include/linux/slab.h > @@ -182,8 +182,32 @@ int kmem_cache_shrink(struct kmem_cache *s); > void * __must_check krealloc(const void *objp, size_t new_size, gfp_t flags) __alloc_size(2); > void kfree(const void *objp); > void kfree_sensitive(const void *objp); > + > +/** > + * ksize - get the actual amount of memory allocated for a given object > + * @objp: Pointer to the object > + * > + * kmalloc may internally round up allocations and return more memory > + * than requested. ksize() can be used to determine the actual amount of > + * memory allocated. The caller may use this additional memory, even though > + * a smaller amount of memory was initially specified with the kmalloc call. > + * The caller must guarantee that objp points to a valid object previously > + * allocated with either kmalloc() or kmem_cache_alloc(). The object > + * must not be freed during the duration of the call. > + * > + * Return: size of the actual memory used by @objp in bytes > + */ > +#define ksize(objp) ({ \ > + /* \ > + * Getting the actual allocation size means the __alloc_size \ > + * hints are no longer valid, and the compiler needs to \ > + * forget about them. \ > + */ \ > + OPTIMIZER_HIDE_VAR(objp); \ > + _ksize(objp); \ > +}) So per that ClangBuiltLinux issue I'm gleaning that the __alloc_size annotations are actually causing the compiler to generate wrong code? Possibly due to the compiler thinking that the accesses must stay within some bound, and anything beyond that will be "undefined behaviour"? Clearly, per the slab APIs, in particular with the provision of ksize(), the compiler is wrong. At first I thought this was only related to UBSAN bounds checking generating false positives, in which case a simple workaround as you present above would probably take care of most cases. But if the real issue is the compiler suddenly doing more aggressive compiler optimizations because it thinks accesses beyond the object size (per __alloc_size) is UB, but UB can never happen, and thus does crazy things [1], I think the answer (at least with what we have right now) should be to find a different solution that is more reliable. [1] https://lore.kernel.org/all/20220218131358.3032912-1-gregkh@linuxfoundation.org/ Because who's to say that there's not some code that does: foo = kmalloc(...); ... bar = foo; s = ksize(bar); ... makes access address-dependent on 's' and 'foo' (but not 'bar') ... This doesn't look like code anyone would write, but I fear with enough macro and inline function magic, it's not too unlikely. I can see a few options: 1. Dropping __alloc_size. 2. Somehow statically computing the size-class's size (kmalloc_index() might help here), removing __alloc_size from allocation functions and instead use some wrapper. 3. Teaching the compiler to drop *all* object sizes upon encountering a ksize(). So I think #1 is probably not what you want. #2 seems quite complicated, and in many cases likely too relaxed and would miss bugs, so also not ideal. #3 would be the most reliable, but OPTIMIZER_HIDE_VAR() doesn't cut it, and we need something stronger. The downside of #3 is that it might pessimize code generation, but given ksize() is used sparingly, might be ok. Thanks, -- Marco