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

On 2026-08-18 23:48, [email protected] wrote:
> 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`.

The cross-compile tests had consistent output between e.g., ARM and
other architectures. I'll take a look later.

> - [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.

The cross-compile tests had consistent output among all architectures,
thus this doesn't seem to be a problem. Assuming that table entries
might have non-sequential/non-sorted relocations would probably incur
significant design changes, which doesn't seem worth it or required at
this point.

> - [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.

Fixed.

> - [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`.

Fixed.

> --
> 
> --- 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.

-- 
Mauricio