| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230112214059.o4vq474c47edjup6@ldmartin-desk2>
Date: Thu, 12 Jan 2023 14:40:59 -0700
From: Lucas De Marchi <lucas.demarchi@...el.com>
To: Gary Guo <gary@...yguo.net>
CC: Michael Ellerman <mpe@...erman.id.au>,
Nicholas Piggin <npiggin@...il.com>,
Christophe Leroy <christophe.leroy@...roup.eu>,
"Luis Chamberlain" <mcgrof@...nel.org>,
Masahiro Yamada <masahiroy@...nel.org>,
Nathan Chancellor <nathan@...nel.org>,
Nick Desaulniers <ndesaulniers@...gle.com>,
Nicolas Schier <nicolas@...sle.eu>,
Miguel Ojeda <ojeda@...nel.org>,
Alex Gaynor <alex.gaynor@...il.com>,
Wedson Almeida Filho <wedsonaf@...il.com>,
Boqun Feng <boqun.feng@...il.com>,
"Björn Roy Baron" <bjorn3_gh@...tonmail.com>,
Kees Cook <keescook@...omium.org>,
"Joel Stanley" <joel@....id.au>,
Julia Lawall <Julia.Lawall@...ia.fr>,
Guo Zhengkui <guozhengkui@...o.com>,
Wedson Almeida Filho <wedsonaf@...gle.com>,
<linuxppc-dev@...ts.ozlabs.org>, <linux-kernel@...r.kernel.org>,
<linux-modules@...r.kernel.org>, <linux-kbuild@...r.kernel.org>,
<rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH] modpost: support arbitrary symbol length in modversion
On Wed, Jan 11, 2023 at 04:11:51PM +0000, Gary Guo wrote:
>Currently modversion uses a fixed size array of size (64 - sizeof(long))
>to store symbol names, thus placing a hard limit on length of symbols.
>Rust symbols (which encodes crate and module names) can be quite a bit
>longer. The length limit in kallsyms is increased to 512 for this reason.
>
>It's a waste of space to simply expand the fixed array size to 512 in
>modversion info entries. I therefore make it variably sized, with offset
>to the next entry indicated by the initial "next" field.
>
>In addition to supporting longer-than-56/60 byte symbols, this patch also
>reduce the size for short symbols by getting rid of excessive 0 paddings.
>There are still some zero paddings to ensure "next" and "crc" fields are
>properly aligned.
>
>This patch does have a tiny drawback that it makes ".mod.c" files generated
>a bit less easy to read, as code like
>
> "\x08\x00\x00\x00\x78\x56\x34\x12"
> "symbol\0\0"
>
>is generated as opposed to
>
> { 0x12345678, "symbol" },
>
>because the structure is now variable-length. But hopefully nobody reads
>the generated file :)
>
>Link: b8a94bfb3395 ("kallsyms: increase maximum kernel symbol length to 512")
>Link: https://github.com/Rust-for-Linux/linux/pull/379
>
>Signed-off-by: Gary Guo <gary@...yguo.net>
>---
> arch/powerpc/kernel/module_64.c | 3 ++-
> include/linux/module.h | 6 ++++--
> kernel/module/version.c | 21 +++++++++------------
> scripts/export_report.pl | 9 +++++----
> scripts/mod/modpost.c | 33 +++++++++++++++++++++++----------
> 5 files changed, 43 insertions(+), 29 deletions(-)
>
>diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
>index ff045644f13f..eac23c11d579 100644
>--- a/arch/powerpc/kernel/module_64.c
>+++ b/arch/powerpc/kernel/module_64.c
>@@ -236,10 +236,11 @@ static void dedotify_versions(struct modversion_info *vers,
> {
> struct modversion_info *end;
>
>- for (end = (void *)vers + size; vers < end; vers++)
>+ for (end = (void *)vers + size; vers < end; vers = (void *)vers + vers->next) {
> if (vers->name[0] == '.') {
> memmove(vers->name, vers->name+1, strlen(vers->name));
> }
>+ }
> }
>
> /*
>diff --git a/include/linux/module.h b/include/linux/module.h
>index 8c5909c0076c..37cb25af9099 100644
>--- a/include/linux/module.h
>+++ b/include/linux/module.h
>@@ -34,8 +34,10 @@
> #define MODULE_NAME_LEN MAX_PARAM_PREFIX_LEN
>
> struct modversion_info {
>- unsigned long crc;
>- char name[MODULE_NAME_LEN];
>+ /* Offset of the next modversion entry in relation to this one. */
>+ u32 next;
>+ u32 crc;
>+ char name[0];
although not really exported as uapi, this will break userspace as this is
used in the elf file generated for the modules. I think
this change must be made in a backward compatible way and kmod updated
to deal with the variable name length:
kmod $ git grep "\[64"
libkmod/libkmod-elf.c: char name[64 - sizeof(uint32_t)];
libkmod/libkmod-elf.c: char name[64 - sizeof(uint64_t)];
in kmod we have both 32 and 64 because a 64-bit kmod can read both 32
and 64 bit module, and vice versa.
Lucas De Marchi
> };
>
> struct module;
>diff --git a/kernel/module/version.c b/kernel/module/version.c
>index 53f43ac5a73e..af7478dcc158 100644
>--- a/kernel/module/version.c
>+++ b/kernel/module/version.c
>@@ -17,32 +17,29 @@ int check_version(const struct load_info *info,
> {
> Elf_Shdr *sechdrs = info->sechdrs;
> unsigned int versindex = info->index.vers;
>- unsigned int i, num_versions;
>- struct modversion_info *versions;
>+ struct modversion_info *versions, *end;
>+ u32 crcval;
>
> /* Exporting module didn't supply crcs? OK, we're already tainted. */
> if (!crc)
> return 1;
>+ crcval = *crc;
>
> /* No versions at all? modprobe --force does this. */
> if (versindex == 0)
> return try_to_force_load(mod, symname) == 0;
>
> versions = (void *)sechdrs[versindex].sh_addr;
>- num_versions = sechdrs[versindex].sh_size
>- / sizeof(struct modversion_info);
>+ end = (void *)versions + sechdrs[versindex].sh_size;
>
>- for (i = 0; i < num_versions; i++) {
>- u32 crcval;
>-
>- if (strcmp(versions[i].name, symname) != 0)
>+ for (; versions < end; versions = (void *)versions + versions->next) {
>+ if (strcmp(versions->name, symname) != 0)
> continue;
>
>- crcval = *crc;
>- if (versions[i].crc == crcval)
>+ if (versions->crc == crcval)
> return 1;
>- pr_debug("Found checksum %X vs module %lX\n",
>- crcval, versions[i].crc);
>+ pr_debug("Found checksum %X vs module %X\n",
>+ crcval, versions->crc);
> goto bad_version;
> }
>
>diff --git a/scripts/export_report.pl b/scripts/export_report.pl
>index feb3d5542a62..1117646f3141 100755
>--- a/scripts/export_report.pl
>+++ b/scripts/export_report.pl
>@@ -116,18 +116,19 @@ foreach my $thismod (@allcfiles) {
> while ( <$module> ) {
> chomp;
> if ($state == 0) {
>- $state = 1 if ($_ =~ /static const struct modversion_info/);
>+ $state = 1 if ($_ =~ /static const char ____versions/);
> next;
> }
> if ($state == 1) {
>- $state = 2 if ($_ =~ /__attribute__\(\(section\("__versions"\)\)\)/);
>+ $state = 2 if ($_ =~ /__used __section\("__versions"\)/);
> next;
> }
> if ($state == 2) {
>- if ( $_ !~ /0x[0-9a-f]+,/ ) {
>+ if ( $_ !~ /\\0"/ ) {
>+ last if ($_ =~ /;/);
> next;
> }
>- my $sym = (split /([,"])/,)[4];
>+ my $sym = (split /(["\\])/,)[2];
> my ($module, $value, $symbol, $gpl) = @{$SYMBOL{$sym}};
> $SYMBOL{ $sym } = [ $module, $value+1, $symbol, $gpl];
> push(@{$MODULE{$thismod}} , $sym);
>diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
>index efff8078e395..334d170de31f 100644
>--- a/scripts/mod/modpost.c
>+++ b/scripts/mod/modpost.c
>@@ -2046,13 +2046,17 @@ static void add_exported_symbols(struct buffer *buf, struct module *mod)
> static void add_versions(struct buffer *b, struct module *mod)
> {
> struct symbol *s;
>+ unsigned int name_len;
>+ unsigned int name_len_padded;
>+ unsigned int tmp;
>+ unsigned char *tmp_view = (unsigned char *)&tmp;
>
> if (!modversions)
> return;
>
> buf_printf(b, "\n");
>- buf_printf(b, "static const struct modversion_info ____versions[]\n");
>- buf_printf(b, "__used __section(\"__versions\") = {\n");
>+ buf_printf(b, "static const char ____versions[]\n");
>+ buf_printf(b, "__used __section(\"__versions\") =\n");
>
> list_for_each_entry(s, &mod->unresolved_symbols, list) {
> if (!s->module)
>@@ -2062,16 +2066,25 @@ static void add_versions(struct buffer *b, struct module *mod)
> s->name, mod->name);
> continue;
> }
>- if (strlen(s->name) >= MODULE_NAME_LEN) {
>- error("too long symbol \"%s\" [%s.ko]\n",
>- s->name, mod->name);
>- break;
>- }
>- buf_printf(b, "\t{ %#8x, \"%s\" },\n",
>- s->crc, s->name);
>+ name_len = strlen(s->name);
>+ name_len_padded = (name_len + 1 + 3) & ~3;
>+
>+ /* Offset to next entry */
>+ tmp = TO_NATIVE(8 + name_len_padded);
>+ buf_printf(b, "\t\"\\x%02x\\x%02x\\x%02x\\x%02x",
>+ tmp_view[0], tmp_view[1], tmp_view[2], tmp_view[3]);
>+
>+ tmp = TO_NATIVE(s->crc);
>+ buf_printf(b, "\\x%02x\\x%02x\\x%02x\\x%02x\"\n",
>+ tmp_view[0], tmp_view[1], tmp_view[2], tmp_view[3]);
>+
>+ buf_printf(b, "\t\"%s", s->name);
>+ for (; name_len < name_len_padded; name_len++)
>+ buf_printf(b, "\\0");
>+ buf_printf(b, "\"\n");
> }
>
>- buf_printf(b, "};\n");
>+ buf_printf(b, ";\n");
> }
>
> static void add_depends(struct buffer *b, struct module *mod)
>--
>2.34.1
>
Powered by blists - more mailing lists