Re: [PATCH 2/3] modpost: Extended modversion support

[Luis Chamberlain]

On Wed, Nov 15, 2023 at 06:50:10PM +0000, Matthew Maurer wrote:
> Adds a new format for modversions which stores each field in a separate
> elf section.

The "why" is critical and not mentioned. And I'd like to also see
documented this with foresight, if Rust needed could this be used
in the future for other things?

Also please include folks CC'd in *one* patch to *all* patches as
otherwise we have no context.

> This initially adds support for variable length names, but
> could later be used to add additional fields to modversions in a
> backwards compatible way if needed.
> 
> Adding support for variable length names makes it possible to enable
> MODVERSIONS and RUST at the same time.
> 
> Signed-off-by: Matthew Maurer <mmaurer@xxxxxxxxxx>
> ---
>  arch/powerpc/kernel/module_64.c | 24 +++++++++-

Why was only powerpc modified? If the commit log explained this it would
make it easier for review.

> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index c8b7b4dcf782..0c188c96a045 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -80,7 +80,7 @@ struct load_info {
>  	unsigned int used_pages;
>  #endif
>  	struct {
> -		unsigned int sym, str, mod, vers, info, pcpu;
> +		unsigned int sym, str, mod, vers, info, pcpu, vers_ext_crc, vers_ext_name;

We might as well modify this in a preliminary patch to add each new
unsinged int in a new line, so that it is easier to blame when each new
entry gets added. It should not grow the size of the struct at all but
it would make futur extensions easier to review what is new and git
blame easier to spot when something was added.

Although we don't use this extensively today this can easily grow for
convenience and making code easier to read.

> diff --git a/kernel/module/version.c b/kernel/module/version.c
> index 53f43ac5a73e..93d97dad8c77 100644
> --- a/kernel/module/version.c
> +++ b/kernel/module/version.c
> @@ -19,11 +19,28 @@ int check_version(const struct load_info *info,
>  	unsigned int versindex = info->index.vers;
>  	unsigned int i, num_versions;
>  	struct modversion_info *versions;
> +	struct modversion_info_ext version_ext;
>  
>  	/* Exporting module didn't supply crcs?  OK, we're already tainted. */
>  	if (!crc)
>  		return 1;
>  
> +	/* If we have extended version info, rely on it */
> +	if (modversion_ext_start(info, &version_ext) >= 0) {

There are two things we need to do to make processing modules easier:

  1) ELF validation
  2) Once checked then process the information

We used to have this split up but also had a few places which did both
1) and 2) together. This was wrong and so I want to keep things tidy
and ensure we do things which validate the ELF separate. To that
end please put the checks to validate the ELF first so that we report
to users with a proper error/debug check in case the ELF is wrong,
this enables futher debug checks for that to be done instead of
confusing users who end up scratching their heads why something
failed.

So please split up the ELF validation check and put that into
elf_validity_cache_copy() which runs *earlier* than this.

Then *if* if has this, you just process it. Please take care to be
very pedantic in the elf_validity_cache_copy() and extend the checks
you have for validation in modversion_ext_start() and bring them to
elf_validity_cache_copy() with perhaps *more* stuff which does any
insane checks to verify it is 100% correct.

> +		do {
> +			if (strncmp(version_ext.name.value, symname,
> +				    version_ext.name.end - version_ext.name.value) != 0)
> +				continue;
> +
> +			if (*version_ext.crc.value == *crc)
> +				return 1;
> +			pr_debug("Found checksum %X vs module %X\n",
> +				 *crc, *version_ext.crc.value);
> +			goto bad_version;
> +		} while (modversion_ext_advance(&version_ext) == 0);

Can you do a for_each_foo()) type loop here instead after validation?
Because the validation would ensure your loop is bounded then. Look at
for_each_mod_mem_type() for inspiration.

> +		goto broken_toolchain;

The broken toolchain thing would then be an issue reported in the
ELF validation.

> @@ -87,6 +105,65 @@ int same_magic(const char *amagic, const char *bmagic,
>  	return strcmp(amagic, bmagic) == 0;
>  }
>  
> +#define MODVERSION_FIELD_START(sec, field) \
> +	field.value = (typeof(field.value))sec.sh_addr; \
> +	field.end = field.value + sec.sh_size
> +
> +ssize_t modversion_ext_start(const struct load_info *info,
> +			     struct modversion_info_ext *start)
> +{
> +	unsigned int crc_idx = info->index.vers_ext_crc;
> +	unsigned int name_idx = info->index.vers_ext_name;
> +	Elf_Shdr *sechdrs = info->sechdrs;
> +
> +	// Both of these fields are needed for this to be useful
> +	// Any future fields should be initialized to NULL if absent.

Curious, what gave you the impression // type style comments are
welcomed, please replace that with either a one line 

/* foo comment */

Or a multi-line:

/*
 * stuff and go into great deatils
 * more elaaborate explanation
 */

Of even better, since you are moving this to ELF Validation please add
undertand what elf_validity_cache_copy() does, and add kdoc style
comments for it and then extend it with why Rust needs these magical things.

> +	if ((crc_idx == 0) || (name_idx == 0))
> +		return -EINVAL;
> +
> +	MODVERSION_FIELD_START(sechdrs[crc_idx], start->crc);
> +	MODVERSION_FIELD_START(sechdrs[name_idx], start->name);
> +
> +	return (start->crc.end - start->crc.value) / sizeof(*start->crc.value);
> +}
> +
> +static int modversion_ext_s32_advance(struct modversion_info_ext_s32 *field)
> +{
> +	if (!field->value)
> +		return 0;
> +	if (field->value >= field->end)
> +		return -EINVAL;
> +	field->value++;
> +	return 0;
> +}
> +
> +static int modversion_ext_string_advance(struct modversion_info_ext_string *s)
> +{
> +	if (!s->value)
> +		return 0;
> +	if (s->value >= s->end)
> +		return -EINVAL;
> +	s->value += strnlen(s->value, s->end - s->value - 1) + 1;
> +	if (s->value >= s->end)
> +		return -EINVAL;
> +	return 0;
> +}
> +
> +int modversion_ext_advance(struct modversion_info_ext *start)
> +{
> +	int ret;
> +
> +	ret = modversion_ext_s32_advance(&start->crc);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = modversion_ext_string_advance(&start->name);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}

Please add all the validation as part of the ELF validation sanity checks
and make sure you rant so toolchains get easily debugged and fixed.
That would make the processing of data a secodnary step and it is
easier to read and simpler code. The validation then becomes the part
which kicks issues out early.

>  /*
>   * Generate the signature for all relevant module structures here.
>   * If these change, we don't want to try to parse the module.
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 973b5e5ae2dd..884860c2e833 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1910,15 +1910,42 @@ static void add_versions(struct buffer *b, struct module *mod)
>  			continue;
>  		}
>  		if (strlen(s->name) >= MODULE_NAME_LEN) {
> -			error("too long symbol \"%s\" [%s.ko]\n",
> -			      s->name, mod->name);
> -			break;
> +			/* this symbol will only be in the extended info */
> +			continue;

I cannot grok why this is being done, but hopefully in the next patch
series this will be easier to understand.

 Luis