Re: [PATCH v2 1/2] module: Sanitize the undefined sh_name of SHT_NULL sections

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] module: OOB memory read via SHT_NULL section aliasing
--

commit 536b936ee0704f1857141d93c7d4113160b708c4
Author: Fang Xieyan <fangxy@xiaopeng.com>
module: Sanitize the undefined sh_name of SHT_NULL sections

This commit sets the sh_name of SHT_NULL sections to 0 in order to provide a
safe, in-bounds value for section name walkers. By giving undefined values the
empty string at index 0, valid modules remain unaffected while out-of-bounds
reads are prevented.

> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index d0e1e0bd2ad06..e36bfe4734d59 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2060,9 +2060,18 @@ static int elf_validity_cache_secstrings(struct load_info *info)
>  
>  	for (i = 0; i < info->hdr->e_shnum; i++) {
>  		shdr = &info->sechdrs[i];
> -		/* SHT_NULL means sh_name has an undefined value */
> -		if (shdr->sh_type == SHT_NULL)
> +		/*
> +		 * SHT_NULL means sh_name has an undefined value. The section
> +		 * name walkers that follow (find_any_unique_sec(),
> +		 * module_mark_ro_after_init(), ...) look the name up as
> +		 * secstrings + sh_name for every section, so give the undefined
> +		 * value a safe in-bounds meaning instead of skipping the check:
> +		 * the empty string at index 0.
> +		 */
> +		if (shdr->sh_type == SHT_NULL) {
> +			shdr->sh_name = 0;

[Severity: High]
Does this safely neutralize SHT_NULL sections?

This forcefully sets the name to index 0, assuming index 0 is always
guaranteed to be an empty string. However, earlier in
elf_validity_cache_secstrings(), the string table validation checks the last
byte but fails to ensure that secstrings[0] == '\0':

kernel/module/main.c:elf_validity_cache_secstrings() {
    ...
	if (secstrings[strhdr->sh_size - 1] != '\0') {
		pr_err("ELF Spec violation: section name table isn't null terminated\n");
		return -ENOEXEC;
	}
    ...
}

Because of this, can an attacker craft a string table starting with a critical
section name like .modinfo so that a SHT_NULL section at index i > 0 aliases
to .modinfo?

Since walkers like find_any_unique_sec() do not filter out SHT_NULL sections,
this spoofed name at index 0 could be successfully matched:

kernel/module/main.c:find_any_unique_sec() {
    ...
	for (i = 1; i < info->hdr->e_shnum; i++) {
		if (strcmp(info->secstrings + info->sechdrs[i].sh_name,
			   name) == 0) {
    ...
}

Furthermore, SHT_NULL sections are explicitly exempted from offset validation
in elf_validity_cache_sechdrs():

kernel/module/main.c:elf_validity_cache_sechdrs() {
    ...
		switch (shdr->sh_type) {
		case SHT_NULL:
		case SHT_NOBITS:
			/* No contents, offset/size don't mean anything */
			continue;
    ...
}

Could this allow functions like get_next_modinfo() to dereference the
unvalidated sh_offset of the aliased SHT_NULL section?

kernel/module/main.c:get_next_modinfo() {
    ...
	char *modinfo = (char *)info->hdr + infosec->sh_offset;
    ...
}

Could this lead to an out-of-bounds memory read and a potential kernel panic?

>  			continue;
> +		}
>  		if (shdr->sh_name >= strhdr->sh_size) {
>  			pr_err("Invalid ELF section name in module (section %u type %u)\n",
>  			       i, shdr->sh_type);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917190618.94759-1-fangxy@xiaopeng.com?part=1