Hi Kees,

On Fri, Sep 23, 2022 at 10:35 PM Kees Cook keescook@chromium.org wrote:

The __malloc attribute should not be applied to "realloc" functions, as the returned pointer may alias the storage of the prior pointer. Instead of splitting __malloc from __alloc_size, which would be a huge amount of churn, just create __realloc_size for the few cases where it is needed.

Additionally removes the conditional test for __alloc_size__, which is always defined now.

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: Vlastimil Babka vbabka@suse.cz Cc: Roman Gushchin roman.gushchin@linux.dev Cc: Hyeonggon Yoo 42.hyeyoo@gmail.com Cc: Marco Elver elver@google.com Cc: linux-mm@kvack.org Signed-off-by: Kees Cook keescook@chromium.org

Thanks for your patch, which is now commit 63caa04ec60583b1 ("slab: Remove __malloc attribute from realloc functions") in next-20220927.

Noreply@ellerman.id.au reported all gcc8-based builds to fail (e.g. [1], more at [2]):

In file included from <command-line>: ./include/linux/percpu.h: In function ‘__alloc_reserved_percpu’: ././include/linux/compiler_types.h:279:30: error: expected declaration specifiers before ‘__alloc_size__’ #define __alloc_size(x, ...) __alloc_size__(x, ## __VA_ARGS__) __malloc ^~~~~~~~~~~~~~ ./include/linux/percpu.h:120:74: note: in expansion of macro ‘__alloc_size’ [...]

It's building fine with e.g. gcc-9 (which is my usual m68k cross-compiler). Reverting this commit on next-20220927 fixes the issue.

[1] http://kisskb.ellerman.id.au/kisskb/buildresult/14803908/ [2] http://kisskb.ellerman.id.au/kisskb/head/1bd8b75fe6adeaa89d02968bdd811ffe708...

---

include/linux/compiler_types.h | 13 +++++-------- include/linux/slab.h | 12 ++++++------ mm/slab_common.c | 4 ++-- 3 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index 4f2a819fd60a..f141a6f6b9f6 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -271,15 +271,12 @@ struct ftrace_likely_data {

/*

• Any place that could be marked with the "alloc_size" attribute is also

• • a place to be marked with the "malloc" attribute. Do this as part of the
• • __alloc_size macro to avoid redundant attributes and to avoid missing a
• • __malloc marking.

• • a place to be marked with the "malloc" attribute, except those that may
• • be performing a _reallocation_, as that may alias the existing pointer.
• • For these, use __realloc_size().
  */

-#ifdef __alloc_size__ -# define __alloc_size(x, ...) __alloc_size__(x, ## __VA_ARGS__) __malloc -#else -# define __alloc_size(x, ...) __malloc -#endif +#define __alloc_size(x, ...) __alloc_size__(x, ## __VA_ARGS__) __malloc +#define __realloc_size(x, ...) __alloc_size__(x, ## __VA_ARGS__)

#ifndef asm_volatile_goto #define asm_volatile_goto(x...) asm goto(x) diff --git a/include/linux/slab.h b/include/linux/slab.h index 0fefdf528e0d..41bd036e7551 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -184,7 +184,7 @@ int kmem_cache_shrink(struct kmem_cache *s); /*

• Common kmalloc functions provided by all allocators

*/ -void * __must_check krealloc(const void *objp, size_t new_size, gfp_t flags) __alloc_size(2); +void * __must_check krealloc(const void *objp, size_t new_size, gfp_t flags) __realloc_size(2); void kfree(const void *objp); void kfree_sensitive(const void *objp); size_t __ksize(const void *objp); @@ -647,10 +647,10 @@ static inline __alloc_size(1, 2) void *kmalloc_array(size_t n, size_t size, gfp_

• @new_size: new size of a single member of the array
• @flags: the type of memory to allocate (see kmalloc)

*/ -static inline __alloc_size(2, 3) void * __must_check krealloc_array(void *p,

•                                                               size_t new_n,
  

•                                                               size_t new_size,
  

•                                                               gfp_t flags)
  

+static inline __realloc_size(2, 3) void * __must_check krealloc_array(void *p,

•                                                                 size_t new_n,
  

•                                                                 size_t new_size,
  

•                                                                 gfp_t flags)
  

{ size_t bytes;

@@ -774,7 +774,7 @@ static inline __alloc_size(1, 2) void *kvcalloc(size_t n, size_t size, gfp_t fla }

extern void *kvrealloc(const void *p, size_t oldsize, size_t newsize, gfp_t flags)

•                 __alloc_size(3);
  

•                 __realloc_size(3);
  

extern void kvfree(const void *addr); extern void kvfree_sensitive(const void *addr, size_t len);

diff --git a/mm/slab_common.c b/mm/slab_common.c index 17996649cfe3..457671ace7eb 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -1134,8 +1134,8 @@ module_init(slab_proc_init);

#endif /* CONFIG_SLAB || CONFIG_SLUB_DEBUG */

-static __always_inline void *__do_krealloc(const void *p, size_t new_size,

•                                      gfp_t flags)
  

+static __always_inline __realloc_size(2) void * +__do_krealloc(const void *p, size_t new_size, gfp_t flags) { void *ret; size_t ks; -- 2.34.1

-- Gr{oetje,eeting}s,

Geert

-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds

Kees Cook keescook@chromium.org writes:

On Wed, Sep 28, 2022 at 09:26:15AM +0200, Geert Uytterhoeven wrote:

...

On Fri, Sep 23, 2022 at 10:35 PM Kees Cook keescook@chromium.org wrote:

...

The __malloc attribute should not be applied to "realloc" functions, as the returned pointer may alias the storage of the prior pointer. Instead of splitting __malloc from __alloc_size, which would be a huge amount of churn, just create __realloc_size for the few cases where it is needed.

Additionally removes the conditional test for __alloc_size__, which is always defined now.

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: Vlastimil Babka vbabka@suse.cz Cc: Roman Gushchin roman.gushchin@linux.dev Cc: Hyeonggon Yoo 42.hyeyoo@gmail.com Cc: Marco Elver elver@google.com Cc: linux-mm@kvack.org Signed-off-by: Kees Cook keescook@chromium.org

Thanks for your patch, which is now commit 63caa04ec60583b1 ("slab: Remove __malloc attribute from realloc functions") in next-20220927.

Noreply@ellerman.id.au reported all gcc8-based builds to fail (e.g. [1], more at [2]):

In file included from <command-line>:
./include/linux/percpu.h: In function ‘__alloc_reserved_percpu’:
././include/linux/compiler_types.h:279:30: error: expected

declaration specifiers before ‘__alloc_size__’ #define __alloc_size(x, ...) __alloc_size__(x, ## __VA_ARGS__) __malloc ^~~~~~~~~~~~~~ ./include/linux/percpu.h:120:74: note: in expansion of macro ‘__alloc_size’ [...]

It's building fine with e.g. gcc-9 (which is my usual m68k cross-compiler). Reverting this commit on next-20220927 fixes the issue.

[1] http://kisskb.ellerman.id.au/kisskb/buildresult/14803908/ [2] http://kisskb.ellerman.id.au/kisskb/head/1bd8b75fe6adeaa89d02968bdd811ffe708...

Eek! Thanks for letting me know. I'm confused about this -- __alloc_size__ wasn't optional in compiler_attributes.h -- but obviously I broke something! I'll go figure this out.

This fixes it for me.

cheers

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index f141a6f6b9f6..0717534f8364 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -275,8 +275,13 @@ struct ftrace_likely_data { * be performing a _reallocation_, as that may alias the existing pointer. * For these, use __realloc_size(). */ -#define __alloc_size(x, ...) __alloc_size__(x, ## __VA_ARGS__) __malloc -#define __realloc_size(x, ...) __alloc_size__(x, ## __VA_ARGS__) +#ifdef __alloc_size__ +# define __alloc_size(x, ...) __alloc_size__(x, ## __VA_ARGS__) __malloc +# define __realloc_size(x, ...) __alloc_size__(x, ## __VA_ARGS__) +#else +# define __alloc_size(x, ...) __malloc +# define __realloc_size(x, ...) +#endif

#ifndef asm_volatile_goto #define asm_volatile_goto(x...) asm goto(x)

On Fri, Sep 23, 2022 at 01:28:07PM -0700, Kees Cook wrote:

The __malloc attribute should not be applied to "realloc" functions, as the returned pointer may alias the storage of the prior pointer. Instead of splitting __malloc from __alloc_size, which would be a huge amount of churn, just create __realloc_size for the few cases where it is needed.

Additionally removes the conditional test for __alloc_size__, which is always defined now.

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: Vlastimil Babka vbabka@suse.cz Cc: Roman Gushchin roman.gushchin@linux.dev Cc: Hyeonggon Yoo 42.hyeyoo@gmail.com Cc: Marco Elver elver@google.com Cc: linux-mm@kvack.org Signed-off-by: Kees Cook keescook@chromium.org

---

include/linux/compiler_types.h | 13 +++++-------- include/linux/slab.h | 12 ++++++------ mm/slab_common.c | 4 ++-- 3 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index 4f2a819fd60a..f141a6f6b9f6 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -271,15 +271,12 @@ struct ftrace_likely_data { /*

• Any place that could be marked with the "alloc_size" attribute is also

• • a place to be marked with the "malloc" attribute. Do this as part of the
• • __alloc_size macro to avoid redundant attributes and to avoid missing a
• • __malloc marking.

• • a place to be marked with the "malloc" attribute, except those that may
• • be performing a _reallocation_, as that may alias the existing pointer.
• • For these, use __realloc_size().
  */

-#ifdef __alloc_size__ -# define __alloc_size(x, ...) __alloc_size__(x, ## __VA_ARGS__) __malloc -#else -# define __alloc_size(x, ...) __malloc -#endif +#define __alloc_size(x, ...) __alloc_size__(x, ## __VA_ARGS__) __malloc +#define __realloc_size(x, ...) __alloc_size__(x, ## __VA_ARGS__) #ifndef asm_volatile_goto #define asm_volatile_goto(x...) asm goto(x) diff --git a/include/linux/slab.h b/include/linux/slab.h index 0fefdf528e0d..41bd036e7551 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -184,7 +184,7 @@ int kmem_cache_shrink(struct kmem_cache *s); /*

• Common kmalloc functions provided by all allocators

*/ -void * __must_check krealloc(const void *objp, size_t new_size, gfp_t flags) __alloc_size(2); +void * __must_check krealloc(const void *objp, size_t new_size, gfp_t flags) __realloc_size(2); void kfree(const void *objp); void kfree_sensitive(const void *objp); size_t __ksize(const void *objp); @@ -647,10 +647,10 @@ static inline __alloc_size(1, 2) void *kmalloc_array(size_t n, size_t size, gfp_

• @new_size: new size of a single member of the array
• @flags: the type of memory to allocate (see kmalloc)

*/ -static inline __alloc_size(2, 3) void * __must_check krealloc_array(void *p,

• 						    size_t new_n,
  

• 						    size_t new_size,
  

• 						    gfp_t flags)
  

+static inline __realloc_size(2, 3) void * __must_check krealloc_array(void *p,

• 						      size_t new_n,
  

• 						      size_t new_size,
  

• 						      gfp_t flags)
  

{ size_t bytes; @@ -774,7 +774,7 @@ static inline __alloc_size(1, 2) void *kvcalloc(size_t n, size_t size, gfp_t fla } extern void *kvrealloc(const void *p, size_t oldsize, size_t newsize, gfp_t flags)

•       __alloc_size(3);
  

•       __realloc_size(3);
  

extern void kvfree(const void *addr); extern void kvfree_sensitive(const void *addr, size_t len); diff --git a/mm/slab_common.c b/mm/slab_common.c index 17996649cfe3..457671ace7eb 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -1134,8 +1134,8 @@ module_init(slab_proc_init); #endif /* CONFIG_SLAB || CONFIG_SLUB_DEBUG */ -static __always_inline void *__do_krealloc(const void *p, size_t new_size,

• 			   gfp_t flags)
  

+static __always_inline __realloc_size(2) void * +__do_krealloc(const void *p, size_t new_size, gfp_t flags) { void *ret; size_t ks; -- 2.34.1

This is now squashed with later one. (so undefined __alloc_size__ issues are fixed) for the latest version of this patch:

Looks good to me, Acked-by: Hyeonggon Yoo 42.hyeyoo@gmail.com

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); + /* can't use ksize for kmem_cache_alloc memory, only kmalloc */ size_t __ksize(const void *block) {

On Fri, Sep 23, 2022 at 01:28:08PM -0700, 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

---

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);

With this now we have two conflicting kernel-doc comments about ksize in mm/slab_common.c and include/linux/slab.h.

#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);

#ifdef CONFIG_ZONE_DMA #define KMALLOC_DMA_NAME(sz) .name[KMALLOC_DMA] = "dma-kmalloc-" #sz,

#else

2.34.1

Otherwise looks good!

On Fri, Sep 23, 2022 at 01:28:09PM -0700, Kees Cook wrote:

Instead of discovering the kmalloc bucket size _after_ allocation, round up proactively so the allocation is explicitly made for the full size, allowing the compiler to correctly reason about the resulting size of the buffer through the existing __alloc_size() hint.

This will allow for kernels built with CONFIG_UBSAN_BOUNDS or the coming dynamic bounds checking under CONFIG_FORTIFY_SOURCE to gain back the __alloc_size() hints that were temporarily reverted in commit 93dd04ab0b2b ("slab: remove __alloc_size attribute from __kmalloc_track_caller")

Additionally tries to normalize size variables to u32 from int. Most interfaces are using "int", but notably __alloc_skb uses unsigned int.

Also fix some reverse Christmas tree and comments while touching nearby code.

Something in this patch is breaking things -- I've refactored it again to avoid overwriting the incoming size argument, and instead add a dedicated outgoing size variable. Here's what will be v3 ...

--- net/core/skbuff.c | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 974bbbbe7138..9b5a9fb69d9d 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -346,11 +346,12 @@ EXPORT_SYMBOL(napi_build_skb); * memory is free */ static void *kmalloc_reserve(size_t size, gfp_t flags, int node, - bool *pfmemalloc) + bool *pfmemalloc, size_t *alloc_size) { void *obj; bool ret_pfmemalloc = false;

+ size = kmalloc_size_roundup(size); /* * Try a regular allocation, when that fails and we're not entitled * to the reserves, fail. @@ -369,6 +370,7 @@ static void *kmalloc_reserve(size_t size, gfp_t flags, int node, if (pfmemalloc) *pfmemalloc = ret_pfmemalloc;

+ *alloc_size = size; return obj; }

@@ -400,7 +402,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask, { struct kmem_cache *cache; struct sk_buff *skb; - unsigned int osize; + size_t alloc_size; bool pfmemalloc; u8 *data;

@@ -427,15 +429,15 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask, */ size = SKB_DATA_ALIGN(size); size += SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); - data = kmalloc_reserve(size, gfp_mask, node, &pfmemalloc); - if (unlikely(!data)) - goto nodata; - /* kmalloc(size) might give us more room than requested. + /* kmalloc(size) might give us more room than requested, so + * allocate the true bucket size up front. * 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); + data = kmalloc_reserve(size, gfp_mask, node, &pfmemalloc, &alloc_size); + if (unlikely(!data)) + goto nodata; + size = SKB_WITH_OVERHEAD(alloc_size); prefetchw(data + size);

/* @@ -444,7 +446,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask, * the tail pointer in struct sk_buff! */ memset(skb, 0, offsetof(struct sk_buff, tail)); - __build_skb_around(skb, data, osize); + __build_skb_around(skb, data, alloc_size); skb->pfmemalloc = pfmemalloc;

if (flags & SKB_ALLOC_FCLONE) { @@ -1709,6 +1711,7 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail, { int i, osize = skb_end_offset(skb); int size = osize + nhead + ntail; + size_t alloc_size; long off; u8 *data;

@@ -1723,10 +1726,10 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail, if (skb_pfmemalloc(skb)) gfp_mask |= __GFP_MEMALLOC; data = kmalloc_reserve(size + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)), - gfp_mask, NUMA_NO_NODE, NULL); + gfp_mask, NUMA_NO_NODE, NULL, &alloc_size); if (!data) goto nodata; - size = SKB_WITH_OVERHEAD(ksize(data)); + size = SKB_WITH_OVERHEAD(alloc_size);

/* Copy only real data... and, alas, header. This should be * optimized for the cases when header is void. @@ -6063,19 +6066,19 @@ static int pskb_carve_inside_header(struct sk_buff *skb, const u32 off, int i; int size = skb_end_offset(skb); int new_hlen = headlen - off; + size_t alloc_size; u8 *data;

size = SKB_DATA_ALIGN(size);

if (skb_pfmemalloc(skb)) gfp_mask |= __GFP_MEMALLOC; - data = kmalloc_reserve(size + - SKB_DATA_ALIGN(sizeof(struct skb_shared_info)), - gfp_mask, NUMA_NO_NODE, NULL); + data = kmalloc_reserve(size + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)), + gfp_mask, NUMA_NO_NODE, NULL, &alloc_size); if (!data) return -ENOMEM;

- size = SKB_WITH_OVERHEAD(ksize(data)); + size = SKB_WITH_OVERHEAD(alloc_size);

/* Copy real data, and all frags */ skb_copy_from_linear_data_offset(skb, off, data, new_hlen); @@ -6184,18 +6187,18 @@ static int pskb_carve_inside_nonlinear(struct sk_buff *skb, const u32 off, u8 *data; const int nfrags = skb_shinfo(skb)->nr_frags; struct skb_shared_info *shinfo; + size_t alloc_size;

size = SKB_DATA_ALIGN(size);

if (skb_pfmemalloc(skb)) gfp_mask |= __GFP_MEMALLOC; - data = kmalloc_reserve(size + - SKB_DATA_ALIGN(sizeof(struct skb_shared_info)), - gfp_mask, NUMA_NO_NODE, NULL); + data = kmalloc_reserve(size + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)), + gfp_mask, NUMA_NO_NODE, NULL, &alloc_size); if (!data) return -ENOMEM;

- size = SKB_WITH_OVERHEAD(ksize(data)); + size = SKB_WITH_OVERHEAD(alloc_size);

memcpy((struct skb_shared_info *)(data + size), skb_shinfo(skb), offsetof(struct skb_shared_info, frags[0]));

On Fri, 2022-09-23 at 13:28 -0700, Kees Cook wrote:

All callers of APIs that allowed a 0-sized frag_size appear to be passing actual size information already

AFAICS, not yet:

drivers/net/ethernet/qlogic/qed/qed_ll2.c: skb = build_skb(buffer->data, 0); // -> __build_skb(..., 0)  // -> __build_skb_around()

drivers/net/ethernet/broadcom/bnx2.c: skb = build_skb(data, 0);

I guess some more drivers have calls leading to

__build_skb_around(..., 0)

there are several call path to checks...

, so this use of ksize() can be removed. However, just in case there is something still depending on this behavior, issue a WARN and fall back to as before to ksize() which means we'll also potentially get KASAN warnings.

Cc: "David S. Miller" davem@davemloft.net Cc: Eric Dumazet edumazet@google.com Cc: Jakub Kicinski kuba@kernel.org Cc: Paolo Abeni pabeni@redhat.com Cc: netdev@vger.kernel.org Signed-off-by: Kees Cook keescook@chromium.org

---

net/core/skbuff.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 0b30fbdbd0d0..84ca89c781cd 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -195,7 +195,11 @@ static void __build_skb_around(struct sk_buff *skb, void *data, unsigned int frag_size) { struct skb_shared_info *shinfo;

• unsigned int size = frag_size ? : ksize(data);

• unsigned int size = frag_size;
• /* All callers should be setting frag size now? */
• if (WARN_ON_ONCE(size == 0))

• size = ksize(data);
  

At some point in the future, I guess we could even drop this check, right?

Thanks!

Paolo

-----Original Message----- From: Kees Cook keescook@chromium.org Sent: Friday, September 23, 2022 4:28 PM To: Vlastimil Babka vbabka@suse.cz Cc: Kees Cook keescook@chromium.org; Brandeburg, Jesse jesse.brandeburg@intel.com; Nguyen, Anthony L anthony.l.nguyen@intel.com; David S. Miller davem@davemloft.net; Eric Dumazet edumazet@google.com; Jakub Kicinski kuba@kernel.org; Paolo Abeni pabeni@redhat.com; intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Ruhl, Michael J michael.j.ruhl@intel.com; Hyeonggon Yoo 42.hyeyoo@gmail.com; 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; 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; 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; linux-mm@kvack.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; dev@openvswitch.org; x86@kernel.org; llvm@lists.linux.dev; linux-hardening@vger.kernel.org Subject: [PATCH v2 06/16] igb: Proactively round up to kmalloc bucket size

In preparation for removing the "silently change allocation size" users of ksize(), explicitly round up all q_vector allocations so that allocations can be correctly compared to ksize().

Additionally fix potential use-after-free in the case of new allocation failure: only free memory if the replacement allocation succeeds.

Cc: Jesse Brandeburg jesse.brandeburg@intel.com Cc: Tony Nguyen anthony.l.nguyen@intel.com Cc: "David S. Miller" davem@davemloft.net Cc: Eric Dumazet edumazet@google.com Cc: Jakub Kicinski kuba@kernel.org Cc: Paolo Abeni pabeni@redhat.com Cc: intel-wired-lan@lists.osuosl.org Cc: netdev@vger.kernel.org Signed-off-by: Kees Cook keescook@chromium.org

---

drivers/net/ethernet/intel/igb/igb_main.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c index 2796e81d2726..eb51e531c096 100644 --- a/drivers/net/ethernet/intel/igb/igb_main.c +++ b/drivers/net/ethernet/intel/igb/igb_main.c @@ -1195,15 +1195,16 @@ static int igb_alloc_q_vector(struct igb_adapter *adapter, return -ENOMEM;

ring_count = txr_count + rxr_count;

• size = struct_size(q_vector, ring, ring_count);

• size = kmalloc_size_roundup(struct_size(q_vector, ring, ring_count));

This looks good to me...

/* allocate q_vector and rings */ q_vector = adapter->q_vector[v_idx]; if (!q_vector) { q_vector = kzalloc(size, GFP_KERNEL); } else if (size > ksize(q_vector)) {

• kfree_rcu(q_vector, rcu);
  

  q_vector = kzalloc(size, GFP_KERNEL);

• if (q_vector)
  

• 	kfree_rcu(q_vector, rcu);
  

Even though this is in the ksize part, this seems like an unrelated change? Should this be in a different patch?

Also, the kfree_rcu will free q_vector after the RCU grace period?

Is that what you want to do?

How does rcu distinguish between the original q_vector, and the newly kzalloced one?

Thanks,

Mike

} else { memset(q_vector, 0, size); } -- 2.34.1

On Mon, Sep 26, 2022 at 03:50:43PM +0200, Vlastimil Babka wrote:

On 9/23/22 22:28, Kees Cook wrote:

...

Round up allocations with kmalloc_size_roundup() so that mempool's use of ksize() is always accurate and no special handling of the memory is needed by KASAN, UBSAN_BOUNDS, nor FORTIFY_SOURCE.

Cc: Andrew Morton akpm@linux-foundation.org Cc: linux-mm@kvack.org Signed-off-by: Kees Cook keescook@chromium.org

---

mm/mempool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/mempool.c b/mm/mempool.c index 96488b13a1ef..0f3107b28e6b 100644 --- a/mm/mempool.c +++ b/mm/mempool.c @@ -526,7 +526,7 @@ EXPORT_SYMBOL(mempool_free_slab); */ void *mempool_kmalloc(gfp_t gfp_mask, void *pool_data) {

• size_t size = (size_t)pool_data;

• size_t size = kmalloc_size_roundup((size_t)pool_data);

Hm it is kinda wasteful to call into kmalloc_size_roundup for every allocation that has the same input. We could do it just once in mempool_init_node() for adjusting pool->pool_data ?

But looking more closely, I wonder why poison_element() and kasan_unpoison_element() in mm/mempool.c even have to use ksize()/__ksize() and not just operate on the requested size (again, pool->pool_data). If no kmalloc mempool's users use ksize() to write beyond requested size, then we don't have to unpoison/poison that area either?

Yeah, I think that's a fair point. I will adjust this.

On Fri, 23 Sept 2022 at 22:28, Kees Cook keescook@chromium.org wrote:

In preparation for no longer unpoisoning in ksize(), remove the behavioral self-tests for ksize().

Cc: Andrey Ryabinin ryabinin.a.a@gmail.com Cc: Alexander Potapenko glider@google.com Cc: Andrey Konovalov andreyknvl@gmail.com Cc: Dmitry Vyukov dvyukov@google.com Cc: Vincenzo Frascino vincenzo.frascino@arm.com Cc: Andrew Morton akpm@linux-foundation.org Cc: kasan-dev@googlegroups.com Cc: linux-mm@kvack.org Signed-off-by: Kees Cook keescook@chromium.org

---

lib/test_kasan.c | 42 ------------------------------------------ mm/kasan/shadow.c | 4 +--- 2 files changed, 1 insertion(+), 45 deletions(-)

diff --git a/lib/test_kasan.c b/lib/test_kasan.c index 58c1b01ccfe2..bdd0ced8f8d7 100644 --- a/lib/test_kasan.c +++ b/lib/test_kasan.c @@ -753,46 +753,6 @@ static void kasan_global_oob_left(struct kunit *test) KUNIT_EXPECT_KASAN_FAIL(test, *(volatile char *)p); }

-/* Check that ksize() makes the whole object accessible. */ -static void ksize_unpoisons_memory(struct kunit *test) -{

•   char *ptr;
  

•   size_t size = 123, real_size;
  

•   ptr = kmalloc(size, GFP_KERNEL);
  

•   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
  

•   real_size = ksize(ptr);
  

•   OPTIMIZER_HIDE_VAR(ptr);
  

•   /* This access shouldn't trigger a KASAN report. */
  

•   ptr[size] = 'x';
  

I would rather keep the tests and update to the new behavior. We had bugs in ksize, we need test coverage. I assume ptr[size] access must now produce an error even after ksize.

•   /* This one must. */
  

•   KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[real_size]);
  

•   kfree(ptr);
  

-}

-/*

• • Check that a use-after-free is detected by ksize() and via normal accesses
• • after it.
• */

-static void ksize_uaf(struct kunit *test) -{

•   char *ptr;
  

•   int size = 128 - KASAN_GRANULE_SIZE;
  

•   ptr = kmalloc(size, GFP_KERNEL);
  

•   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
  

•   kfree(ptr);
  

•   OPTIMIZER_HIDE_VAR(ptr);
  

•   KUNIT_EXPECT_KASAN_FAIL(test, ksize(ptr));
  

This is still a bug that should be detected, right? Calling ksize on a freed pointer is a bug.

•   KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[0]);
  

•   KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[size]);
  

-}

static void kasan_stack_oob(struct kunit *test) { char stack_array[10]; @@ -1392,8 +1352,6 @@ static struct kunit_case kasan_kunit_test_cases[] = { KUNIT_CASE(kasan_stack_oob), KUNIT_CASE(kasan_alloca_oob_left), KUNIT_CASE(kasan_alloca_oob_right),

•   KUNIT_CASE(ksize_unpoisons_memory),
  

•   KUNIT_CASE(ksize_uaf),
    KUNIT_CASE(kmem_cache_double_free),
    KUNIT_CASE(kmem_cache_invalid_free),
    KUNIT_CASE(kmem_cache_double_destroy),
  

diff --git a/mm/kasan/shadow.c b/mm/kasan/shadow.c index 0e3648b603a6..0895c73e9b69 100644 --- a/mm/kasan/shadow.c +++ b/mm/kasan/shadow.c @@ -124,9 +124,7 @@ void kasan_unpoison(const void *addr, size_t size, bool init) addr = kasan_reset_tag(addr);

    /*

•    * Skip KFENCE memory if called explicitly outside of sl*b. Also note
  

•    * that calls to ksize(), where size is not a multiple of machine-word
  

•    * size, would otherwise poison the invalid portion of the word.
  

•    * Skip KFENCE memory if called explicitly outside of sl*b.
     */
    if (is_kfence_address(addr))
            return;
  

-- 2.34.1