On Wed, Mar 15, 2023 at 01:15:03AM +0100, Vincenzo Palazzo wrote:
> > In practice, this is what I'm testing at the moment:
> >
> > ---
> > diff --git a/arch/powerpc/kernel/module_64.c
> > b/arch/powerpc/kernel/module_64.c
> > index ff045644f13f..ea6c830ed1e7 100644
> > --- a/arch/powerpc/kernel/module_64.c
> > +++ b/arch/powerpc/kernel/module_64.c
> > @@ -234,12 +234,13 @@ static unsigned long get_stubs_size(const Elf64_Ehdr
> > *hdr,
> > static void dedotify_versions(struct modversion_info *vers,
> > unsigned long size)
> > {
> > - struct modversion_info *end;
> > + struct modversion_info *end = (void *)vers + size;
> >
> > - for (end = (void *)vers + size; vers < end; vers++)
> > + for (; vers < end && vers->next; 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..4744901bdf63 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -34,9 +34,11 @@
> > #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];
> > +} __packed;
> >
> > struct module;
> > struct exception_table_entry;
> > diff --git a/kernel/module/version.c b/kernel/module/version.c
> > index 53f43ac5a73e..5528f98c42dc 100644
> > --- a/kernel/module/version.c
> > +++ b/kernel/module/version.c
> > @@ -17,32 +17,30 @@ 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->next;
> > + 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..55335ae98f4f 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,27 @@ 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 = 8 + name_len_padded;
> > + tmp = TO_NATIVE(tmp);
> > + 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");
> > + /* Always end with a dummy NULL entry */
> > + buf_printf(b, "\t\"\\x00\\x00\\x00\\x00\\x00\\x00\\x00\\x00\";\n");
> > }
> >
> > static void add_depends(struct buffer *b, struct module *mod)
> This looks promissing, do you had a small reproducer to this just to
> make the code a little bit clearn to the reader?
My reproducer was to simply boot the latest 6.2 kernel with this patch
appplied on top on a ppc64 server (without the additional changes the
system just fails to boot as soon as the first module is loaded,
spitting random errors/oops to the console, due to random memory
accesses). I don't have a smaller reproducer than this, maybe I can
clarify a bit more the purpose of the last dummy entry in the comment to
help the reader understand why we need this empty entry (if I understand
your request correctly).
For the TO_NATIVE(tmp) change, instead, it's enough to build the kernel
with a recent gcc (I'm using gcc 12.2.0-17ubuntu1) and the build just
fails.
-Andrea
