linux-kernel - Re: [PATCH v4] 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: <1302e2b6-8980-e6e5-7433-6754634c98f3@csgroup.eu>
Date:   Tue, 31 Jan 2023 10:58:56 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Peter Zijlstra <peterz@...radead.org>, Song Liu <song@...nel.org>
CC:     "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@...a.com" <kernel-team@...a.com>,
        Luis Chamberlain <mcgrof@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Guenter Roeck <linux@...ck-us.net>
Subject: Re: [PATCH v4] module: replace module_layout with module_memory



Le 31/01/2023 à 10:09, Peter Zijlstra a écrit :

>> @@ -573,23 +574,33 @@ bool __is_module_percpu_address(unsigned long addr, unsigned long *can_addr);
>>   bool is_module_percpu_address(unsigned long addr);
>>   bool is_module_text_address(unsigned long addr);
>>   
>> +static inline bool within_module_mem_type(unsigned long addr,
>> +					  const struct module *mod,
>> +					  enum mod_mem_type type)
>> +{
>> +	unsigned long base, size;
>> +
>> +	base = (unsigned long)mod->mem[type].base;
>> +	size = mod->mem[type].size;
>> +
>> +	return base <= addr && addr < base + size;
> 
> Possible (as inspired by all the above is_{init,core}() etc..
> 
> 	return addr - base < size;
> 

In kernel/module/main.c we have a function called within(). Maybe that 
function could be lifted in module.h and used.

> 
> static inline bool within_module_mem_types(unsigned long addr,
> 					   const struct module *mod,
> 					   enum mod_mem_type first,
> 					   enum mod_mem_type last)
> {
> 	for (enum mod_mem_type type = first; type <= last; type++) {
> 		if (within_module_mem_type(addr, mod, type))
> 			return true;
> 	}
> 	return false;
> }

Well, ok but what garanties it will always be contiguous types ?
And you can't anymore see at first look what types it is.

I prefer it to be explicit with within_module_mem_type(TYPE1) || 
within_module_mem_type(TYPE2) || within_module_mem_type(TYPE3). By the 
way we could make the function name shorter, even within() may be a 
better name as it is used only inside module code.

Something like

	return within(addr, mod, MOD_TEXT) || within(addr, mod, MOD_DATA) ||
	       within(addr, mod, MOD_RODATA) || within(addr, mod, 
MOD_RO_AFTER_INIT);


> 
>> +
>>   static inline bool within_module_core(unsigned long addr,
>>   				      const struct module *mod)
>>   {
>> -#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
>> -	if ((unsigned long)mod->data_layout.base <= addr &&
>> -	    addr < (unsigned long)mod->data_layout.base + mod->data_layout.size)
>> -		return true;
>> -#endif
>> -	return (unsigned long)mod->core_layout.base <= addr &&
>> -	       addr < (unsigned long)mod->core_layout.base + mod->core_layout.size;
>> +	return within_module_mem_type(addr, mod, MOD_TEXT) ||
>> +		within_module_mem_type(addr, mod, MOD_DATA) ||
>> +		within_module_mem_type(addr, mod, MOD_RODATA) ||
>> +		within_module_mem_type(addr, mod, MOD_RO_AFTER_INIT);
>>   }
> 
> within_module_mem_types(addr, mod, MOD_TEXT, MOD_RO_AFTER_INIT);
> 
>>   static inline bool within_module_init(unsigned long addr,
>>   				      const struct module *mod)
>>   {
>> -	return (unsigned long)mod->init_layout.base <= addr &&
>> -	       addr < (unsigned long)mod->init_layout.base + mod->init_layout.size;
>> +	return within_module_mem_type(addr, mod, MOD_INIT_TEXT) ||
>> +		within_module_mem_type(addr, mod, MOD_INIT_DATA) ||
>> +		within_module_mem_type(addr, mod, MOD_INIT_RODATA);
>>   }
> 
> within_module_mem_types(addr, mod, MOD_INIT_TEXT, MOD_INIT_RODATA);

Christophe