| Message ID | 2hhrajjoxixnkhtlhhqzjxki4iuhr362345wgrmg6uzbfhlupo@hgbjsb5wizir (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | module: check symbol name offsets | expand |
| Context | Check | Description |
|---|---|---|
| mcgrof/vmtest-modules-next-PR | fail | merge-conflict |
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
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
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
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(+) -- 2.47.0