| Message ID | a2a93370d43ec85b02abaf8d007a15b464212221.1530018818.git.andreyknvl@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
On Wed, Jul 25, 2018 at 3:44 PM, Vincenzo Frascino@Foss <vincenzo.frascino@arm.com> 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. But that is probably something we'll have to deal with if we're aiming for "near-production". I'll add this change to v5, thanks!
On 07/31/2018 04:05 PM, Andrey Konovalov wrote: > On Wed, Jul 25, 2018 at 3:44 PM, Vincenzo Frascino@Foss > <vincenzo.frascino@arm.com> 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. As for non-SLAB_TYPESAFE_BY_RCU caches with constructors, it's probably ok to reinitialize and retag such objects. I don't see how could any code rely on the current ->ctor() behavior in non-SLAB_TYPESAFE_BY_RCU case, unless it does something extremely stupid or weird. But let's not do it now. If you care, you cand do it later, with a separate patch, so we could just revert it if anything goes wrong.
On Tue, Jul 31, 2018 at 4:50 PM, Andrey Ryabinin <aryabinin@virtuozzo.com> 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@arm.com> 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? Or I am missing something subtle? What would be a canonical usage of SLAB_TYPESAFE_BY_RCU slab without a ctor?
On Tue, Jul 31, 2018 at 4:50 PM, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote: > On 07/31/2018 04:05 PM, Andrey Konovalov wrote: >> 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. > > As for non-SLAB_TYPESAFE_BY_RCU caches with constructors, it's probably ok to reinitialize and retag such objects. > I don't see how could any code rely on the current ->ctor() behavior in non-SLAB_TYPESAFE_BY_RCU case, > unless it does something extremely stupid or weird. > But let's not do it now. If you care, you cand do it later, with a separate patch, so we could just revert > it if anything goes wrong. OK, will do it then when there's either a constructor or the slab is SLAB_TYPESAFE_BY_RCU.
On Tue, 31 Jul 2018, Dmitry Vyukov wrote: > > 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? > Or I am missing something subtle? What would be a canonical usage of > SLAB_TYPESAFE_BY_RCU slab without a ctor? True that sounds fishy. Would someone post a list of SLAB_TYPESAFE_BY_RCU slabs without ctors?
On Tue, Jul 31, 2018 at 5:38 PM, Christopher Lameter <cl@linux.com> wrote: > On Tue, 31 Jul 2018, Dmitry Vyukov wrote: > >> > 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? >> Or I am missing something subtle? What would be a canonical usage of >> SLAB_TYPESAFE_BY_RCU slab without a ctor? > > True that sounds fishy. Would someone post a list of SLAB_TYPESAFE_BY_RCU > slabs without ctors? https://elixir.bootlin.com/linux/latest/source/fs/jbd2/journal.c#L2395 https://elixir.bootlin.com/linux/latest/source/fs/kernfs/mount.c#L415 https://elixir.bootlin.com/linux/latest/source/net/netfilter/nf_conntrack_core.c#L2065 https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/i915/i915_gem.c#L5501 https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/i915/selftests/mock_gem_device.c#L212 https://elixir.bootlin.com/linux/latest/source/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c#L1131 Also these in proto structs: https://elixir.bootlin.com/linux/latest/source/net/dccp/ipv4.c#L959 https://elixir.bootlin.com/linux/latest/source/net/dccp/ipv6.c#L1048 https://elixir.bootlin.com/linux/latest/source/net/ipv4/tcp_ipv4.c#L2461 https://elixir.bootlin.com/linux/latest/source/net/ipv6/tcp_ipv6.c#L1980 https://elixir.bootlin.com/linux/latest/source/net/llc/af_llc.c#L145 https://elixir.bootlin.com/linux/latest/source/net/smc/af_smc.c#L105 They later created in net/core/sock.c without ctor.
On 07/31/2018 06:03 PM, Dmitry Vyukov wrote: > On Tue, Jul 31, 2018 at 4:50 PM, Andrey Ryabinin > <aryabinin@virtuozzo.com> 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@arm.com> 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.
On Tue, Jul 31, 2018 at 6:04 PM, Andrey Ryabinin <aryabinin@virtuozzo.com> 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. Agree on both fronts: theoretically possible but easy to fuck up. Even if it works, complexity of the code should be brain damaging and there are unlikely good reasons to just not be more explicit and use a ctor. > 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. I have another hypothesis: they are not bogus, just don't need SLAB_TYPESAFE_BY_RCU :) > 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. > > > -- > You received this message because you are subscribed to the Google Groups "kasan-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com. > To post to this group, send email to kasan-dev@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/b6b58786-85c9-e831-5571-58b5580c84ba%40virtuozzo.com. > For more options, visit https://groups.google.com/d/optout.
On 07/31/2018 07:08 PM, Dmitry Vyukov wrote: > On Tue, Jul 31, 2018 at 6:04 PM, Andrey Ryabinin > <aryabinin@virtuozzo.com> 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. > > > Agree on both fronts: theoretically possible but easy to fuck up. Even > if it works, complexity of the code should be brain damaging and there > are unlikely good reasons to just not be more explicit and use a ctor. > > >> 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. > > I have another hypothesis: they are not bogus, just don't need > SLAB_TYPESAFE_BY_RCU :) > I'd call this a bug too.
diff --git a/mm/kasan/common.c b/mm/kasan/common.c index 656baa8984c7..1e96ca050c75 100644 --- a/mm/kasan/common.c +++ b/mm/kasan/common.c @@ -140,6 +140,9 @@ void kasan_poison_shadow(const void *address, size_t size, u8 value) { void *shadow_start, *shadow_end; + /* Perform shadow offset calculation based on untagged address */ + address = reset_tag(address); + shadow_start = kasan_mem_to_shadow(address); shadow_end = kasan_mem_to_shadow(address + size); @@ -148,11 +151,20 @@ void kasan_poison_shadow(const void *address, size_t size, u8 value) void kasan_unpoison_shadow(const void *address, size_t size) { - kasan_poison_shadow(address, size, 0); + u8 tag = get_tag(address); + + /* Perform shadow offset calculation based on untagged address */ + address = reset_tag(address); + + kasan_poison_shadow(address, size, tag); if (size & KASAN_SHADOW_MASK) { u8 *shadow = (u8 *)kasan_mem_to_shadow(address + size); - *shadow = size & KASAN_SHADOW_MASK; + + if (IS_ENABLED(CONFIG_KASAN_HW)) + *shadow = tag; + else + *shadow = size & KASAN_SHADOW_MASK; } } @@ -200,8 +212,9 @@ void kasan_unpoison_stack_above_sp_to(const void *watermark) void kasan_alloc_pages(struct page *page, unsigned int order) { - if (likely(!PageHighMem(page))) - kasan_unpoison_shadow(page_address(page), PAGE_SIZE << order); + if (unlikely(PageHighMem(page))) + return; + kasan_unpoison_shadow(page_address(page), PAGE_SIZE << order); } void kasan_free_pages(struct page *page, unsigned int order) @@ -235,6 +248,7 @@ void kasan_cache_create(struct kmem_cache *cache, unsigned int *size, slab_flags_t *flags) { unsigned int orig_size = *size; + unsigned int redzone_size = 0; int redzone_adjust; /* Add alloc meta. */ @@ -242,20 +256,20 @@ void kasan_cache_create(struct kmem_cache *cache, unsigned int *size, *size += sizeof(struct kasan_alloc_meta); /* Add free meta. */ - if (cache->flags & SLAB_TYPESAFE_BY_RCU || cache->ctor || - cache->object_size < sizeof(struct kasan_free_meta)) { + if (IS_ENABLED(CONFIG_KASAN_GENERIC) && + (cache->flags & SLAB_TYPESAFE_BY_RCU || cache->ctor || + cache->object_size < sizeof(struct kasan_free_meta))) { cache->kasan_info.free_meta_offset = *size; *size += sizeof(struct kasan_free_meta); } - redzone_adjust = optimal_redzone(cache->object_size) - - (*size - cache->object_size); + redzone_size = optimal_redzone(cache->object_size); + redzone_adjust = redzone_size - (*size - cache->object_size); if (redzone_adjust > 0) *size += redzone_adjust; *size = min_t(unsigned int, KMALLOC_MAX_SIZE, - max(*size, cache->object_size + - optimal_redzone(cache->object_size))); + max(*size, cache->object_size + redzone_size)); /* * If the metadata doesn't fit, don't enable KASAN at all. @@ -268,6 +282,8 @@ void kasan_cache_create(struct kmem_cache *cache, unsigned int *size, return; } + cache->align = round_up(cache->align, KASAN_SHADOW_SCALE_SIZE); + *flags |= SLAB_KASAN; } @@ -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); + } + return object; +} + +static inline bool shadow_invalid(u8 tag, s8 shadow_byte) +{ + if (IS_ENABLED(CONFIG_KASAN_GENERIC)) + return shadow_byte < 0 || + shadow_byte >= KASAN_SHADOW_SCALE_SIZE; + else + return tag != (u8)shadow_byte; } static bool __kasan_slab_free(struct kmem_cache *cache, void *object, unsigned long ip, bool quarantine) { s8 shadow_byte; + u8 tag; + void *tagged_object; unsigned long rounded_up_size; + tag = get_tag(object); + tagged_object = object; + object = reset_tag(object); + if (unlikely(nearest_obj(cache, virt_to_head_page(object), object) != object)) { - kasan_report_invalid_free(object, ip); + kasan_report_invalid_free(tagged_object, ip); return true; } @@ -345,20 +384,22 @@ static bool __kasan_slab_free(struct kmem_cache *cache, void *object, return false; shadow_byte = READ_ONCE(*(s8 *)kasan_mem_to_shadow(object)); - if (shadow_byte < 0 || shadow_byte >= KASAN_SHADOW_SCALE_SIZE) { - kasan_report_invalid_free(object, ip); + if (shadow_invalid(tag, shadow_byte)) { + kasan_report_invalid_free(tagged_object, ip); return true; } rounded_up_size = round_up(cache->object_size, KASAN_SHADOW_SCALE_SIZE); kasan_poison_shadow(object, rounded_up_size, KASAN_KMALLOC_FREE); - if (!quarantine || unlikely(!(cache->flags & SLAB_KASAN))) + if ((IS_ENABLED(CONFIG_KASAN_GENERIC) && !quarantine) || + unlikely(!(cache->flags & SLAB_KASAN))) return false; set_track(&get_alloc_info(cache, object)->free_track, GFP_NOWAIT); quarantine_put(get_free_info(cache, object), cache); - return true; + + return IS_ENABLED(CONFIG_KASAN_GENERIC); } bool kasan_slab_free(struct kmem_cache *cache, void *object, unsigned long ip) @@ -371,6 +412,7 @@ void *kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size, { unsigned long redzone_start; unsigned long redzone_end; + u8 tag; if (gfpflags_allow_blocking(flags)) quarantine_reduce(); @@ -383,14 +425,15 @@ void *kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size, redzone_end = round_up((unsigned long)object + cache->object_size, KASAN_SHADOW_SCALE_SIZE); - kasan_unpoison_shadow(object, size); + tag = random_tag(); + kasan_unpoison_shadow(set_tag(object, tag), size); kasan_poison_shadow((void *)redzone_start, redzone_end - redzone_start, KASAN_KMALLOC_REDZONE); if (cache->flags & SLAB_KASAN) set_track(&get_alloc_info(cache, object)->alloc_track, flags); - return (void *)object; + return set_tag(object, tag); } EXPORT_SYMBOL(kasan_kmalloc); @@ -440,7 +483,7 @@ void kasan_poison_kfree(void *ptr, unsigned long ip) page = virt_to_head_page(ptr); if (unlikely(!PageSlab(page))) { - if (ptr != page_address(page)) { + if (reset_tag(ptr) != page_address(page)) { kasan_report_invalid_free(ptr, ip); return; } @@ -453,7 +496,7 @@ void kasan_poison_kfree(void *ptr, unsigned long ip) void kasan_kfree_large(void *ptr, unsigned long ip) { - if (ptr != page_address(virt_to_head_page(ptr))) + if (reset_tag(ptr) != page_address(virt_to_head_page(ptr))) kasan_report_invalid_free(ptr, ip); /* The object will be poisoned by page_alloc. */ } diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h index d60859d26be7..6f4f2ebf5f57 100644 --- a/mm/kasan/kasan.h +++ b/mm/kasan/kasan.h @@ -12,10 +12,18 @@ #define KHWASAN_TAG_INVALID 0xFE /* inaccessible memory tag */ #define KHWASAN_TAG_MAX 0xFD /* maximum value for random tags */ +#ifdef CONFIG_KASAN_GENERIC #define KASAN_FREE_PAGE 0xFF /* page was freed */ #define KASAN_PAGE_REDZONE 0xFE /* redzone for kmalloc_large allocations */ #define KASAN_KMALLOC_REDZONE 0xFC /* redzone inside slub object */ #define KASAN_KMALLOC_FREE 0xFB /* object was freed (kmem_cache_free/kfree) */ +#else +#define KASAN_FREE_PAGE KHWASAN_TAG_INVALID +#define KASAN_PAGE_REDZONE KHWASAN_TAG_INVALID +#define KASAN_KMALLOC_REDZONE KHWASAN_TAG_INVALID +#define KASAN_KMALLOC_FREE KHWASAN_TAG_INVALID +#endif + #define KASAN_GLOBAL_REDZONE 0xFA /* redzone for global variable */ /* diff --git a/mm/kasan/khwasan.c b/mm/kasan/khwasan.c index d34679b8f8c7..fd1725022794 100644 --- a/mm/kasan/khwasan.c +++ b/mm/kasan/khwasan.c @@ -88,15 +88,52 @@ void *khwasan_reset_tag(const void *addr) void check_memory_region(unsigned long addr, size_t size, bool write, unsigned long ret_ip) { + u8 tag; + u8 *shadow_first, *shadow_last, *shadow; + void *untagged_addr; + + tag = get_tag((const void *)addr); + + /* Ignore accesses for pointers tagged with 0xff (native kernel + * pointer tag) to suppress false positives caused by kmap. + * + * Some kernel code was written to account for archs that don't keep + * high memory mapped all the time, but rather map and unmap particular + * pages when needed. Instead of storing a pointer to the kernel memory, + * this code saves the address of the page structure and offset within + * that page for later use. Those pages are then mapped and unmapped + * with kmap/kunmap when necessary and virt_to_page is used to get the + * virtual address of the page. For arm64 (that keeps the high memory + * mapped all the time), kmap is turned into a page_address call. + + * The issue is that with use of the page_address + virt_to_page + * sequence the top byte value of the original pointer gets lost (gets + * set to KHWASAN_TAG_KERNEL (0xFF). + */ + if (tag == KHWASAN_TAG_KERNEL) + return; + + untagged_addr = reset_tag((const void *)addr); + shadow_first = kasan_mem_to_shadow(untagged_addr); + shadow_last = kasan_mem_to_shadow(untagged_addr + size - 1); + + for (shadow = shadow_first; shadow <= shadow_last; shadow++) { + if (*shadow != tag) { + kasan_report(addr, size, write, ret_ip); + return; + } + } } #define DEFINE_HWASAN_LOAD_STORE(size) \ void __hwasan_load##size##_noabort(unsigned long addr) \ { \ + check_memory_region(addr, size, false, _RET_IP_); \ } \ EXPORT_SYMBOL(__hwasan_load##size##_noabort); \ void __hwasan_store##size##_noabort(unsigned long addr) \ { \ + check_memory_region(addr, size, true, _RET_IP_); \ } \ EXPORT_SYMBOL(__hwasan_store##size##_noabort) @@ -108,15 +145,18 @@ DEFINE_HWASAN_LOAD_STORE(16); void __hwasan_loadN_noabort(unsigned long addr, unsigned long size) { + check_memory_region(addr, size, false, _RET_IP_); } EXPORT_SYMBOL(__hwasan_loadN_noabort); void __hwasan_storeN_noabort(unsigned long addr, unsigned long size) { + check_memory_region(addr, size, true, _RET_IP_); } EXPORT_SYMBOL(__hwasan_storeN_noabort); void __hwasan_tag_memory(unsigned long addr, u8 tag, unsigned long size) { + kasan_poison_shadow((void *)addr, size, tag); } EXPORT_SYMBOL(__hwasan_tag_memory);
This commit adds KHWASAN specific hooks implementation and adjusts common KASAN and KHWASAN ones. 1. When a new slab cache is created, KHWASAN rounds up the size of the objects in this cache to KASAN_SHADOW_SCALE_SIZE (== 16). 2. On each kmalloc KHWASAN generates a random tag, sets the shadow memory, that corresponds to this object to this tag, and embeds this tag value into the top byte of the returned pointer. 3. On each kfree KHWASAN poisons the shadow memory with a random tag to allow detection of use-after-free bugs. The rest of the logic of the hook implementation is very much similar to the one provided by KASAN. KHWASAN saves allocation and free stack metadata to the slab object the same was KASAN does this. Signed-off-by: Andrey Konovalov <andreyknvl@google.com> --- mm/kasan/common.c | 83 +++++++++++++++++++++++++++++++++++----------- mm/kasan/kasan.h | 8 +++++ mm/kasan/khwasan.c | 40 ++++++++++++++++++++++ 3 files changed, 111 insertions(+), 20 deletions(-)