Re: [PATCH RFC v3 13/13] modpost: handle MODULE_SYSCTL_TABLE symbols

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Double addition of `st_value` when calculating relocation offsets for `SHT_REL` sections.
- [High] `get_reloc_secindex()` overly restricts relocation sections by mandating the `SHF_INFO_LINK` flag, risking build failures on older toolchains.
- [High] Unvalidated symbol indices when resolving `.path` and `.procname` pointers can lead to out-of-bounds reads and garbage alias generation.
- [Medium] `do_sysctl_entries` assumes ELF relocation entries are sorted by `r_offset`, which can prematurely abort the loop and miss sysctl aliases.
--

--- Patch [13]: [PATCH RFC v3 13/13] modpost: handle MODULE_SYSCTL_TABLE symbols ---
commit c9abe55361fd772a202db345bc7ccf9707457699
Author: Mauricio Faria de Oliveira <[email protected]>

modpost: handle MODULE_SYSCTL_TABLE symbols

This commit updates file2alias.c to handle mod_devicetable symbols with
type sysctl. It introduces helpers to iterate over relocation entries,
extracting the path and procname fields to generate sysctl module aliases.

> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index 8d36c74dec2d5..abf6ce69831e2 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
[ ... ]
> +static bool do_sysctl_entries(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Sym *sym,
> +			      Elf_Addr r_offset, Elf_Addr r_addend, void *data)
> +{
> +	struct sysctl_entries *sysctl_entries = (struct sysctl_entries *) data;
> +
> +	/* Skip until .table starts */
> +	if (r_offset < sysctl_entries->table_offset)
> +		return false;
> +
> +	/* Stop after .table ends */
> +	if (r_offset >= sysctl_entries->table_offset + sysctl_entries->table_size)
> +		return true;

[Severity: Medium]
Does this code assume relocation entries are strictly sorted by r_offset?
The ELF specification does not guarantee that relocation entries are sorted.
If a compiler emits a relocation for a different variable earlier in the
section, could this prematurely abort the loop and miss valid sysctl aliases?

> +
> +	/* Check for alignment with an array entry (.procname at offset zero) */
> +	if ((r_offset - sysctl_entries->table_offset) % sysctl_entries->entry_size == 0) {
> +
> +		/* The symbol for .procname points to a string */
> +		const char *procname = (const char *)
> +				       sym_get_data_addend(elf, sym, r_addend);

[Severity: High]
Could this read out-of-bounds memory or garbage data if the procname pointer
references an external string?

If the symbol section index is SHN_UNDEF or SHN_ABS, sym_get_data_by_offset()
might read from offset 0 (the ELF header) or an index far outside the bounds
of the section headers array. Should the symbol's section index be validated
before fetching the data?

> +
> +		if (!procname) {
> +			warn("%s [%s] found entry with NULL .procname (skip)\n",
> +			     sysctl_entries->modsymname, sysctl_entries->mod->name);
> +			return false;
> +		}
> +
> +		do_sysctl_entry(procname, sysctl_entries->path, sysctl_entries->mod);
> +	}
> +
> +	/* Continue at next entry */
> +	return false;
> +}
[ ... ]
> +static void do_sysctl_table(const char *modsymname, void *modsymval,
> +			    Elf_Sym *modsym, struct module *mod,
> +			    struct elf_info *info)
> +{
[ ... ]
> +	/* The .path symbol can be read directly. */
> +	sysctl_entries.path = (const char *)
> +			   sym_get_data_addend(info, sysctl_pointers.path_sym,
> +						sysctl_pointers.path_r_addend);

[Severity: High]
Similarly to the procname symbol above, is it possible for path_sym to be
SHN_UNDEF or SHN_ABS? Reading it without validating the section index could
cause modpost to crash or embed garbage aliases.

> +
> +	/* The .table symbol is the struct ctl_table array. */
> +	sysctl_entries.table_offset = sysctl_pointers.table_sym->st_value +
> +				   sysctl_pointers.table_r_addend;
> +
> +	/* The .entry_size value must not be zero. */
> +	if (sysctl_entries.entry_size == 0) {
> +		error("%s [%s.ko] invalid entry size (zero)\n",
> +		      modsymname, mod->name);
> +		return;
> +	}
> +
> +	/* Add module aliases for entries in the struct ctl_table array. */
> +	for_each_reloc(info, shndx, do_sysctl_entries, &sysctl_entries);
> +}
[ ... ]
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index d18a87b882a67..10eb7273cdd81 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -340,6 +340,13 @@ void *sym_get_data(const struct elf_info *info, const Elf_Sym *sym)
>  				      sym->st_value);
>  }
>  
> +void *sym_get_data_addend(const struct elf_info *info, const Elf_Sym *sym,
> +			  Elf_Addr r_addend)
> +{
> +	return sym_get_data_by_offset(info, get_secindex(info, sym),
> +				      sym->st_value + r_addend);
> +}

[Severity: High]
Will this result in a double-addition of st_value for SHT_REL sections?

In for_each_reloc(), addend_rel() is called for SHT_REL sections, which
already adds sym->st_value to the addend. Adding it again here computes
the offset as 2 * st_value + addend, which could read from the wrong offset
or cause an out-of-bounds access.

> +
>  static const char *sech_name(const struct elf_info *info, Elf_Shdr *sechdr)
>  {
>  	return sym_get_data_by_offset(info, info->secindex_strings,
[ ... ]
> diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
> index a98811552bd29..409fdfc844c4b 100644
> --- a/scripts/mod/modpost.h
> +++ b/scripts/mod/modpost.h
> @@ -186,6 +186,25 @@ static inline unsigned int get_secindex(const struct elf_info *info,
>  	return index;
>  }
>  
> +/* Find the relocation section for the section of a symbol */
> +static inline unsigned int get_reloc_secindex(const struct elf_info *info,
> +					      const Elf_Sym *sym)
> +{
> +	unsigned int sym_secindex = get_secindex(info, sym);
> +	unsigned int secindex;
> +
> +	for (secindex = 0; secindex < info->num_sections; secindex++) {
> +		Elf_Shdr *shdr = &info->sechdrs[secindex];
> +
> +		if ((shdr->sh_type == SHT_RELA || shdr->sh_type == SHT_REL) &&
> +		    (shdr->sh_flags & SHF_INFO_LINK) &&
> +		    shdr->sh_info == sym_secindex)
> +			return secindex;
> +	}

[Severity: High]
Is it safe to mandate the SHF_INFO_LINK flag for relocation sections?

Older toolchains or alternative linkers might omit this flag even when
sh_info is correctly populated. If this returns SHN_UNDEF, the build will
fail unconditionally due to the error() call in do_sysctl_table().

> +
> +	return SHN_UNDEF;
> +}
> +
>  /*
>   * If there's no name there, ignore it; likewise, ignore it if it's
>   * one of the magic symbols emitted used by current tools.
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=13