Hi Petr
On 2026/9/7 20:17, Petr Pavlu wrote: > 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. > Right. Without the mapping it is not dereferenceable. bpftrace passes the kallsyms value to bpf_per_cpu_ptr(), which does the addition in the kernel. I would expect most tools do the same (crash, for example). > 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. > OK, I will keep it hidden for now. We can revisit it some day if a real need for the entry ever comes up. Thanks Best Regards Hao
