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]

Date:   Thu, 9 Feb 2023 06:37:55 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Song Liu <songliubraving@...a.com>
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



Le 09/02/2023 à 01:16, Song Liu a écrit :
> 
> 
>> 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?

It is up to you. If you think it is not worth the effort, it's fine for me.

Allthough it could probably be easily fixed by doing (untested) :

struct mod_tree_root {
#ifdef CONFIG_MODULES_TREE_LOOKUP
	struct latch_tree_root root;
#endif
#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
	unsigned long addr_min;
	unsigned long addr_max;
	unsigned long data_addr_min;
	unsigned long data_addr_max;
#else
	union {
		unsigned long addr_min;
		unsigned long data_addr_min;
	};
	union {
		unsigned long addr_max;
		unsigned long data_addr_max;
	}:
#endif
};


> 
> 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
> 
> [...]