Message-ID: <9734d93d-73c8-464e-8f32-6117c6f6c952@samsung.com>
Date: Thu, 28 Nov 2024 21:23:03 +0100
From: Daniel Gomez <da.gomez@...sung.com>
To: Petr Pavlu <petr.pavlu@...e.com>
CC: Christophe Leroy <christophe.leroy@...roup.eu>, Luis Chamberlain
	<mcgrof@...nel.org>, Sami Tolvanen <samitolvanen@...gle.com>, Kees Cook
	<kees@...nel.org>, <linux-modules@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [RFC PATCH 2/3] module: Don't fail module loading when setting
 ro_after_init section RO failed

On 11/12/2024 3:35 PM, Petr Pavlu wrote:
> On 11/12/24 10:43, Daniel Gomez wrote:
>> On Mon Nov 11, 2024 at 7:53 PM CET, Christophe Leroy wrote:
>>>
>>>
>>> Le 09/11/2024 à 23:17, Daniel Gomez a écrit :
>>>> On Sat Nov 9, 2024 at 11:35 AM CET, Christophe Leroy wrote:
>>>>> Once module init has succeded it is too late to cancel loading.
>>>>> If setting ro_after_init data section to read-only fails, all we
>>>>> can do is to inform the user through a warning.
>>>>>
>>>>> Reported-by: Thomas Gleixner <tglx@...utronix.de>
>>>>> Closes: https://protect2.fireeye.com/v1/url?k=d3deb284-b2a35ac3-d3df39cb-74fe485fff30-288375d7d91e4ad9&q=1&e=701066ca-634d-4525-a77d-1a44451f897a&u=https%3A%2F%2Feur01.safelinks.protection.outlook.com%2F%3Furl%3Dhttps%253A%252F%252Flore.kernel.org%252Fall%252F20230915082126.4187913-1-ruanjinjie%2540huawei.com%252F%26data%3D05%257C02%257Cchristophe.leroy%2540csgroup.eu%257C26b5ca7363e54210439b08dd010c4865%257C8b87af7d86474dc78df45f69a2011bb5%257C0%257C0%257C638667874457200373%257CUnknown%257CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%253D%253D%257C0%257C%257C%257C%26sdata%3DZeJ%252F3%252B2Nx%252FBf%252FWLFEkhxKlDhZk8LNkz0fs%252Fg2xMcOjY%253D%26reserved%3D0
>>>>> Fixes: d1909c022173 ("module: Don't ignore errors from set_memory_XX()")
>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@...roup.eu>
>>>>> ---
>>>>>    kernel/module/main.c | 6 +++---
>>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>>>>> index 2de4ad7af335..1bf4b0db291b 100644
>>>>> --- a/kernel/module/main.c
>>>>> +++ b/kernel/module/main.c
>>>>> @@ -2583,7 +2583,9 @@ static noinline int do_init_module(struct module *mod)
>>>>>    #endif
>>>>>    	ret = module_enable_rodata_ro_after_init(mod);
>>>>>    	if (ret)
>>>>> -		goto fail_mutex_unlock;
>>>>> +		pr_warn("%s: %s() returned %d, ro_after_init data might still be writable\n",
>>>>> +			mod->name, __func__, ret);
>>>>> +
>>>>>    	mod_tree_remove_init(mod);
>>>>>    	module_arch_freeing_init(mod);
>>>>>    	for_class_mod_mem_type(type, init) {
>>>>> @@ -2622,8 +2624,6 @@ static noinline int do_init_module(struct module *mod)
>>>>>    
>>>>>    	return 0;
>>>>
>>>> I think it would make sense to propagate the error. But that would
>>>> require changing modprobe.c. What kind of error can we expect when this
>>>> happens?
>>>
>>> AFAIK, on powerpc it fails with EINVAL when
>>> - The area is a vmalloc or module area and is a hugepage area
>>> - The area is not vmalloc or io register and MMU is not powerpc radix MMU
>>>
>>> Otherwise it propagates the error from apply_to_existing_page_range().
>>> IIUC it will return EINVAL when it hits a leaf PTE in upper directories.
>>
>> Looking at that path I see we can also fail at __apply_to_page_range()
>> -> apply_to_p4d_range() and return with -ENOMEM.
>>
>> My proposal was to do something like the change below in modprobe:
>>
>> diff --git a/tools/modprobe.c b/tools/modprobe.c
>> index ec66e6f..8876e27 100644
>> --- a/tools/modprobe.c
>> +++ b/tools/modprobe.c
>> @@ -572,6 +572,11 @@ static int insmod_insert(struct kmod_module *mod, int flags, const char *extra_o
>>                  err = 0;
>>          else {
>>                  switch (err) {
>> +               case -EINVAL:
>> +                       ERR("module '%s'inserted: ro_after_init data might"
>> +                           "still be writable (see dmesg)\n",
>> +                           kmod_module_get_name(mod));
>> +                       break;
>>                  case -EEXIST:
>>                          ERR("could not insert '%s': Module already in kernel\n",
>>                              kmod_module_get_name(mod));
>>
>> But I think these error codes may be also be reported in other parts
>> such as simplify_symbols() so may not be a good idea after all.
> 
> It isn't really possible to make a sensible use of the return code from
> init_module(), besides some basic check for -EEXIST. The problem is that
> any error code from a module's init function is also propagated as
> a result from the syscall.
> 
>>
>> Maybe we just need to change the default/catch all error message in
>> modprobe.c and to indicate/include this case:
>>
>> diff --git a/tools/modprobe.c b/tools/modprobe.c
>> index ec66e6f..3647d37 100644
>> --- a/tools/modprobe.c
>> +++ b/tools/modprobe.c
>> @@ -582,7 +582,8 @@ static int insmod_insert(struct kmod_module *mod, int flags, const char *extra_o
>>                              kmod_module_get_name(mod));
>>                          break;
>>                  default:
>> -                       ERR("could not insert '%s': %s\n", kmod_module_get_name(mod),
>> +                       ERR("could not insert '%s' or inserted with error %s, "
>> +                           "(see dmesg)\n", kmod_module_get_name(mod),
>>                              strerror(-err));
>>                          break;
>>                  }
>>
>>
>>>
>>> On other architectures it can be different, I know some architecture try
>>> to split the pages when they hit hugepages and that can fail.
>>
>> Is it worth it adding an error code for this case in case we want to
>> report it back?
> 
> I feel that the proposed kernel warning about this situation is
> sufficient and the loader should then return 0 to indicate that the
> module got loaded. It would be more confusing to return an error but
> with the module actually remaining inserted.
> 
> A module loaded without having its RO-after-init section changed
> properly to RO is still fully functional. In practice, if this final
> set_memory_ro() call fails, the system is already in such a state where
> the additional warning is the least of the issues?
> 

__ro_after_init is used for kernel self protection. We are loading 
"successfully" the module yes, but variables with this attribute are 
marked read-only to reduce the attack surface [1]. Since we have 
considered this stage already too late to unload the module, IMHO we 
should at least indicate that there was an error during the module 
initialization and propagate that to the loader, so it can decide the 
best action for their particular case. Warning once in the kernel log 
system, does not seem sufficient to me.

[1] Documentation/security/self-protection.rst

Powered by blists - more mailing lists