From: Kees Cook <keescook@chromium.org>
To: Vlastimil Babka <vbabka@suse.cz>
Cc: "Christoph Lameter" <cl@linux.com>,
"Pekka Enberg" <penberg@kernel.org>,
"David Rientjes" <rientjes@google.com>,
"Joonsoo Kim" <iamjoonsoo.kim@lge.com>,
"Andrew Morton" <akpm@linux-foundation.org>,
linux-mm@kvack.org, "Ruhl, Michael J" <michael.j.ruhl@intel.com>,
"Hyeonggon Yoo" <42.hyeyoo@gmail.com>,
"David S. Miller" <davem@davemloft.net>,
"Eric Dumazet" <edumazet@google.com>,
"Jakub Kicinski" <kuba@kernel.org>,
"Paolo Abeni" <pabeni@redhat.com>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Nick Desaulniers" <ndesaulniers@google.com>,
"Alex Elder" <elder@kernel.org>,
"Josef Bacik" <josef@toxicpanda.com>,
"David Sterba" <dsterba@suse.com>,
"Sumit Semwal" <sumit.semwal@linaro.org>,
"Christian König" <christian.koenig@amd.com>,
"Jesse Brandeburg" <jesse.brandeburg@intel.com>,
"Daniel Micay" <danielmicay@gmail.com>,
"Yonghong Song" <yhs@fb.com>, "Marco Elver" <elver@google.com>,
"Miguel Ojeda" <ojeda@kernel.org>,
linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
linux-btrfs@vger.kernel.org, linux-media@vger.kernel.org,
dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org,
linux-fsdevel@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
dev@openvswitch.org, x86@kernel.org, llvm@lists.linux.dev,
linux-hardening@vger.kernel.org
Subject: Re: [PATCH v2 02/16] slab: Introduce kmalloc_size_roundup()
Date: Mon, 26 Sep 2022 10:50:36 -0700 [thread overview]
Message-ID: <202209261050.560459B@keescook> (raw)
In-Reply-To: <e0326835-9b0d-af1b-bd22-2aadb178bd25@suse.cz>
On Mon, Sep 26, 2022 at 03:15:22PM +0200, Vlastimil Babka wrote:
> On 9/23/22 22:28, Kees Cook wrote:
> > In the effort to help the compiler reason about buffer sizes, the
> > __alloc_size attribute was added to allocators. This improves the scope
> > of the compiler's ability to apply CONFIG_UBSAN_BOUNDS and (in the near
> > future) CONFIG_FORTIFY_SOURCE. For most allocations, this works well,
> > as the vast majority of callers are not expecting to use more memory
> > than what they asked for.
> >
> > There is, however, one common exception to this: anticipatory resizing
> > of kmalloc allocations. These cases all use ksize() to determine the
> > actual bucket size of a given allocation (e.g. 128 when 126 was asked
> > for). This comes in two styles in the kernel:
> >
> > 1) An allocation has been determined to be too small, and needs to be
> > resized. Instead of the caller choosing its own next best size, it
> > wants to minimize the number of calls to krealloc(), so it just uses
> > ksize() plus some additional bytes, forcing the realloc into the next
> > bucket size, from which it can learn how large it is now. For example:
> >
> > data = krealloc(data, ksize(data) + 1, gfp);
> > data_len = ksize(data);
> >
> > 2) The minimum size of an allocation is calculated, but since it may
> > grow in the future, just use all the space available in the chosen
> > bucket immediately, to avoid needing to reallocate later. A good
> > example of this is skbuff's allocators:
> >
> > data = kmalloc_reserve(size, gfp_mask, node, &pfmemalloc);
> > ...
> > /* kmalloc(size) might give us more room than requested.
> > * Put skb_shared_info exactly at the end of allocated zone,
> > * to allow max possible filling before reallocation.
> > */
> > osize = ksize(data);
> > size = SKB_WITH_OVERHEAD(osize);
> >
> > In both cases, the "how much was actually allocated?" question is answered
> > _after_ the allocation, where the compiler hinting is not in an easy place
> > to make the association any more. This mismatch between the compiler's
> > view of the buffer length and the code's intention about how much it is
> > going to actually use has already caused problems[1]. It is possible to
> > fix this by reordering the use of the "actual size" information.
> >
> > We can serve the needs of users of ksize() and still have accurate buffer
> > length hinting for the compiler by doing the bucket size calculation
> > _before_ the allocation. Code can instead ask "how large an allocation
> > would I get for a given size?".
> >
> > Introduce kmalloc_size_roundup(), to serve this function so we can start
> > replacing the "anticipatory resizing" uses of ksize().
> >
> > [1] https://github.com/ClangBuiltLinux/linux/issues/1599
> > https://github.com/KSPP/linux/issues/183
> >
> > Cc: Vlastimil Babka <vbabka@suse.cz>
> > Cc: Christoph Lameter <cl@linux.com>
> > Cc: Pekka Enberg <penberg@kernel.org>
> > Cc: David Rientjes <rientjes@google.com>
> > Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: linux-mm@kvack.org
> > Signed-off-by: Kees Cook <keescook@chromium.org>
>
> OK, added patch 1+2 to slab.git for-next branch.
> Had to adjust this one a bit, see below.
>
> > ---
> > include/linux/slab.h | 31 +++++++++++++++++++++++++++++++
> > mm/slab.c | 9 ++++++---
> > mm/slab_common.c | 20 ++++++++++++++++++++
> > 3 files changed, 57 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/slab.h b/include/linux/slab.h
> > index 41bd036e7551..727640173568 100644
> > --- a/include/linux/slab.h
> > +++ b/include/linux/slab.h
> > @@ -188,7 +188,21 @@ void * __must_check krealloc(const void *objp, size_t new_size, gfp_t flags) __r
> > void kfree(const void *objp);
> > void kfree_sensitive(const void *objp);
> > size_t __ksize(const void *objp);
> > +
> > +/**
> > + * ksize - Report actual allocation size of associated object
> > + *
> > + * @objp: Pointer returned from a prior kmalloc()-family allocation.
> > + *
> > + * This should not be used for writing beyond the originally requested
> > + * allocation size. Either use krealloc() or round up the allocation size
> > + * with kmalloc_size_roundup() prior to allocation. If this is used to
> > + * access beyond the originally requested allocation size, UBSAN_BOUNDS
> > + * and/or FORTIFY_SOURCE may trip, since they only know about the
> > + * originally allocated size via the __alloc_size attribute.
> > + */
> > size_t ksize(const void *objp);
> > +
> > #ifdef CONFIG_PRINTK
> > bool kmem_valid_obj(void *object);
> > void kmem_dump_obj(void *object);
> > @@ -779,6 +793,23 @@ extern void kvfree(const void *addr);
> > extern void kvfree_sensitive(const void *addr, size_t len);
> > unsigned int kmem_cache_size(struct kmem_cache *s);
> > +
> > +/**
> > + * kmalloc_size_roundup - Report allocation bucket size for the given size
> > + *
> > + * @size: Number of bytes to round up from.
> > + *
> > + * This returns the number of bytes that would be available in a kmalloc()
> > + * allocation of @size bytes. For example, a 126 byte request would be
> > + * rounded up to the next sized kmalloc bucket, 128 bytes. (This is strictly
> > + * for the general-purpose kmalloc()-based allocations, and is not for the
> > + * pre-sized kmem_cache_alloc()-based allocations.)
> > + *
> > + * Use this to kmalloc() the full bucket size ahead of time instead of using
> > + * ksize() to query the size after an allocation.
> > + */
> > +size_t kmalloc_size_roundup(size_t size);
> > +
> > void __init kmem_cache_init_late(void);
> > #if defined(CONFIG_SMP) && defined(CONFIG_SLAB)
> > diff --git a/mm/slab.c b/mm/slab.c
> > index 10e96137b44f..2da862bf6226 100644
> > --- a/mm/slab.c
> > +++ b/mm/slab.c
> > @@ -4192,11 +4192,14 @@ void __check_heap_object(const void *ptr, unsigned long n,
> > #endif /* CONFIG_HARDENED_USERCOPY */
> > /**
> > - * __ksize -- Uninstrumented ksize.
> > + * __ksize -- Report full size of underlying allocation
> > * @objp: pointer to the object
> > *
> > - * Unlike ksize(), __ksize() is uninstrumented, and does not provide the same
> > - * safety checks as ksize() with KASAN instrumentation enabled.
> > + * This should only be used internally to query the true size of allocations.
> > + * It is not meant to be a way to discover the usable size of an allocation
> > + * after the fact. Instead, use kmalloc_size_roundup(). Using memory beyond
> > + * the originally requested allocation size may trigger KASAN, UBSAN_BOUNDS,
> > + * and/or FORTIFY_SOURCE.
> > *
> > * Return: size of the actual memory used by @objp in bytes
> > */
> > diff --git a/mm/slab_common.c b/mm/slab_common.c
> > index 457671ace7eb..d7420cf649f8 100644
> > --- a/mm/slab_common.c
> > +++ b/mm/slab_common.c
> > @@ -721,6 +721,26 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags)
> > return kmalloc_caches[kmalloc_type(flags)][index];
> > }
> > +size_t kmalloc_size_roundup(size_t size)
> > +{
> > + struct kmem_cache *c;
> > +
> > + /* Short-circuit the 0 size case. */
> > + if (unlikely(size == 0))
> > + return 0;
> > + /* Short-circuit saturated "too-large" case. */
> > + if (unlikely(size == SIZE_MAX))
> > + return SIZE_MAX;
> > + /* Above the smaller buckets, size is a multiple of page size. */
> > + if (size > KMALLOC_MAX_CACHE_SIZE)
> > + return PAGE_SIZE << get_order(size);
> > +
> > + /* The flags don't matter since size_index is common to all. */
> > + c = kmalloc_slab(size, GFP_KERNEL);
> > + return c ? c->object_size : 0;
> > +}
> > +EXPORT_SYMBOL(kmalloc_size_roundup);
>
> We need a SLOB version too as it's not yet removed... I added this:
>
> diff --git a/mm/slob.c b/mm/slob.c
> index 2bd4f476c340..5dbdf6ad8bcc 100644
> --- a/mm/slob.c
> +++ b/mm/slob.c
> @@ -574,6 +574,20 @@ void kfree(const void *block)
> }
> EXPORT_SYMBOL(kfree);
> +size_t kmalloc_size_roundup(size_t size)
> +{
> + /* Short-circuit the 0 size case. */
> + if (unlikely(size == 0))
> + return 0;
> + /* Short-circuit saturated "too-large" case. */
> + if (unlikely(size == SIZE_MAX))
> + return SIZE_MAX;
> +
> + return ALIGN(size, ARCH_KMALLOC_MINALIGN);
> +}
> +
> +EXPORT_SYMBOL(kmalloc_size_roundup);
Ah, perfect! Thanks for catching that. :)
FWIW:
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
next prev parent reply other threads:[~2022-09-26 18:07 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-23 20:28 [PATCH v2 00/16] slab: Introduce kmalloc_size_roundup() Kees Cook
2022-09-23 20:28 ` [PATCH v2 01/16] slab: Remove __malloc attribute from realloc functions Kees Cook
2022-09-28 7:26 ` Geert Uytterhoeven
2022-09-28 16:27 ` Vlastimil Babka
2022-09-28 17:13 ` Kees Cook
2022-09-28 21:39 ` Vlastimil Babka
2022-09-29 8:36 ` Michael Ellerman
2022-09-29 9:00 ` Geert Uytterhoeven
2022-10-01 16:09 ` Hyeonggon Yoo
2022-09-23 20:28 ` [PATCH v2 02/16] slab: Introduce kmalloc_size_roundup() Kees Cook
2022-09-26 13:15 ` Vlastimil Babka
2022-09-26 17:50 ` Kees Cook [this message]
2022-10-01 16:28 ` Hyeonggon Yoo
2022-09-23 20:28 ` [PATCH v2 03/16] skbuff: Proactively round up to kmalloc bucket size Kees Cook
2022-09-24 9:11 ` Kees Cook
2022-09-23 20:28 ` [PATCH v2 04/16] skbuff: Phase out ksize() fallback for frag_size Kees Cook
2022-09-25 7:17 ` Paolo Abeni
2022-09-26 0:41 ` Kees Cook
2022-09-23 20:28 ` [PATCH v2 05/16] net: ipa: Proactively round up to kmalloc bucket size Kees Cook
2022-09-23 20:28 ` [PATCH v2 06/16] igb: " Kees Cook
2022-09-26 15:49 ` Ruhl, Michael J
2022-09-23 20:28 ` [PATCH v2 07/16] btrfs: send: " Kees Cook
2022-09-23 20:28 ` [PATCH v2 08/16] dma-buf: " Kees Cook
2022-09-26 9:29 ` [Linaro-mm-sig] " Christian König
2022-09-23 20:28 ` [PATCH v2 09/16] coredump: " Kees Cook
2022-09-23 20:28 ` [PATCH v2 10/16] openvswitch: Use kmalloc_size_roundup() to match ksize() usage Kees Cook
2022-09-23 20:28 ` [PATCH v2 11/16] bpf: " Kees Cook
2022-09-23 20:28 ` [PATCH v2 12/16] devres: " Kees Cook
2022-09-23 20:28 ` [PATCH v2 13/16] mempool: " Kees Cook
2022-09-26 13:50 ` Vlastimil Babka
2022-09-26 18:24 ` Kees Cook
2022-09-23 20:28 ` [PATCH v2 14/16] kasan: Remove ksize()-related tests Kees Cook
2022-09-24 8:15 ` Dmitry Vyukov
2022-09-26 0:38 ` Kees Cook
2022-09-23 20:28 ` [PATCH v2 15/16] mm: Make ksize() a reporting-only function Kees Cook
2022-09-23 20:28 ` [PATCH v2 16/16] slab: Restore __alloc_size attribute to __kmalloc_track_caller Kees Cook
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=202209261050.560459B@keescook \
--to=keescook@chromium.org \
--cc=42.hyeyoo@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=christian.koenig@amd.com \
--cc=cl@linux.com \
--cc=danielmicay@gmail.com \
--cc=davem@davemloft.net \
--cc=dev@openvswitch.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=dsterba@suse.com \
--cc=edumazet@google.com \
--cc=elder@kernel.org \
--cc=elver@google.com \
--cc=gregkh@linuxfoundation.org \
--cc=iamjoonsoo.kim@lge.com \
--cc=intel-wired-lan@lists.osuosl.org \
--cc=jesse.brandeburg@intel.com \
--cc=josef@toxicpanda.com \
--cc=kuba@kernel.org \
--cc=linaro-mm-sig@lists.linaro.org \
--cc=linux-btrfs@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=llvm@lists.linux.dev \
--cc=michael.j.ruhl@intel.com \
--cc=ndesaulniers@google.com \
--cc=netdev@vger.kernel.org \
--cc=ojeda@kernel.org \
--cc=pabeni@redhat.com \
--cc=penberg@kernel.org \
--cc=rientjes@google.com \
--cc=sumit.semwal@linaro.org \
--cc=vbabka@suse.cz \
--cc=x86@kernel.org \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).