Re: arch/riscv/kernel/module.c:727 add_relocation_to_accumulate() error: dereferencing freed memory 'rel_head'

[Charlie Jenkins <charlie@rivosinc.com>]

On Wed, Dec 13, 2023 at 04:19:26PM +0300, Dan Carpenter wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head:   88035e5694a86a7167d490bb95e9df97a9bb162b
> commit: d8792a5734b0f3e58b898c2e2f910bfac48e9ee3 riscv: Safely remove entries from relocation list
> config: riscv-randconfig-r071-20231211 (https://download.01.org/0day-ci/archive/20231213/202312132019.iYGTwW0L-lkp@intel.com/config)
> compiler: riscv64-linux-gcc (GCC) 13.2.0
> reproduce: (https://download.01.org/0day-ci/archive/20231213/202312132019.iYGTwW0L-lkp@intel.com/reproduce)
> 
> 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/202312132019.iYGTwW0L-lkp@intel.com/
> 
> New smatch warnings:
> arch/riscv/kernel/module.c:727 add_relocation_to_accumulate() error: dereferencing freed memory 'rel_head'
> arch/riscv/kernel/module.c:792 apply_relocate_add() warn: unsigned 'hashtable_bits' is never less than zero.
> 
> vim +/rel_head +727 arch/riscv/kernel/module.c
> 
> 8fd6c5142395a1 Charlie Jenkins      2023-11-01  717  		INIT_LIST_HEAD(rel_head->rel_entry);
> 8fd6c5142395a1 Charlie Jenkins      2023-11-01  718  		rel_head->location = location;
> 8fd6c5142395a1 Charlie Jenkins      2023-11-01  719  		INIT_HLIST_NODE(&rel_head->node);
> 8fd6c5142395a1 Charlie Jenkins      2023-11-01  720  		if (!current_head->first) {
> 8fd6c5142395a1 Charlie Jenkins      2023-11-01  721  			bucket =
> 8fd6c5142395a1 Charlie Jenkins      2023-11-01  722  				kmalloc(sizeof(struct used_bucket), GFP_KERNEL);
> d8792a5734b0f3 Charlie Jenkins      2023-11-27  723  
> d8792a5734b0f3 Charlie Jenkins      2023-11-27  724  			if (!bucket) {
> d8792a5734b0f3 Charlie Jenkins      2023-11-27  725  				kfree(entry);
> d8792a5734b0f3 Charlie Jenkins      2023-11-27  726  				kfree(rel_head);
> d8792a5734b0f3 Charlie Jenkins      2023-11-27 @727  				kfree(rel_head->rel_entry);
> 
> Swap these two frees to avoid a use after free.

Thanks for pointing this out.

> 
> d8792a5734b0f3 Charlie Jenkins      2023-11-27  728  				return -ENOMEM;
> d8792a5734b0f3 Charlie Jenkins      2023-11-27  729  			}
> d8792a5734b0f3 Charlie Jenkins      2023-11-27  730  
> 8fd6c5142395a1 Charlie Jenkins      2023-11-01  731  			INIT_LIST_HEAD(&bucket->head);
> 8fd6c5142395a1 Charlie Jenkins      2023-11-01  732  			bucket->bucket = current_head;
> d8792a5734b0f3 Charlie Jenkins      2023-11-27  733  			list_add(&bucket->head, used_buckets_list);
> 8fd6c5142395a1 Charlie Jenkins      2023-11-01  734  		}
> 8fd6c5142395a1 Charlie Jenkins      2023-11-01  735  		hlist_add_head(&rel_head->node, current_head);
> 8fd6c5142395a1 Charlie Jenkins      2023-11-01  736  	}
> 
> [ snip ]
> 
> e2c0cdfba7f699 Palmer Dabbelt       2017-07-10  773  int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
> e2c0cdfba7f699 Palmer Dabbelt       2017-07-10  774  		       unsigned int symindex, unsigned int relsec,
> e2c0cdfba7f699 Palmer Dabbelt       2017-07-10  775  		       struct module *me)
> e2c0cdfba7f699 Palmer Dabbelt       2017-07-10  776  {
> e2c0cdfba7f699 Palmer Dabbelt       2017-07-10  777  	Elf_Rela *rel = (void *) sechdrs[relsec].sh_addr;
> 8cbe0accc4a6ba Emil Renner Berthing 2023-11-01  778  	int (*handler)(struct module *me, void *location, Elf_Addr v);
> e2c0cdfba7f699 Palmer Dabbelt       2017-07-10  779  	Elf_Sym *sym;
> 8cbe0accc4a6ba Emil Renner Berthing 2023-11-01  780  	void *location;
> e2c0cdfba7f699 Palmer Dabbelt       2017-07-10  781  	unsigned int i, type;
> e2c0cdfba7f699 Palmer Dabbelt       2017-07-10  782  	Elf_Addr v;
> e2c0cdfba7f699 Palmer Dabbelt       2017-07-10  783  	int res;
> 8fd6c5142395a1 Charlie Jenkins      2023-11-01  784  	unsigned int num_relocations = sechdrs[relsec].sh_size / sizeof(*rel);
> d8792a5734b0f3 Charlie Jenkins      2023-11-27  785  	struct hlist_head *relocation_hashtable;
> d8792a5734b0f3 Charlie Jenkins      2023-11-27  786  	struct list_head used_buckets_list;
> d8792a5734b0f3 Charlie Jenkins      2023-11-27  787  	unsigned int hashtable_bits;
>                                                         ^^^^^^^^
> 
> d8792a5734b0f3 Charlie Jenkins      2023-11-27  788  
> d8792a5734b0f3 Charlie Jenkins      2023-11-27  789  	hashtable_bits = initialize_relocation_hashtable(num_relocations,
> d8792a5734b0f3 Charlie Jenkins      2023-11-27  790  							 &relocation_hashtable);
> d8792a5734b0f3 Charlie Jenkins      2023-11-27  791  
> d8792a5734b0f3 Charlie Jenkins      2023-11-27 @792  	if (hashtable_bits < 0)
>                                                             ^^^^^^^^^^^^^^^^^^
> Can't be less than zero.

I am returning a negative number in an unsigned function, oops. Since
the only possible failure in the function is an out-of-memory error and
0 is not a valid return, I will return 0 in the out-of-memory case.

- Charlie

> 
> d8792a5734b0f3 Charlie Jenkins      2023-11-27  793  		return hashtable_bits;
> d8792a5734b0f3 Charlie Jenkins      2023-11-27  794  
> d8792a5734b0f3 Charlie Jenkins      2023-11-27  795  	INIT_LIST_HEAD(&used_buckets_list);
> 
> -- 
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
>