| Message ID | 20240730-kasan-tsbrcu-v5-1-48d3cbdfccc5@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | allow KASAN to detect UAF in SLAB_TYPESAFE_BY_RCU slabs | expand |
On Tue, Jul 30, 2024 at 1:06 PM Jann Horn <jannh@google.com> wrote: > > Currently, when KASAN is combined with init-on-free behavior, the > initialization happens before KASAN's "invalid free" checks. > > More importantly, a subsequent commit will want to RCU-delay the actual > SLUB freeing of an object, and we'd like KASAN to still validate > synchronously that freeing the object is permitted. (Otherwise this > change will make the existing testcase kmem_cache_invalid_free fail.) > > So add a new KASAN hook that allows KASAN to pre-validate a > kmem_cache_free() operation before SLUB actually starts modifying the > object or its metadata. A few more minor comments below. With that: Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com> Thank you! > Inside KASAN, this: > > - moves checks from poison_slab_object() into check_slab_free() > - moves kasan_arch_is_ready() up into callers of poison_slab_object() > - removes "ip" argument of poison_slab_object() and __kasan_slab_free() > (since those functions no longer do any reporting) > - renames check_slab_free() to check_slab_allocation() check_slab_allocation() is introduced in this patch, so technically you don't rename anything. > Acked-by: Vlastimil Babka <vbabka@suse.cz> #slub > Signed-off-by: Jann Horn <jannh@google.com> > --- > include/linux/kasan.h | 43 ++++++++++++++++++++++++++++++++++--- > mm/kasan/common.c | 59 +++++++++++++++++++++++++++++++-------------------- > mm/slub.c | 7 ++++++ > 3 files changed, 83 insertions(+), 26 deletions(-) > > diff --git a/include/linux/kasan.h b/include/linux/kasan.h > index 70d6a8f6e25d..34cb7a25aacb 100644 > --- a/include/linux/kasan.h > +++ b/include/linux/kasan.h > @@ -172,19 +172,50 @@ static __always_inline void * __must_check kasan_init_slab_obj( > { > if (kasan_enabled()) > return __kasan_init_slab_obj(cache, object); > return (void *)object; > } > > -bool __kasan_slab_free(struct kmem_cache *s, void *object, > - unsigned long ip, bool init); > +bool __kasan_slab_pre_free(struct kmem_cache *s, void *object, > + unsigned long ip); > +/** > + * kasan_slab_pre_free - Validate a slab object freeing request. > + * @object: Object to free. > + * > + * This function checks whether freeing the given object might be permitted; it > + * checks things like whether the given object is properly aligned and not > + * already freed. > + * > + * This function is only intended for use by the slab allocator. > + * > + * @Return true if freeing the object is known to be invalid; false otherwise. > + */ Let's reword this to: kasan_slab_pre_free - Check whether freeing a slab object is safe. @object: Object to be freed. This function checks whether freeing the given object is safe. It performs checks to detect double-free and invalid-free bugs and reports them. This function is intended only for use by the slab allocator. @Return true if freeing the object is not safe; false otherwise. > +static __always_inline bool kasan_slab_pre_free(struct kmem_cache *s, > + void *object) > +{ > + if (kasan_enabled()) > + return __kasan_slab_pre_free(s, object, _RET_IP_); > + return false; > +} > + > +bool __kasan_slab_free(struct kmem_cache *s, void *object, bool init); > +/** > + * kasan_slab_free - Possibly handle slab object freeing. > + * @object: Object to free. > + * > + * This hook is called from the slab allocator to give KASAN a chance to take > + * ownership of the object and handle its freeing. > + * kasan_slab_pre_free() must have already been called on the same object. > + * > + * @Return true if KASAN took ownership of the object; false otherwise. > + */ kasan_slab_free - Poison, initialize, and quarantine a slab object. @object: Object to be freed. @init: Whether to initialize the object. This function poisons a slab object and saves a free stack trace for it, except for SLAB_TYPESAFE_BY_RCU caches. For KASAN modes that have integrated memory initialization (kasan_has_integrated_init() == true), this function also initializes the object's memory. For other modes, the @init argument is ignored. For the Generic mode, this function might also quarantine the object. When this happens, KASAN will defer freeing the object to a later stage and handle it internally then. The return value indicates whether the object was quarantined. This function is intended only for use by the slab allocator. @Return true if KASAN quarantined the object; false otherwise. > static __always_inline bool kasan_slab_free(struct kmem_cache *s, > void *object, bool init) > { > if (kasan_enabled()) > - return __kasan_slab_free(s, object, _RET_IP_, init); > + return __kasan_slab_free(s, object, init); > return false; > } > > void __kasan_kfree_large(void *ptr, unsigned long ip); > static __always_inline void kasan_kfree_large(void *ptr) > { > @@ -368,12 +399,18 @@ static inline void kasan_poison_new_object(struct kmem_cache *cache, > void *object) {} > static inline void *kasan_init_slab_obj(struct kmem_cache *cache, > const void *object) > { > return (void *)object; > } > + > +static inline bool kasan_slab_pre_free(struct kmem_cache *s, void *object) > +{ > + return false; > +} > + > static inline bool kasan_slab_free(struct kmem_cache *s, void *object, bool init) > { > return false; > } > static inline void kasan_kfree_large(void *ptr) {} > static inline void *kasan_slab_alloc(struct kmem_cache *s, void *object, > diff --git a/mm/kasan/common.c b/mm/kasan/common.c > index 85e7c6b4575c..8cede1ce00e1 100644 > --- a/mm/kasan/common.c > +++ b/mm/kasan/common.c > @@ -205,59 +205,65 @@ void * __must_check __kasan_init_slab_obj(struct kmem_cache *cache, > /* Tag is ignored in set_tag() without CONFIG_KASAN_SW/HW_TAGS */ > object = set_tag(object, assign_tag(cache, object, true)); > > return (void *)object; > } > > -static inline bool poison_slab_object(struct kmem_cache *cache, void *object, > - unsigned long ip, bool init) > +/* returns true for invalid request */ "Returns true when freeing the object is not safe." > +static bool check_slab_allocation(struct kmem_cache *cache, void *object, > + unsigned long ip) > { > - void *tagged_object; > - > - if (!kasan_arch_is_ready()) > - return false; > + void *tagged_object = object; > > - tagged_object = object; > object = kasan_reset_tag(object); > > if (unlikely(nearest_obj(cache, virt_to_slab(object), object) != object)) { > kasan_report_invalid_free(tagged_object, ip, KASAN_REPORT_INVALID_FREE); > return true; > } > > - /* RCU slabs could be legally used after free within the RCU period. */ > - if (unlikely(cache->flags & SLAB_TYPESAFE_BY_RCU)) > - return false; > - > if (!kasan_byte_accessible(tagged_object)) { > kasan_report_invalid_free(tagged_object, ip, KASAN_REPORT_DOUBLE_FREE); > return true; > } > > + return false; > +} > + > +static inline void poison_slab_object(struct kmem_cache *cache, void *object, > + bool init) > +{ > + void *tagged_object = object; > + > + object = kasan_reset_tag(object); > + > + /* RCU slabs could be legally used after free within the RCU period. */ > + if (unlikely(cache->flags & SLAB_TYPESAFE_BY_RCU)) > + return; > + > kasan_poison(object, round_up(cache->object_size, KASAN_GRANULE_SIZE), > KASAN_SLAB_FREE, init); > > if (kasan_stack_collection_enabled()) > kasan_save_free_info(cache, tagged_object); > +} > > - return false; > +bool __kasan_slab_pre_free(struct kmem_cache *cache, void *object, > + unsigned long ip) > +{ > + if (!kasan_arch_is_ready() || is_kfence_address(object)) > + return false; > + return check_slab_allocation(cache, object, ip); > } > > -bool __kasan_slab_free(struct kmem_cache *cache, void *object, > - unsigned long ip, bool init) > +bool __kasan_slab_free(struct kmem_cache *cache, void *object, bool init) > { > - if (is_kfence_address(object)) > + if (!kasan_arch_is_ready() || is_kfence_address(object)) > return false; > > - /* > - * If the object is buggy, do not let slab put the object onto the > - * freelist. The object will thus never be allocated again and its > - * metadata will never get released. > - */ > - if (poison_slab_object(cache, object, ip, init)) > - return true; > + poison_slab_object(cache, object, init); > > /* > * If the object is put into quarantine, do not let slab put the object > * onto the freelist for now. The object's metadata is kept until the > * object gets evicted from quarantine. > */ > @@ -503,15 +509,22 @@ bool __kasan_mempool_poison_object(void *ptr, unsigned long ip) > kasan_poison(ptr, folio_size(folio), KASAN_PAGE_FREE, false); > return true; > } > > if (is_kfence_address(ptr)) > return false; > + if (!kasan_arch_is_ready()) > + return true; Hm, I think we had a bug here: the function should return true in both cases. This seems reasonable: if KASAN is not checking the object, the caller can do whatever they want with it. > slab = folio_slab(folio); > - return !poison_slab_object(slab->slab_cache, ptr, ip, false); > + > + if (check_slab_allocation(slab->slab_cache, ptr, ip)) > + return false; > + > + poison_slab_object(slab->slab_cache, ptr, false); > + return true; > } > > void __kasan_mempool_unpoison_object(void *ptr, size_t size, unsigned long ip) > { > struct slab *slab; > gfp_t flags = 0; /* Might be executing under a lock. */ > diff --git a/mm/slub.c b/mm/slub.c > index 3520acaf9afa..0c98b6a2124f 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -2223,12 +2223,19 @@ bool slab_free_hook(struct kmem_cache *s, void *x, bool init) > __kcsan_check_access(x, s->object_size, > KCSAN_ACCESS_WRITE | KCSAN_ACCESS_ASSERT); > > if (kfence_free(x)) > return false; > > + /* > + * Give KASAN a chance to notice an invalid free operation before we > + * modify the object. > + */ > + if (kasan_slab_pre_free(s, x)) > + return false; > + > /* > * As memory initialization might be integrated into KASAN, > * kasan_slab_free and initialization memset's must be > * kept together to avoid discrepancies in behavior. > * > * The initialization memset's clear the object and the metadata, > > -- > 2.46.0.rc1.232.g9752f9e123-goog >
(I'll address the other feedback later) On Thu, Aug 1, 2024 at 2:23 AM Andrey Konovalov <andreyknvl@gmail.com> wrote: > > On Tue, Jul 30, 2024 at 1:06 PM Jann Horn <jannh@google.com> wrote: > > > > Currently, when KASAN is combined with init-on-free behavior, the > > initialization happens before KASAN's "invalid free" checks. > > > > More importantly, a subsequent commit will want to RCU-delay the actual > > SLUB freeing of an object, and we'd like KASAN to still validate > > synchronously that freeing the object is permitted. (Otherwise this > > change will make the existing testcase kmem_cache_invalid_free fail.) > > > > So add a new KASAN hook that allows KASAN to pre-validate a > > kmem_cache_free() operation before SLUB actually starts modifying the > > object or its metadata. [...] > > @@ -503,15 +509,22 @@ bool __kasan_mempool_poison_object(void *ptr, unsigned long ip) > > kasan_poison(ptr, folio_size(folio), KASAN_PAGE_FREE, false); > > return true; > > } > > > > if (is_kfence_address(ptr)) > > return false; > > + if (!kasan_arch_is_ready()) > > + return true; > > Hm, I think we had a bug here: the function should return true in both > cases. This seems reasonable: if KASAN is not checking the object, the > caller can do whatever they want with it. But if the object is a kfence allocation, we maybe do want the caller to free it quickly so that kfence can catch potential UAF access? So "return false" in that case seems appropriate. Or maybe we don't because that makes the probability of catching an OOB access much lower if the mempool is going to always return non-kfence allocations as long as the pool isn't empty? Also I guess whether kfence vetoes reuse of kfence objects probably shouldn't depend on whether the kernel is built with KASAN... so I guess it would be more consistent to either put "return true" there or change the !KASAN stub of this to check for kfence objects or something like that? Honestly I think the latter would be most appropriate, though then maybe the hook shouldn't have "kasan" in its name... Either way, I agree that the current situation wrt mempools and kfence is inconsistent, but I think I should probably leave that as-is in my series for now, and the kfence mempool issue can be addressed separately afterwards? I also would like to avoid changing kfence behavior as part of this patch. If you want, I can add a comment above the "if (is_kfence_address())" that notes the inconsistency?
On Thu, Aug 1, 2024 at 6:01 AM Jann Horn <jannh@google.com> wrote: > > > > @@ -503,15 +509,22 @@ bool __kasan_mempool_poison_object(void *ptr, unsigned long ip) > > > kasan_poison(ptr, folio_size(folio), KASAN_PAGE_FREE, false); > > > return true; > > > } > > > > > > if (is_kfence_address(ptr)) > > > return false; > > > + if (!kasan_arch_is_ready()) > > > + return true; > > > > Hm, I think we had a bug here: the function should return true in both > > cases. This seems reasonable: if KASAN is not checking the object, the > > caller can do whatever they want with it. > > But if the object is a kfence allocation, we maybe do want the caller > to free it quickly so that kfence can catch potential UAF access? So > "return false" in that case seems appropriate. Return false would mean: allocation is buggy, do not use it and do not free it (note that the return value meaning here is inverse compared to the newly added check_slab_allocation()). And this doesn't seem like something we want for KFENCE-managed objects. But regardless of the return value here, the callers tend not to free these allocations to the slab allocator, that's the point of mempools. So KFENCE won't catch a UAF either way. > Or maybe we don't > because that makes the probability of catching an OOB access much > lower if the mempool is going to always return non-kfence allocations > as long as the pool isn't empty? Also I guess whether kfence vetoes > reuse of kfence objects probably shouldn't depend on whether the > kernel is built with KASAN... so I guess it would be more consistent > to either put "return true" there or change the !KASAN stub of this to > check for kfence objects or something like that? Honestly I think the > latter would be most appropriate, though then maybe the hook shouldn't > have "kasan" in its name... Yeah, we could add some custom handling of mempool to KFENCE as well. But that would be a separate effort. > Either way, I agree that the current situation wrt mempools and kfence > is inconsistent, but I think I should probably leave that as-is in my > series for now, and the kfence mempool issue can be addressed > separately afterwards? I also would like to avoid changing kfence > behavior as part of this patch. Sure, sounds good to me. > If you want, I can add a comment above the "if (is_kfence_address())" > that notes the inconsistency? Up to you, I'll likely add a note to the bug tracker to fix this once the patch lands anyway. Thanks!
On Thu, Aug 1, 2024 at 2:23 AM Andrey Konovalov <andreyknvl@gmail.com> wrote: > On Tue, Jul 30, 2024 at 1:06 PM Jann Horn <jannh@google.com> wrote: > > > > Currently, when KASAN is combined with init-on-free behavior, the > > initialization happens before KASAN's "invalid free" checks. > > > > More importantly, a subsequent commit will want to RCU-delay the actual > > SLUB freeing of an object, and we'd like KASAN to still validate > > synchronously that freeing the object is permitted. (Otherwise this > > change will make the existing testcase kmem_cache_invalid_free fail.) > > > > So add a new KASAN hook that allows KASAN to pre-validate a > > kmem_cache_free() operation before SLUB actually starts modifying the > > object or its metadata. > > A few more minor comments below. With that: > > Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com> > > Thank you! > > > Inside KASAN, this: > > > > - moves checks from poison_slab_object() into check_slab_free() > > - moves kasan_arch_is_ready() up into callers of poison_slab_object() > > - removes "ip" argument of poison_slab_object() and __kasan_slab_free() > > (since those functions no longer do any reporting) > > > - renames check_slab_free() to check_slab_allocation() > > check_slab_allocation() is introduced in this patch, so technically > you don't rename anything. Right, I'll fix the commit message. > > Acked-by: Vlastimil Babka <vbabka@suse.cz> #slub > > Signed-off-by: Jann Horn <jannh@google.com> > > --- > > include/linux/kasan.h | 43 ++++++++++++++++++++++++++++++++++--- > > mm/kasan/common.c | 59 +++++++++++++++++++++++++++++++-------------------- > > mm/slub.c | 7 ++++++ > > 3 files changed, 83 insertions(+), 26 deletions(-) > > > > diff --git a/include/linux/kasan.h b/include/linux/kasan.h > > index 70d6a8f6e25d..34cb7a25aacb 100644 > > --- a/include/linux/kasan.h > > +++ b/include/linux/kasan.h > > @@ -172,19 +172,50 @@ static __always_inline void * __must_check kasan_init_slab_obj( > > { > > if (kasan_enabled()) > > return __kasan_init_slab_obj(cache, object); > > return (void *)object; > > } > > > > -bool __kasan_slab_free(struct kmem_cache *s, void *object, > > - unsigned long ip, bool init); > > +bool __kasan_slab_pre_free(struct kmem_cache *s, void *object, > > + unsigned long ip); > > +/** > > + * kasan_slab_pre_free - Validate a slab object freeing request. > > + * @object: Object to free. > > + * > > + * This function checks whether freeing the given object might be permitted; it > > + * checks things like whether the given object is properly aligned and not > > + * already freed. > > + * > > + * This function is only intended for use by the slab allocator. > > + * > > + * @Return true if freeing the object is known to be invalid; false otherwise. > > + */ > > Let's reword this to: > > kasan_slab_pre_free - Check whether freeing a slab object is safe. > @object: Object to be freed. > > This function checks whether freeing the given object is safe. It > performs checks to detect double-free and invalid-free bugs and > reports them. > > This function is intended only for use by the slab allocator. > > @Return true if freeing the object is not safe; false otherwise. Ack, will apply this for v6. But I'll replace "not safe" with "unsafe", and change "It performs checks to detect double-free and invalid-free bugs and reports them" to "It may check for double-free and invalid-free bugs and report them.", since KASAN only sometimes performs such checks (depending on CONFIG_KASAN, kasan_enabled(), kasan_arch_is_ready(), and so on). > > +static __always_inline bool kasan_slab_pre_free(struct kmem_cache *s, > > + void *object) > > +{ > > + if (kasan_enabled()) > > + return __kasan_slab_pre_free(s, object, _RET_IP_); > > + return false; > > +} > > + > > +bool __kasan_slab_free(struct kmem_cache *s, void *object, bool init); > > +/** > > + * kasan_slab_free - Possibly handle slab object freeing. > > + * @object: Object to free. > > + * > > + * This hook is called from the slab allocator to give KASAN a chance to take > > + * ownership of the object and handle its freeing. > > + * kasan_slab_pre_free() must have already been called on the same object. > > + * > > + * @Return true if KASAN took ownership of the object; false otherwise. > > + */ > > kasan_slab_free - Poison, initialize, and quarantine a slab object. > @object: Object to be freed. > @init: Whether to initialize the object. > > This function poisons a slab object and saves a free stack trace for > it, except for SLAB_TYPESAFE_BY_RCU caches. > > For KASAN modes that have integrated memory initialization > (kasan_has_integrated_init() == true), this function also initializes > the object's memory. For other modes, the @init argument is ignored. As an aside: Is this actually reliably true? It would be false for kfence objects, but luckily we can't actually get kfence objects passed to this function (which I guess maybe we should maybe document here as part of the API). It would also be wrong if __kasan_slab_free() can be reached while kasan_arch_is_ready() is false, which I guess would happen if you ran a CONFIG_KASAN=y kernel on a powerpc machine without radix or something like that? (And similarly I wonder if the check of kasan_has_integrated_init() in slab_post_alloc_hook() is racy, but I haven't checked in which phase of boot KASAN is enabled for HWASAN.) But I guess that's out of scope for this series. > For the Generic mode, this function might also quarantine the object. > When this happens, KASAN will defer freeing the object to a later > stage and handle it internally then. The return value indicates > whether the object was quarantined. > > This function is intended only for use by the slab allocator. > > @Return true if KASAN quarantined the object; false otherwise. Same thing as I wrote on patch 2/2: To me this seems like too much implementation detail for the documentation of an API between components of the kernel? I agree that the meaning of the "init" argument is important to document here, and it should be documented that the hook can take ownership of the object (and I guess it's fine to mention that this is for quarantine purposes), but I would leave out details about differences in behavior between KASAN modes. Basically my heuristic here is that in my opinion, this header comment should mostly describe as much of the function as SLUB has to know to properly use it. So I'd do something like: <<< kasan_slab_free - Poison, initialize, and quarantine a slab object. @object: Object to be freed. @init: Whether to initialize the object. This function informs that a slab object has been freed and is not supposed to be accessed anymore, except for objects in SLAB_TYPESAFE_BY_RCU caches. For KASAN modes that have integrated memory initialization (kasan_has_integrated_init() == true), this function also initializes the object's memory. For other modes, the @init argument is ignored. This function might also take ownership of the object to quarantine it. When this happens, KASAN will defer freeing the object to a later stage and handle it internally until then. The return value indicates whether KASAN took ownership of the object. This function is intended only for use by the slab allocator. @Return true if KASAN took ownership of the object; false otherwise. >>> But if you disagree, I'll add your full comment as you suggested. [...] > > diff --git a/mm/kasan/common.c b/mm/kasan/common.c > > index 85e7c6b4575c..8cede1ce00e1 100644 > > --- a/mm/kasan/common.c > > +++ b/mm/kasan/common.c > > @@ -205,59 +205,65 @@ void * __must_check __kasan_init_slab_obj(struct kmem_cache *cache, > > /* Tag is ignored in set_tag() without CONFIG_KASAN_SW/HW_TAGS */ > > object = set_tag(object, assign_tag(cache, object, true)); > > > > return (void *)object; > > } > > > > -static inline bool poison_slab_object(struct kmem_cache *cache, void *object, > > - unsigned long ip, bool init) > > +/* returns true for invalid request */ > > "Returns true when freeing the object is not safe." ack, applied
On Thu, Aug 1, 2024 at 2:54 PM Andrey Konovalov <andreyknvl@gmail.com> wrote: > On Thu, Aug 1, 2024 at 6:01 AM Jann Horn <jannh@google.com> wrote: > > > > > > @@ -503,15 +509,22 @@ bool __kasan_mempool_poison_object(void *ptr, unsigned long ip) > > > > kasan_poison(ptr, folio_size(folio), KASAN_PAGE_FREE, false); > > > > return true; > > > > } > > > > > > > > if (is_kfence_address(ptr)) > > > > return false; > > > > + if (!kasan_arch_is_ready()) > > > > + return true; > > > > > > Hm, I think we had a bug here: the function should return true in both > > > cases. This seems reasonable: if KASAN is not checking the object, the > > > caller can do whatever they want with it. > > > > But if the object is a kfence allocation, we maybe do want the caller > > to free it quickly so that kfence can catch potential UAF access? So > > "return false" in that case seems appropriate. > > Return false would mean: allocation is buggy, do not use it and do not > free it (note that the return value meaning here is inverse compared > to the newly added check_slab_allocation()). And this doesn't seem > like something we want for KFENCE-managed objects. But regardless of > the return value here, the callers tend not to free these allocations > to the slab allocator, that's the point of mempools. So KFENCE won't > catch a UAF either way. Oooh, right, I misunderstood the semantics of the function. I'll change it in v6.
On Fri, Aug 2, 2024 at 11:57 AM Jann Horn <jannh@google.com> wrote: > > > > > Let's reword this to: > > > > kasan_slab_pre_free - Check whether freeing a slab object is safe. > > @object: Object to be freed. > > > > This function checks whether freeing the given object is safe. It > > performs checks to detect double-free and invalid-free bugs and > > reports them. > > > > This function is intended only for use by the slab allocator. > > > > @Return true if freeing the object is not safe; false otherwise. > > Ack, will apply this for v6. But I'll replace "not safe" with > "unsafe", and change "It performs checks to detect double-free and > invalid-free bugs and reports them" to "It may check for double-free > and invalid-free bugs and report them.", since KASAN only sometimes > performs such checks (depending on CONFIG_KASAN, kasan_enabled(), > kasan_arch_is_ready(), and so on). Ok! > > kasan_slab_free - Poison, initialize, and quarantine a slab object. > > @object: Object to be freed. > > @init: Whether to initialize the object. > > > > This function poisons a slab object and saves a free stack trace for > > it, except for SLAB_TYPESAFE_BY_RCU caches. > > > > For KASAN modes that have integrated memory initialization > > (kasan_has_integrated_init() == true), this function also initializes > > the object's memory. For other modes, the @init argument is ignored. > > As an aside: Is this actually reliably true? It would be false for > kfence objects, but luckily we can't actually get kfence objects > passed to this function (which I guess maybe we should maybe document > here as part of the API). It would also be wrong if > __kasan_slab_free() can be reached while kasan_arch_is_ready() is > false, which I guess would happen if you ran a CONFIG_KASAN=y kernel > on a powerpc machine without radix or something like that? > > (And similarly I wonder if the check of kasan_has_integrated_init() in > slab_post_alloc_hook() is racy, but I haven't checked in which phase > of boot KASAN is enabled for HWASAN.) > > But I guess that's out of scope for this series. Yeah, valid concerns. Documenting all of them is definitely too much, though. > > For the Generic mode, this function might also quarantine the object. > > When this happens, KASAN will defer freeing the object to a later > > stage and handle it internally then. The return value indicates > > whether the object was quarantined. > > > > This function is intended only for use by the slab allocator. > > > > @Return true if KASAN quarantined the object; false otherwise. > > Same thing as I wrote on patch 2/2: To me this seems like too much > implementation detail for the documentation of an API between > components of the kernel? I agree that the meaning of the "init" > argument is important to document here, and it should be documented > that the hook can take ownership of the object (and I guess it's fine > to mention that this is for quarantine purposes), but I would leave > out details about differences in behavior between KASAN modes. > Basically my heuristic here is that in my opinion, this header comment > should mostly describe as much of the function as SLUB has to know to > properly use it. > > So I'd do something like: > > <<< > kasan_slab_free - Poison, initialize, and quarantine a slab object. > @object: Object to be freed. > @init: Whether to initialize the object. > > This function informs that a slab object has been freed and is not > supposed to be accessed anymore, except for objects in > SLAB_TYPESAFE_BY_RCU caches. > > For KASAN modes that have integrated memory initialization > (kasan_has_integrated_init() == true), this function also initializes > the object's memory. For other modes, the @init argument is ignored. > > This function might also take ownership of the object to quarantine it. > When this happens, KASAN will defer freeing the object to a later > stage and handle it internally until then. The return value indicates > whether KASAN took ownership of the object. > > This function is intended only for use by the slab allocator. > > @Return true if KASAN took ownership of the object; false otherwise. > >>> Looks good to me.
diff --git a/include/linux/kasan.h b/include/linux/kasan.h index 70d6a8f6e25d..34cb7a25aacb 100644 --- a/include/linux/kasan.h +++ b/include/linux/kasan.h @@ -172,19 +172,50 @@ static __always_inline void * __must_check kasan_init_slab_obj( { if (kasan_enabled()) return __kasan_init_slab_obj(cache, object); return (void *)object; } -bool __kasan_slab_free(struct kmem_cache *s, void *object, - unsigned long ip, bool init); +bool __kasan_slab_pre_free(struct kmem_cache *s, void *object, + unsigned long ip); +/** + * kasan_slab_pre_free - Validate a slab object freeing request. + * @object: Object to free. + * + * This function checks whether freeing the given object might be permitted; it + * checks things like whether the given object is properly aligned and not + * already freed. + * + * This function is only intended for use by the slab allocator. + * + * @Return true if freeing the object is known to be invalid; false otherwise. + */ +static __always_inline bool kasan_slab_pre_free(struct kmem_cache *s, + void *object) +{ + if (kasan_enabled()) + return __kasan_slab_pre_free(s, object, _RET_IP_); + return false; +} + +bool __kasan_slab_free(struct kmem_cache *s, void *object, bool init); +/** + * kasan_slab_free - Possibly handle slab object freeing. + * @object: Object to free. + * + * This hook is called from the slab allocator to give KASAN a chance to take + * ownership of the object and handle its freeing. + * kasan_slab_pre_free() must have already been called on the same object. + * + * @Return true if KASAN took ownership of the object; false otherwise. + */ static __always_inline bool kasan_slab_free(struct kmem_cache *s, void *object, bool init) { if (kasan_enabled()) - return __kasan_slab_free(s, object, _RET_IP_, init); + return __kasan_slab_free(s, object, init); return false; } void __kasan_kfree_large(void *ptr, unsigned long ip); static __always_inline void kasan_kfree_large(void *ptr) { @@ -368,12 +399,18 @@ static inline void kasan_poison_new_object(struct kmem_cache *cache, void *object) {} static inline void *kasan_init_slab_obj(struct kmem_cache *cache, const void *object) { return (void *)object; } + +static inline bool kasan_slab_pre_free(struct kmem_cache *s, void *object) +{ + return false; +} + static inline bool kasan_slab_free(struct kmem_cache *s, void *object, bool init) { return false; } static inline void kasan_kfree_large(void *ptr) {} static inline void *kasan_slab_alloc(struct kmem_cache *s, void *object, diff --git a/mm/kasan/common.c b/mm/kasan/common.c index 85e7c6b4575c..8cede1ce00e1 100644 --- a/mm/kasan/common.c +++ b/mm/kasan/common.c @@ -205,59 +205,65 @@ void * __must_check __kasan_init_slab_obj(struct kmem_cache *cache, /* Tag is ignored in set_tag() without CONFIG_KASAN_SW/HW_TAGS */ object = set_tag(object, assign_tag(cache, object, true)); return (void *)object; } -static inline bool poison_slab_object(struct kmem_cache *cache, void *object, - unsigned long ip, bool init) +/* returns true for invalid request */ +static bool check_slab_allocation(struct kmem_cache *cache, void *object, + unsigned long ip) { - void *tagged_object; - - if (!kasan_arch_is_ready()) - return false; + void *tagged_object = object; - tagged_object = object; object = kasan_reset_tag(object); if (unlikely(nearest_obj(cache, virt_to_slab(object), object) != object)) { kasan_report_invalid_free(tagged_object, ip, KASAN_REPORT_INVALID_FREE); return true; } - /* RCU slabs could be legally used after free within the RCU period. */ - if (unlikely(cache->flags & SLAB_TYPESAFE_BY_RCU)) - return false; - if (!kasan_byte_accessible(tagged_object)) { kasan_report_invalid_free(tagged_object, ip, KASAN_REPORT_DOUBLE_FREE); return true; } + return false; +} + +static inline void poison_slab_object(struct kmem_cache *cache, void *object, + bool init) +{ + void *tagged_object = object; + + object = kasan_reset_tag(object); + + /* RCU slabs could be legally used after free within the RCU period. */ + if (unlikely(cache->flags & SLAB_TYPESAFE_BY_RCU)) + return; + kasan_poison(object, round_up(cache->object_size, KASAN_GRANULE_SIZE), KASAN_SLAB_FREE, init); if (kasan_stack_collection_enabled()) kasan_save_free_info(cache, tagged_object); +} - return false; +bool __kasan_slab_pre_free(struct kmem_cache *cache, void *object, + unsigned long ip) +{ + if (!kasan_arch_is_ready() || is_kfence_address(object)) + return false; + return check_slab_allocation(cache, object, ip); } -bool __kasan_slab_free(struct kmem_cache *cache, void *object, - unsigned long ip, bool init) +bool __kasan_slab_free(struct kmem_cache *cache, void *object, bool init) { - if (is_kfence_address(object)) + if (!kasan_arch_is_ready() || is_kfence_address(object)) return false; - /* - * If the object is buggy, do not let slab put the object onto the - * freelist. The object will thus never be allocated again and its - * metadata will never get released. - */ - if (poison_slab_object(cache, object, ip, init)) - return true; + poison_slab_object(cache, object, init); /* * If the object is put into quarantine, do not let slab put the object * onto the freelist for now. The object's metadata is kept until the * object gets evicted from quarantine. */ @@ -503,15 +509,22 @@ bool __kasan_mempool_poison_object(void *ptr, unsigned long ip) kasan_poison(ptr, folio_size(folio), KASAN_PAGE_FREE, false); return true; } if (is_kfence_address(ptr)) return false; + if (!kasan_arch_is_ready()) + return true; slab = folio_slab(folio); - return !poison_slab_object(slab->slab_cache, ptr, ip, false); + + if (check_slab_allocation(slab->slab_cache, ptr, ip)) + return false; + + poison_slab_object(slab->slab_cache, ptr, false); + return true; } void __kasan_mempool_unpoison_object(void *ptr, size_t size, unsigned long ip) { struct slab *slab; gfp_t flags = 0; /* Might be executing under a lock. */ diff --git a/mm/slub.c b/mm/slub.c index 3520acaf9afa..0c98b6a2124f 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -2223,12 +2223,19 @@ bool slab_free_hook(struct kmem_cache *s, void *x, bool init) __kcsan_check_access(x, s->object_size, KCSAN_ACCESS_WRITE | KCSAN_ACCESS_ASSERT); if (kfence_free(x)) return false; + /* + * Give KASAN a chance to notice an invalid free operation before we + * modify the object. + */ + if (kasan_slab_pre_free(s, x)) + return false; + /* * As memory initialization might be integrated into KASAN, * kasan_slab_free and initialization memset's must be * kept together to avoid discrepancies in behavior. * * The initialization memset's clear the object and the metadata,