diff mbox series

module: .strtab must be null terminated

Message ID 5uwynkpfhtqbrq47nqvp2ixpjhstjl7o7uxqp3b6snj233tvzi@avfrbljjpdel (mailing list archive)
State Handled Elsewhere
Headers show
Series module: .strtab must be null terminated | expand

Checks

Context Check Description
mcgrof/vmtest-modules-next-VM_Test-0 success Logs for build-kernel
mcgrof/vmtest-modules-next-VM_Test-1 pending Logs for Run kdevops CI on Self-hosted Runner
mcgrof/vmtest-modules-next-PR fail merge-conflict

Commit Message

Tobias Stoeckmann Oct. 19, 2024, 2:14 p.m. UTC
The string table must be NUL-terminated, just like the section name table.

Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
---
In order to create a proof of concept, which I can't get into a simple
script right now, it's easiest to move '.strtab' to the end of the module
file, write as many 'A' characters at the end as required according to the
section size, and try to insert the module.

In dmesg, you can see lines like:

```
poc: Unknown symbol AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA\xc6\xe9<\xfe\xff\xff\x8b
\xff\xc1\xe9\x03\xf3H\xa5\xeb\xa1I\x8b~ \xe8 (err -2)
```
---
 kernel/module/main.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

--
2.47.0

Comments

Luis Chamberlain Oct. 21, 2024, 7:54 p.m. UTC | #1
On Sat, Oct 19, 2024 at 04:14:40PM +0200, Tobias Stoeckmann wrote:
> The string table must be NUL-terminated, just like the section name table.
> 
> Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
> ---
> In order to create a proof of concept, which I can't get into a simple
> script right now, it's easiest to move '.strtab' to the end of the module
> file, write as many 'A' characters at the end as required according to the
> section size, and try to insert the module.
> 
> In dmesg, you can see lines like:
> 
> ```
> poc: Unknown symbol AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA\xc6\xe9<\xfe\xff\xff\x8b
> \xff\xc1\xe9\x03\xf3H\xa5\xeb\xa1I\x8b~ \xe8 (err -2)
> ```
> ---

Thanks can you post rebased aginst modules-next on the modules tree [0].
But please consider sending both patches and the new test one in a
series, I'll reply shortly about the other patch.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/modules/linux.git/

  Luisk
diff mbox series

Patch

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 49b9bca9d..9c5b373a7 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1847,6 +1847,18 @@  static int elf_validity_cache_copy(struct load_info *info, int flags)
 	info->index.str = shdr->sh_link;
 	info->strtab = (char *)info->hdr + info->sechdrs[info->index.str].sh_offset;

+	/*
+	 * The string table must be NUL-terminated, as required
+	 * by the spec. This makes strcmp and pr_* calls that access
+	 * strings in the section safe.
+	 */
+	strhdr = &info->sechdrs[info->index.str];
+	if (strhdr->sh_size > 0 && info->strtab[strhdr->sh_size - 1] != '\0') {
+		pr_err("module %s: string table isn't null terminated\n",
+		       info->name ?: "(missing .modinfo section or name field)");
+		goto no_exec;
+	}
+
 	/*
 	 * The ".gnu.linkonce.this_module" ELF section is special. It is
 	 * what modpost uses to refer to __this_module and let's use rely