public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* arch/riscv/kernel/module.c:639 process_accumulated_relocations() error: uninitialized symbol 'curr_type'.
@ 2023-12-13 13:05 Dan Carpenter
  2023-12-13 19:27 ` Charlie Jenkins
  0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2023-12-13 13:05 UTC (permalink / raw)
  To: oe-kbuild, Charlie Jenkins
  Cc: lkp, oe-kbuild-all, linux-kernel, Palmer Dabbelt

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().

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: arch/riscv/kernel/module.c:639 process_accumulated_relocations() error: uninitialized symbol 'curr_type'.
  2023-12-13 13:05 arch/riscv/kernel/module.c:639 process_accumulated_relocations() error: uninitialized symbol 'curr_type' Dan Carpenter
@ 2023-12-13 19:27 ` Charlie Jenkins
  2023-12-14  8:00   ` Dan Carpenter
  0 siblings, 1 reply; 7+ messages in thread
From: Charlie Jenkins @ 2023-12-13 19:27 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: oe-kbuild, lkp, oe-kbuild-all, linux-kernel, Palmer Dabbelt

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
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: arch/riscv/kernel/module.c:639 process_accumulated_relocations() error: uninitialized symbol 'curr_type'.
  2023-12-13 19:27 ` Charlie Jenkins
@ 2023-12-14  8:00   ` Dan Carpenter
  2023-12-14 19:26     ` Charlie Jenkins
  2023-12-28  0:59     ` Charlie Jenkins
  0 siblings, 2 replies; 7+ messages in thread
From: Dan Carpenter @ 2023-12-14  8:00 UTC (permalink / raw)
  To: Charlie Jenkins
  Cc: oe-kbuild, lkp, oe-kbuild-all, linux-kernel, Palmer Dabbelt

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: arch/riscv/kernel/module.c:639 process_accumulated_relocations() error: uninitialized symbol 'curr_type'.
  2023-12-14  8:00   ` Dan Carpenter
@ 2023-12-14 19:26     ` Charlie Jenkins
  2023-12-28  0:59     ` Charlie Jenkins
  1 sibling, 0 replies; 7+ messages in thread
From: Charlie Jenkins @ 2023-12-14 19:26 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: oe-kbuild, lkp, oe-kbuild-all, linux-kernel, Palmer Dabbelt

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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: arch/riscv/kernel/module.c:639 process_accumulated_relocations() error: uninitialized symbol 'curr_type'.
  2023-12-14  8:00   ` Dan Carpenter
  2023-12-14 19:26     ` Charlie Jenkins
@ 2023-12-28  0:59     ` Charlie Jenkins
  2024-01-02 12:37       ` Dan Carpenter
  1 sibling, 1 reply; 7+ messages in thread
From: Charlie Jenkins @ 2023-12-28  0:59 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: oe-kbuild, lkp, oe-kbuild-all, linux-kernel, Palmer Dabbelt

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/20231213-module_loading_fix-v1-1-da9b7c92ade5@rivosinc.com/


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: arch/riscv/kernel/module.c:639 process_accumulated_relocations() error: uninitialized symbol 'curr_type'.
  2023-12-28  0:59     ` Charlie Jenkins
@ 2024-01-02 12:37       ` Dan Carpenter
  2024-01-03 20:27         ` Charlie Jenkins
  0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2024-01-02 12:37 UTC (permalink / raw)
  To: Charlie Jenkins
  Cc: oe-kbuild, lkp, oe-kbuild-all, linux-kernel, Palmer Dabbelt

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/20231213-module_loading_fix-v1-1-da9b7c92ade5@rivosinc.com/


Most subsystems wouldn't merge a patch like this which does so many
things at the same time...

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: arch/riscv/kernel/module.c:639 process_accumulated_relocations() error: uninitialized symbol 'curr_type'.
  2024-01-02 12:37       ` Dan Carpenter
@ 2024-01-03 20:27         ` Charlie Jenkins
  0 siblings, 0 replies; 7+ messages in thread
From: Charlie Jenkins @ 2024-01-03 20:27 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: oe-kbuild, lkp, oe-kbuild-all, linux-kernel, Palmer Dabbelt

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/20231213-module_loading_fix-v1-1-da9b7c92ade5@rivosinc.com/
> 
> 
> 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/20240103-module_loading_fix-v2-0-292b160552c9@rivosinc.com/

> 
> regards,
> dan carpenter
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2024-01-03 20:27 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-13 13:05 arch/riscv/kernel/module.c:639 process_accumulated_relocations() error: uninitialized symbol 'curr_type' Dan Carpenter
2023-12-13 19:27 ` Charlie Jenkins
2023-12-14  8:00   ` Dan Carpenter
2023-12-14 19:26     ` Charlie Jenkins
2023-12-28  0:59     ` Charlie Jenkins
2024-01-02 12:37       ` Dan Carpenter
2024-01-03 20:27         ` Charlie Jenkins

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox