Re: [PATCH 3/3] module: Improve module __ro_after_init handling

[Petr Mladek]

On Thu 2019-06-13 20:07:24, Josh Poimboeuf wrote:
> module_enable_ro() can be called in the following scenario:
> 
>   [load livepatch module]
>     initcall
>       klp_enable_patch()
>         klp_init_patch()
>           klp_init_object()
>             klp_init_object_loaded()
>               module_enable_ro(pmod, true)
> 
> In this case, module_enable_ro()'s 'after_init' argument is true, which
> prematurely changes the module's __ro_after_init area to read-only.
> 
> If, theoretically, a registrant of the MODULE_STATE_LIVE module notifier
> tried to write to the livepatch module's __ro_after_init section, an
> oops would occur.
> 
> Remove the 'after_init' argument and instead make __module_enable_ro()
> smart enough to only frob the __ro_after_init section after the module
> has gone live.
> 
> Reported-by: Petr Mladek <pmladek@xxxxxxxx>
> Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> ---
>  arch/arm64/kernel/ftrace.c |  2 +-
>  include/linux/module.h     |  4 ++--
>  kernel/livepatch/core.c    |  4 ++--
>  kernel/module.c            | 14 +++++++-------
>  4 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
> index 65a51331088e..c17d98aafc93 100644
> --- a/arch/arm64/kernel/ftrace.c
> +++ b/arch/arm64/kernel/ftrace.c
> @@ -120,7 +120,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
>  			/* point the trampoline to our ftrace entry point */
>  			module_disable_ro(mod);
>  			*mod->arch.ftrace_trampoline = trampoline;
> -			module_enable_ro(mod, true);
> +			module_enable_ro(mod);
>  
>  			/* update trampoline before patching in the branch */
>  			smp_wmb();
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 188998d3dca9..4d6922f3760e 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -844,12 +844,12 @@ extern int module_sysfs_initialized;
>  #ifdef CONFIG_STRICT_MODULE_RWX
>  extern void set_all_modules_text_rw(void);
>  extern void set_all_modules_text_ro(void);
> -extern void module_enable_ro(const struct module *mod, bool after_init);
> +extern void module_enable_ro(const struct module *mod);
>  extern void module_disable_ro(const struct module *mod);
>  #else
>  static inline void set_all_modules_text_rw(void) { }
>  static inline void set_all_modules_text_ro(void) { }
> -static inline void module_enable_ro(const struct module *mod, bool after_init) { }
> +static inline void module_enable_ro(const struct module *mod) { }
>  static inline void module_disable_ro(const struct module *mod) { }
>  #endif
>  
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index c4ce08f43bd6..f9882ffa2f44 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -724,13 +724,13 @@ static int klp_init_object_loaded(struct klp_patch *patch,
>  	module_disable_ro(patch->mod);
>  	ret = klp_write_object_relocations(patch->mod, obj);
>  	if (ret) {
> -		module_enable_ro(patch->mod, true);
> +		module_enable_ro(patch->mod);
>  		mutex_unlock(&text_mutex);
>  		return ret;
>  	}
>  
>  	arch_klp_init_object_loaded(patch, obj);
> -	module_enable_ro(patch->mod, true);
> +	module_enable_ro(patch->mod);
>  
>  	mutex_unlock(&text_mutex);
>  
> diff --git a/kernel/module.c b/kernel/module.c
> index e43a90ee2d23..fb3561e0c5b0 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1956,7 +1956,7 @@ void module_disable_ro(const struct module *mod)
>  	frob_rodata(&mod->init_layout, set_memory_rw);
>  }
>  
> -void __module_enable_ro(const struct module *mod, bool after_init)
> +static void __module_enable_ro(const struct module *mod)
>  {
>  	if (!rodata_enabled)
>  		return;
> @@ -1973,15 +1973,15 @@ void __module_enable_ro(const struct module *mod, bool after_init)
>  
>  	frob_rodata(&mod->init_layout, set_memory_ro);
>  
> -	if (after_init)
> +	if (mod->state == MODULE_STATE_LIVE)
>  		frob_ro_after_init(&mod->core_layout, set_memory_ro);

This works only now because __module_enable_ro() is called only from
three locations (klp_init_object_loaded(),  complete_formation(),
and do_init_module(). And they all are called in a well defined order
from load_module().

Only the final call in do_init_module() should touch the after_init
section.

IMHO, the most clean solutiuon would be to call frob_ro_after_init()
from extra __module_after_init_enable_ro() or so. This should be
called only from the single place.

Best Regards,
Petr