On Thu, Apr 22, 2021 at 11:17 AM Maninder Singh <maninder1.s@xxxxxxxxxxx> wrote:
>
> when KASAN multishot is ON and some buggy code hits same code path
> of KASAN issue repetetively, it can flood logs on console.
>
> Check for allocaton, free and backtrace path at time of KASAN error,
> if these are same then it is duplicate error and avoid these prints
> from KASAN.
>
> Co-developed-by: Vaneet Narang <v.narang@xxxxxxxxxxx>
> Signed-off-by: Vaneet Narang <v.narang@xxxxxxxxxxx>
> Signed-off-by: Maninder Singh <maninder1.s@xxxxxxxxxxx>
> ---
> mm/kasan/kasan.h | 6 +++++
> mm/kasan/report.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 73 insertions(+)
>
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 78cf99247139..d14ccce246ba 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -102,6 +102,12 @@ struct kasan_access_info {
> unsigned long ip;
> };
>
> +struct kasan_record {
> + depot_stack_handle_t bt_handle;
> + depot_stack_handle_t alloc_handle;
> + depot_stack_handle_t free_handle;
> +};
Hi Maninder,
There is no need to declare this in the header, it can be declared
more locally in report.h.
> +
> /* The layout of struct dictated by compiler */
> struct kasan_source_location {
> const char *filename;
> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> index 87b271206163..4576de76991b 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -39,6 +39,10 @@ static unsigned long kasan_flags;
> #define KASAN_BIT_REPORTED 0
> #define KASAN_BIT_MULTI_SHOT 1
>
> +#define MAX_RECORDS (200)
s/MAX_RECORDS/KASAN_MAX_RECORDS/
> +static struct kasan_record kasan_records[MAX_RECORDS];
Since all fields in kasan_record are stack handles, the code will be
simpler and more uniform, if we store just an array of handles w/o
distinguishing between alloc/free/access.
> +static int stored_kasan_records;
> +
> bool kasan_save_enable_multi_shot(void)
> {
> return test_and_set_bit(KASAN_BIT_MULTI_SHOT, &kasan_flags);
> @@ -360,6 +364,65 @@ void kasan_report_invalid_free(void *object, unsigned long ip)
> end_report(&flags, (unsigned long)object);
> }
>
> +/*
> + * @save_report()
> + *
> + * returns false if same record is already saved.
s/same/the same/
> + * returns true if its new record and saved in database of KASAN.
s/its/it's/
s/database/the database/
> + */
> +static bool save_report(void *addr, struct kasan_access_info *info, u8 tag, unsigned long *flags)
> +{
> + struct kasan_record record = {0};
> + depot_stack_handle_t bt_handle;
> + int i = 0;
> + const char *bug_type;
> + struct kasan_alloc_meta *alloc_meta;
> + struct kasan_track *free_track;
> + struct page *page;
> + bool ret = true;
> +
> + kasan_disable_current();
> + spin_lock_irqsave(&report_lock, *flags);
Reusing the caller flags looks strange, do we need it?
But also the very next function start_report() also does the same
dance: kasan_disable_current/spin_lock_irqsave. It feels reasonable to
lock once, check for dups and return early if it's a dup.
> + bug_type = kasan_get_bug_type(info);
> + page = kasan_addr_to_page(addr);
> + bt_handle = kasan_save_stack(GFP_KERNEL);
ASsign directly to record.bt_handle.
> + if (page && PageSlab(page)) {
> + struct kmem_cache *cache = page->slab_cache;
> + void *object = nearest_obj(cache, page, addr);
Since you already declare new var in this block, move
alloc_meta/free_track here as well.
> +
> + alloc_meta = kasan_get_alloc_meta(cache, object);
> + free_track = kasan_get_free_track(cache, object, tag);
> + record.alloc_handle = alloc_meta->alloc_track.stack;
> + if (free_track)
> + record.free_handle = free_track->stack;
> + }
> +
> + record.bt_handle = bt_handle;
> +
> + for (i = 0; i < stored_kasan_records; i++) {
> + if (record.bt_handle != kasan_records[i].bt_handle)
> + continue;
> + if (record.alloc_handle != kasan_records[i].alloc_handle)
> + continue;
> + if (!strncmp("use-after-free", bug_type, 15) &&
Comparing strings is unreliable and will break in future. Compare
handle with 0 instead, you already assume that 0 handle is "no
handle".
> + (record.free_handle != kasan_records[i].free_handle))
> + continue;
> +
> + ret = false;
> + goto done;
> + }
> +
> + memcpy(&kasan_records[stored_kasan_records], &record, sizeof(struct kasan_record));
> + stored_kasan_records++;
I think you just introduced an out-of-bounds write into KASAN, check
for MAX_RECORDS ;)
> +
> +done:
> + spin_unlock_irqrestore(&report_lock, *flags);
> + kasan_enable_current();
> + return ret;
> +}
> +
> static void __kasan_report(unsigned long addr, size_t size, bool is_write,
> unsigned long ip)
> {
> @@ -388,6 +451,10 @@ static void __kasan_report(unsigned long addr, size_t size, bool is_write,
> info.is_write = is_write;
> info.ip = ip;
>
> + if (addr_has_metadata(untagged_addr) &&
Why addr_has_metadata check?
The kernel will probably crash later anyway, but from point of view of
this code, I don't see reasons to not dedup wild accesses.
> + !save_report(untagged_addr, &info, get_tag(tagged_addr), &flags))
> + return;
> +
> start_report(&flags);
>
> print_error_description(&info);
> --
> 2.17.1
>
