Re: [PATCH v2 4/5] mm/slub: Improve redzone check and zeroing for krealloc()

[Feng Tang <feng.tang@intel.com>]

Hi Dan,

Thanks for the report!

On Thu, Sep 12, 2024 at 06:21:15PM +0300, Dan Carpenter wrote:
> Hi Feng,
> 
> kernel test robot noticed the following build warnings:
> 
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Feng-Tang/mm-kasan-Don-t-store-metadata-inside-kmalloc-object-when-slub_debug_orig_size-is-on/20240911-144724
> base:   next-20240910
> patch link:    https://lore.kernel.org/r/20240911064535.557650-5-feng.tang%40intel.com
> patch subject: [PATCH v2 4/5] mm/slub: Improve redzone check and zeroing for krealloc()
> config: i386-randconfig-141-20240912 (https://download.01.org/0day-ci/archive/20240912/202409122122.U5RpHfCY-lkp@intel.com/config)
> compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202409122122.U5RpHfCY-lkp@intel.com/
> 
> smatch warnings:
> mm/slub.c:4748 __do_krealloc() error: uninitialized symbol 's'.
> 
> vim +/s +4748 mm/slub.c
> 
> 474fff8e68012d Feng Tang       2024-09-11  4715  static __always_inline __realloc_size(2) void *
> 474fff8e68012d Feng Tang       2024-09-11  4716  __do_krealloc(const void *p, size_t new_size, gfp_t flags)
> 474fff8e68012d Feng Tang       2024-09-11  4717  {
> 474fff8e68012d Feng Tang       2024-09-11  4718  	void *ret;
> 474fff8e68012d Feng Tang       2024-09-11  4719  	size_t ks;
> 89a7731cc91033 Feng Tang       2024-09-11  4720  	int orig_size = 0;
> 89a7731cc91033 Feng Tang       2024-09-11  4721  	struct kmem_cache *s;
> 474fff8e68012d Feng Tang       2024-09-11  4722  
> 89a7731cc91033 Feng Tang       2024-09-11  4723  	/* Check for double-free. */
> 474fff8e68012d Feng Tang       2024-09-11  4724  	if (likely(!ZERO_OR_NULL_PTR(p))) {
> 474fff8e68012d Feng Tang       2024-09-11  4725  		if (!kasan_check_byte(p))
> 474fff8e68012d Feng Tang       2024-09-11  4726  			return NULL;
> 89a7731cc91033 Feng Tang       2024-09-11  4727  
> 89a7731cc91033 Feng Tang       2024-09-11  4728  		s = virt_to_cache(p);
> 89a7731cc91033 Feng Tang       2024-09-11  4729  		orig_size = get_orig_size(s, (void *)p);
> 89a7731cc91033 Feng Tang       2024-09-11  4730  		ks = s->object_size;
> 474fff8e68012d Feng Tang       2024-09-11  4731  	} else
> 474fff8e68012d Feng Tang       2024-09-11  4732  		ks = 0;
> 
> ks is zero
> 
> 474fff8e68012d Feng Tang       2024-09-11  4733  
> 89a7731cc91033 Feng Tang       2024-09-11  4734  	/* If the object doesn't fit, allocate a bigger one */
> 89a7731cc91033 Feng Tang       2024-09-11  4735  	if (new_size > ks)
> 
> Assume new_size is also zero so this is false

Yes, this is possible, though unlikely :)

Below patch should fix it:
---
diff --git a/mm/slub.c b/mm/slub.c
index e0fb0a26c796..d5219634e96a 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -4728,8 +4728,9 @@ __do_krealloc(const void *p, size_t new_size, gfp_t flags)
 		s = virt_to_cache(p);
 		orig_size = get_orig_size(s, (void *)p);
 		ks = s->object_size;
-	} else
-		ks = 0;
+	} else {
+		goto alloc_new;
+	}
 
 	/* If the object doesn't fit, allocate a bigger one */
 	if (new_size > ks)


Thanks,
Feng

> 
> 89a7731cc91033 Feng Tang       2024-09-11  4736  		goto alloc_new;
> 89a7731cc91033 Feng Tang       2024-09-11  4737  
> 474fff8e68012d Feng Tang       2024-09-11  4738  	/* Zero out spare memory. */
> 474fff8e68012d Feng Tang       2024-09-11  4739  	if (want_init_on_alloc(flags)) {
> 474fff8e68012d Feng Tang       2024-09-11  4740  		kasan_disable_current();
> 89a7731cc91033 Feng Tang       2024-09-11  4741  		if (orig_size < new_size)
> 89a7731cc91033 Feng Tang       2024-09-11  4742  			memset((void *)p + orig_size, 0, new_size - orig_size);
> 89a7731cc91033 Feng Tang       2024-09-11  4743  		else
> 474fff8e68012d Feng Tang       2024-09-11  4744  			memset((void *)p + new_size, 0, ks - new_size);
> 474fff8e68012d Feng Tang       2024-09-11  4745  		kasan_enable_current();
> 474fff8e68012d Feng Tang       2024-09-11  4746  	}
> 474fff8e68012d Feng Tang       2024-09-11  4747  
> 89a7731cc91033 Feng Tang       2024-09-11 @4748  	if (slub_debug_orig_size(s) && !is_kfence_address(p)) {
>                                                                                  ^
> Uninitialized in that case
> 
> 89a7731cc91033 Feng Tang       2024-09-11  4749  		set_orig_size(s, (void *)p, new_size);
> 89a7731cc91033 Feng Tang       2024-09-11  4750  		if (s->flags & SLAB_RED_ZONE && new_size < ks)
> 
> -- 
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
>