Message-ID: <95871917-9747-40d4-8305-51bc5d75cf82@samsung.com>
Date: Tue, 10 Dec 2024 11:49:32 +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 12/4/2024 4:14 PM, Petr Pavlu wrote:
> On 11/28/24 21:23, Daniel Gomez wrote:
>> 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
> 
> I'd be careful about introducing this new state where (f)init_module()
> returns an error, yet the module actually gets loaded.

Perhaps we just need this new stage? module loaded with 
permission/security error?

> 
> The init_module() interface has one return value which can originate
> from anywhere during the load process, including from the module's own
> init function. As mentioned, this means that the userspace cannot
> distinguish why the module load failed. It would be needed to have
> a specific error code returned only for this case, or to extend the
> interface further in some way.
> 
> Another concern is consistency of the module loader interface as
> a whole. Returning an error from init_module() in this case would mean
> that only the specific caller is made aware that the module was loaded
> with some issues. A different task that then decides to check the module
> state under /sys/module would see it as normally loaded, and similarly a

Maybe we need to change this state too to indicate the problem.

> task trying to insert it again would get -EEXIST. That likely would need
> changing too.
> 
> What I'd like to understand is how reporting this as an error to the
> userspace would help in practice. I think the userspace can decide to
> report it as a warning and continue, or treat is as a hard problem and
> stop the system? I would expect that most tools/systems would opt for
> the former, but then this doesn't seem much different to me than when
> the kernel produces the warning itself. The second option, with some
> stretch, could be implemented with panic_on_warn=1.

Ideally, we would reverse the module initialization when encountering 
this error [1]. However, since it's not feasible to undo it correctly at 
this stage, reporting the error back to the caller allows them to assess 
and decide whether to accept the risk.

[1] https://lore.kernel.org/all/Zuv0nmFblHUwuT8v@bombadil.infradead.org/
> 
> Do you envision that the userspace would handle this problem differently
> and it is worth adding the complexity?

What complexity do you mean?

A module driver has ro_after_init for the purpose of protecting the 
kernel from attack [2]. If we ignore it by warning once, the caller will 
not be aware of such risk (unless the caller it's parsing the kernel log).

[2] 
https://lore.kernel.org/all/1455748879-21872-1-git-send-email-keescook@chromium.org/

Another option could be adding a kconfig to decide to report or not 
which I would strongly suggest to report by default.


> 
> As a side node, I've noticed that the module loader could mark also
> static_call sections as ro_after_init. I'll post patches for that.
> Additionally, both __jump_table and static_call sections could be
> possibly turned RO earlier, after prepare_coming_module() ->
> blocking_notifier_call_chain_robust() -> ... ->
> jump_label_add_module()/static_call_add_module().
> 

Powered by blists - more mailing lists