Re: [PATCH v9] module: replace module_layout with module_memory

[Song Liu <songliubraving@meta.com>]

> On Feb 6, 2023, at 1:45 PM, Thomas Gleixner <tglx@linutronix.de> 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

[...]