> On Feb 6, 2023, at 1:45 PM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
[...]
>> @@ -67,7 +67,7 @@ static bool plt_entries_equal(const struct plt_entry *a,
>>
>> static bool in_init(const struct module *mod, void *loc)
>> {
>> - return (u64)loc - (u64)mod->init_layout.base < mod->init_layout.size;
>> + return within_module_init((unsigned long)loc, mod);
>> }
>
> Wouldn't it make sense to get rid of these indirections in arm[64]
> completely ?
Yeah, we can remove them.
>
>> struct mod_kallsyms {
>> @@ -418,12 +448,8 @@ struct module {
>> /* Startup function. */
>> int (*init)(void);
>>
>> - /* Core layout: rbtree is accessed frequently, so keep together. */
>> - struct module_layout core_layout __module_layout_align;
>> - struct module_layout init_layout;
>> -#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
>> - struct module_layout data_layout;
>> -#endif
>> + /* rbtree is accessed frequently, so keep together. */
>
> I'm confused about the rbtree comment here.
Let me remove it in v10.
>
>> + struct module_memory mem[MOD_MEM_NUM_TYPES] __module_memory_align;
[...]
>> +static void free_mod_mem(struct module *mod)
>> +{
>> + /* free the memory in the right order to avoid use-after-free */
>
> How do we end up with a UAF when the ordering is different?
IIUC, we need remove MOD_DATA at last, which hosts "mod".
>
>> + static enum mod_mem_type mod_mem_free_order[MOD_MEM_NUM_TYPES] = {
>> + /* first free init sections */
>> + MOD_INIT_TEXT,
>> + MOD_INIT_DATA,
>> + MOD_INIT_RODATA,
>> +
>> + /* then core sections, except rw data */
>> + MOD_TEXT,
>> + MOD_RODATA,
>> + MOD_RO_AFTER_INIT,
>> +
>> + /* last, rw data */
>> + MOD_DATA,
>> + };
>
> That's fragile when we ever add a new section type.
>
> static const enum mod_mem_type mod_mem_free_order[] = {
> ....
> };
>
> BUILD_BUG_ON(ARRAY_SIZE(mod_mem_free_order) != MOD_MEM_NUM_TYPES);
>
> Hmm?
Will add this in v10.
>
>>
>> static bool module_init_layout_section(const char *sname)
>> @@ -1428,6 +1506,20 @@ static void layout_sections(struct module *mod, struct load_info *info)
>> { SHF_WRITE | SHF_ALLOC, ARCH_SHF_SMALL },
>> { ARCH_SHF_SMALL | SHF_ALLOC, 0 }
>> };
>> + static int core_m_to_mem_type[] = {
>
> const?
>
>> + MOD_TEXT,
>> + MOD_RODATA,
>> + MOD_RO_AFTER_INIT,
>> + MOD_DATA,
>> + MOD_INVALID,
>
> What's the point of this MOD_INVALID here?
>
>> + };
>> + static int init_m_to_mem_type[] = {
>> + MOD_INIT_TEXT,
>> + MOD_INIT_RODATA,
>> + MOD_INVALID,
>> + MOD_INIT_DATA,
>> + MOD_INVALID,
>> + };
>> unsigned int m, i;
>>
>> for (i = 0; i < info->hdr->e_shnum; i++)
>> @@ -1435,41 +1527,30 @@ static void layout_sections(struct module *mod, struct load_info *info)
>>
>> pr_debug("Core section allocation order:\n");
>> for (m = 0; m < ARRAY_SIZE(masks); ++m) {
>> + enum mod_mem_type type = core_m_to_mem_type[m];
>
> Oh. This deals with ARRAY_SIZE(masks) being larger than the
> *_to_mem_type[] ones. A comment on the *to_mem_type arrays would be
> appreciated.
Will add this in v10.
Thanks,
Song
[...]
