Re: arch/riscv/kernel/module.c:639 process_accumulated_relocations() error: uninitialized symbol 'curr_type'.
From: Charlie Jenkins
Date: Wed Dec 13 2023 - 14:27:13 EST
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@xxxxxxxxx/config)
> compiler: riscv64-linux-gcc (GCC) 13.2.0
> reproduce: (https://download.01.org/0day-ci/archive/20231213/202312130859.wnkuzVWY-lkp@xxxxxxxxx/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@xxxxxxxxx>
> | Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> | Closes: https://lore.kernel.org/r/202312130859.wnkuzVWY-lkp@xxxxxxxxx/
>
> 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
>