Re: [PATCH v4 13/17] khwasan: add hooks implementation

[Andrey Ryabinin]

On 07/31/2018 06:03 PM, Dmitry Vyukov wrote:
> On Tue, Jul 31, 2018 at 4:50 PM, Andrey Ryabinin
> <aryabinin@xxxxxxxxxxxxx> wrote:
>>
>>
>> On 07/31/2018 04:05 PM, Andrey Konovalov wrote:
>>> On Wed, Jul 25, 2018 at 3:44 PM, Vincenzo Frascino@Foss
>>> <vincenzo.frascino@xxxxxxx> wrote:
>>>> On 06/26/2018 02:15 PM, Andrey Konovalov wrote:
>>>>
>>>>> @@ -325,18 +341,41 @@ void kasan_init_slab_obj(struct kmem_cache *cache,
>>>>> const void *object)
>>>>>     void *kasan_slab_alloc(struct kmem_cache *cache, void *object, gfp_t
>>>>> flags)
>>>>>   {
>>>>> -       return kasan_kmalloc(cache, object, cache->object_size, flags);
>>>>> +       object = kasan_kmalloc(cache, object, cache->object_size, flags);
>>>>> +       if (IS_ENABLED(CONFIG_KASAN_HW) && unlikely(cache->ctor)) {
>>>>> +               /*
>>>>> +                * Cache constructor might use object's pointer value to
>>>>> +                * initialize some of its fields.
>>>>> +                */
>>>>> +               cache->ctor(object);
>>>>>
>>>> This seams breaking the kmem_cache_create() contract: "The @ctor is run when
>>>> new pages are allocated by the cache."
>>>> (https://elixir.bootlin.com/linux/v3.7/source/mm/slab_common.c#L83)
>>>>
>>>> Since there might be preexisting code relying on it, this could lead to
>>>> global side effects. Did you verify that this is not the case?
>>>>
>>>> Another concern is performance related if we consider this solution suitable
>>>> for "near-production", since with the current implementation you call the
>>>> ctor (where present) on an object multiple times and this ends up memsetting
>>>> and repopulating the memory every time (i.e. inode.c: inode_init_once). Do
>>>> you know what is the performance impact?
>>>
>>> We can assign tags to objects with constructors when a slab is
>>> allocated and call constructors once as usual. The downside is that
>>> such object would always have the same tag when it is reallocated, so
>>> we won't catch use-after-frees.
>>
>> Actually you should do this for SLAB_TYPESAFE_BY_RCU slabs. Usually they are with ->ctors but there
>> are few without constructors.
>> We can't reinitialize or even retag them. The latter will definitely cause false-positive use-after-free reports.
> 
> Somewhat offtopic, but I can't understand how SLAB_TYPESAFE_BY_RCU
> slabs can be useful without ctors or at least memset(0). Objects in
> such slabs need to be type-stable, but I can't understand how it's
> possible to establish type stability without a ctor... Are these bugs?

Yeah, I puzzled by this too. However, I think it's hard but possible to make it work, at least in theory.
There must be an initializer, which consists of two parts:
a) initilize objects fields
b) expose object to the world (add it to list or something like that)

(a) part must somehow to be ok to race with another cpu which might already use the object.
(b) part must must use e.g. barriers to make sure that racy users will see previously inilized fields.
Racy users must have parring barrier of course.

But it sound fishy, and very easy to fuck up. I won't be surprised if every single one SLAB_TYPESAFE_BY_RCU user
without ->ctor is bogus. It certainly would be better to convert those to use ->ctor.

Such caches seems used by networking subsystem in proto_register():

		prot->slab = kmem_cache_create_usercopy(prot->name,
					prot->obj_size, 0,
					SLAB_HWCACHE_ALIGN | SLAB_ACCOUNT |
					prot->slab_flags,
					prot->useroffset, prot->usersize,
					NULL);

And certain protocols specify SLAB_TYPESAFE_BY_RCU in ->slab_flags, such as:
llc_proto, smc_proto, smc_proto6, tcp_prot, tcpv6_prot, dccp_v6_prot, dccp_v4_prot.


Also nf_conntrack_cachep, kernfs_node_cache, jbd2_journal_head_cache and i915_request cache.