Re: [PATCH 1/2] mm/kasan: avoid duplicate KASAN issues from reporting

[Dmitry Vyukov]

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
>