Re: [PATCH RFC v2 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 `sym->st_value` causes `modpost` to read sysctl table pointers from incorrect offsets on ARM and other architectures utilizing `SHT_REL`.
- [Medium] The sysctl table entries processing aborts iteration upon encountering a relocation offset beyond the table's bounds, invalidly assuming ELF relocations are strictly sorted.
- [Low] The string replacement loop in `do_sysctl_entry()` skips the character immediately following a '%' without bounds checking, leading to an out-of-bounds read.
- [Low] The `entry_size` field parsed from the ELF file is used as a divisor without zero verification, allowing a malformed module to trigger a division-by-zero crash in `modpost`.
--

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

modpost: handle MODULE_SYSCTL_TABLE symbols

Update file2alias.c to handle 'mod_devicetable' symbols with type 'sysctl'.
The key operation is to iterate over relocation entries and add module aliases
for each entry.

> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index 8d36c74dec2d5..797cfc2f478e2 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -1531,6 +1531,231 @@ static const struct devtable devtable[] = {

[ ... ]

> +// Looks like: sysctl:*/path/procname
> +static void do_sysctl_entry(const char *procname, const char *path,
> +			    struct module *mod)
> +{
> +	const char *src;
> +	char *dst, buf[256], *end = buf + sizeof(buf) - 1; /* -1 for NUL byte */
> +
> +	/* Replace '%s' from path template with '*' for wildcard in modprobe. */
> +	for (src = path, dst = buf; *src && dst < end; src++, dst++)
> +		*dst = (*src == '%') ? (src++, '*') : *src;

[Severity: Low]
Does this loop advance past the end of the string if the path ends exactly
with a '%' character?

If *src is '%', the inline ternary operator does a src++. Then the loop's
update step does another src++, which would skip the NUL terminator and lead
to an out-of-bounds read on the next iteration.

> +	*dst = '\0';
> +
> +	module_alias_printf(mod, false, "sysctl:*/%s/%s", buf, procname);
> +}

[ ... ]

> +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]
Is it safe to assume that relocation entries in the ELF section are strictly
sorted by r_offset?

By returning true here, the loop over relocation entries is aborted. The ELF
specification does not guarantee strict sorting by offset. If a linker emits
an out-of-order relocation entry earlier in the section but pointing past the
table, could this prematurely abort the traversal and miss valid entries?

> +
> +	/* Check for alignment with an array entry (.procname at offset zero) */
> +	if ((r_offset - sysctl_entries->table_offset) % sysctl_entries->entry_size == 0) {

[Severity: Low]
Can entry_size be zero here?

Since entry_size is parsed directly from the ELF module, a malformed module
with a size of 0 could trigger a division-by-zero crash in modpost.

> +
> +		/* The symbol for .procname points to a string */
> +		const char *procname = (const char *)
> +				       sym_get_data_addend(elf, sym, r_addend);

[ ... ]

> 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]
Does this result in adding sym->st_value twice on architectures like ARM
that use SHT_REL?

In for_each_reloc(), r_addend is calculated using addend_rel(). On ARM,
addend_rel() already returns the instruction value plus sym->st_value.

If sym->st_value is added again here (and in the manual table_offset
calculation in do_sysctl_table()), modpost will read from the wrong
offset, silently failing to extract .procname and missing the module
aliases.

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