Skip to contentSkip to conversation

riscv: modules: Fix module loading error handling

PATCH Applied Charlie Jenkins 3 participants 6 messages Jan 4, 2024 – Jan 11, 2024 RISC-V

lore
Share & tools
lore
#0 lore raw

v3 cover riscv: modules: Fix module loading error handling

When modules are loaded while there is not ample allocatable memory, there was previously not proper error handling. This series fixes a use-after-free error and a different issue that caused a non graceful exit after memory was not properly allocated.

---
Changes in v3:
- Drop patch using do-while
- Link to v2: https://lore.kernel.org/r/[email protected]

Changes in v2:
- Split changes across multiple patches
- Link to v1: https://lore.kernel.org/r/[email protected]

---
Charlie Jenkins (3):
riscv: Fix module loading free order
riscv: Correctly free relocation hashtable on error
riscv: Fix relocation_hashtable size

 arch/riscv/kernel/module.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

---
base-commit: a39b6ac3781d46ba18193c9dbb2110f31e9bffe9
change-id: 20231213-module_loading_fix-3ac6d4ea8129

Signature · 1 line
- Charlie
#1 lore raw

PATCH v3 1/3 riscv: Fix module loading free order

Reverse order of kfree calls to resolve use-after-free error.

---
 arch/riscv/kernel/module.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/riscv/kernel/module.c +1 -1
index aac019ed63b1..21c7a773a8ef 100644
@@ -723,8 +723,8 @@ static int add_relocation_to_accumulate(struct module *me, int type,
 
 			if (!bucket) {
 				kfree(entry);
-				kfree(rel_head);
 				kfree(rel_head->rel_entry);
+				kfree(rel_head);
 				return -ENOMEM;
 			}
 
Signature · 1 line
2.43.0
#2 lore raw

PATCH v3 2/3 riscv: Correctly free relocation hashtable on error

In reply to Charlie Jenkins

When there is not enough allocatable memory for the relocation hashtable, module loading should exit gracefully. Previously, this was attempted to be accomplished by checking if an unsigned number is less than zero which does not work. Instead have the caller check if the hashtable was correctly allocated and add a comment explaining that hashtable_bits that is 0 is valid.

---
 arch/riscv/kernel/module.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
arch/riscv/kernel/module.c +7 -3
index 21c7a773a8ef..32743180e8ef 100644
@@ -747,6 +747,10 @@ initialize_relocation_hashtable(unsigned int num_relocations,
 {
 	/* Can safely assume that bits is not greater than sizeof(long) */
 	unsigned long hashtable_size = roundup_pow_of_two(num_relocations);
+	/*
+	 * When hashtable_size == 1, hashtable_bits == 0.
+	 * This is valid because the hashing algorithm returns 0 in this case.
+	 */
 	unsigned int hashtable_bits = ilog2(hashtable_size);
 
 	/*
@@ -763,7 +767,7 @@ initialize_relocation_hashtable(unsigned int num_relocations,
 					      sizeof(*relocation_hashtable),
 					      GFP_KERNEL);
 	if (!*relocation_hashtable)
-		return -ENOMEM;
+		return 0;
 
 	__hash_init(*relocation_hashtable, hashtable_size);
 
@@ -789,8 +793,8 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
 	hashtable_bits = initialize_relocation_hashtable(num_relocations,
 							 &relocation_hashtable);
 
-	if (hashtable_bits < 0)
-		return hashtable_bits;
+	if (!relocation_hashtable)
+		return -ENOMEM;
 
 	INIT_LIST_HEAD(&used_buckets_list);
 
Signature · 1 line
2.43.0
#3 lore raw

PATCH v3 3/3 riscv: Fix relocation_hashtable size

In reply to Charlie Jenkins

A second dereference is needed to get the accurate size of the relocation_hashtable.

Fixes d8792a5734b0 ("riscv: Safely remove entries from relocation list")
---
 arch/riscv/kernel/module.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/riscv/kernel/module.c +1 -1
index 32743180e8ef..ceb0adb38715 100644
@@ -764,7 +764,7 @@ initialize_relocation_hashtable(unsigned int num_relocations,
 	hashtable_size <<= should_double_size;
 
 	*relocation_hashtable = kmalloc_array(hashtable_size,
-					      sizeof(*relocation_hashtable),
+					      sizeof(**relocation_hashtable),
 					      GFP_KERNEL);
 	if (!*relocation_hashtable)
 		return 0;
Signature · 1 line
2.43.0
Reviewed-by on the v3 series
#4 lore raw
In reply to Charlie Jenkins
Quoted 6 lines · Charlie Jenkins

When modules are loaded while there is not ample allocatable memory, there was previously not proper error handling. This series fixes a use-after-free error and a different issue that caused a non graceful exit after memory was not properly allocated.

Thanks!

regards,
dan carpenter

#5 lore raw
In reply to Charlie Jenkins
This series was applied to riscv/linux.git (for-next) by Palmer Dabbelt <[email protected]>:

Hello:

This series was applied to riscv/linux.git (for-next) by Palmer Dabbelt <[email protected]>:

Quoted 8 lines · On Thu, 04 Jan 2024 11:42:46 -0800 you wrote:

When modules are loaded while there is not ample allocatable memory, there was previously not proper error handling. This series fixes a use-after-free error and a different issue that caused a non graceful exit after memory was not properly allocated.

[...]

Here is the summary with links:
- [v3,1/3] riscv: Fix module loading free order

    https://git.kernel.org/riscv/c/78996eee79eb
  - [v3,2/3] riscv: Correctly free relocation hashtable on error
    https://git.kernel.org/riscv/c/4b38b36bfbd8
  - [v3,3/3] riscv: Fix relocation_hashtable size
    https://git.kernel.org/riscv/c/a35551c7244d

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html