diff mbox series

[10/11] kasan: fix bug detection via ksize for HW_TAGS mode

Message ID a83aa371e2ef96e79cbdefceebaa960a34957a79.1609871239.git.andreyknvl@google.com (mailing list archive)
State New, archived
Headers show
Series kasan: HW_TAGS tests support and fixes | expand

Commit Message

Andrey Konovalov Jan. 5, 2021, 6:27 p.m. UTC 
The currently existing kasan_check_read/write() annotations are intended
to be used for kernel modules that have KASAN compiler instrumentation
disabled. Thus, they are only relevant for the software KASAN modes that
rely on compiler instrumentation.

However there's another use case for these annotations: ksize() checks
that the object passed to it is indeed accessible before unpoisoning the
whole object. This is currently done via __kasan_check_read(), which is
compiled away for the hardware tag-based mode that doesn't rely on
compiler instrumentation. This leads to KASAN missing detecting some
memory corruptions.

Provide another annotation called kasan_check_byte() that is available
for all KASAN modes. As the implementation rename and reuse
kasan_check_invalid_free(). Use this new annotation in ksize().

Also add a new ksize_uaf() test that checks that a use-after-free is
detected via ksize() itself, and via plain accesses that happen later.

Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
Link: https://linux-review.googlesource.com/id/Iaabf771881d0f9ce1b969f2a62938e99d3308ec5
---
 include/linux/kasan-checks.h |  6 ++++++
 include/linux/kasan.h        | 13 +++++++++++++
 lib/test_kasan.c             | 20 ++++++++++++++++++++
 mm/kasan/common.c            | 11 ++++++++++-
 mm/kasan/generic.c           |  4 ++--
 mm/kasan/kasan.h             | 10 +++++-----
 mm/kasan/sw_tags.c           |  6 +++---
 mm/slab_common.c             | 15 +++++++++------
 8 files changed, 68 insertions(+), 17 deletions(-)

Comments

kernel test robot Jan. 5, 2021, 9:04 p.m. UTC | #1 
Hi Andrey,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.11-rc2]
[cannot apply to arm64/for-next/core hnaz-linux-mm/master next-20210104]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Andrey-Konovalov/kasan-HW_TAGS-tests-support-and-fixes/20210106-022940
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git e71ba9452f0b5b2e8dc8aa5445198cd9214a6a62
config: x86_64-randconfig-a006-20210105 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 5c951623bc8965fa1e89660f2f5f4a2944e4981a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/15d82adbf82e57e44789e091da9e141ba4247dba
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Andrey-Konovalov/kasan-HW_TAGS-tests-support-and-fixes/20210106-022940
        git checkout 15d82adbf82e57e44789e091da9e141ba4247dba
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from arch/x86/kernel/asm-offsets.c:9:
   In file included from include/linux/crypto.h:20:
   In file included from include/linux/slab.h:136:
>> include/linux/kasan.h:314:77: error: non-void function does not return a value [-Werror,-Wreturn-type]
   static inline bool kasan_check_byte(const void *address, unsigned long ip) {}
                                                                               ^
   1 error generated.
--
   In file included from arch/x86/kernel/asm-offsets.c:9:
   In file included from include/linux/crypto.h:20:
   In file included from include/linux/slab.h:136:
>> include/linux/kasan.h:314:77: error: non-void function does not return a value [-Werror,-Wreturn-type]
   static inline bool kasan_check_byte(const void *address, unsigned long ip) {}
                                                                               ^
   1 error generated.
   make[2]: *** [scripts/Makefile.build:117: arch/x86/kernel/asm-offsets.s] Error 1
   make[2]: Target '__build' not remade because of errors.
   make[1]: *** [Makefile:1206: prepare0] Error 2
   make[1]: Target 'modules_prepare' not remade because of errors.
   make: *** [Makefile:185: __sub-make] Error 2
   make: Target 'modules_prepare' not remade because of errors.
--
   In file included from arch/x86/kernel/asm-offsets.c:9:
   In file included from include/linux/crypto.h:20:
   In file included from include/linux/slab.h:136:
>> include/linux/kasan.h:314:77: error: non-void function does not return a value [-Werror,-Wreturn-type]
   static inline bool kasan_check_byte(const void *address, unsigned long ip) {}
                                                                               ^
   1 error generated.
   make[2]: *** [scripts/Makefile.build:117: arch/x86/kernel/asm-offsets.s] Error 1
   make[2]: Target '__build' not remade because of errors.
   make[1]: *** [Makefile:1206: prepare0] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [Makefile:185: __sub-make] Error 2
   make: Target 'prepare' not remade because of errors.


vim +314 include/linux/kasan.h

   262	
   263	static inline bool kasan_enabled(void)
   264	{
   265		return false;
   266	}
   267	static inline slab_flags_t kasan_never_merge(void)
   268	{
   269		return 0;
   270	}
   271	static inline void kasan_unpoison_range(const void *address, size_t size) {}
   272	static inline void kasan_alloc_pages(struct page *page, unsigned int order) {}
   273	static inline void kasan_free_pages(struct page *page, unsigned int order) {}
   274	static inline void kasan_cache_create(struct kmem_cache *cache,
   275					      unsigned int *size,
   276					      slab_flags_t *flags) {}
   277	static inline size_t kasan_metadata_size(struct kmem_cache *cache) { return 0; }
   278	static inline void kasan_poison_slab(struct page *page) {}
   279	static inline void kasan_unpoison_object_data(struct kmem_cache *cache,
   280						void *object) {}
   281	static inline void kasan_poison_object_data(struct kmem_cache *cache,
   282						void *object) {}
   283	static inline void *kasan_init_slab_obj(struct kmem_cache *cache,
   284					const void *object)
   285	{
   286		return (void *)object;
   287	}
   288	static inline bool kasan_slab_free(struct kmem_cache *s, void *object,
   289					   unsigned long ip)
   290	{
   291		return false;
   292	}
   293	static inline void kasan_slab_free_mempool(void *ptr, unsigned long ip) {}
   294	static inline void *kasan_slab_alloc(struct kmem_cache *s, void *object,
   295					   gfp_t flags)
   296	{
   297		return object;
   298	}
   299	static inline void *kasan_kmalloc(struct kmem_cache *s, const void *object,
   300					size_t size, gfp_t flags)
   301	{
   302		return (void *)object;
   303	}
   304	static inline void *kasan_kmalloc_large(const void *ptr, size_t size, gfp_t flags)
   305	{
   306		return (void *)ptr;
   307	}
   308	static inline void *kasan_krealloc(const void *object, size_t new_size,
   309					 gfp_t flags)
   310	{
   311		return (void *)object;
   312	}
   313	static inline void kasan_kfree_large(void *ptr, unsigned long ip) {}
 > 314	static inline bool kasan_check_byte(const void *address, unsigned long ip) {}
   315	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot Jan. 6, 2021, 12:09 a.m. UTC | #2 
Hi Andrey,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.11-rc2]
[cannot apply to arm64/for-next/core hnaz-linux-mm/master next-20210104]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Andrey-Konovalov/kasan-HW_TAGS-tests-support-and-fixes/20210106-022940
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git e71ba9452f0b5b2e8dc8aa5445198cd9214a6a62
config: x86_64-randconfig-a003-20210105 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 5c951623bc8965fa1e89660f2f5f4a2944e4981a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/15d82adbf82e57e44789e091da9e141ba4247dba
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Andrey-Konovalov/kasan-HW_TAGS-tests-support-and-fixes/20210106-022940
        git checkout 15d82adbf82e57e44789e091da9e141ba4247dba
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from arch/x86/boot/compressed/cmdline.c:2:
   In file included from arch/x86/boot/compressed/misc.h:30:
   In file included from include/linux/acpi.h:14:
   In file included from include/linux/resource_ext.h:11:
   In file included from include/linux/slab.h:136:
>> include/linux/kasan.h:314:77: warning: non-void function does not return a value [-Wreturn-type]
   static inline bool kasan_check_byte(const void *address, unsigned long ip) {}
                                                                               ^
   1 warning generated.


vim +314 include/linux/kasan.h

   262	
   263	static inline bool kasan_enabled(void)
   264	{
   265		return false;
   266	}
   267	static inline slab_flags_t kasan_never_merge(void)
   268	{
   269		return 0;
   270	}
   271	static inline void kasan_unpoison_range(const void *address, size_t size) {}
   272	static inline void kasan_alloc_pages(struct page *page, unsigned int order) {}
   273	static inline void kasan_free_pages(struct page *page, unsigned int order) {}
   274	static inline void kasan_cache_create(struct kmem_cache *cache,
   275					      unsigned int *size,
   276					      slab_flags_t *flags) {}
   277	static inline size_t kasan_metadata_size(struct kmem_cache *cache) { return 0; }
   278	static inline void kasan_poison_slab(struct page *page) {}
   279	static inline void kasan_unpoison_object_data(struct kmem_cache *cache,
   280						void *object) {}
   281	static inline void kasan_poison_object_data(struct kmem_cache *cache,
   282						void *object) {}
   283	static inline void *kasan_init_slab_obj(struct kmem_cache *cache,
   284					const void *object)
   285	{
   286		return (void *)object;
   287	}
   288	static inline bool kasan_slab_free(struct kmem_cache *s, void *object,
   289					   unsigned long ip)
   290	{
   291		return false;
   292	}
   293	static inline void kasan_slab_free_mempool(void *ptr, unsigned long ip) {}
   294	static inline void *kasan_slab_alloc(struct kmem_cache *s, void *object,
   295					   gfp_t flags)
   296	{
   297		return object;
   298	}
   299	static inline void *kasan_kmalloc(struct kmem_cache *s, const void *object,
   300					size_t size, gfp_t flags)
   301	{
   302		return (void *)object;
   303	}
   304	static inline void *kasan_kmalloc_large(const void *ptr, size_t size, gfp_t flags)
   305	{
   306		return (void *)ptr;
   307	}
   308	static inline void *kasan_krealloc(const void *object, size_t new_size,
   309					 gfp_t flags)
   310	{
   311		return (void *)object;
   312	}
   313	static inline void kasan_kfree_large(void *ptr, unsigned long ip) {}
 > 314	static inline bool kasan_check_byte(const void *address, unsigned long ip) {}
   315	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Andrew Morton Jan. 7, 2021, 12:02 a.m. UTC | #3 
On Wed, 6 Jan 2021 08:09:00 +0800 kernel test robot <lkp@intel.com> wrote:

> Hi Andrey,
>    In file included from arch/x86/boot/compressed/cmdline.c:2:
>    In file included from arch/x86/boot/compressed/misc.h:30:
>    In file included from include/linux/acpi.h:14:
>    In file included from include/linux/resource_ext.h:11:
>    In file included from include/linux/slab.h:136:
> >> include/linux/kasan.h:314:77: warning: non-void function does not return a value [-Wreturn-type]
>    static inline bool kasan_check_byte(const void *address, unsigned long ip) {}
>                                                                                ^
>    1 warning generated.
> 

This?

--- a/include/linux/kasan.h~kasan-fix-bug-detection-via-ksize-for-hw_tags-mode-fix
+++ a/include/linux/kasan.h
@@ -311,7 +311,10 @@ static inline void *kasan_krealloc(const
 	return (void *)object;
 }
 static inline void kasan_kfree_large(void *ptr, unsigned long ip) {}
-static inline bool kasan_check_byte(const void *address, unsigned long ip) {}
+static inline bool kasan_check_byte(const void *address, unsigned long ip)
+{
+	return true;
+}
 
 #endif /* CONFIG_KASAN */
 

btw, "kasan_check_byte" isn't a good function name.  Check for what? 
Does it return true for a check which passed, or for a check which
failed?  Something like "kasan_byte_valid" would be better - the name
explains the return value.
Andrey Konovalov Jan. 7, 2021, 1:59 a.m. UTC | #4 
On Thu, Jan 7, 2021 at 1:02 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed, 6 Jan 2021 08:09:00 +0800 kernel test robot <lkp@intel.com> wrote:
>
> > Hi Andrey,
> >    In file included from arch/x86/boot/compressed/cmdline.c:2:
> >    In file included from arch/x86/boot/compressed/misc.h:30:
> >    In file included from include/linux/acpi.h:14:
> >    In file included from include/linux/resource_ext.h:11:
> >    In file included from include/linux/slab.h:136:
> > >> include/linux/kasan.h:314:77: warning: non-void function does not return a value [-Wreturn-type]
> >    static inline bool kasan_check_byte(const void *address, unsigned long ip) {}
> >                                                                                ^
> >    1 warning generated.
> >
>
> This?
>
> --- a/include/linux/kasan.h~kasan-fix-bug-detection-via-ksize-for-hw_tags-mode-fix
> +++ a/include/linux/kasan.h
> @@ -311,7 +311,10 @@ static inline void *kasan_krealloc(const
>         return (void *)object;
>  }
>  static inline void kasan_kfree_large(void *ptr, unsigned long ip) {}
> -static inline bool kasan_check_byte(const void *address, unsigned long ip) {}
> +static inline bool kasan_check_byte(const void *address, unsigned long ip)
> +{
> +       return true;
> +}
>
>  #endif /* CONFIG_KASAN */

Yes.

> btw, "kasan_check_byte" isn't a good function name.  Check for what?
> Does it return true for a check which passed, or for a check which
> failed?  Something like "kasan_byte_valid" would be better - the name
> explains the return value.

Sounds good, will fix in v2 along with the warning.

Thank you!
Marco Elver Jan. 12, 2021, 2:32 p.m. UTC | #5 
On Tue, Jan 05, 2021 at 07:27PM +0100, Andrey Konovalov wrote:
> The currently existing kasan_check_read/write() annotations are intended
> to be used for kernel modules that have KASAN compiler instrumentation
> disabled. Thus, they are only relevant for the software KASAN modes that
> rely on compiler instrumentation.
> 
> However there's another use case for these annotations: ksize() checks
> that the object passed to it is indeed accessible before unpoisoning the
> whole object. This is currently done via __kasan_check_read(), which is
> compiled away for the hardware tag-based mode that doesn't rely on
> compiler instrumentation. This leads to KASAN missing detecting some
> memory corruptions.
> 
> Provide another annotation called kasan_check_byte() that is available
> for all KASAN modes. As the implementation rename and reuse
> kasan_check_invalid_free(). Use this new annotation in ksize().
> 
> Also add a new ksize_uaf() test that checks that a use-after-free is
> detected via ksize() itself, and via plain accesses that happen later.
> 
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> Link: https://linux-review.googlesource.com/id/Iaabf771881d0f9ce1b969f2a62938e99d3308ec5
> ---
>  include/linux/kasan-checks.h |  6 ++++++
>  include/linux/kasan.h        | 13 +++++++++++++
>  lib/test_kasan.c             | 20 ++++++++++++++++++++
>  mm/kasan/common.c            | 11 ++++++++++-
>  mm/kasan/generic.c           |  4 ++--
>  mm/kasan/kasan.h             | 10 +++++-----
>  mm/kasan/sw_tags.c           |  6 +++---
>  mm/slab_common.c             | 15 +++++++++------
>  8 files changed, 68 insertions(+), 17 deletions(-)
> 
> diff --git a/include/linux/kasan-checks.h b/include/linux/kasan-checks.h
> index ca5e89fb10d3..3d6d22a25bdc 100644
> --- a/include/linux/kasan-checks.h
> +++ b/include/linux/kasan-checks.h
> @@ -4,6 +4,12 @@
>  
>  #include <linux/types.h>
>  
> +/*
> + * The annotations present in this file are only relevant for the software
> + * KASAN modes that rely on compiler instrumentation, and will be optimized
> + * away for the hardware tag-based KASAN mode. Use kasan_check_byte() instead.
> + */
> +
>  /*
>   * __kasan_check_*: Always available when KASAN is enabled. This may be used
>   * even in compilation units that selectively disable KASAN, but must use KASAN
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index 5e0655fb2a6f..992ba5c653a3 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -243,6 +243,18 @@ static __always_inline void kasan_kfree_large(void *ptr, unsigned long ip)
>  		__kasan_kfree_large(ptr, ip);
>  }
>  
> +/*
> + * Unlike kasan_check_read/write(), kasan_check_byte() is performed even for
> + * the hardware tag-based mode that doesn't rely on compiler instrumentation.
> + */

We have too many check-functions, and the name needs to be more precise.
Intuitively, I would have thought this should have access-type, i.e.
read or write, effectively mirroring a normal access.

Would kasan_check_byte_read() be better (and just not have a 'write'
variant because we do not need it)? This would restore ksize() closest
to what it was before (assuming reporting behaviour is fixed, too).

> +bool __kasan_check_byte(const void *addr, unsigned long ip);
> +static __always_inline bool kasan_check_byte(const void *addr, unsigned long ip)
> +{
> +	if (kasan_enabled())
> +		return __kasan_check_byte(addr, ip);
> +	return true;
> +}
> +
>  bool kasan_save_enable_multi_shot(void);
>  void kasan_restore_multi_shot(bool enabled);
>  
> @@ -299,6 +311,7 @@ static inline void *kasan_krealloc(const void *object, size_t new_size,
>  	return (void *)object;
>  }
>  static inline void kasan_kfree_large(void *ptr, unsigned long ip) {}
> +static inline bool kasan_check_byte(const void *address, unsigned long ip) {}
>  
>  #endif /* CONFIG_KASAN */
>  
> diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> index 3ea52da52714..6261521e57ad 100644
> --- a/lib/test_kasan.c
> +++ b/lib/test_kasan.c
> @@ -490,6 +490,7 @@ static void kasan_global_oob(struct kunit *test)
>  	KUNIT_EXPECT_KASAN_FAIL(test, *(volatile char *)p);
>  }
>  
> +/* Check that ksize() makes the whole object accessible. */
>  static void ksize_unpoisons_memory(struct kunit *test)
>  {
>  	char *ptr;
> @@ -508,6 +509,24 @@ static void ksize_unpoisons_memory(struct kunit *test)
>  	kfree(ptr);
>  }
>  
> +/*
> + * Check that a use-after-free is detected by ksize() and via normal accesses
> + * after it.
> + */
> +static void ksize_uaf(struct kunit *test)
> +{
> +	char *ptr;
> +	int size = 128 - KASAN_GRANULE_SIZE;
> +
> +	ptr = kmalloc(size, GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
> +	kfree(ptr);
> +
> +	KUNIT_EXPECT_KASAN_FAIL(test, ksize(ptr));
> +	KUNIT_EXPECT_KASAN_FAIL(test, kasan_int_result = *ptr);
> +	KUNIT_EXPECT_KASAN_FAIL(test, kasan_int_result = *(ptr + size));
> +}
> +
>  static void kasan_stack_oob(struct kunit *test)
>  {
>  	char stack_array[10];
> @@ -937,6 +956,7 @@ static struct kunit_case kasan_kunit_test_cases[] = {
>  	KUNIT_CASE(kasan_alloca_oob_left),
>  	KUNIT_CASE(kasan_alloca_oob_right),
>  	KUNIT_CASE(ksize_unpoisons_memory),
> +	KUNIT_CASE(ksize_uaf),
>  	KUNIT_CASE(kmem_cache_double_free),
>  	KUNIT_CASE(kmem_cache_invalid_free),
>  	KUNIT_CASE(kasan_memchr),
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index eedc3e0fe365..45ab2c7073a8 100644
> --- a/mm/kasan/common.c
> +++ b/mm/kasan/common.c
> @@ -345,7 +345,7 @@ static bool ____kasan_slab_free(struct kmem_cache *cache, void *object,
>  	if (unlikely(cache->flags & SLAB_TYPESAFE_BY_RCU))
>  		return false;
>  
> -	if (kasan_check_invalid_free(tagged_object)) {
> +	if (!kasan_check(tagged_object)) {
>  		kasan_report_invalid_free(tagged_object, ip);
>  		return true;
>  	}
> @@ -490,3 +490,12 @@ void __kasan_kfree_large(void *ptr, unsigned long ip)
>  		kasan_report_invalid_free(ptr, ip);
>  	/* The object will be poisoned by kasan_free_pages(). */
>  }
> +
> +bool __kasan_check_byte(const void *address, unsigned long ip)
> +{
> +	if (!kasan_check(address)) {
> +		kasan_report_invalid_free((void *)address, ip);

This is strange: why does it report an invalid free? Should this be a
use-after-free? I think this could just call kasan_report(....) for 1
byte, and we'd get the right report.

> +		return false;
> +	}
> +	return true;
> +}
> diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> index acab8862dc67..b3631ad9a8ef 100644
> --- a/mm/kasan/generic.c
> +++ b/mm/kasan/generic.c
> @@ -185,11 +185,11 @@ bool kasan_check_range(unsigned long addr, size_t size, bool write,
>  	return check_region_inline(addr, size, write, ret_ip);
>  }
>  
> -bool kasan_check_invalid_free(void *addr)
> +bool kasan_check(const void *addr)
>  {
>  	s8 shadow_byte = READ_ONCE(*(s8 *)kasan_mem_to_shadow(addr));
>  
> -	return shadow_byte < 0 || shadow_byte >= KASAN_GRANULE_SIZE;
> +	return shadow_byte >= 0 && shadow_byte < KASAN_GRANULE_SIZE;
>  }
>  
>  void kasan_cache_shrink(struct kmem_cache *cache)
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 292dfbc37deb..f17591545279 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -329,20 +329,20 @@ static inline void kasan_unpoison(const void *address, size_t size)
>  			round_up(size, KASAN_GRANULE_SIZE), get_tag(address));
>  }
>  
> -static inline bool kasan_check_invalid_free(void *addr)
> +static inline bool kasan_check(const void *addr)
>  {
>  	u8 ptr_tag = get_tag(addr);
> -	u8 mem_tag = hw_get_mem_tag(addr);
> +	u8 mem_tag = hw_get_mem_tag((void *)addr);
>  
> -	return (mem_tag == KASAN_TAG_INVALID) ||
> -		(ptr_tag != KASAN_TAG_KERNEL && ptr_tag != mem_tag);
> +	return (mem_tag != KASAN_TAG_INVALID) &&
> +		(ptr_tag == KASAN_TAG_KERNEL || ptr_tag == mem_tag);
>  }
>  
>  #else /* CONFIG_KASAN_HW_TAGS */
>  
>  void kasan_poison(const void *address, size_t size, u8 value);
>  void kasan_unpoison(const void *address, size_t size);
> -bool kasan_check_invalid_free(void *addr);
> +bool kasan_check(const void *addr);

Definitely prefer shorted names, but we're in the unfortunate situation
of having numerous kasan_check-functions, so we probably need to be more
precise.

kasan_check() makes me think this also does reporting, but it does not
(it seems to only check the metadata for validity).

The internal function could therefore be kasan_check_allocated() (it's
now the inverse of kasan_check_invalid_free()).

>  
>  #endif /* CONFIG_KASAN_HW_TAGS */
>  
> diff --git a/mm/kasan/sw_tags.c b/mm/kasan/sw_tags.c
> index cc271fceb5d5..e326caaaaca3 100644
> --- a/mm/kasan/sw_tags.c
> +++ b/mm/kasan/sw_tags.c
> @@ -118,13 +118,13 @@ bool kasan_check_range(unsigned long addr, size_t size, bool write,
>  	return true;
>  }
>  
> -bool kasan_check_invalid_free(void *addr)
> +bool kasan_check(const void *addr)
>  {
>  	u8 tag = get_tag(addr);
>  	u8 shadow_byte = READ_ONCE(*(u8 *)kasan_mem_to_shadow(kasan_reset_tag(addr)));
>  
> -	return (shadow_byte == KASAN_TAG_INVALID) ||
> -		(tag != KASAN_TAG_KERNEL && tag != shadow_byte);
> +	return (shadow_byte != KASAN_TAG_INVALID) &&
> +		(tag == KASAN_TAG_KERNEL || tag == shadow_byte);
>  }
>  
>  #define DEFINE_HWASAN_LOAD_STORE(size)					\
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index e981c80d216c..a3bb44516623 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -1157,11 +1157,13 @@ size_t ksize(const void *objp)
>  	size_t size;
>  
>  	/*
> -	 * We need to check that the pointed to object is valid, and only then
> -	 * unpoison the shadow memory below. We use __kasan_check_read(), to
> -	 * generate a more useful report at the time ksize() is called (rather
> -	 * than later where behaviour is undefined due to potential
> -	 * use-after-free or double-free).
> +	 * We need to first check that the pointer to the object is valid, and
> +	 * only then unpoison the memory. The report printed from ksize() is
> +	 * more useful, then when it's printed later when the behaviour could
> +	 * be undefined due to a potential use-after-free or double-free.
> +	 *
> +	 * We use kasan_check_byte(), which is supported for hardware tag-based
> +	 * KASAN mode, unlike kasan_check_read/write().
>  	 *
>  	 * If the pointed to memory is invalid we return 0, to avoid users of
>  	 * ksize() writing to and potentially corrupting the memory region.
> @@ -1169,7 +1171,8 @@ size_t ksize(const void *objp)
>  	 * We want to perform the check before __ksize(), to avoid potentially
>  	 * crashing in __ksize() due to accessing invalid metadata.
>  	 */
> -	if (unlikely(ZERO_OR_NULL_PTR(objp)) || !__kasan_check_read(objp, 1))
> +	if (unlikely(ZERO_OR_NULL_PTR(objp)) ||
> +	    !kasan_check_byte(objp, _RET_IP_))
>  		return 0;
>  
>  	size = __ksize(objp);
> -- 
> 2.29.2.729.g45daf8777d-goog
>
Andrey Konovalov Jan. 12, 2021, 9:16 p.m. UTC | #6 
On Tue, Jan 12, 2021 at 3:32 PM Marco Elver <elver@google.com> wrote:
>
> > +/*
> > + * Unlike kasan_check_read/write(), kasan_check_byte() is performed even for
> > + * the hardware tag-based mode that doesn't rely on compiler instrumentation.
> > + */
>
> We have too many check-functions, and the name needs to be more precise.
> Intuitively, I would have thought this should have access-type, i.e.
> read or write, effectively mirroring a normal access.
>
> Would kasan_check_byte_read() be better (and just not have a 'write'
> variant because we do not need it)? This would restore ksize() closest
> to what it was before (assuming reporting behaviour is fixed, too).

> >  void kasan_poison(const void *address, size_t size, u8 value);
> >  void kasan_unpoison(const void *address, size_t size);
> > -bool kasan_check_invalid_free(void *addr);
> > +bool kasan_check(const void *addr);
>
> Definitely prefer shorted names, but we're in the unfortunate situation
> of having numerous kasan_check-functions, so we probably need to be more
> precise.
>
> kasan_check() makes me think this also does reporting, but it does not
> (it seems to only check the metadata for validity).
>
> The internal function could therefore be kasan_check_allocated() (it's
> now the inverse of kasan_check_invalid_free()).

Re: kasan_check_byte():

I think the _read suffix is only making the name longer. ksize() isn't
checking that the memory is readable (or writable), it's checking that
it's addressable. At least that's the intention of the annotation, so
it makes sense to name it correspondingly despite the implementation.

Having all kasan_check_*() functions both checking and reporting makes
sense, so let's keep the kasan_check_ prefix.

What isn't obvious from the name is that this function is present for
every kasan mode. Maybe kasan_check_byte_always()? Although it also
seems too long.

But I'm OK with keeping kasan_check_byte().

Re kasan_check():

Here we can use Andrew's suggestion about the name being related to
what the function returns. And also drop the kasan_check_ prefix as
this function only does the checking.

Let's use kasan_byte_accessible() instead of kasan_check().

> > +bool __kasan_check_byte(const void *address, unsigned long ip)
> > +{
> > +     if (!kasan_check(address)) {
> > +             kasan_report_invalid_free((void *)address, ip);
>
> This is strange: why does it report an invalid free? Should this be a
> use-after-free? I think this could just call kasan_report(....) for 1
> byte, and we'd get the right report.

Will fix in v2.

Thanks!
Marco Elver Jan. 12, 2021, 10:54 p.m. UTC | #7 
On Tue, 12 Jan 2021 at 22:16, Andrey Konovalov <andreyknvl@google.com> wrote:
>
> On Tue, Jan 12, 2021 at 3:32 PM Marco Elver <elver@google.com> wrote:
> >
> > > +/*
> > > + * Unlike kasan_check_read/write(), kasan_check_byte() is performed even for
> > > + * the hardware tag-based mode that doesn't rely on compiler instrumentation.
> > > + */
> >
> > We have too many check-functions, and the name needs to be more precise.
> > Intuitively, I would have thought this should have access-type, i.e.
> > read or write, effectively mirroring a normal access.
> >
> > Would kasan_check_byte_read() be better (and just not have a 'write'
> > variant because we do not need it)? This would restore ksize() closest
> > to what it was before (assuming reporting behaviour is fixed, too).
>
> > >  void kasan_poison(const void *address, size_t size, u8 value);
> > >  void kasan_unpoison(const void *address, size_t size);
> > > -bool kasan_check_invalid_free(void *addr);
> > > +bool kasan_check(const void *addr);
> >
> > Definitely prefer shorted names, but we're in the unfortunate situation
> > of having numerous kasan_check-functions, so we probably need to be more
> > precise.
> >
> > kasan_check() makes me think this also does reporting, but it does not
> > (it seems to only check the metadata for validity).
> >
> > The internal function could therefore be kasan_check_allocated() (it's
> > now the inverse of kasan_check_invalid_free()).
>
> Re: kasan_check_byte():
>
> I think the _read suffix is only making the name longer. ksize() isn't
> checking that the memory is readable (or writable), it's checking that
> it's addressable. At least that's the intention of the annotation, so
> it makes sense to name it correspondingly despite the implementation.
>
> Having all kasan_check_*() functions both checking and reporting makes
> sense, so let's keep the kasan_check_ prefix.
>
> What isn't obvious from the name is that this function is present for
> every kasan mode. Maybe kasan_check_byte_always()? Although it also
> seems too long.
>
> But I'm OK with keeping kasan_check_byte().

This is fine.

> Re kasan_check():
>
> Here we can use Andrew's suggestion about the name being related to
> what the function returns. And also drop the kasan_check_ prefix as
> this function only does the checking.
>
> Let's use kasan_byte_accessible() instead of kasan_check().

Sounds reasonable to me.

Thanks,
-- Marco
diff mbox series

Patch

diff --git a/include/linux/kasan-checks.h b/include/linux/kasan-checks.h
index ca5e89fb10d3..3d6d22a25bdc 100644
--- a/include/linux/kasan-checks.h
+++ b/include/linux/kasan-checks.h
@@ -4,6 +4,12 @@ 
 
 #include <linux/types.h>
 
+/*
+ * The annotations present in this file are only relevant for the software
+ * KASAN modes that rely on compiler instrumentation, and will be optimized
+ * away for the hardware tag-based KASAN mode. Use kasan_check_byte() instead.
+ */
+
 /*
  * __kasan_check_*: Always available when KASAN is enabled. This may be used
  * even in compilation units that selectively disable KASAN, but must use KASAN
diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index 5e0655fb2a6f..992ba5c653a3 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -243,6 +243,18 @@  static __always_inline void kasan_kfree_large(void *ptr, unsigned long ip)
 		__kasan_kfree_large(ptr, ip);
 }
 
+/*
+ * Unlike kasan_check_read/write(), kasan_check_byte() is performed even for
+ * the hardware tag-based mode that doesn't rely on compiler instrumentation.
+ */
+bool __kasan_check_byte(const void *addr, unsigned long ip);
+static __always_inline bool kasan_check_byte(const void *addr, unsigned long ip)
+{
+	if (kasan_enabled())
+		return __kasan_check_byte(addr, ip);
+	return true;
+}
+
 bool kasan_save_enable_multi_shot(void);
 void kasan_restore_multi_shot(bool enabled);
 
@@ -299,6 +311,7 @@  static inline void *kasan_krealloc(const void *object, size_t new_size,
 	return (void *)object;
 }
 static inline void kasan_kfree_large(void *ptr, unsigned long ip) {}
+static inline bool kasan_check_byte(const void *address, unsigned long ip) {}
 
 #endif /* CONFIG_KASAN */
 
diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index 3ea52da52714..6261521e57ad 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -490,6 +490,7 @@  static void kasan_global_oob(struct kunit *test)
 	KUNIT_EXPECT_KASAN_FAIL(test, *(volatile char *)p);
 }
 
+/* Check that ksize() makes the whole object accessible. */
 static void ksize_unpoisons_memory(struct kunit *test)
 {
 	char *ptr;
@@ -508,6 +509,24 @@  static void ksize_unpoisons_memory(struct kunit *test)
 	kfree(ptr);
 }
 
+/*
+ * Check that a use-after-free is detected by ksize() and via normal accesses
+ * after it.
+ */
+static void ksize_uaf(struct kunit *test)
+{
+	char *ptr;
+	int size = 128 - KASAN_GRANULE_SIZE;
+
+	ptr = kmalloc(size, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
+	kfree(ptr);
+
+	KUNIT_EXPECT_KASAN_FAIL(test, ksize(ptr));
+	KUNIT_EXPECT_KASAN_FAIL(test, kasan_int_result = *ptr);
+	KUNIT_EXPECT_KASAN_FAIL(test, kasan_int_result = *(ptr + size));
+}
+
 static void kasan_stack_oob(struct kunit *test)
 {
 	char stack_array[10];
@@ -937,6 +956,7 @@  static struct kunit_case kasan_kunit_test_cases[] = {
 	KUNIT_CASE(kasan_alloca_oob_left),
 	KUNIT_CASE(kasan_alloca_oob_right),
 	KUNIT_CASE(ksize_unpoisons_memory),
+	KUNIT_CASE(ksize_uaf),
 	KUNIT_CASE(kmem_cache_double_free),
 	KUNIT_CASE(kmem_cache_invalid_free),
 	KUNIT_CASE(kasan_memchr),
diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index eedc3e0fe365..45ab2c7073a8 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -345,7 +345,7 @@  static bool ____kasan_slab_free(struct kmem_cache *cache, void *object,
 	if (unlikely(cache->flags & SLAB_TYPESAFE_BY_RCU))
 		return false;
 
-	if (kasan_check_invalid_free(tagged_object)) {
+	if (!kasan_check(tagged_object)) {
 		kasan_report_invalid_free(tagged_object, ip);
 		return true;
 	}
@@ -490,3 +490,12 @@  void __kasan_kfree_large(void *ptr, unsigned long ip)
 		kasan_report_invalid_free(ptr, ip);
 	/* The object will be poisoned by kasan_free_pages(). */
 }
+
+bool __kasan_check_byte(const void *address, unsigned long ip)
+{
+	if (!kasan_check(address)) {
+		kasan_report_invalid_free((void *)address, ip);
+		return false;
+	}
+	return true;
+}
diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
index acab8862dc67..b3631ad9a8ef 100644
--- a/mm/kasan/generic.c
+++ b/mm/kasan/generic.c
@@ -185,11 +185,11 @@  bool kasan_check_range(unsigned long addr, size_t size, bool write,
 	return check_region_inline(addr, size, write, ret_ip);
 }
 
-bool kasan_check_invalid_free(void *addr)
+bool kasan_check(const void *addr)
 {
 	s8 shadow_byte = READ_ONCE(*(s8 *)kasan_mem_to_shadow(addr));
 
-	return shadow_byte < 0 || shadow_byte >= KASAN_GRANULE_SIZE;
+	return shadow_byte >= 0 && shadow_byte < KASAN_GRANULE_SIZE;
 }
 
 void kasan_cache_shrink(struct kmem_cache *cache)
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 292dfbc37deb..f17591545279 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -329,20 +329,20 @@  static inline void kasan_unpoison(const void *address, size_t size)
 			round_up(size, KASAN_GRANULE_SIZE), get_tag(address));
 }
 
-static inline bool kasan_check_invalid_free(void *addr)
+static inline bool kasan_check(const void *addr)
 {
 	u8 ptr_tag = get_tag(addr);
-	u8 mem_tag = hw_get_mem_tag(addr);
+	u8 mem_tag = hw_get_mem_tag((void *)addr);
 
-	return (mem_tag == KASAN_TAG_INVALID) ||
-		(ptr_tag != KASAN_TAG_KERNEL && ptr_tag != mem_tag);
+	return (mem_tag != KASAN_TAG_INVALID) &&
+		(ptr_tag == KASAN_TAG_KERNEL || ptr_tag == mem_tag);
 }
 
 #else /* CONFIG_KASAN_HW_TAGS */
 
 void kasan_poison(const void *address, size_t size, u8 value);
 void kasan_unpoison(const void *address, size_t size);
-bool kasan_check_invalid_free(void *addr);
+bool kasan_check(const void *addr);
 
 #endif /* CONFIG_KASAN_HW_TAGS */
 
diff --git a/mm/kasan/sw_tags.c b/mm/kasan/sw_tags.c
index cc271fceb5d5..e326caaaaca3 100644
--- a/mm/kasan/sw_tags.c
+++ b/mm/kasan/sw_tags.c
@@ -118,13 +118,13 @@  bool kasan_check_range(unsigned long addr, size_t size, bool write,
 	return true;
 }
 
-bool kasan_check_invalid_free(void *addr)
+bool kasan_check(const void *addr)
 {
 	u8 tag = get_tag(addr);
 	u8 shadow_byte = READ_ONCE(*(u8 *)kasan_mem_to_shadow(kasan_reset_tag(addr)));
 
-	return (shadow_byte == KASAN_TAG_INVALID) ||
-		(tag != KASAN_TAG_KERNEL && tag != shadow_byte);
+	return (shadow_byte != KASAN_TAG_INVALID) &&
+		(tag == KASAN_TAG_KERNEL || tag == shadow_byte);
 }
 
 #define DEFINE_HWASAN_LOAD_STORE(size)					\
diff --git a/mm/slab_common.c b/mm/slab_common.c
index e981c80d216c..a3bb44516623 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -1157,11 +1157,13 @@  size_t ksize(const void *objp)
 	size_t size;
 
 	/*
-	 * We need to check that the pointed to object is valid, and only then
-	 * unpoison the shadow memory below. We use __kasan_check_read(), to
-	 * generate a more useful report at the time ksize() is called (rather
-	 * than later where behaviour is undefined due to potential
-	 * use-after-free or double-free).
+	 * We need to first check that the pointer to the object is valid, and
+	 * only then unpoison the memory. The report printed from ksize() is
+	 * more useful, then when it's printed later when the behaviour could
+	 * be undefined due to a potential use-after-free or double-free.
+	 *
+	 * We use kasan_check_byte(), which is supported for hardware tag-based
+	 * KASAN mode, unlike kasan_check_read/write().
 	 *
 	 * If the pointed to memory is invalid we return 0, to avoid users of
 	 * ksize() writing to and potentially corrupting the memory region.
@@ -1169,7 +1171,8 @@  size_t ksize(const void *objp)
 	 * We want to perform the check before __ksize(), to avoid potentially
 	 * crashing in __ksize() due to accessing invalid metadata.
 	 */
-	if (unlikely(ZERO_OR_NULL_PTR(objp)) || !__kasan_check_read(objp, 1))
+	if (unlikely(ZERO_OR_NULL_PTR(objp)) ||
+	    !kasan_check_byte(objp, _RET_IP_))
 		return 0;
 
 	size = __ksize(objp);