It must be verified that the symbol name offsets point into the
string table, not outside of it.

Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
---
Proof of Concept:

1. Create "poc.sh"

```
cat > poc.sh << EOF
#!/bin/sh
# Sets an illegal symbol name offset in supplied uncompressed module
# usage: ./poc file.ko

MODULE="$1"
BASE=$(readelf -S $MODULE | grep '\.symtab' | awk '{ print $5 }')
if [ $(getconf LONG_BIT) = '64' ]
then
	OFF=24
else
	OFF=16
fi
ADDR=$(python -c "print(int(0x$BASE) + $OFF)")
echo -n 'AAAA' | dd bs=1 count=4 of=$MODULE seek=$ADDR conv=notrunc
echo $ADDR
EOF
```

2. Choose a module which works for your system (adjust if compressed)

```
cp $(find /lib/modules/$(uname -r) |grep ko$ | head -n 1) poc.ko
```

3. Modify module

```
sh poc.sh poc.ko
```

4. Try to insert

```
insmod poc.ko
```

In dmesg, you can see lines like:

```
BUG: unable to handle page fault for address: ffff9802022d6f81
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
PGD 100000067 P4D 100000067 PUD 0
---
 kernel/module/main.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 9c5b373a7..c926960ae 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1688,6 +1688,7 @@ static int elf_validity_cache_copy(struct load_info *info, int flags)
 {
 	unsigned int i;
 	Elf_Shdr *shdr, *strhdr;
+	Elf_Sym *sym;
 	int err;
 	unsigned int num_mod_secs = 0, mod_idx;
 	unsigned int num_info_secs = 0, info_idx;
@@ -1859,6 +1860,17 @@ static int elf_validity_cache_copy(struct load_info *info, int flags)
 		goto no_exec;
 	}

+	/* Symbol names must point into string table. */
+	shdr = &info->sechdrs[info->index.sym];
+	sym = (void *)info->hdr + shdr->sh_offset;
+	for (i = 1; i < shdr->sh_size / sizeof(Elf_Sym); i++) {
+		if (sym[i].st_name >= strhdr->sh_size) {
+			pr_err("module %s: illegal symbol name offset encountered\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

On Sat, Oct 19, 2024 at 04:15:32PM +0200, Tobias Stoeckmann wrote:
> It must be verified that the symbol name offsets point into the
> string table, not outside of it.
> 
> Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
> ---
> Proof of Concept:
> 
> 1. Create "poc.sh"
> 
> ```
> cat > poc.sh << EOF
> #!/bin/sh
> # Sets an illegal symbol name offset in supplied uncompressed module
> # usage: ./poc file.ko
> 
> MODULE="$1"
> BASE=$(readelf -S $MODULE | grep '\.symtab' | awk '{ print $5 }')
> if [ $(getconf LONG_BIT) = '64' ]
> then
> 	OFF=24
> else
> 	OFF=16
> fi
> ADDR=$(python -c "print(int(0x$BASE) + $OFF)")
> echo -n 'AAAA' | dd bs=1 count=4 of=$MODULE seek=$ADDR conv=notrunc
> echo $ADDR
> EOF
> ```
> 
> 2. Choose a module which works for your system (adjust if compressed)
> 
> ```
> cp $(find /lib/modules/$(uname -r) |grep ko$ | head -n 1) poc.ko
> ```
> 
> 3. Modify module
> 
> ```
> sh poc.sh poc.ko
> ```
> 
> 4. Try to insert
> 
> ```
> insmod poc.ko
> ```
> 
> In dmesg, you can see lines like:
> 
> ```
> BUG: unable to handle page fault for address: ffff9802022d6f81
> #PF: supervisor read access in kernel mode
> #PF: error_code(0x0000) - not-present page
> PGD 100000067 P4D 100000067 PUD 0
> ---

Thanks! Any chance I can convince you to write you PoC as a new test
under lib/tests/module/, see my new patch which adds a new module
dedicated test [0] which you can build upon to add a new test there.

And then you can make a series with 3 patches for this and your prior one,
and you can just refer to the PoC in the fix.

[0] https://lkml.kernel.org/r/20241021193310.2014131-1-mcgrof@kernel.org

  Luis

Hi Luis,

On Mon, Oct 21, 2024 at 12:55:34PM -0700, Luis Chamberlain wrote:
> And then you can make a series with 3 patches for this and your prior one,
> and you can just refer to the PoC in the fix.

Thanks for the hint to rebase on modules-next. There is no need for my
patches, because the checks have been written by Matthew Maurer, which
cover these cases.

So... Sorry for the noise and thanks to Matthew for writing them.
Clarifies that specs are correct and checks were missing. :)


With kind regards

Tobias

On Mon, Oct 21, 2024 at 10:20:38PM +0200, Tobias Stoeckmann wrote:
> Hi Luis,
> 
> On Mon, Oct 21, 2024 at 12:55:34PM -0700, Luis Chamberlain wrote:
> > And then you can make a series with 3 patches for this and your prior one,
> > and you can just refer to the PoC in the fix.
> 
> Thanks for the hint to rebase on modules-next. There is no need for my
> patches, because the checks have been written by Matthew Maurer, which
> cover these cases.
> 
> So... Sorry for the noise and thanks to Matthew for writing them.
> Clarifies that specs are correct and checks were missing. :)

It does not mean that older kernels have them. And selftests are used to
test older kernel, and so the question here is if a patch from Matthew
should be backported, and if so which one? So your PoC test is still welcome.

  Luis

On Sat, Oct 19, 2024 at 04:15:33PM +0200, Tobias Stoeckmann wrote:
> +		if (sym[i].st_name >= strhdr->sh_size) {

Please note that this commit only makes sense being applied AFTER
the other patch sent, i.e. "module: .strtab must be null terminated"
because that patch modifies strhdr before this line.

Sorry for messing up the PATCH 1/2 and 2/2 connection.


With kind regards

Tobias