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

[Maninder Singh]

Hi Dmitry,
 
Sorry for late response.
 
> --- 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.
> 
 
Actual we  wanted to send both patches in 1 patch, then we though 
to break in 2 ideas for better review, first one is to give idea
of remove duplicate KASAN errors and second is to save KASAN metadata.
and structure was required in other files in second patch so it was 
decalred in header
 
>> +
>>  /* 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/
 
OK
 
>> +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.
 
Ok got your point.
 
>> +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/
 
ok
 
>> +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.
 
ok, will check that (if only first patch seems to be good for mainline)
 
>> +       bug_type = kasan_get_bug_type(info);
>> +       page = kasan_addr_to_page(addr);
>> +       bt_handle = kasan_save_stack(GFP_KERNEL);
> 
 
OK
> 
>> +       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.
 
ok
 
>> +               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".
 
Ok will check that also
 
>> +                       (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 ;)
 
 
:), it was taken care in second patch [2/2]
 
> 
>> +
>> +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.
 
Just to align with current code.
 
 
>> +               !save_report(untagged_addr, &info, get_tag(tagged_addr), &flags))
>> +               return;
>> +
>>         start_report(&flags);
>>
>>         print_error_description(&info);
>> --
>> 2.17.1
>>
 
I will revert on other threads also[2/2], and then please let me know
if only first patch can be good for mainline 
 
 
Thanks,
Maninder Singh