kmalloc's API family is critical for mm, and one of its nature is that
it will round up the request size to a fixed one (mostly power of 2).
When user requests memory for '2^n + 1' bytes, actually 2^(n+1) bytes
could be allocated, so there is an extra space than what is originally
requested.
This patchset tries to extend the redzone sanity check to the extra
kmalloced buffer than requested, to better detect un-legitimate access
to it. (dependson SLAB_STORE_USER & SLAB_RED_ZONE)
The redzone part has been tested with code below:
for (shift = 3; shift <= 12; shift++) {
size = 1 << shift;
buf = kmalloc(size + 4, GFP_KERNEL);
/* We have 96, 196 kmalloc size, which is not power of 2 */
if (size == 64 || size == 128)
oob_size = 16;
else
oob_size = size - 4;
memset(buf + size + 4, 0xee, oob_size);
kfree(buf);
}
(This is against slab tree's 'for-6.2/slub-sysfs' branch, with
HEAD 54736f702526)
Please help to review, thanks!
- Feng
---
Changelogs:
since v6:
* 1/4 patch of kmalloc memory wastage debug patch was merged
to 6.1-rc1, so drop it
* refine the kasan patch by extending existing APIs and hiding
kasan internal data structure info (Andrey Konovalov)
* only reduce zeroing size when slub debug is enabled to
avoid security risk (Kees Cook/Andrey Konovalov)
* collect Acked-by tag from Hyeonggon Yoo
since v5:
* Refine code/comments and add more perf info in commit log for
kzalloc change (Hyeonggoon Yoo)
* change the kasan param name and refine comments about
kasan+redzone handling (Andrey Konovalov)
* put free pointer in meta data to make redzone check cover all
kmalloc objects (Hyeonggoon Yoo)
since v4:
* fix a race issue in v3, by moving kmalloc debug init into
alloc_debug_processing (Hyeonggon Yoo)
* add 'partial_conext' for better parameter passing in get_partial()
call chain (Vlastimil Babka)
* update 'slub.rst' for 'alloc_traces' part (Hyeonggon Yoo)
* update code comments for 'orig_size'
since v3:
* rebase against latest post 6.0-rc1 slab tree's 'for-next' branch
* fix a bug reported by 0Day, that kmalloc-redzoned data and kasan's
free meta data overlaps in the same kmalloc object data area
since v2:
* rebase against slab tree's 'for-next' branch
* fix pointer handling (Kefeng Wang)
* move kzalloc zeroing handling change to a separate patch (Vlastimil Babka)
* make 'orig_size' only depend on KMALLOC & STORE_USER flag
bits (Vlastimil Babka)
since v1:
* limit the 'orig_size' to kmalloc objects only, and save
it after track in metadata (Vlastimil Babka)
* fix a offset calculation problem in print_trailer
since RFC:
* fix problems in kmem_cache_alloc_bulk() and records sorting,
improve the print format (Hyeonggon Yoo)
* fix a compiling issue found by 0Day bot
* update the commit log based info from iova developers
Feng Tang (3):
mm/slub: only zero requested size of buffer for kzalloc when debug
enabled
mm: kasan: Extend kasan_metadata_size() to also cover in-object size
mm/slub: extend redzone check to extra allocated kmalloc space than
requested
include/linux/kasan.h | 5 ++--
mm/kasan/generic.c | 19 +++++++++----
mm/slab.c | 7 +++--
mm/slab.h | 22 +++++++++++++--
mm/slab_common.c | 4 +++
mm/slub.c | 65 +++++++++++++++++++++++++++++++++++++------
6 files changed, 100 insertions(+), 22 deletions(-)
--
2.34.1
When kasan is enabled for slab/slub, it may save kasan' free_meta
data in the former part of slab object data area in slab object's
free path, which works fine.
There is ongoing effort to extend slub's debug function which will
redzone the latter part of kmalloc object area, and when both of
the debug are enabled, there is possible conflict, especially when
the kmalloc object has small size, as caught by 0Day bot [1].
To solve it, slub code needs to know the in-object kasan's meta
data size. Currently, there is existing kasan_metadata_size()
which returns the kasan's metadata size inside slub's metadata
area, so extend it to also cover the in-object meta size by
adding a boolean flag 'in_object'.
There is no functional change to existing code logic.
[1]. https://lore.kernel.org/lkml/YuYm3dWwpZwH58Hu@xsang-OptiPlex-9020/
Reported-by: kernel test robot <[email protected]>
Suggested-by: Andrey Konovalov <[email protected]>
Signed-off-by: Feng Tang <[email protected]>
Reviewed-by: Andrey Konovalov <[email protected]>
Cc: Andrey Ryabinin <[email protected]>
Cc: Alexander Potapenko <[email protected]>
Cc: Dmitry Vyukov <[email protected]>
Cc: Vincenzo Frascino <[email protected]>
---
include/linux/kasan.h | 5 +++--
mm/kasan/generic.c | 19 +++++++++++++------
mm/slub.c | 4 ++--
3 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index d811b3d7d2a1..96c9d56e5510 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -302,7 +302,7 @@ static inline void kasan_unpoison_task_stack(struct task_struct *task) {}
#ifdef CONFIG_KASAN_GENERIC
-size_t kasan_metadata_size(struct kmem_cache *cache);
+size_t kasan_metadata_size(struct kmem_cache *cache, bool in_object);
slab_flags_t kasan_never_merge(void);
void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
slab_flags_t *flags);
@@ -315,7 +315,8 @@ void kasan_record_aux_stack_noalloc(void *ptr);
#else /* CONFIG_KASAN_GENERIC */
/* Tag-based KASAN modes do not use per-object metadata. */
-static inline size_t kasan_metadata_size(struct kmem_cache *cache)
+static inline size_t kasan_metadata_size(struct kmem_cache *cache,
+ bool in_object)
{
return 0;
}
diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
index d8b5590f9484..b076f597a378 100644
--- a/mm/kasan/generic.c
+++ b/mm/kasan/generic.c
@@ -450,15 +450,22 @@ void kasan_init_object_meta(struct kmem_cache *cache, const void *object)
__memset(alloc_meta, 0, sizeof(*alloc_meta));
}
-size_t kasan_metadata_size(struct kmem_cache *cache)
+size_t kasan_metadata_size(struct kmem_cache *cache, bool in_object)
{
+ struct kasan_cache *info = &cache->kasan_info;
+
if (!kasan_requires_meta())
return 0;
- return (cache->kasan_info.alloc_meta_offset ?
- sizeof(struct kasan_alloc_meta) : 0) +
- ((cache->kasan_info.free_meta_offset &&
- cache->kasan_info.free_meta_offset != KASAN_NO_FREE_META) ?
- sizeof(struct kasan_free_meta) : 0);
+
+ if (in_object)
+ return (info->free_meta_offset ?
+ 0 : sizeof(struct kasan_free_meta));
+ else
+ return (info->alloc_meta_offset ?
+ sizeof(struct kasan_alloc_meta) : 0) +
+ ((info->free_meta_offset &&
+ info->free_meta_offset != KASAN_NO_FREE_META) ?
+ sizeof(struct kasan_free_meta) : 0);
}
static void __kasan_record_aux_stack(void *addr, bool can_alloc)
diff --git a/mm/slub.c b/mm/slub.c
index 17292c2d3eee..adff7553b54e 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -910,7 +910,7 @@ static void print_trailer(struct kmem_cache *s, struct slab *slab, u8 *p)
if (slub_debug_orig_size(s))
off += sizeof(unsigned int);
- off += kasan_metadata_size(s);
+ off += kasan_metadata_size(s, false);
if (off != size_from_object(s))
/* Beginning of the filler is the free pointer */
@@ -1070,7 +1070,7 @@ static int check_pad_bytes(struct kmem_cache *s, struct slab *slab, u8 *p)
off += sizeof(unsigned int);
}
- off += kasan_metadata_size(s);
+ off += kasan_metadata_size(s, false);
if (size_from_object(s) == off)
return 1;
--
2.34.1
On Fri, Oct 21, 2022 at 5:24 AM Feng Tang <[email protected]> wrote:
>
> When kasan is enabled for slab/slub, it may save kasan' free_meta
> data in the former part of slab object data area in slab object's
> free path, which works fine.
>
> There is ongoing effort to extend slub's debug function which will
> redzone the latter part of kmalloc object area, and when both of
> the debug are enabled, there is possible conflict, especially when
> the kmalloc object has small size, as caught by 0Day bot [1].
>
> To solve it, slub code needs to know the in-object kasan's meta
> data size. Currently, there is existing kasan_metadata_size()
> which returns the kasan's metadata size inside slub's metadata
> area, so extend it to also cover the in-object meta size by
> adding a boolean flag 'in_object'.
>
> There is no functional change to existing code logic.
>
> [1]. https://lore.kernel.org/lkml/YuYm3dWwpZwH58Hu@xsang-OptiPlex-9020/
> Reported-by: kernel test robot <[email protected]>
> Suggested-by: Andrey Konovalov <[email protected]>
> Signed-off-by: Feng Tang <[email protected]>
> Reviewed-by: Andrey Konovalov <[email protected]>
> Cc: Andrey Ryabinin <[email protected]>
> Cc: Alexander Potapenko <[email protected]>
> Cc: Dmitry Vyukov <[email protected]>
> Cc: Vincenzo Frascino <[email protected]>
> ---
> include/linux/kasan.h | 5 +++--
> mm/kasan/generic.c | 19 +++++++++++++------
> mm/slub.c | 4 ++--
> 3 files changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index d811b3d7d2a1..96c9d56e5510 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -302,7 +302,7 @@ static inline void kasan_unpoison_task_stack(struct task_struct *task) {}
>
> #ifdef CONFIG_KASAN_GENERIC
>
> -size_t kasan_metadata_size(struct kmem_cache *cache);
> +size_t kasan_metadata_size(struct kmem_cache *cache, bool in_object);
> slab_flags_t kasan_never_merge(void);
> void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
> slab_flags_t *flags);
> @@ -315,7 +315,8 @@ void kasan_record_aux_stack_noalloc(void *ptr);
> #else /* CONFIG_KASAN_GENERIC */
>
> /* Tag-based KASAN modes do not use per-object metadata. */
> -static inline size_t kasan_metadata_size(struct kmem_cache *cache)
> +static inline size_t kasan_metadata_size(struct kmem_cache *cache,
> + bool in_object)
> {
> return 0;
> }
> diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> index d8b5590f9484..b076f597a378 100644
> --- a/mm/kasan/generic.c
> +++ b/mm/kasan/generic.c
> @@ -450,15 +450,22 @@ void kasan_init_object_meta(struct kmem_cache *cache, const void *object)
> __memset(alloc_meta, 0, sizeof(*alloc_meta));
> }
>
> -size_t kasan_metadata_size(struct kmem_cache *cache)
> +size_t kasan_metadata_size(struct kmem_cache *cache, bool in_object)
> {
> + struct kasan_cache *info = &cache->kasan_info;
> +
> if (!kasan_requires_meta())
> return 0;
> - return (cache->kasan_info.alloc_meta_offset ?
> - sizeof(struct kasan_alloc_meta) : 0) +
> - ((cache->kasan_info.free_meta_offset &&
> - cache->kasan_info.free_meta_offset != KASAN_NO_FREE_META) ?
> - sizeof(struct kasan_free_meta) : 0);
> +
> + if (in_object)
> + return (info->free_meta_offset ?
> + 0 : sizeof(struct kasan_free_meta));
> + else
> + return (info->alloc_meta_offset ?
> + sizeof(struct kasan_alloc_meta) : 0) +
> + ((info->free_meta_offset &&
> + info->free_meta_offset != KASAN_NO_FREE_META) ?
> + sizeof(struct kasan_free_meta) : 0);
> }
>
> static void __kasan_record_aux_stack(void *addr, bool can_alloc)
> diff --git a/mm/slub.c b/mm/slub.c
> index 17292c2d3eee..adff7553b54e 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -910,7 +910,7 @@ static void print_trailer(struct kmem_cache *s, struct slab *slab, u8 *p)
> if (slub_debug_orig_size(s))
> off += sizeof(unsigned int);
>
> - off += kasan_metadata_size(s);
> + off += kasan_metadata_size(s, false);
>
> if (off != size_from_object(s))
> /* Beginning of the filler is the free pointer */
> @@ -1070,7 +1070,7 @@ static int check_pad_bytes(struct kmem_cache *s, struct slab *slab, u8 *p)
> off += sizeof(unsigned int);
> }
>
> - off += kasan_metadata_size(s);
> + off += kasan_metadata_size(s, false);
>
> if (size_from_object(s) == off)
> return 1;
> --
> 2.34.1
>
Reviewed-by: Andrey Konovalov <[email protected]>
Thanks!
On 10/21/22 05:24, Feng Tang wrote:
> kmalloc's API family is critical for mm, and one of its nature is that
> it will round up the request size to a fixed one (mostly power of 2).
> When user requests memory for '2^n + 1' bytes, actually 2^(n+1) bytes
> could be allocated, so there is an extra space than what is originally
> requested.
>
> This patchset tries to extend the redzone sanity check to the extra
> kmalloced buffer than requested, to better detect un-legitimate access
> to it. (dependson SLAB_STORE_USER & SLAB_RED_ZONE)
>
> The redzone part has been tested with code below:
>
> for (shift = 3; shift <= 12; shift++) {
> size = 1 << shift;
> buf = kmalloc(size + 4, GFP_KERNEL);
> /* We have 96, 196 kmalloc size, which is not power of 2 */
> if (size == 64 || size == 128)
> oob_size = 16;
> else
> oob_size = size - 4;
> memset(buf + size + 4, 0xee, oob_size);
> kfree(buf);
> }
Sounds like a new slub_kunit test would be useful? :) doesn't need to be
that exhaustive wrt all sizes, we could just pick one and check that a write
beyond requested kmalloc size is detected?
Thanks!
On Fri, Nov 11, 2022 at 04:16:32PM +0800, Vlastimil Babka wrote:
> On 10/21/22 05:24, Feng Tang wrote:
> > kmalloc's API family is critical for mm, and one of its nature is that
> > it will round up the request size to a fixed one (mostly power of 2).
> > When user requests memory for '2^n + 1' bytes, actually 2^(n+1) bytes
> > could be allocated, so there is an extra space than what is originally
> > requested.
> >
> > This patchset tries to extend the redzone sanity check to the extra
> > kmalloced buffer than requested, to better detect un-legitimate access
> > to it. (dependson SLAB_STORE_USER & SLAB_RED_ZONE)
> >
> > The redzone part has been tested with code below:
> >
> > for (shift = 3; shift <= 12; shift++) {
> > size = 1 << shift;
> > buf = kmalloc(size + 4, GFP_KERNEL);
> > /* We have 96, 196 kmalloc size, which is not power of 2 */
> > if (size == 64 || size == 128)
> > oob_size = 16;
> > else
> > oob_size = size - 4;
> > memset(buf + size + 4, 0xee, oob_size);
> > kfree(buf);
> > }
>
> Sounds like a new slub_kunit test would be useful? :) doesn't need to be
> that exhaustive wrt all sizes, we could just pick one and check that a write
> beyond requested kmalloc size is detected?
Just git-grepped out slub_kunit.c :), will try to add a case to it.
I'll also check if the case will also be caught by other sanitizer
tools like kasan/kfence etc.
Thanks,
Feng
> Thanks!
>
On Fri, Nov 11, 2022 at 04:29:43PM +0800, Tang, Feng wrote:
> On Fri, Nov 11, 2022 at 04:16:32PM +0800, Vlastimil Babka wrote:
> > > for (shift = 3; shift <= 12; shift++) {
> > > size = 1 << shift;
> > > buf = kmalloc(size + 4, GFP_KERNEL);
> > > /* We have 96, 196 kmalloc size, which is not power of 2 */
> > > if (size == 64 || size == 128)
> > > oob_size = 16;
> > > else
> > > oob_size = size - 4;
> > > memset(buf + size + 4, 0xee, oob_size);
> > > kfree(buf);
> > > }
> >
> > Sounds like a new slub_kunit test would be useful? :) doesn't need to be
> > that exhaustive wrt all sizes, we could just pick one and check that a write
> > beyond requested kmalloc size is detected?
>
> Just git-grepped out slub_kunit.c :), will try to add a case to it.
> I'll also check if the case will also be caught by other sanitizer
> tools like kasan/kfence etc.
Just checked, kasan has already has API to disable kasan check
temporarily, and I did see sometime kfence can chime in (4 out of 178
runs) so we need skip kfenced address.
Here is the draft patch, thanks!
From 45bf8d0072e532f43063dbda44c6bb3adcc388b6 Mon Sep 17 00:00:00 2001
From: Feng Tang <[email protected]>
Date: Mon, 21 Nov 2022 13:17:11 +0800
Subject: [PATCH] mm/slub, kunit: Add a case for kmalloc redzone functionality
kmalloc redzone check for slub has been merged, and it's better to add
a kunit case for it, which is inspired by a real-world case as described
in commit 120ee599b5bf ("staging: octeon-usb: prevent memory corruption"):
"
octeon-hcd will crash the kernel when SLOB is used. This usually happens
after the 18-byte control transfer when a device descriptor is read.
The DMA engine is always transfering full 32-bit words and if the
transfer is shorter, some random garbage appears after the buffer.
The problem is not visible with SLUB since it rounds up the allocations
to word boundary, and the extra bytes will go undetected.
"
Suggested-by: Vlastimil Babka <[email protected]>
Signed-off-by: Feng Tang <[email protected]>
---
lib/slub_kunit.c | 42 ++++++++++++++++++++++++++++++++++++++++++
mm/slab.h | 15 +++++++++++++++
mm/slub.c | 4 ++--
3 files changed, 59 insertions(+), 2 deletions(-)
diff --git a/lib/slub_kunit.c b/lib/slub_kunit.c
index 7a0564d7cb7a..0653eed19bff 100644
--- a/lib/slub_kunit.c
+++ b/lib/slub_kunit.c
@@ -120,6 +120,47 @@ static void test_clobber_redzone_free(struct kunit *test)
kmem_cache_destroy(s);
}
+
+/*
+ * This case is simulating a real world case, that a device driver
+ * requests 18 bytes buffer, but the device HW has obligation to
+ * operate on 32 bits granularity, so it may actually read or write
+ * 20 bytes to the buffer, and possibly pollute 2 extra bytes after
+ * the requested space.
+ */
+static void test_kmalloc_redzone_access(struct kunit *test)
+{
+ u8 *p;
+
+ if (!is_slub_debug_flags_enabled(SLAB_STORE_USER | SLAB_RED_ZONE))
+ kunit_skip(test, "Test required SLAB_STORE_USER & SLAB_RED_ZONE flags on");
+
+ p = kmalloc(18, GFP_KERNEL);
+
+#ifdef CONFIG_KFENCE
+ {
+ int max_retry = 10;
+
+ while (is_kfence_address(p) && max_retry--) {
+ kfree(p);
+ p = kmalloc(18, GFP_KERNEL);
+ }
+
+ if (!max_retry)
+ kunit_skip(test, "Fail to get non-kfenced memory");
+ }
+#endif
+
+ kasan_disable_current();
+
+ p[18] = 0xab;
+ p[19] = 0xab;
+ kfree(p);
+
+ KUNIT_EXPECT_EQ(test, 3, slab_errors);
+ kasan_enable_current();
+}
+
static int test_init(struct kunit *test)
{
slab_errors = 0;
@@ -139,6 +180,7 @@ static struct kunit_case test_cases[] = {
#endif
KUNIT_CASE(test_clobber_redzone_free),
+ KUNIT_CASE(test_kmalloc_redzone_access),
{}
};
diff --git a/mm/slab.h b/mm/slab.h
index e3b3231af742..72f7a85e01ab 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -413,6 +413,17 @@ static inline bool __slub_debug_enabled(void)
{
return static_branch_unlikely(&slub_debug_enabled);
}
+
+extern slab_flags_t slub_debug;
+
+/*
+ * This should only be used in post-boot time, after 'slub_debug'
+ * gets initialized.
+ */
+static inline bool is_slub_debug_flags_enabled(slab_flags_t flags)
+{
+ return (slub_debug & flags) == flags;
+}
#else
static inline void print_tracking(struct kmem_cache *s, void *object)
{
@@ -421,6 +432,10 @@ static inline bool __slub_debug_enabled(void)
{
return false;
}
+static inline bool is_slub_debug_flags_enabled(slab_flags_t flags)
+{
+ return false;
+}
#endif
/*
diff --git a/mm/slub.c b/mm/slub.c
index a24b71041b26..6ef72b8f6291 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -638,9 +638,9 @@ static inline void *restore_red_left(struct kmem_cache *s, void *p)
* Debug settings:
*/
#if defined(CONFIG_SLUB_DEBUG_ON)
-static slab_flags_t slub_debug = DEBUG_DEFAULT_FLAGS;
+slab_flags_t slub_debug = DEBUG_DEFAULT_FLAGS;
#else
-static slab_flags_t slub_debug;
+slab_flags_t slub_debug;
#endif
static char *slub_debug_string;
--
2.34.1
On 11/21/22 07:38, Feng Tang wrote:
> On Fri, Nov 11, 2022 at 04:29:43PM +0800, Tang, Feng wrote:
>> On Fri, Nov 11, 2022 at 04:16:32PM +0800, Vlastimil Babka wrote:
>> > > for (shift = 3; shift <= 12; shift++) {
>> > > size = 1 << shift;
>> > > buf = kmalloc(size + 4, GFP_KERNEL);
>> > > /* We have 96, 196 kmalloc size, which is not power of 2 */
>> > > if (size == 64 || size == 128)
>> > > oob_size = 16;
>> > > else
>> > > oob_size = size - 4;
>> > > memset(buf + size + 4, 0xee, oob_size);
>> > > kfree(buf);
>> > > }
>> >
>> > Sounds like a new slub_kunit test would be useful? :) doesn't need to be
>> > that exhaustive wrt all sizes, we could just pick one and check that a write
>> > beyond requested kmalloc size is detected?
>>
>> Just git-grepped out slub_kunit.c :), will try to add a case to it.
>> I'll also check if the case will also be caught by other sanitizer
>> tools like kasan/kfence etc.
>
> Just checked, kasan has already has API to disable kasan check
> temporarily, and I did see sometime kfence can chime in (4 out of 178
> runs) so we need skip kfenced address.
>
> Here is the draft patch, thanks!
>
> From 45bf8d0072e532f43063dbda44c6bb3adcc388b6 Mon Sep 17 00:00:00 2001
> From: Feng Tang <[email protected]>
> Date: Mon, 21 Nov 2022 13:17:11 +0800
> Subject: [PATCH] mm/slub, kunit: Add a case for kmalloc redzone functionality
>
> kmalloc redzone check for slub has been merged, and it's better to add
> a kunit case for it, which is inspired by a real-world case as described
> in commit 120ee599b5bf ("staging: octeon-usb: prevent memory corruption"):
>
> "
> octeon-hcd will crash the kernel when SLOB is used. This usually happens
> after the 18-byte control transfer when a device descriptor is read.
> The DMA engine is always transfering full 32-bit words and if the
> transfer is shorter, some random garbage appears after the buffer.
> The problem is not visible with SLUB since it rounds up the allocations
> to word boundary, and the extra bytes will go undetected.
> "
> Suggested-by: Vlastimil Babka <[email protected]>
> Signed-off-by: Feng Tang <[email protected]>
> ---
> lib/slub_kunit.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> mm/slab.h | 15 +++++++++++++++
> mm/slub.c | 4 ++--
> 3 files changed, 59 insertions(+), 2 deletions(-)
>
> diff --git a/lib/slub_kunit.c b/lib/slub_kunit.c
> index 7a0564d7cb7a..0653eed19bff 100644
> --- a/lib/slub_kunit.c
> +++ b/lib/slub_kunit.c
> @@ -120,6 +120,47 @@ static void test_clobber_redzone_free(struct kunit *test)
> kmem_cache_destroy(s);
> }
>
> +
> +/*
> + * This case is simulating a real world case, that a device driver
> + * requests 18 bytes buffer, but the device HW has obligation to
> + * operate on 32 bits granularity, so it may actually read or write
> + * 20 bytes to the buffer, and possibly pollute 2 extra bytes after
> + * the requested space.
> + */
> +static void test_kmalloc_redzone_access(struct kunit *test)
> +{
> + u8 *p;
> +
> + if (!is_slub_debug_flags_enabled(SLAB_STORE_USER | SLAB_RED_ZONE))
> + kunit_skip(test, "Test required SLAB_STORE_USER & SLAB_RED_ZONE flags on");
Hrmm, this is not great. I didn't realize that we're testing kmalloc()
specific code, so we can't simply create test-specific caches as in the
other kunit tests.
What if we did create a fake kmalloc cache with the necessary flags and used
it with kmalloc_trace() instead of kmalloc()? We would be bypassing the
kmalloc() inline layer so theoretically orig_size handling bugs could be
introduced there that the test wouldn't catch, but I think that's rather
unlikely. Importantly we would still be stressing the orig_size saving and
the adjusted redzone check using this info.
> + p = kmalloc(18, GFP_KERNEL);
> +
> +#ifdef CONFIG_KFENCE
> + {
> + int max_retry = 10;
> +
> + while (is_kfence_address(p) && max_retry--) {
> + kfree(p);
> + p = kmalloc(18, GFP_KERNEL);
> + }
> +
> + if (!max_retry)
> + kunit_skip(test, "Fail to get non-kfenced memory");
> + }
> +#endif
With the test-specific cache we could also pass SLAB_SKIP_KFENCE there to
handle this. BTW, don't all slub kunit test need to do that in fact?
Thanks,
Vlastimil
> +
> + kasan_disable_current();
> +
> + p[18] = 0xab;
> + p[19] = 0xab;
> + kfree(p);
> +
> + KUNIT_EXPECT_EQ(test, 3, slab_errors);
> + kasan_enable_current();
> +}
> +
> static int test_init(struct kunit *test)
> {
> slab_errors = 0;
> @@ -139,6 +180,7 @@ static struct kunit_case test_cases[] = {
> #endif
>
> KUNIT_CASE(test_clobber_redzone_free),
> + KUNIT_CASE(test_kmalloc_redzone_access),
> {}
> };
>
> diff --git a/mm/slab.h b/mm/slab.h
> index e3b3231af742..72f7a85e01ab 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -413,6 +413,17 @@ static inline bool __slub_debug_enabled(void)
> {
> return static_branch_unlikely(&slub_debug_enabled);
> }
> +
> +extern slab_flags_t slub_debug;
> +
> +/*
> + * This should only be used in post-boot time, after 'slub_debug'
> + * gets initialized.
> + */
> +static inline bool is_slub_debug_flags_enabled(slab_flags_t flags)
> +{
> + return (slub_debug & flags) == flags;
> +}
> #else
> static inline void print_tracking(struct kmem_cache *s, void *object)
> {
> @@ -421,6 +432,10 @@ static inline bool __slub_debug_enabled(void)
> {
> return false;
> }
> +static inline bool is_slub_debug_flags_enabled(slab_flags_t flags)
> +{
> + return false;
> +}
> #endif
>
> /*
> diff --git a/mm/slub.c b/mm/slub.c
> index a24b71041b26..6ef72b8f6291 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -638,9 +638,9 @@ static inline void *restore_red_left(struct kmem_cache *s, void *p)
> * Debug settings:
> */
> #if defined(CONFIG_SLUB_DEBUG_ON)
> -static slab_flags_t slub_debug = DEBUG_DEFAULT_FLAGS;
> +slab_flags_t slub_debug = DEBUG_DEFAULT_FLAGS;
> #else
> -static slab_flags_t slub_debug;
> +slab_flags_t slub_debug;
> #endif
>
> static char *slub_debug_string;
On Wed, Nov 23, 2022 at 10:48:50AM +0100, Vlastimil Babka wrote:
> On 11/21/22 07:38, Feng Tang wrote:
> > On Fri, Nov 11, 2022 at 04:29:43PM +0800, Tang, Feng wrote:
> >> On Fri, Nov 11, 2022 at 04:16:32PM +0800, Vlastimil Babka wrote:
> >> > > for (shift = 3; shift <= 12; shift++) {
> >> > > size = 1 << shift;
> >> > > buf = kmalloc(size + 4, GFP_KERNEL);
> >> > > /* We have 96, 196 kmalloc size, which is not power of 2 */
> >> > > if (size == 64 || size == 128)
> >> > > oob_size = 16;
> >> > > else
> >> > > oob_size = size - 4;
> >> > > memset(buf + size + 4, 0xee, oob_size);
> >> > > kfree(buf);
> >> > > }
> >> >
> >> > Sounds like a new slub_kunit test would be useful? :) doesn't need to be
> >> > that exhaustive wrt all sizes, we could just pick one and check that a write
> >> > beyond requested kmalloc size is detected?
> >>
> >> Just git-grepped out slub_kunit.c :), will try to add a case to it.
> >> I'll also check if the case will also be caught by other sanitizer
> >> tools like kasan/kfence etc.
> >
> > Just checked, kasan has already has API to disable kasan check
> > temporarily, and I did see sometime kfence can chime in (4 out of 178
> > runs) so we need skip kfenced address.
> >
> > Here is the draft patch, thanks!
> >
> > From 45bf8d0072e532f43063dbda44c6bb3adcc388b6 Mon Sep 17 00:00:00 2001
> > From: Feng Tang <[email protected]>
> > Date: Mon, 21 Nov 2022 13:17:11 +0800
> > Subject: [PATCH] mm/slub, kunit: Add a case for kmalloc redzone functionality
> >
> > kmalloc redzone check for slub has been merged, and it's better to add
> > a kunit case for it, which is inspired by a real-world case as described
> > in commit 120ee599b5bf ("staging: octeon-usb: prevent memory corruption"):
> >
> > "
> > octeon-hcd will crash the kernel when SLOB is used. This usually happens
> > after the 18-byte control transfer when a device descriptor is read.
> > The DMA engine is always transfering full 32-bit words and if the
> > transfer is shorter, some random garbage appears after the buffer.
> > The problem is not visible with SLUB since it rounds up the allocations
> > to word boundary, and the extra bytes will go undetected.
> > "
> > Suggested-by: Vlastimil Babka <[email protected]>
> > Signed-off-by: Feng Tang <[email protected]>
> > ---
> > lib/slub_kunit.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> > mm/slab.h | 15 +++++++++++++++
> > mm/slub.c | 4 ++--
> > 3 files changed, 59 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/slub_kunit.c b/lib/slub_kunit.c
> > index 7a0564d7cb7a..0653eed19bff 100644
> > --- a/lib/slub_kunit.c
> > +++ b/lib/slub_kunit.c
> > @@ -120,6 +120,47 @@ static void test_clobber_redzone_free(struct kunit *test)
> > kmem_cache_destroy(s);
> > }
> >
> > +
> > +/*
> > + * This case is simulating a real world case, that a device driver
> > + * requests 18 bytes buffer, but the device HW has obligation to
> > + * operate on 32 bits granularity, so it may actually read or write
> > + * 20 bytes to the buffer, and possibly pollute 2 extra bytes after
> > + * the requested space.
> > + */
> > +static void test_kmalloc_redzone_access(struct kunit *test)
> > +{
> > + u8 *p;
> > +
> > + if (!is_slub_debug_flags_enabled(SLAB_STORE_USER | SLAB_RED_ZONE))
> > + kunit_skip(test, "Test required SLAB_STORE_USER & SLAB_RED_ZONE flags on");
>
> Hrmm, this is not great. I didn't realize that we're testing kmalloc()
> specific code, so we can't simply create test-specific caches as in the
> other kunit tests.
> What if we did create a fake kmalloc cache with the necessary flags and used
> it with kmalloc_trace() instead of kmalloc()? We would be bypassing the
> kmalloc() inline layer so theoretically orig_size handling bugs could be
> introduced there that the test wouldn't catch, but I think that's rather
> unlikely. Importantly we would still be stressing the orig_size saving and
> the adjusted redzone check using this info.
Nice trick! Will go this way.
> > + p = kmalloc(18, GFP_KERNEL);
> > +
> > +#ifdef CONFIG_KFENCE
> > + {
> > + int max_retry = 10;
> > +
> > + while (is_kfence_address(p) && max_retry--) {
> > + kfree(p);
> > + p = kmalloc(18, GFP_KERNEL);
> > + }
> > +
> > + if (!max_retry)
> > + kunit_skip(test, "Fail to get non-kfenced memory");
> > + }
> > +#endif
>
> With the test-specific cache we could also pass SLAB_SKIP_KFENCE there to
> handle this.
Yep, the handling will be much simpler, thanks
>
> BTW, don't all slub kunit test need to do that in fact?
Yes, I think they also need.
With default kfence setting test, kence address wasn't hit in
250 times of boot test. And by changing CONFIG_KFENCE_NUM_OBJECTS
from 255 to 16383, and CONFIG_KFENCE_SAMPLE_INTERVAL from 100
to 5, the kfence allocation did hit once in about 300 tims of
boot test.
Will add the flag bit for all kmem_cache creation.
Thanks,
Feng
> Thanks,
> Vlastimil