Re: arch/riscv/kernel/module.c:639 process_accumulated_relocations() error: uninitialized symbol 'curr_type'.

[Charlie Jenkins <charlie@rivosinc.com>]

On Wed, Dec 13, 2023 at 04:05:06PM +0300, Dan Carpenter wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head:   cf52eed70e555e864120cfaf280e979e2a035c66
> commit: 8fd6c5142395a106b63c8668e9f4a7106b6a0772 riscv: Add remaining module relocations
> config: riscv-randconfig-r071-20231211 (https://download.01.org/0day-ci/archive/20231213/202312130859.wnkuzVWY-lkp@intel.com/config)
> compiler: riscv64-linux-gcc (GCC) 13.2.0
> reproduce: (https://download.01.org/0day-ci/archive/20231213/202312130859.wnkuzVWY-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/202312130859.wnkuzVWY-lkp@intel.com/
> 
> New smatch warnings:
> arch/riscv/kernel/module.c:639 process_accumulated_relocations() error: uninitialized symbol 'curr_type'.
> 
> Old smatch warnings:
> arch/riscv/kernel/module.c:632 process_accumulated_relocations() error: dereferencing freed memory 'rel_entry_iter'
> arch/riscv/kernel/module.c:629 process_accumulated_relocations() error: dereferencing freed memory 'rel_head_iter'
> arch/riscv/kernel/module.c:628 process_accumulated_relocations() error: dereferencing freed memory 'bucket_iter'
> 
> vim +/curr_type +639 arch/riscv/kernel/module.c
> 
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  602  void process_accumulated_relocations(struct module *me)
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  603  {
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  604  	/*
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  605  	 * Only ADD/SUB/SET/ULEB128 should end up here.
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  606  	 *
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  607  	 * Each bucket may have more than one relocation location. All
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  608  	 * relocations for a location are stored in a list in a bucket.
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  609  	 *
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  610  	 * Relocations are applied to a temp variable before being stored to the
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  611  	 * provided location to check for overflow. This also allows ULEB128 to
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  612  	 * properly decide how many entries are needed before storing to
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  613  	 * location. The final value is stored into location using the handler
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  614  	 * for the last relocation to an address.
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  615  	 *
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  616  	 * Three layers of indexing:
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  617  	 *	- Each of the buckets in use
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  618  	 *	- Groups of relocations in each bucket by location address
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  619  	 *	- Each relocation entry for a location address
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  620  	 */
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  621  	struct used_bucket *bucket_iter;
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  622  	struct relocation_head *rel_head_iter;
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  623  	struct relocation_entry *rel_entry_iter;
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  624  	int curr_type;
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  625  	void *location;
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  626  	long buffer;
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  627  
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  628  	list_for_each_entry(bucket_iter, &used_buckets_list, head) {
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  629  		hlist_for_each_entry(rel_head_iter, bucket_iter->bucket, node) {
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  630  			buffer = 0;
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  631  			location = rel_head_iter->location;
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  632  			list_for_each_entry(rel_entry_iter,
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  633  					    rel_head_iter->rel_entry, head) {
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  634  				curr_type = rel_entry_iter->type;
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  635  				reloc_handlers[curr_type].reloc_handler(
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  636  					me, &buffer, rel_entry_iter->value);
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  637  				kfree(rel_entry_iter);
> 
> This kfree() will lead to a NULL dereference on the next iteration
> through the loop.  You need to use list_for_each_entry_safe().
> 

This has been fixed in 6.7-rc5.

> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  638  			}
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01 @639  			reloc_handlers[curr_type].accumulate_handler(
>                                                                                        ^^^^^^^^^
> Can the list be empty?  Uninitialized in that case.

That's a tricky one, the list cannot be empty. Each bucket in the
bucket_iter is guarunteed to have at least one rel_entry. I can probably
resolve this by extracting this for loop into a do-while loop.

- Charlie

> 
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  640  				me, location, buffer);
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  641  			kfree(rel_head_iter);
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  642  		}
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  643  		kfree(bucket_iter);
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  644  	}
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  645  
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  646  	kfree(relocation_hashtable);
> 8fd6c5142395a1 Charlie Jenkins 2023-11-01  647  }
> 
> -- 
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
>