| Message ID | 20240613153924.961511-13-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| 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 [...]
On Thu, 2024-06-13 at 16:30 -0700, SeongJae Park wrote: > 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 > > [...] Thanks for the report. Apparently not all architectures have __memset(). We should probably go back to memset_no_sanitize_memory() [1], but this time mark it with noinline __maybe_unused __no_sanitize_memory, like it's done in, e.g., 32/35. Alexander, what do you think? [1] https://lore.kernel.org/lkml/20231121220155.1217090-14-iii@linux.ibm.com/
On Fri, Jun 14, 2024 at 1:44 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > On Thu, 2024-06-13 at 16:30 -0700, SeongJae Park wrote: > > 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 > > > > [...] > > Thanks for the report. > > Apparently not all architectures have __memset(). We should probably go > back to memset_no_sanitize_memory() [1], but this time mark it with > noinline __maybe_unused __no_sanitize_memory, like it's done in, e.g., > 32/35. > > Alexander, what do you think? We could probably go without __no_sanitize_memory assuming that platforms supporting KMSAN always have __memset(): #if defined(CONFIG_KMSAN) static inline void *memset_no_sanitize_memory(void *s, int c, size_t n) { return __memset(s, c, n); } #else static inline void *memset_no_sanitize_memory(void *s, int c, size_t n) { return memset(s, c, n); } #endif
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(-)