linux-kernel - Re: [PATCH v10] module: replace module_layout with module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <70DCC6C0-53E2-4C18-B147-B2F600C66E54@fb.com>
Date:   Thu, 9 Feb 2023 00:16:46 +0000
From:   Song Liu <songliubraving@...a.com>
To:     Christophe Leroy <christophe.leroy@...roup.eu>
CC:     Song Liu <song@...nel.org>,
        "linux-modules@...r.kernel.org" <linux-modules@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "hch@....de" <hch@....de>, Kernel Team <kernel-team@...a.com>,
        Luis Chamberlain <mcgrof@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Guenter Roeck <linux@...ck-us.net>
Subject: Re: [PATCH v10] module: replace module_layout with module_memory



> On Feb 8, 2023, at 9:48 AM, Christophe Leroy <christophe.leroy@...roup.eu> wrote:

[...]

>>  {
>>   unsigned long min = (unsigned long)base;
>>   unsigned long max = min + size;
>> 
>> +#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
> 
> A #ifdef shouldn't be required. You can use IS_ENABLED() instead:
> 
> 
> 
>> + if (mod_mem_type_is_core_data(type)) {
> 
> if (IS_ENABLED(CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC) &&
>    mod_mem_type_is_core_data(type))
> 
>> + if (min < tree->data_addr_min)
>> + tree->data_addr_min = min;
>> + if (max > tree->data_addr_max)
>> + tree->data_addr_max = max;
>> + return;
>> + }
>> +#endif

To use IS_ENABLED() here, we also need to keep data_addr_[min|max] 
around. Do we really want them? 

Btw, we will think about all these details again with the type aware 
module_alloc(). 

[...]

>> 
>> +
>> +static void free_mod_mem(struct module *mod)
>> +{
>> + /* free the memory in the right order to avoid use-after-free */
> 
> Instead of 'right order', explain what the right order is.
> As far as I understand it is only to free MOD_DATA last. Everything else 
> doesn't matter.

I rewrote the function as:

static void free_mod_mem(struct module *mod)
{
        for_each_mod_mem_type(type) {
                struct module_memory *mod_mem = &mod->mem[type];

                /* free MOD_DATA at the end, as it hosts mod */
                if (type == MOD_DATA)
                        continue;

                /* Free lock-classes; relies on the preceding sync_rcu(). */
                lockdep_free_key_range(mod_mem->base, mod_mem->size);
                if (mod_mem->size)
                        module_memory_free(mod_mem->base, type);
        }

        /* free MOD_DATA at the end, as it hosts mod */
        lockdep_free_key_range(mod->mem[MOD_DATA].base, mod->mem[MOD_DATA].size);
        module_memory_free(mod->mem[MOD_DATA].base, MOD_DATA);
}

Does this look good?

Thanks,
Song

[...]