Message-ID: <d6a7fbda-4938-4630-b264-a4f6611bcef3@suse.com>
Date: Fri, 3 Jan 2025 16:40:46 +0100
From: Petr Pavlu <petr.pavlu@...e.com>
To: Daniel Gomez <da.gomez@...sung.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/10/24 11:49, Daniel Gomez wrote:
> 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?

The complexity that I was referring to here is mainly the earlier
described limitation of the current init_module() interface and the
consistency of the module loader interface as a whole.

Another aspect is that a number of modules is loaded directly by the
kernel via request_module(). I'm not sure how the new error would be
handled in such cases. I suspect request_module() would be also only
able to log it as a kernel warning.

If I had to choose how to handle this corner case better (in long term),
I would rather try to avoid the error in the first place, potentially as
mentioned in my other reply by splitting set_memory_ro().

-- 
Thanks,
Petr

Powered by blists - more mailing lists