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/[email protected]/config)
compiler: riscv64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231213/[email protected]/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 <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
| Closes: https://lore.kernel.org/r/[email protected]/
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().
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.
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
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/[email protected]/config)
> compiler: riscv64-linux-gcc (GCC) 13.2.0
> reproduce: (https://download.01.org/0day-ci/archive/20231213/[email protected]/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 <[email protected]>
> | Reported-by: Dan Carpenter <[email protected]>
> | Closes: https://lore.kernel.org/r/[email protected]/
>
> 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
>
On Wed, Dec 13, 2023 at 11:27:02AM -0800, Charlie Jenkins wrote:
> > 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.
You can just ignore false positives. It's not really a fix to change it
to a do-while loop. I reviewed the do while code before reading this
email and I still wondered about empty lists, but just to hear that it's
not going to be empty is enough. Just the email was sufficient.
regards,
dan carpenter
On Thu, Dec 14, 2023 at 11:00:46AM +0300, Dan Carpenter wrote:
> On Wed, Dec 13, 2023 at 11:27:02AM -0800, Charlie Jenkins wrote:
> > > 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.
>
> You can just ignore false positives. It's not really a fix to change it
> to a do-while loop. I reviewed the do while code before reading this
> email and I still wondered about empty lists, but just to hear that it's
> not going to be empty is enough. Just the email was sufficient.
>
> regards,
> dan carpenter
>
The freeing was actually broken so that needed to be fixed and I figured
that it was worthwhile to include the do-while in the same patch to get
rid of the warning.
- Charlie
On Thu, Dec 14, 2023 at 11:00:46AM +0300, Dan Carpenter wrote:
> On Wed, Dec 13, 2023 at 11:27:02AM -0800, Charlie Jenkins wrote:
> > > 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.
>
> You can just ignore false positives. It's not really a fix to change it
> to a do-while loop. I reviewed the do while code before reading this
> email and I still wondered about empty lists, but just to hear that it's
> not going to be empty is enough. Just the email was sufficient.
>
> regards,
> dan carpenter
>
The fix isn't the do-while loop but rather the use after free, the
incorrect sizeof, and incorrect error handling when
initialize_relocation_hashtable fails. I decided to include the do-while
code because I was already touching the surrounding code. Can you review
[1]? If you would prefer that the do-while is reverted, I can do that,
but it is important that the rest of the fixes are merged before 6.7 is
released.
[1] https://lore.kernel.org/all/[email protected]/
On Wed, Dec 27, 2023 at 04:59:25PM -0800, Charlie Jenkins wrote:
> On Thu, Dec 14, 2023 at 11:00:46AM +0300, Dan Carpenter wrote:
> > On Wed, Dec 13, 2023 at 11:27:02AM -0800, Charlie Jenkins wrote:
> > > > 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.
> >
> > You can just ignore false positives. It's not really a fix to change it
> > to a do-while loop. I reviewed the do while code before reading this
> > email and I still wondered about empty lists, but just to hear that it's
> > not going to be empty is enough. Just the email was sufficient.
> >
> > regards,
> > dan carpenter
> >
>
> The fix isn't the do-while loop but rather the use after free, the
> incorrect sizeof, and incorrect error handling when
> initialize_relocation_hashtable fails. I decided to include the do-while
> code because I was already touching the surrounding code. Can you review
> [1]? If you would prefer that the do-while is reverted, I can do that,
> but it is important that the rest of the fixes are merged before 6.7 is
> released.
>
> [1] https://lore.kernel.org/all/[email protected]/
Most subsystems wouldn't merge a patch like this which does so many
things at the same time...
regards,
dan carpenter
On Tue, Jan 02, 2024 at 03:37:34PM +0300, Dan Carpenter wrote:
> On Wed, Dec 27, 2023 at 04:59:25PM -0800, Charlie Jenkins wrote:
> > On Thu, Dec 14, 2023 at 11:00:46AM +0300, Dan Carpenter wrote:
> > > On Wed, Dec 13, 2023 at 11:27:02AM -0800, Charlie Jenkins wrote:
> > > > > 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.
> > >
> > > You can just ignore false positives. It's not really a fix to change it
> > > to a do-while loop. I reviewed the do while code before reading this
> > > email and I still wondered about empty lists, but just to hear that it's
> > > not going to be empty is enough. Just the email was sufficient.
> > >
> > > regards,
> > > dan carpenter
> > >
> >
> > The fix isn't the do-while loop but rather the use after free, the
> > incorrect sizeof, and incorrect error handling when
> > initialize_relocation_hashtable fails. I decided to include the do-while
> > code because I was already touching the surrounding code. Can you review
> > [1]? If you would prefer that the do-while is reverted, I can do that,
> > but it is important that the rest of the fixes are merged before 6.7 is
> > released.
> >
> > [1] https://lore.kernel.org/all/[email protected]/
>
>
> Most subsystems wouldn't merge a patch like this which does so many
> things at the same time...
That is a good point, I split it across 4 different patches.
https://lore.kernel.org/linux-riscv/[email protected]/
>
> regards,
> dan carpenter
>