[PATCH] module: .strtab must be null terminated

[PATCH] module: .strtab must be null terminated

[Tobias Stoeckmann]

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

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
--
2.47.0

Re: [PATCH] module: .strtab must be null terminated

[Alexey Dobriyan]

> +	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;
> +	}

Reviewed-off-by: Alexey Dobriyan <adobriyan@gmail.com>

The _very_ correct test is "there must be NUL between last index used
and the end of the section somewhere".

.shstrtab has the same overly restrictive test:

	if (info->secstrings[strhdr->sh_size - 1] != '\0') {

But, of course, it doesn't matter in practice.

Re: [PATCH] module: .strtab must be null terminated

[Luis Chamberlain]

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