[PATCH] module: Add CONFIG_MODULE_LOAD_IN_SEQUENCE option

[PATCH] module: Add CONFIG_MODULE_LOAD_IN_SEQUENCE option

[Joey Jiao]

When modprobe cmds are executed one by one, the final loaded modules
are not in fixed sequence as expected.

Add the option to make sure modules are in fixed sequence across reboot.

Signed-off-by: Joey Jiao <quic_jiangenj@quicinc.com>
---
 kernel/module/Kconfig | 11 +++++++++++
 kernel/module/main.c  |  6 ++++++
 2 files changed, 17 insertions(+)

diff --git a/kernel/module/Kconfig b/kernel/module/Kconfig
index 33a2e991f608..b45a45f31d6d 100644
--- a/kernel/module/Kconfig
+++ b/kernel/module/Kconfig
@@ -389,4 +389,15 @@ config MODULES_TREE_LOOKUP
 	def_bool y
 	depends on PERF_EVENTS || TRACING || CFI_CLANG
 
+config MODULE_LOAD_IN_SEQUENCE
+	bool "Load module in sequence"
+	default n
+	help
+	  By default, modules are loaded in random sequence depending on when modprobe
+	  is executed.
+
+	  This option allows modules to be loaded in sequence if modprobe cmds are
+	  executed one by one in sequence. This option is helpful during syzkaller fuzzing
+	  to make sure module is loaded into fixed address across device reboot.
+
 endif # MODULES
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 98fedfdb8db5..587fd84083ae 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2593,11 +2593,17 @@ static noinline int do_init_module(struct module *mod)
 	 * be cleaned up needs to sync with the queued work - ie
 	 * rcu_barrier()
 	 */
+#ifdef CONFIG_MODULE_LOAD_IN_SEQUENCE
+	llist_add(&freeinit->node, &init_free_list);
+#else
 	if (llist_add(&freeinit->node, &init_free_list))
 		schedule_work(&init_free_wq);
+#endif
 
 	mutex_unlock(&module_mutex);
+#ifdef CONFIG_MODULE_LOAD_IN_SEQUENCE
 	wake_up_all(&module_wq);
+#endif
 
 	mod_stat_add_long(text_size, &total_text_size);
 	mod_stat_add_long(total_size, &total_mod_size);
-- 
2.42.0

Re: [PATCH] module: Add CONFIG_MODULE_LOAD_IN_SEQUENCE option

[Luis Chamberlain]

On Mon, Oct 09, 2023 at 10:26:35AM +0530, Joey Jiao wrote:
> When modprobe cmds are executed one by one, the final loaded modules
> are not in fixed sequence as expected.
> 
> Add the option to make sure modules are in fixed sequence across reboot.
> 
> Signed-off-by: Joey Jiao <quic_jiangenj@quicinc.com>
> ---
>  kernel/module/Kconfig | 11 +++++++++++
>  kernel/module/main.c  |  6 ++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/kernel/module/Kconfig b/kernel/module/Kconfig
> index 33a2e991f608..b45a45f31d6d 100644
> --- a/kernel/module/Kconfig
> +++ b/kernel/module/Kconfig
> @@ -389,4 +389,15 @@ config MODULES_TREE_LOOKUP
>  	def_bool y
>  	depends on PERF_EVENTS || TRACING || CFI_CLANG
>  
> +config MODULE_LOAD_IN_SEQUENCE
> +	bool "Load module in sequence"
> +	default n
> +	help
> +	  By default, modules are loaded in random sequence depending on when modprobe
> +	  is executed.
> +
> +	  This option allows modules to be loaded in sequence if modprobe cmds are
> +	  executed one by one in sequence. This option is helpful during syzkaller fuzzing
> +	  to make sure module is loaded into fixed address across device reboot.
> +
>  endif # MODULES
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 98fedfdb8db5..587fd84083ae 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2593,11 +2593,17 @@ static noinline int do_init_module(struct module *mod)
>  	 * be cleaned up needs to sync with the queued work - ie
>  	 * rcu_barrier()
>  	 */
> +#ifdef CONFIG_MODULE_LOAD_IN_SEQUENCE
> +	llist_add(&freeinit->node, &init_free_list);
> +#else
>  	if (llist_add(&freeinit->node, &init_free_list))
>  		schedule_work(&init_free_wq);
> +#endif

How is ignoring an error ensuring ordering?

>  	mutex_unlock(&module_mutex);
> +#ifdef CONFIG_MODULE_LOAD_IN_SEQUENCE
>  	wake_up_all(&module_wq);
> +#endif

Why are you making this only now be called with this new kconfig option?

  Luis

RE: [PATCH] module: Add CONFIG_MODULE_LOAD_IN_SEQUENCE option

[Joey Jiao (QUIC)]

Hi Luis,

> How is ignoring an error ensuring ordering?
It's not ignoring an error ensuring ordering.
I think It's because `schedule_work(&init_free_wq);` changes when the free_wq will happen.
If sometimes freeing init for module A happens before loading another module B, that memory is freed. B might be loaded into that freed address.
If sometimes freeing init for module A happens after loading another module B, B might be loaded into another memory address.
The change is just to disable the schedule_work.

> Why are you making this only now be called with this new kconfig option?
This sequence loading is especially helpful for syzkaller coverage decoding.
When kaslr is disabled, address inside core kernel is fixed, so syzkaller can always get right function/line number from addr2line.
But module address keeps change across rebooting, in first booting, it might be loaded at X1, and at X2 after reboot, and at X3 after another reboot.
In this way, syzkaller just can't decode correctly for module address. And syzkaller currently uses PC and branch info for coverage guided things.

There was a discussion previously here https://groups.google.com/g/syzkaller/c/1Pnm_BjrZO8/m/WOyAKx8ZAgAJ for modprobe.

THX
Joey
-----Original Message-----
From: Luis Chamberlain <mcgrof@infradead.org> On Behalf Of Luis Chamberlain
Sent: Wednesday, October 11, 2023 6:36 AM
To: Joey Jiao (QUIC) <quic_jiangenj@quicinc.com>
Cc: linux-modules@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] module: Add CONFIG_MODULE_LOAD_IN_SEQUENCE option

On Mon, Oct 09, 2023 at 10:26:35AM +0530, Joey Jiao wrote:
> When modprobe cmds are executed one by one, the final loaded modules 
> are not in fixed sequence as expected.
> 
> Add the option to make sure modules are in fixed sequence across reboot.
> 
> Signed-off-by: Joey Jiao <quic_jiangenj@quicinc.com>
> ---
>  kernel/module/Kconfig | 11 +++++++++++  kernel/module/main.c  |  6 
> ++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/kernel/module/Kconfig b/kernel/module/Kconfig index 
> 33a2e991f608..b45a45f31d6d 100644
> --- a/kernel/module/Kconfig
> +++ b/kernel/module/Kconfig
> @@ -389,4 +389,15 @@ config MODULES_TREE_LOOKUP
>  	def_bool y
>  	depends on PERF_EVENTS || TRACING || CFI_CLANG
>  
> +config MODULE_LOAD_IN_SEQUENCE
> +	bool "Load module in sequence"
> +	default n
> +	help
> +	  By default, modules are loaded in random sequence depending on when modprobe
> +	  is executed.
> +
> +	  This option allows modules to be loaded in sequence if modprobe cmds are
> +	  executed one by one in sequence. This option is helpful during syzkaller fuzzing
> +	  to make sure module is loaded into fixed address across device reboot.
> +
>  endif # MODULES
> diff --git a/kernel/module/main.c b/kernel/module/main.c index 
> 98fedfdb8db5..587fd84083ae 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2593,11 +2593,17 @@ static noinline int do_init_module(struct module *mod)
>  	 * be cleaned up needs to sync with the queued work - ie
>  	 * rcu_barrier()
>  	 */
> +#ifdef CONFIG_MODULE_LOAD_IN_SEQUENCE
> +	llist_add(&freeinit->node, &init_free_list); #else
>  	if (llist_add(&freeinit->node, &init_free_list))
>  		schedule_work(&init_free_wq);
> +#endif

How is ignoring an error ensuring ordering?

>  	mutex_unlock(&module_mutex);
> +#ifdef CONFIG_MODULE_LOAD_IN_SEQUENCE
>  	wake_up_all(&module_wq);
> +#endif

Why are you making this only now be called with this new kconfig option?

  Luis

Re: [PATCH] module: Add CONFIG_MODULE_LOAD_IN_SEQUENCE option

[Luis Chamberlain]

Please find a good email client to reply to patches.

On Wed, Oct 11, 2023 at 01:57:58AM +0000, Joey Jiao (QUIC) wrote:
> Hi Luis,
> 
> > How is ignoring an error ensuring ordering?
> The change is just to disable the schedule_work.

That's different and can be made clearer. Try:

if (!IS_ENABLED(CONFIG_FOO))
	schedule_stuff

> > Why are you making this only now be called with this new kconfig option?
> This sequence loading is especially helpful for syzkaller coverage decoding.
> When kaslr is disabled, address inside core kernel is fixed, so syzkaller can always get right function/line number from addr2line.
> But module address keeps change across rebooting, in first booting, it might be loaded at X1, and at X2 after reboot, and at X3 after another reboot.
> In this way, syzkaller just can't decode correctly for module address. And syzkaller currently uses PC and branch info for coverage guided things.
> 
> There was a discussion previously here https://groups.google.com/g/syzkaller/c/1Pnm_BjrZO8/m/WOyAKx8ZAgAJ for modprobe.

You are missing my point, you are disabling in effect a piece of code
where it was not before.

  Luis

Re: [PATCH] module: Add CONFIG_MODULE_LOAD_IN_SEQUENCE option

[Joey Jiao]

Thanks Luis, will recheck these two points.

On Tue, Oct 10, 2023 at 07:21:13PM -0700, Luis Chamberlain wrote:
> Please find a good email client to reply to patches.
> 
> On Wed, Oct 11, 2023 at 01:57:58AM +0000, Joey Jiao (QUIC) wrote:
> > Hi Luis,
> > 
> > > How is ignoring an error ensuring ordering?
> > The change is just to disable the schedule_work.
> 
> That's different and can be made clearer. Try:
> 
> if (!IS_ENABLED(CONFIG_FOO))
> 	schedule_stuff
> 
> > > Why are you making this only now be called with this new kconfig option?
> > This sequence loading is especially helpful for syzkaller coverage decoding.
> > When kaslr is disabled, address inside core kernel is fixed, so syzkaller can always get right function/line number from addr2line.
> > But module address keeps change across rebooting, in first booting, it might be loaded at X1, and at X2 after reboot, and at X3 after another reboot.
> > In this way, syzkaller just can't decode correctly for module address. And syzkaller currently uses PC and branch info for coverage guided things.
> > 
> > There was a discussion previously here https://groups.google.com/g/syzkaller/c/1Pnm_BjrZO8/m/WOyAKx8ZAgAJ for modprobe.
> 
> You are missing my point, you are disabling in effect a piece of code
> where it was not before.
> 
>   Luis

Re: [PATCH] module: Add CONFIG_MODULE_LOAD_IN_SEQUENCE option

[Christoph Hellwig]

On Mon, Oct 09, 2023 at 10:26:35AM +0530, Joey Jiao wrote:
> When modprobe cmds are executed one by one, the final loaded modules
> are not in fixed sequence as expected.

And why does this matter?

If this is important enough to matter it should just be the default,
and have really good reason for that.  Doing something like this as
a config option does not make any sense.