On 9/7/26 8:10 AM, Hao Ge wrote: > Hi Petr > > On 2026/9/4 23:25, Petr Pavlu wrote: >> On 9/3/26 8:00 AM, Hao Ge wrote: >>> Hi Petr and Suren >>> >>> On 2026/9/2 16:33, [email protected] wrote: >>>> Thank you for your contribution! Sashiko AI review found 1 potential >>>> issue(s) to consider: >>>> - [Medium] The `.data..percpu` section is incorrectly exported in sysfs >>>> with a dangling pointer to a freed temporary buffer. >>>> -- >>>> >>>> commit 5082dc654c1b8850af405eef473b249c0d936de9 >>>> Author: Hao Ge <[email protected]> >>>> >>>> module: introduce SH_ENTSIZE_STANDALONE for separately allocated sections >>>> >>>> This commit introduces a new marker, SH_ENTSIZE_STANDALONE, to indicate >>>> sections with a separate allocation. Instead of clearing the SHF_ALLOC >>>> flag, it utilizes sh_entsize so that sections with separate allocations >>>> like the percpu section can be correctly skipped during layout. >>>> >>>> Link: >>>> https://lore.kernel.org/all/[email protected]/ >>>> >>>>> diff --git a/kernel/module/main.c b/kernel/module/main.c >>>>> index d0e1e0bd2ad06..b114b815187a9 100644 >>>>> --- a/kernel/module/main.c >>>>> +++ b/kernel/module/main.c >>>> [ ... ] >>>>> @@ -2822,7 +2810,8 @@ static int move_module(struct module *mod, struct >>>>> load_info *info) >>>>> Elf_Shdr *shdr = &info->sechdrs[i]; >>>>> const char *sname; >>>>> >>>>> - if (!(shdr->sh_flags & SHF_ALLOC)) >>>>> + if (!(shdr->sh_flags & SHF_ALLOC) >>>>> + || shdr->sh_entsize == SH_ENTSIZE_STANDALONE) >>>>> continue; >>>>> >>>>> sname = info->secstrings + shdr->sh_name; >>>> [ ... ] >>>>> @@ -2967,8 +2957,13 @@ static struct module *layout_and_allocate(struct >>>>> load_info *info, int flags) >>>>> if (err < 0) >>>>> return ERR_PTR(err); >>>>> >>>>> + /* Repurpose sh_entsize to track where each section is allocated. */ >>>>> + for (i = 0; i < info->hdr->e_shnum; i++) >>>>> + info->sechdrs[i].sh_entsize = ~0UL; >>>>> + >>>>> /* We will do a special allocation for per-cpu sections later. */ >>>>> - info->sechdrs[info->index.pcpu].sh_flags &= ~(unsigned long)SHF_ALLOC; >>>>> + if (info->index.pcpu) >>>>> + info->sechdrs[info->index.pcpu].sh_entsize = >>>>> SH_ENTSIZE_STANDALONE; >>>>> >>>>> /* >>>>> * Mark relevant sections as SHF_RO_AFTER_INIT so layout_sections() can >>>> >>>> [Severity: Medium] >>>> Because SHF_ALLOC is no longer cleared here, does this cause the >>>> .data..percpu section to be incorrectly exported via sysfs? >>>> >>>> With SHF_ALLOC remaining intact, sect_empty() in kernel/module/sysfs.c now >>>> returns false for the percpu section, causing it to be processed instead of >>>> skipped. >>>> >>>> Additionally, because move_module() explicitly skips sections marked with >>>> SH_ENTSIZE_STANDALONE, the sh_addr is never updated to the final allocated >>>> memory. >>>> >>>> This means when add_sect_attrs() exports the section: >>>> >>>> kernel/module/sysfs.c:add_sect_attrs() { >>>> ... >>>> if (sect_empty(sec)) >>>> continue; >>>> ... >>>> sattr->private = (void *)sec->sh_addr; >>>> ... >>>> } >>>> >>>> Will sattr->private end up storing an unmodified sh_addr pointing to the >>>> freed temporary load_info buffer (info->hdr)? >>>> >>>> This appears to leak a stale vmalloc pointer address to userspace when >>>> reading the sysfs file for the section. >>>> >>> Yes, Sashiko is correct. The fix is just one line after percpu_modcopy(): >>> >>> info->sechdrs[info->index.pcpu].sh_addr = (unsigned long)mod_percpu(mod); >> >> mod_percpu() (mod->percpu) isn't really the execution address of the >> .data..percpu section. That section in the ELF file is only a template >> to initialize the individual per-CPU variable areas. >> > > Yes, I used mod_percpu() to stay consistent with /proc/kallsyms.
That's a good point. /proc/kallsyms directly shows the st_value of each symbol. For per-CPU symbols in modules, that is mod->percpu+offset. I wonder whether it possible to make any use of such an address unless one also knows the per_cpu_offset() mapping. However, maybe it is really simpler not to export .data..percpu under /sys/module/*/sections/, rather than trying to determine what address it should provide. -- Thanks, Petr
