| Message ID | 20210612045156.44763-3-kylee0686026@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kasan: add memory corruption identification for hw tag-based kasan | expand |
On Sat, 12 Jun 2021 at 06:52, Kuan-Ying Lee <kylee0686026@gmail.com> wrote: > 1. Move kasan_get_free_track() and kasan_set_free_info() > into tags.c > 2. Move kasan_get_bug_type() to header file > > Signed-off-by: Kuan-Ying Lee <kylee0686026@gmail.com> > Suggested-by: Marco Elver <elver@google.com> > 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: Andrew Morton <akpm@linux-foundation.org> > --- > mm/kasan/Makefile | 4 +-- > mm/kasan/hw_tags.c | 22 --------------- > mm/kasan/report_hw_tags.c | 6 +--- > mm/kasan/report_sw_tags.c | 46 +------------------------------ > mm/kasan/report_tags.h | 56 +++++++++++++++++++++++++++++++++++++ > mm/kasan/sw_tags.c | 41 --------------------------- > mm/kasan/tags.c | 58 +++++++++++++++++++++++++++++++++++++++ > 7 files changed, 118 insertions(+), 115 deletions(-) > create mode 100644 mm/kasan/report_tags.h > create mode 100644 mm/kasan/tags.c [...] > diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c > index ed5e5b833d61..4ea8c368b5b8 100644 > --- a/mm/kasan/hw_tags.c > +++ b/mm/kasan/hw_tags.c > @@ -216,28 +216,6 @@ void __init kasan_init_hw_tags(void) > pr_info("KernelAddressSanitizer initialized\n"); > } > > -void kasan_set_free_info(struct kmem_cache *cache, > - void *object, u8 tag) > -{ > - struct kasan_alloc_meta *alloc_meta; > - > - alloc_meta = kasan_get_alloc_meta(cache, object); > - if (alloc_meta) > - kasan_set_track(&alloc_meta->free_track[0], GFP_NOWAIT); > -} > - > -struct kasan_track *kasan_get_free_track(struct kmem_cache *cache, > - void *object, u8 tag) > -{ > - struct kasan_alloc_meta *alloc_meta; > - > - alloc_meta = kasan_get_alloc_meta(cache, object); > - if (!alloc_meta) > - return NULL; > - > - return &alloc_meta->free_track[0]; > -} > - > void kasan_alloc_pages(struct page *page, unsigned int order, gfp_t flags) > { > /* > diff --git a/mm/kasan/report_hw_tags.c b/mm/kasan/report_hw_tags.c > index 42b2168755d6..ef5e7378f3aa 100644 > --- a/mm/kasan/report_hw_tags.c > +++ b/mm/kasan/report_hw_tags.c > @@ -14,11 +14,7 @@ > #include <linux/types.h> > > #include "kasan.h" > - > -const char *kasan_get_bug_type(struct kasan_access_info *info) > -{ > - return "invalid-access"; > -} > +#include "report_tags.h" > > void *kasan_find_first_bad_addr(void *addr, size_t size) > { > diff --git a/mm/kasan/report_sw_tags.c b/mm/kasan/report_sw_tags.c > index 821a14a19a92..d965a170083e 100644 > --- a/mm/kasan/report_sw_tags.c > +++ b/mm/kasan/report_sw_tags.c > @@ -26,51 +26,7 @@ > > #include <asm/sections.h> > > -#include "kasan.h" > -#include "../slab.h" > - > -const char *kasan_get_bug_type(struct kasan_access_info *info) > -{ > -#ifdef CONFIG_KASAN_TAGS_IDENTIFY > - struct kasan_alloc_meta *alloc_meta; > - struct kmem_cache *cache; > - struct page *page; > - const void *addr; > - void *object; > - u8 tag; > - int i; > - > - tag = get_tag(info->access_addr); > - addr = kasan_reset_tag(info->access_addr); > - page = kasan_addr_to_page(addr); > - if (page && PageSlab(page)) { > - cache = page->slab_cache; > - object = nearest_obj(cache, page, (void *)addr); > - alloc_meta = kasan_get_alloc_meta(cache, object); > - > - if (alloc_meta) { > - for (i = 0; i < KASAN_NR_FREE_STACKS; i++) { > - if (alloc_meta->free_pointer_tag[i] == tag) > - return "use-after-free"; > - } > - } > - return "out-of-bounds"; > - } > - > -#endif > - /* > - * If access_size is a negative number, then it has reason to be > - * defined as out-of-bounds bug type. > - * > - * Casting negative numbers to size_t would indeed turn up as > - * a large size_t and its value will be larger than ULONG_MAX/2, > - * so that this can qualify as out-of-bounds. > - */ > - if (info->access_addr + info->access_size < info->access_addr) > - return "out-of-bounds"; > - > - return "invalid-access"; > -} > +#include "report_tags.h" > > void *kasan_find_first_bad_addr(void *addr, size_t size) > { > diff --git a/mm/kasan/report_tags.h b/mm/kasan/report_tags.h > new file mode 100644 > index 000000000000..4f740d4d99ee > --- /dev/null > +++ b/mm/kasan/report_tags.h > @@ -0,0 +1,56 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __MM_KASAN_REPORT_TAGS_H > +#define __MM_KASAN_REPORT_TAGS_H > + > +#include "kasan.h" > +#include "../slab.h" > + > +#ifdef CONFIG_KASAN_TAGS_IDENTIFY > +const char *kasan_get_bug_type(struct kasan_access_info *info) > +{ [...] > + /* > + * If access_size is a negative number, then it has reason to be > + * defined as out-of-bounds bug type. > + * > + * Casting negative numbers to size_t would indeed turn up as > + * a large size_t and its value will be larger than ULONG_MAX/2, > + * so that this can qualify as out-of-bounds. > + */ > + if (info->access_addr + info->access_size < info->access_addr) > + return "out-of-bounds"; This seems to change behaviour for SW_TAGS because it was there even if !CONFIG_KASAN_TAGS_IDENTIFY. Does it still work as before? > + > + return "invalid-access"; > +} > +#else > +const char *kasan_get_bug_type(struct kasan_access_info *info) > +{ > + return "invalid-access"; > +} > +#endif > + > +#endif > diff --git a/mm/kasan/sw_tags.c b/mm/kasan/sw_tags.c > index dd05e6c801fa..bd3f540feb47 100644 > --- a/mm/kasan/sw_tags.c > +++ b/mm/kasan/sw_tags.c > @@ -167,47 +167,6 @@ void __hwasan_tag_memory(unsigned long addr, u8 tag, unsigned long size) > } > EXPORT_SYMBOL(__hwasan_tag_memory); > > -void kasan_set_free_info(struct kmem_cache *cache, > - void *object, u8 tag) > -{ > - struct kasan_alloc_meta *alloc_meta; > - u8 idx = 0; > - > - alloc_meta = kasan_get_alloc_meta(cache, object); > - if (!alloc_meta) > - return; > - > -#ifdef CONFIG_KASAN_TAGS_IDENTIFY > - idx = alloc_meta->free_track_idx; > - alloc_meta->free_pointer_tag[idx] = tag; > - alloc_meta->free_track_idx = (idx + 1) % KASAN_NR_FREE_STACKS; > -#endif > - > - kasan_set_track(&alloc_meta->free_track[idx], GFP_NOWAIT); > -} > - > -struct kasan_track *kasan_get_free_track(struct kmem_cache *cache, > - void *object, u8 tag) > -{ > - struct kasan_alloc_meta *alloc_meta; > - int i = 0; > - > - alloc_meta = kasan_get_alloc_meta(cache, object); > - if (!alloc_meta) > - return NULL; > - > -#ifdef CONFIG_KASAN_TAGS_IDENTIFY > - for (i = 0; i < KASAN_NR_FREE_STACKS; i++) { > - if (alloc_meta->free_pointer_tag[i] == tag) > - break; > - } > - if (i == KASAN_NR_FREE_STACKS) > - i = alloc_meta->free_track_idx; > -#endif > - > - return &alloc_meta->free_track[i]; > -} > - > void kasan_tag_mismatch(unsigned long addr, unsigned long access_info, > unsigned long ret_ip) > { > diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c > new file mode 100644 > index 000000000000..9c33c0ebe1d1 > --- /dev/null > +++ b/mm/kasan/tags.c > @@ -0,0 +1,58 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * This file contains common tag-based KASAN code. > + * > + * Author: Kuan-Ying Lee <kylee0686026@gmail.com> We appreciate your work on this, but this is misleading. Because you merely copied/moved the code, have a look what sw_tags.c says -- that should either be preserved, or we add nothing here. I prefer to add nothing or the bare minimum (e.g. if the company requires a Copyright line) for non-substantial additions because this stuff becomes out-of-date fast and just isn't useful at all. 'git log' is the source of truth. Cc'ing Greg for process advice. For moved code, does it have to preserve the original Copyright line if there was one? Thanks, -- Marco
On Sat, Jun 12, 2021 at 04:42:44PM +0200, Marco Elver wrote: > On Sat, 12 Jun 2021 at 06:52, Kuan-Ying Lee <kylee0686026@gmail.com> wrote: > > 1. Move kasan_get_free_track() and kasan_set_free_info() > > into tags.c > > 2. Move kasan_get_bug_type() to header file > > > > Signed-off-by: Kuan-Ying Lee <kylee0686026@gmail.com> > > Suggested-by: Marco Elver <elver@google.com> > > 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: Andrew Morton <akpm@linux-foundation.org> > > --- > > mm/kasan/Makefile | 4 +-- > > mm/kasan/hw_tags.c | 22 --------------- > > mm/kasan/report_hw_tags.c | 6 +--- > > mm/kasan/report_sw_tags.c | 46 +------------------------------ > > mm/kasan/report_tags.h | 56 +++++++++++++++++++++++++++++++++++++ > > mm/kasan/sw_tags.c | 41 --------------------------- > > mm/kasan/tags.c | 58 +++++++++++++++++++++++++++++++++++++++ > > 7 files changed, 118 insertions(+), 115 deletions(-) > > create mode 100644 mm/kasan/report_tags.h > > create mode 100644 mm/kasan/tags.c > [...] > > diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c > > index ed5e5b833d61..4ea8c368b5b8 100644 > > --- a/mm/kasan/hw_tags.c > > +++ b/mm/kasan/hw_tags.c > > @@ -216,28 +216,6 @@ void __init kasan_init_hw_tags(void) > > pr_info("KernelAddressSanitizer initialized\n"); > > } > > > > -void kasan_set_free_info(struct kmem_cache *cache, > > - void *object, u8 tag) > > -{ > > - struct kasan_alloc_meta *alloc_meta; > > - > > - alloc_meta = kasan_get_alloc_meta(cache, object); > > - if (alloc_meta) > > - kasan_set_track(&alloc_meta->free_track[0], GFP_NOWAIT); > > -} > > - > > -struct kasan_track *kasan_get_free_track(struct kmem_cache *cache, > > - void *object, u8 tag) > > -{ > > - struct kasan_alloc_meta *alloc_meta; > > - > > - alloc_meta = kasan_get_alloc_meta(cache, object); > > - if (!alloc_meta) > > - return NULL; > > - > > - return &alloc_meta->free_track[0]; > > -} > > - > > void kasan_alloc_pages(struct page *page, unsigned int order, gfp_t flags) > > { > > /* > > diff --git a/mm/kasan/report_hw_tags.c b/mm/kasan/report_hw_tags.c > > index 42b2168755d6..ef5e7378f3aa 100644 > > --- a/mm/kasan/report_hw_tags.c > > +++ b/mm/kasan/report_hw_tags.c > > @@ -14,11 +14,7 @@ > > #include <linux/types.h> > > > > #include "kasan.h" > > - > > -const char *kasan_get_bug_type(struct kasan_access_info *info) > > -{ > > - return "invalid-access"; > > -} > > +#include "report_tags.h" > > > > void *kasan_find_first_bad_addr(void *addr, size_t size) > > { > > diff --git a/mm/kasan/report_sw_tags.c b/mm/kasan/report_sw_tags.c > > index 821a14a19a92..d965a170083e 100644 > > --- a/mm/kasan/report_sw_tags.c > > +++ b/mm/kasan/report_sw_tags.c > > @@ -26,51 +26,7 @@ > > > > #include <asm/sections.h> > > > > -#include "kasan.h" > > -#include "../slab.h" > > - > > -const char *kasan_get_bug_type(struct kasan_access_info *info) > > -{ > > -#ifdef CONFIG_KASAN_TAGS_IDENTIFY > > - struct kasan_alloc_meta *alloc_meta; > > - struct kmem_cache *cache; > > - struct page *page; > > - const void *addr; > > - void *object; > > - u8 tag; > > - int i; > > - > > - tag = get_tag(info->access_addr); > > - addr = kasan_reset_tag(info->access_addr); > > - page = kasan_addr_to_page(addr); > > - if (page && PageSlab(page)) { > > - cache = page->slab_cache; > > - object = nearest_obj(cache, page, (void *)addr); > > - alloc_meta = kasan_get_alloc_meta(cache, object); > > - > > - if (alloc_meta) { > > - for (i = 0; i < KASAN_NR_FREE_STACKS; i++) { > > - if (alloc_meta->free_pointer_tag[i] == tag) > > - return "use-after-free"; > > - } > > - } > > - return "out-of-bounds"; > > - } > > - > > -#endif > > - /* > > - * If access_size is a negative number, then it has reason to be > > - * defined as out-of-bounds bug type. > > - * > > - * Casting negative numbers to size_t would indeed turn up as > > - * a large size_t and its value will be larger than ULONG_MAX/2, > > - * so that this can qualify as out-of-bounds. > > - */ > > - if (info->access_addr + info->access_size < info->access_addr) > > - return "out-of-bounds"; > > - > > - return "invalid-access"; > > -} > > +#include "report_tags.h" > > > > void *kasan_find_first_bad_addr(void *addr, size_t size) > > { > > diff --git a/mm/kasan/report_tags.h b/mm/kasan/report_tags.h > > new file mode 100644 > > index 000000000000..4f740d4d99ee > > --- /dev/null > > +++ b/mm/kasan/report_tags.h > > @@ -0,0 +1,56 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +#ifndef __MM_KASAN_REPORT_TAGS_H > > +#define __MM_KASAN_REPORT_TAGS_H > > + > > +#include "kasan.h" > > +#include "../slab.h" > > + > > +#ifdef CONFIG_KASAN_TAGS_IDENTIFY > > +const char *kasan_get_bug_type(struct kasan_access_info *info) > > +{ > [...] > > + /* > > + * If access_size is a negative number, then it has reason to be > > + * defined as out-of-bounds bug type. > > + * > > + * Casting negative numbers to size_t would indeed turn up as > > + * a large size_t and its value will be larger than ULONG_MAX/2, > > + * so that this can qualify as out-of-bounds. > > + */ > > + if (info->access_addr + info->access_size < info->access_addr) > > + return "out-of-bounds"; > > This seems to change behaviour for SW_TAGS because it was there even > if !CONFIG_KASAN_TAGS_IDENTIFY. Does it still work as before? > You are right. It will change the behavior. However, I think that if !CONFIG_KASAN_TAG_IDENTIFY, it should be reported "invalid-access". Or is it better to keep it in both conditions? > > + > > + return "invalid-access"; > > +} > > +#else > > +const char *kasan_get_bug_type(struct kasan_access_info *info) > > +{ > > + return "invalid-access"; > > +} > > +#endif > > + > > +#endif > > diff --git a/mm/kasan/sw_tags.c b/mm/kasan/sw_tags.c > > index dd05e6c801fa..bd3f540feb47 100644 > > --- a/mm/kasan/sw_tags.c > > +++ b/mm/kasan/sw_tags.c > > @@ -167,47 +167,6 @@ void __hwasan_tag_memory(unsigned long addr, u8 tag, unsigned long size) > > } > > EXPORT_SYMBOL(__hwasan_tag_memory); > > > > -void kasan_set_free_info(struct kmem_cache *cache, > > - void *object, u8 tag) > > -{ > > - struct kasan_alloc_meta *alloc_meta; > > - u8 idx = 0; > > - > > - alloc_meta = kasan_get_alloc_meta(cache, object); > > - if (!alloc_meta) > > - return; > > - > > -#ifdef CONFIG_KASAN_TAGS_IDENTIFY > > - idx = alloc_meta->free_track_idx; > > - alloc_meta->free_pointer_tag[idx] = tag; > > - alloc_meta->free_track_idx = (idx + 1) % KASAN_NR_FREE_STACKS; > > -#endif > > - > > - kasan_set_track(&alloc_meta->free_track[idx], GFP_NOWAIT); > > -} > > - > > -struct kasan_track *kasan_get_free_track(struct kmem_cache *cache, > > - void *object, u8 tag) > > -{ > > - struct kasan_alloc_meta *alloc_meta; > > - int i = 0; > > - > > - alloc_meta = kasan_get_alloc_meta(cache, object); > > - if (!alloc_meta) > > - return NULL; > > - > > -#ifdef CONFIG_KASAN_TAGS_IDENTIFY > > - for (i = 0; i < KASAN_NR_FREE_STACKS; i++) { > > - if (alloc_meta->free_pointer_tag[i] == tag) > > - break; > > - } > > - if (i == KASAN_NR_FREE_STACKS) > > - i = alloc_meta->free_track_idx; > > -#endif > > - > > - return &alloc_meta->free_track[i]; > > -} > > - > > void kasan_tag_mismatch(unsigned long addr, unsigned long access_info, > > unsigned long ret_ip) > > { > > diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c > > new file mode 100644 > > index 000000000000..9c33c0ebe1d1 > > --- /dev/null > > +++ b/mm/kasan/tags.c > > @@ -0,0 +1,58 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * This file contains common tag-based KASAN code. > > + * > > + * Author: Kuan-Ying Lee <kylee0686026@gmail.com> > > We appreciate your work on this, but this is misleading. Because you > merely copied/moved the code, have a look what sw_tags.c says -- that > should either be preserved, or we add nothing here. > > I prefer to add nothing or the bare minimum (e.g. if the company > requires a Copyright line) for non-substantial additions because this > stuff becomes out-of-date fast and just isn't useful at all. 'git log' > is the source of truth. This was my first time to upload a new file. Thanks for the suggestions. :) I will remove this author tag and wait for Greg's process advice. > > Cc'ing Greg for process advice. For moved code, does it have to > preserve the original Copyright line if there was one? > > Thanks, > -- Marco
On Sat, Jun 12, 2021 at 04:42:44PM +0200, Marco Elver wrote: > On Sat, 12 Jun 2021 at 06:52, Kuan-Ying Lee <kylee0686026@gmail.com> wrote: > > diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c > > new file mode 100644 > > index 000000000000..9c33c0ebe1d1 > > --- /dev/null > > +++ b/mm/kasan/tags.c > > @@ -0,0 +1,58 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * This file contains common tag-based KASAN code. > > + * > > + * Author: Kuan-Ying Lee <kylee0686026@gmail.com> > > We appreciate your work on this, but this is misleading. Because you > merely copied/moved the code, have a look what sw_tags.c says -- that > should either be preserved, or we add nothing here. > > I prefer to add nothing or the bare minimum (e.g. if the company > requires a Copyright line) for non-substantial additions because this > stuff becomes out-of-date fast and just isn't useful at all. 'git log' > is the source of truth. > > Cc'ing Greg for process advice. For moved code, does it have to > preserve the original Copyright line if there was one? Yes, it does have to. Unless you want to talk to a lot of lawyers about the issues involved here and can defend the removal of the copyright lines to them. So please keep them. Unless you can get your corporate lawyer to sign off on the patch that does the removal. thanks, greg k-h
On Sat, 12 Jun 2021 at 17:51, Kuan-Ying Lee <kylee0686026@gmail.com> wrote: [...] > > > diff --git a/mm/kasan/report_tags.h b/mm/kasan/report_tags.h > > > new file mode 100644 > > > index 000000000000..4f740d4d99ee > > > --- /dev/null > > > +++ b/mm/kasan/report_tags.h > > > @@ -0,0 +1,56 @@ > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > +#ifndef __MM_KASAN_REPORT_TAGS_H > > > +#define __MM_KASAN_REPORT_TAGS_H > > > + > > > +#include "kasan.h" > > > +#include "../slab.h" > > > + > > > +#ifdef CONFIG_KASAN_TAGS_IDENTIFY > > > +const char *kasan_get_bug_type(struct kasan_access_info *info) > > > +{ > > [...] > > > + /* > > > + * If access_size is a negative number, then it has reason to be > > > + * defined as out-of-bounds bug type. > > > + * > > > + * Casting negative numbers to size_t would indeed turn up as > > > + * a large size_t and its value will be larger than ULONG_MAX/2, > > > + * so that this can qualify as out-of-bounds. > > > + */ > > > + if (info->access_addr + info->access_size < info->access_addr) > > > + return "out-of-bounds"; > > > > This seems to change behaviour for SW_TAGS because it was there even > > if !CONFIG_KASAN_TAGS_IDENTIFY. Does it still work as before? > > > > You are right. It will change the behavior. > However, I think that if !CONFIG_KASAN_TAG_IDENTIFY, it should be reported > "invalid-access". There's no reason that if !CONFIG_KASAN_TAG_IDENTIFY it should be reported as "invalid-acces" if we can do better without the additional state that the config option introduces. It's trivial to give a slightly better report without additional state, see the comment explaining why it's reasonable to infer out-of-bounds here. > Or is it better to keep it in both conditions? We want to make this patch a non-functional change. [...] > > > diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c > > > new file mode 100644 > > > index 000000000000..9c33c0ebe1d1 > > > --- /dev/null > > > +++ b/mm/kasan/tags.c > > > @@ -0,0 +1,58 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * This file contains common tag-based KASAN code. > > > + * > > > + * Author: Kuan-Ying Lee <kylee0686026@gmail.com> > > > > We appreciate your work on this, but this is misleading. Because you > > merely copied/moved the code, have a look what sw_tags.c says -- that > > should either be preserved, or we add nothing here. > > > > I prefer to add nothing or the bare minimum (e.g. if the company > > requires a Copyright line) for non-substantial additions because this > > stuff becomes out-of-date fast and just isn't useful at all. 'git log' > > is the source of truth. > > This was my first time to upload a new file. > Thanks for the suggestions. :) > I will remove this author tag and wait for Greg's process advice. > > > > > Cc'ing Greg for process advice. For moved code, does it have to > > preserve the original Copyright line if there was one? Greg responded, see his emails. Please preserve the original header from the file the code was moved from (hw_tags.c/sw_tags.c). Thanks, -- Marco
On Mon, Jun 14, 2021 at 10:48:27AM +0200, Marco Elver wrote: > On Sat, 12 Jun 2021 at 17:51, Kuan-Ying Lee <kylee0686026@gmail.com> wrote: > [...] > > > > diff --git a/mm/kasan/report_tags.h b/mm/kasan/report_tags.h > > > > new file mode 100644 > > > > index 000000000000..4f740d4d99ee > > > > --- /dev/null > > > > +++ b/mm/kasan/report_tags.h > > > > @@ -0,0 +1,56 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > > +#ifndef __MM_KASAN_REPORT_TAGS_H > > > > +#define __MM_KASAN_REPORT_TAGS_H > > > > + > > > > +#include "kasan.h" > > > > +#include "../slab.h" > > > > + > > > > +#ifdef CONFIG_KASAN_TAGS_IDENTIFY > > > > +const char *kasan_get_bug_type(struct kasan_access_info *info) > > > > +{ > > > [...] > > > > + /* > > > > + * If access_size is a negative number, then it has reason to be > > > > + * defined as out-of-bounds bug type. > > > > + * > > > > + * Casting negative numbers to size_t would indeed turn up as > > > > + * a large size_t and its value will be larger than ULONG_MAX/2, > > > > + * so that this can qualify as out-of-bounds. > > > > + */ > > > > + if (info->access_addr + info->access_size < info->access_addr) > > > > + return "out-of-bounds"; > > > > > > This seems to change behaviour for SW_TAGS because it was there even > > > if !CONFIG_KASAN_TAGS_IDENTIFY. Does it still work as before? > > > > > > > You are right. It will change the behavior. > > However, I think that if !CONFIG_KASAN_TAG_IDENTIFY, it should be reported > > "invalid-access". > > There's no reason that if !CONFIG_KASAN_TAG_IDENTIFY it should be > reported as "invalid-acces" if we can do better without the additional > state that the config option introduces. > > It's trivial to give a slightly better report without additional > state, see the comment explaining why it's reasonable to infer > out-of-bounds here. > > > Or is it better to keep it in both conditions? > > We want to make this patch a non-functional change. > Got it. > [...] > > > > diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c > > > > new file mode 100644 > > > > index 000000000000..9c33c0ebe1d1 > > > > --- /dev/null > > > > +++ b/mm/kasan/tags.c > > > > @@ -0,0 +1,58 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > +/* > > > > + * This file contains common tag-based KASAN code. > > > > + * > > > > + * Author: Kuan-Ying Lee <kylee0686026@gmail.com> > > > > > > We appreciate your work on this, but this is misleading. Because you > > > merely copied/moved the code, have a look what sw_tags.c says -- that > > > should either be preserved, or we add nothing here. > > > > > > I prefer to add nothing or the bare minimum (e.g. if the company > > > requires a Copyright line) for non-substantial additions because this > > > stuff becomes out-of-date fast and just isn't useful at all. 'git log' > > > is the source of truth. > > > > This was my first time to upload a new file. > > Thanks for the suggestions. :) > > I will remove this author tag and wait for Greg's process advice. > > > > > > > > Cc'ing Greg for process advice. For moved code, does it have to > > > preserve the original Copyright line if there was one? > > Greg responded, see his emails. Please preserve the original header > from the file the code was moved from (hw_tags.c/sw_tags.c). Ok. I will do it in v3. Thanks. > > Thanks, > -- Marco
diff --git a/mm/kasan/Makefile b/mm/kasan/Makefile index 9fe39a66388a..634de6c1da9b 100644 --- a/mm/kasan/Makefile +++ b/mm/kasan/Makefile @@ -37,5 +37,5 @@ CFLAGS_sw_tags.o := $(CC_FLAGS_KASAN_RUNTIME) obj-$(CONFIG_KASAN) := common.o report.o obj-$(CONFIG_KASAN_GENERIC) += init.o generic.o report_generic.o shadow.o quarantine.o -obj-$(CONFIG_KASAN_HW_TAGS) += hw_tags.o report_hw_tags.o -obj-$(CONFIG_KASAN_SW_TAGS) += init.o report_sw_tags.o shadow.o sw_tags.o +obj-$(CONFIG_KASAN_HW_TAGS) += hw_tags.o report_hw_tags.o tags.o +obj-$(CONFIG_KASAN_SW_TAGS) += init.o report_sw_tags.o shadow.o sw_tags.o tags.o diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c index ed5e5b833d61..4ea8c368b5b8 100644 --- a/mm/kasan/hw_tags.c +++ b/mm/kasan/hw_tags.c @@ -216,28 +216,6 @@ void __init kasan_init_hw_tags(void) pr_info("KernelAddressSanitizer initialized\n"); } -void kasan_set_free_info(struct kmem_cache *cache, - void *object, u8 tag) -{ - struct kasan_alloc_meta *alloc_meta; - - alloc_meta = kasan_get_alloc_meta(cache, object); - if (alloc_meta) - kasan_set_track(&alloc_meta->free_track[0], GFP_NOWAIT); -} - -struct kasan_track *kasan_get_free_track(struct kmem_cache *cache, - void *object, u8 tag) -{ - struct kasan_alloc_meta *alloc_meta; - - alloc_meta = kasan_get_alloc_meta(cache, object); - if (!alloc_meta) - return NULL; - - return &alloc_meta->free_track[0]; -} - void kasan_alloc_pages(struct page *page, unsigned int order, gfp_t flags) { /* diff --git a/mm/kasan/report_hw_tags.c b/mm/kasan/report_hw_tags.c index 42b2168755d6..ef5e7378f3aa 100644 --- a/mm/kasan/report_hw_tags.c +++ b/mm/kasan/report_hw_tags.c @@ -14,11 +14,7 @@ #include <linux/types.h> #include "kasan.h" - -const char *kasan_get_bug_type(struct kasan_access_info *info) -{ - return "invalid-access"; -} +#include "report_tags.h" void *kasan_find_first_bad_addr(void *addr, size_t size) { diff --git a/mm/kasan/report_sw_tags.c b/mm/kasan/report_sw_tags.c index 821a14a19a92..d965a170083e 100644 --- a/mm/kasan/report_sw_tags.c +++ b/mm/kasan/report_sw_tags.c @@ -26,51 +26,7 @@ #include <asm/sections.h> -#include "kasan.h" -#include "../slab.h" - -const char *kasan_get_bug_type(struct kasan_access_info *info) -{ -#ifdef CONFIG_KASAN_TAGS_IDENTIFY - struct kasan_alloc_meta *alloc_meta; - struct kmem_cache *cache; - struct page *page; - const void *addr; - void *object; - u8 tag; - int i; - - tag = get_tag(info->access_addr); - addr = kasan_reset_tag(info->access_addr); - page = kasan_addr_to_page(addr); - if (page && PageSlab(page)) { - cache = page->slab_cache; - object = nearest_obj(cache, page, (void *)addr); - alloc_meta = kasan_get_alloc_meta(cache, object); - - if (alloc_meta) { - for (i = 0; i < KASAN_NR_FREE_STACKS; i++) { - if (alloc_meta->free_pointer_tag[i] == tag) - return "use-after-free"; - } - } - return "out-of-bounds"; - } - -#endif - /* - * If access_size is a negative number, then it has reason to be - * defined as out-of-bounds bug type. - * - * Casting negative numbers to size_t would indeed turn up as - * a large size_t and its value will be larger than ULONG_MAX/2, - * so that this can qualify as out-of-bounds. - */ - if (info->access_addr + info->access_size < info->access_addr) - return "out-of-bounds"; - - return "invalid-access"; -} +#include "report_tags.h" void *kasan_find_first_bad_addr(void *addr, size_t size) { diff --git a/mm/kasan/report_tags.h b/mm/kasan/report_tags.h new file mode 100644 index 000000000000..4f740d4d99ee --- /dev/null +++ b/mm/kasan/report_tags.h @@ -0,0 +1,56 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __MM_KASAN_REPORT_TAGS_H +#define __MM_KASAN_REPORT_TAGS_H + +#include "kasan.h" +#include "../slab.h" + +#ifdef CONFIG_KASAN_TAGS_IDENTIFY +const char *kasan_get_bug_type(struct kasan_access_info *info) +{ + struct kasan_alloc_meta *alloc_meta; + struct kmem_cache *cache; + struct page *page; + const void *addr; + void *object; + u8 tag; + int i; + + tag = get_tag(info->access_addr); + addr = kasan_reset_tag(info->access_addr); + page = kasan_addr_to_page(addr); + if (page && PageSlab(page)) { + cache = page->slab_cache; + object = nearest_obj(cache, page, (void *)addr); + alloc_meta = kasan_get_alloc_meta(cache, object); + + if (alloc_meta) { + for (i = 0; i < KASAN_NR_FREE_STACKS; i++) { + if (alloc_meta->free_pointer_tag[i] == tag) + return "use-after-free"; + } + } + return "out-of-bounds"; + } + + /* + * If access_size is a negative number, then it has reason to be + * defined as out-of-bounds bug type. + * + * Casting negative numbers to size_t would indeed turn up as + * a large size_t and its value will be larger than ULONG_MAX/2, + * so that this can qualify as out-of-bounds. + */ + if (info->access_addr + info->access_size < info->access_addr) + return "out-of-bounds"; + + return "invalid-access"; +} +#else +const char *kasan_get_bug_type(struct kasan_access_info *info) +{ + return "invalid-access"; +} +#endif + +#endif diff --git a/mm/kasan/sw_tags.c b/mm/kasan/sw_tags.c index dd05e6c801fa..bd3f540feb47 100644 --- a/mm/kasan/sw_tags.c +++ b/mm/kasan/sw_tags.c @@ -167,47 +167,6 @@ void __hwasan_tag_memory(unsigned long addr, u8 tag, unsigned long size) } EXPORT_SYMBOL(__hwasan_tag_memory); -void kasan_set_free_info(struct kmem_cache *cache, - void *object, u8 tag) -{ - struct kasan_alloc_meta *alloc_meta; - u8 idx = 0; - - alloc_meta = kasan_get_alloc_meta(cache, object); - if (!alloc_meta) - return; - -#ifdef CONFIG_KASAN_TAGS_IDENTIFY - idx = alloc_meta->free_track_idx; - alloc_meta->free_pointer_tag[idx] = tag; - alloc_meta->free_track_idx = (idx + 1) % KASAN_NR_FREE_STACKS; -#endif - - kasan_set_track(&alloc_meta->free_track[idx], GFP_NOWAIT); -} - -struct kasan_track *kasan_get_free_track(struct kmem_cache *cache, - void *object, u8 tag) -{ - struct kasan_alloc_meta *alloc_meta; - int i = 0; - - alloc_meta = kasan_get_alloc_meta(cache, object); - if (!alloc_meta) - return NULL; - -#ifdef CONFIG_KASAN_TAGS_IDENTIFY - for (i = 0; i < KASAN_NR_FREE_STACKS; i++) { - if (alloc_meta->free_pointer_tag[i] == tag) - break; - } - if (i == KASAN_NR_FREE_STACKS) - i = alloc_meta->free_track_idx; -#endif - - return &alloc_meta->free_track[i]; -} - void kasan_tag_mismatch(unsigned long addr, unsigned long access_info, unsigned long ret_ip) { diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c new file mode 100644 index 000000000000..9c33c0ebe1d1 --- /dev/null +++ b/mm/kasan/tags.c @@ -0,0 +1,58 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * This file contains common tag-based KASAN code. + * + * Author: Kuan-Ying Lee <kylee0686026@gmail.com> + */ + +#include <linux/init.h> +#include <linux/kasan.h> +#include <linux/kernel.h> +#include <linux/memory.h> +#include <linux/mm.h> +#include <linux/static_key.h> +#include <linux/string.h> +#include <linux/types.h> + +#include "kasan.h" + +void kasan_set_free_info(struct kmem_cache *cache, + void *object, u8 tag) +{ + struct kasan_alloc_meta *alloc_meta; + u8 idx = 0; + + alloc_meta = kasan_get_alloc_meta(cache, object); + if (!alloc_meta) + return; + +#ifdef CONFIG_KASAN_TAGS_IDENTIFY + idx = alloc_meta->free_track_idx; + alloc_meta->free_pointer_tag[idx] = tag; + alloc_meta->free_track_idx = (idx + 1) % KASAN_NR_FREE_STACKS; +#endif + + kasan_set_track(&alloc_meta->free_track[idx], GFP_NOWAIT); +} + +struct kasan_track *kasan_get_free_track(struct kmem_cache *cache, + void *object, u8 tag) +{ + struct kasan_alloc_meta *alloc_meta; + int i = 0; + + alloc_meta = kasan_get_alloc_meta(cache, object); + if (!alloc_meta) + return NULL; + +#ifdef CONFIG_KASAN_TAGS_IDENTIFY + for (i = 0; i < KASAN_NR_FREE_STACKS; i++) { + if (alloc_meta->free_pointer_tag[i] == tag) + break; + } + if (i == KASAN_NR_FREE_STACKS) + i = alloc_meta->free_track_idx; +#endif + + return &alloc_meta->free_track[i]; +}
1. Move kasan_get_free_track() and kasan_set_free_info() into tags.c 2. Move kasan_get_bug_type() to header file Signed-off-by: Kuan-Ying Lee <kylee0686026@gmail.com> Suggested-by: Marco Elver <elver@google.com> 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: Andrew Morton <akpm@linux-foundation.org> --- mm/kasan/Makefile | 4 +-- mm/kasan/hw_tags.c | 22 --------------- mm/kasan/report_hw_tags.c | 6 +--- mm/kasan/report_sw_tags.c | 46 +------------------------------ mm/kasan/report_tags.h | 56 +++++++++++++++++++++++++++++++++++++ mm/kasan/sw_tags.c | 41 --------------------------- mm/kasan/tags.c | 58 +++++++++++++++++++++++++++++++++++++++ 7 files changed, 118 insertions(+), 115 deletions(-) create mode 100644 mm/kasan/report_tags.h create mode 100644 mm/kasan/tags.c