linux-kernel - Re: [PATCH v4 2/7] mm, kasan: SLAB support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <56D5A884.5000803@gmail.com>
Date:	Tue, 1 Mar 2016 17:34:44 +0300
From:	Andrey Ryabinin <ryabinin.a.a@...il.com>
To:	Alexander Potapenko <glider@...gle.com>
Cc:	Andrey Konovalov <adech.fo@...il.com>,
	Christoph Lameter <cl@...ux.com>,
	Dmitriy Vyukov <dvyukov@...gle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Joonsoo Kim <iamjoonsoo.kim@....com>,
	JoonSoo Kim <js1304@...il.com>,
	Kostya Serebryany <kcc@...gle.com>, kasan-dev@...glegroups.com,
	LKML <linux-kernel@...r.kernel.org>,
	Linux Memory Management List <linux-mm@...ck.org>
Subject: Re: [PATCH v4 2/7] mm, kasan: SLAB support



On 02/29/2016 09:28 PM, Alexander Potapenko wrote:

>>>  static void print_address_description(struct kasan_access_info *info)
>>>  {
>>>       const void *addr = info->access_addr;
>>> @@ -126,17 +164,14 @@ static void print_address_description(struct kasan_access_info *info)
>>>               if (PageSlab(page)) {
>>>                       void *object;
>>>                       struct kmem_cache *cache = page->slab_cache;
>>> -                     void *last_object;
>>> -
>>> -                     object = virt_to_obj(cache, page_address(page), addr);
>>> -                     last_object = page_address(page) +
>>> -                             page->objects * cache->size;
>>> -
>>> -                     if (unlikely(object > last_object))
>>> -                             object = last_object; /* we hit into padding */
>>> -
>>> +                     object = nearest_obj(cache, page,
>>> +                                             (void *)info->access_addr);
>>> +#ifdef CONFIG_SLAB
>>> +                     print_object(cache, object);
>>> +#else
>>
>> Instead of these ifdefs, please, make universal API for printing object's information.
> My intention here was to touch the SLUB functionality as little as
> possible to avoid the mess and feature regressions.
> I'll be happy to refactor the code in the upcoming patches once this
> one is landed.
> 

Avoid mess? You create one.
Although I don't understand that don't touch slub thing, but you can just
have object_err(cache, page, str) for slab without touching slub.

>>>                       object_err(cache, page, object,
>>> -                             "kasan: bad access detected");
>>> +                                     "kasan: bad access detected");
>>> +#endif
>>>                       return;
>>>               }
>>>               dump_page(page, "kasan: bad access detected");
>>> @@ -146,8 +181,9 @@ static void print_address_description(struct kasan_access_info *info)
>>>               if (!init_task_stack_addr(addr))
>>>                       pr_err("Address belongs to variable %pS\n", addr);
>>>       }
>>> -
>>> +#ifdef CONFIG_SLUB
>>
>> ???
> Not sure what did you mean here, assuming this comment is related to
> the next one.
>>
>>>       dump_stack();
>>> +#endif
>>>  }
>>>
>>>  static bool row_is_guilty(const void *row, const void *guilty)
>>> @@ -233,6 +269,9 @@ static void kasan_report_error(struct kasan_access_info *info)
>>>               dump_stack();
>>>       } else {
>>>               print_error_description(info);
>>> +#ifdef CONFIG_SLAB
>>
>> I'm lost here. What's the point of reordering dump_stack() for CONFIG_SLAB=y?
> I should have documented this in the patch description properly.
> My intention is to make the KASAN reports look more like those in the
> userspace AddressSanitizer, so I'm moving the memory access stack to
> the top of the report.
> Having seen hundreds and hundreds of ASan reports, we believe that
> important information must go at the beginning of the error report.
> First, people usually do not need to read further once they see the
> access stack.
> Second, the whole report may simply not make it to the log (e.g. in
> the case of a premature shutdown or remote log collection).
>
> As said before, I wasn't going to touch the SLUB output format in this
> patch set, but that also needs to be fixed (I'd also remove some
> unnecessary info, e.g. the memory dump).
>

That's all sounds fine, but this doesn't explain:
a) How this change related to this patch? (the answer is - it doesn't).
b) Why the output of non sl[a,u]b bugs depends on CONFIG_SL[A,U]B ?
 
So, in SLAB's print_objects() you can print stacks in whatever order you like.
That's it, don't change anything else here.

If you are not satisfied with current format output, change it, but in separate patch[es],
with reasoning described in changelog and without weird config dependencies.




>>> +             dump_stack();
>>> +#endif
>>>               print_address_description(info);
>>>               print_shadow_for_address(info->first_bad_addr);
>>>       }
>>> diff --git a/mm/slab.c b/mm/slab.c
>>> index 621fbcb..805b39b 100644
>>