| Message ID | 20240613153924.961511-13-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kmsan: Enable on s390 | expand |
Hi Ilya, On Thu, 13 Jun 2024 17:34:14 +0200 Ilya Leoshkevich <iii@linux.ibm.com> wrote: > Avoid false KMSAN negatives with SLUB_DEBUG by allowing > kmsan_slab_free() to poison the freed memory, and by preventing > init_object() from unpoisoning new allocations by using __memset(). > > There are two alternatives to this approach. First, init_object() > can be marked with __no_sanitize_memory. This annotation should be used > with great care, because it drops all instrumentation from the > function, and any shadow writes will be lost. Even though this is not a > concern with the current init_object() implementation, this may change > in the future. > > Second, kmsan_poison_memory() calls may be added after memset() calls. > The downside is that init_object() is called from > free_debug_processing(), in which case poisoning will erase the > distinction between simply uninitialized memory and UAF. > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > --- > mm/kmsan/hooks.c | 2 +- > mm/slub.c | 13 +++++++++---- > 2 files changed, 10 insertions(+), 5 deletions(-) > [...] > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -1139,7 +1139,12 @@ static void init_object(struct kmem_cache *s, void *object, u8 val) > unsigned int poison_size = s->object_size; > > if (s->flags & SLAB_RED_ZONE) { > - memset(p - s->red_left_pad, val, s->red_left_pad); > + /* > + * Use __memset() here and below in order to avoid overwriting > + * the KMSAN shadow. Keeping the shadow makes it possible to > + * distinguish uninit-value from use-after-free. > + */ > + __memset(p - s->red_left_pad, val, s->red_left_pad); I found my build test[1] fails with below error on latest mm-unstable branch. 'git bisect' points me this patch. CC mm/slub.o /mm/slub.c: In function 'init_object': /mm/slub.c:1147:17: error: implicit declaration of function '__memset'; did you mean 'memset'? [-Werror=implicit-function-declaration] 1147 | __memset(p - s->red_left_pad, val, s->red_left_pad); | ^~~~~~~~ | memset cc1: some warnings being treated as errors I haven't looked in deep, but reporting first. Do you have any idea? [1] https://github.com/awslabs/damon-tests/blob/next/corr/tests/build_m68k.sh Thanks, SJ [...]
diff --git a/mm/kmsan/hooks.c b/mm/kmsan/hooks.c index 267d0afa2e8b..26d86dfdc819 100644 --- a/mm/kmsan/hooks.c +++ b/mm/kmsan/hooks.c @@ -74,7 +74,7 @@ void kmsan_slab_free(struct kmem_cache *s, void *object) return; /* RCU slabs could be legally used after free within the RCU period */ - if (unlikely(s->flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON))) + if (unlikely(s->flags & SLAB_TYPESAFE_BY_RCU)) return; /* * If there's a constructor, freed memory must remain in the same state diff --git a/mm/slub.c b/mm/slub.c index 1373ac365a46..4dd55cabe701 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -1139,7 +1139,12 @@ static void init_object(struct kmem_cache *s, void *object, u8 val) unsigned int poison_size = s->object_size; if (s->flags & SLAB_RED_ZONE) { - memset(p - s->red_left_pad, val, s->red_left_pad); + /* + * Use __memset() here and below in order to avoid overwriting + * the KMSAN shadow. Keeping the shadow makes it possible to + * distinguish uninit-value from use-after-free. + */ + __memset(p - s->red_left_pad, val, s->red_left_pad); if (slub_debug_orig_size(s) && val == SLUB_RED_ACTIVE) { /* @@ -1152,12 +1157,12 @@ static void init_object(struct kmem_cache *s, void *object, u8 val) } if (s->flags & __OBJECT_POISON) { - memset(p, POISON_FREE, poison_size - 1); - p[poison_size - 1] = POISON_END; + __memset(p, POISON_FREE, poison_size - 1); + __memset(p + poison_size - 1, POISON_END, 1); } if (s->flags & SLAB_RED_ZONE) - memset(p + poison_size, val, s->inuse - poison_size); + __memset(p + poison_size, val, s->inuse - poison_size); } static void restore_bytes(struct kmem_cache *s, char *message, u8 data,
Avoid false KMSAN negatives with SLUB_DEBUG by allowing kmsan_slab_free() to poison the freed memory, and by preventing init_object() from unpoisoning new allocations by using __memset(). There are two alternatives to this approach. First, init_object() can be marked with __no_sanitize_memory. This annotation should be used with great care, because it drops all instrumentation from the function, and any shadow writes will be lost. Even though this is not a concern with the current init_object() implementation, this may change in the future. Second, kmsan_poison_memory() calls may be added after memset() calls. The downside is that init_object() is called from free_debug_processing(), in which case poisoning will erase the distinction between simply uninitialized memory and UAF. Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- mm/kmsan/hooks.c | 2 +- mm/slub.c | 13 +++++++++---- 2 files changed, 10 insertions(+), 5 deletions(-)