[PATCH v2 0/3] module: Make .static_call_sites read-only after init

[PATCH v2 0/3] module: Make .static_call_sites read-only after init

[Petr Pavlu]

Section .static_call_sites holds data structures that need to be sorted and
processed only at module load time. The section is never modified
afterwards. Make it therefore read-only after module initialization to
avoid any (non-)accidental modifications.

Changes since v1 [1]:
* Rebase the patches. The kernel now has commit 110b1e070f1d ("module:
  Don't fail module loading when setting ro_after_init section RO failed")
  which addresses a previous problem with handling ro_after_init sections.

[1] https://lore.kernel.org/linux-modules/20241223093840.29417-1-petr.pavlu@suse.com/

Petr Pavlu (3):
  module: Constify parameters of module_enforce_rwx_sections()
  module: Add a separate function to mark sections as read-only after
    init
  module: Make .static_call_sites read-only after init

 kernel/module/internal.h   |  7 ++++--
 kernel/module/main.c       | 18 +++------------
 kernel/module/strict_rwx.c | 47 ++++++++++++++++++++++++++++++++++++--
 3 files changed, 53 insertions(+), 19 deletions(-)


base-commit: 848e076317446f9c663771ddec142d7c2eb4cb43
-- 
2.43.0

[PATCH v2 1/3] module: Constify parameters of module_enforce_rwx_sections()

[Petr Pavlu]

Minor cleanup, this is a non-functional change.

Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 kernel/module/internal.h   | 5 +++--
 kernel/module/strict_rwx.c | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index d09b46ef032f..18f32e791db0 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -333,8 +333,9 @@ int module_enable_rodata_ro(const struct module *mod);
 int module_enable_rodata_ro_after_init(const struct module *mod);
 int module_enable_data_nx(const struct module *mod);
 int module_enable_text_rox(const struct module *mod);
-int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
-				char *secstrings, struct module *mod);
+int module_enforce_rwx_sections(const Elf_Ehdr *hdr, const Elf_Shdr *sechdrs,
+				const char *secstrings,
+				const struct module *mod);
 
 #ifdef CONFIG_MODULE_SIG
 int module_sig_check(struct load_info *info, int flags);
diff --git a/kernel/module/strict_rwx.c b/kernel/module/strict_rwx.c
index 74834ba15615..81278e687055 100644
--- a/kernel/module/strict_rwx.c
+++ b/kernel/module/strict_rwx.c
@@ -86,8 +86,9 @@ int module_enable_data_nx(const struct module *mod)
 	return 0;
 }
 
-int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
-				char *secstrings, struct module *mod)
+int module_enforce_rwx_sections(const Elf_Ehdr *hdr, const Elf_Shdr *sechdrs,
+				const char *secstrings,
+				const struct module *mod)
 {
 	const unsigned long shf_wx = SHF_WRITE | SHF_EXECINSTR;
 	int i;
-- 
2.43.0

[PATCH v2 2/3] module: Add a separate function to mark sections as read-only after init

[Petr Pavlu]

Move the logic to mark special sections as read-only after module
initialization into a separate function, along other related code in
strict_rwx.c. Use a table with names of such sections to make it easier to
add more.

Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 kernel/module/internal.h   |  2 ++
 kernel/module/main.c       | 18 +++---------------
 kernel/module/strict_rwx.c | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index 18f32e791db0..7cd250ad1b51 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -336,6 +336,8 @@ int module_enable_text_rox(const struct module *mod);
 int module_enforce_rwx_sections(const Elf_Ehdr *hdr, const Elf_Shdr *sechdrs,
 				const char *secstrings,
 				const struct module *mod);
+void module_mark_ro_after_init(const Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
+			       const char *secstrings);
 
 #ifdef CONFIG_MODULE_SIG
 int module_sig_check(struct load_info *info, int flags);
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 1fb9ad289a6f..e66d501d1209 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2791,7 +2791,6 @@ core_param(module_blacklist, module_blacklist, charp, 0400);
 static struct module *layout_and_allocate(struct load_info *info, int flags)
 {
 	struct module *mod;
-	unsigned int ndx;
 	int err;
 
 	/* Allow arches to frob section contents and sizes.  */
@@ -2809,22 +2808,11 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
 	info->sechdrs[info->index.pcpu].sh_flags &= ~(unsigned long)SHF_ALLOC;
 
 	/*
-	 * Mark ro_after_init section with SHF_RO_AFTER_INIT so that
-	 * layout_sections() can put it in the right place.
+	 * Mark relevant sections as SHF_RO_AFTER_INIT so layout_sections() can
+	 * put them in the right place.
 	 * Note: ro_after_init sections also have SHF_{WRITE,ALLOC} set.
 	 */
-	ndx = find_sec(info, ".data..ro_after_init");
-	if (ndx)
-		info->sechdrs[ndx].sh_flags |= SHF_RO_AFTER_INIT;
-	/*
-	 * Mark the __jump_table section as ro_after_init as well: these data
-	 * structures are never modified, with the exception of entries that
-	 * refer to code in the __init section, which are annotated as such
-	 * at module load time.
-	 */
-	ndx = find_sec(info, "__jump_table");
-	if (ndx)
-		info->sechdrs[ndx].sh_flags |= SHF_RO_AFTER_INIT;
+	module_mark_ro_after_init(info->hdr, info->sechdrs, info->secstrings);
 
 	/*
 	 * Determine total sizes, and put offsets in sh_entsize.  For now
diff --git a/kernel/module/strict_rwx.c b/kernel/module/strict_rwx.c
index 81278e687055..fa701dad4ed1 100644
--- a/kernel/module/strict_rwx.c
+++ b/kernel/module/strict_rwx.c
@@ -106,3 +106,36 @@ int module_enforce_rwx_sections(const Elf_Ehdr *hdr, const Elf_Shdr *sechdrs,
 
 	return 0;
 }
+
+static const char *const ro_after_init[] = {
+	/*
+	 * Section .data..ro_after_init holds data explicitly annotated by
+	 * __ro_after_init.
+	 */
+	".data..ro_after_init",
+
+	/*
+	 * Section __jump_table holds data structures that are never modified,
+	 * with the exception of entries that refer to code in the __init
+	 * section, which are marked as such at module load time.
+	 */
+	"__jump_table",
+};
+
+void module_mark_ro_after_init(const Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
+			       const char *secstrings)
+{
+	int i, j;
+
+	for (i = 1; i < hdr->e_shnum; i++) {
+		Elf_Shdr *shdr = &sechdrs[i];
+
+		for (j = 0; j < ARRAY_SIZE(ro_after_init); j++) {
+			if (strcmp(secstrings + shdr->sh_name,
+				   ro_after_init[j]) == 0) {
+				shdr->sh_flags |= SHF_RO_AFTER_INIT;
+				break;
+			}
+		}
+	}
+}
-- 
2.43.0

[PATCH v2 3/3] module: Make .static_call_sites read-only after init

[Petr Pavlu]

Section .static_call_sites holds data structures that need to be sorted and
processed only at module load time. This initial processing happens in
static_call_add_module(), which is invoked as a callback to the
MODULE_STATE_COMING notification from prepare_coming_module().

The section is never modified afterwards. Make it therefore read-only after
module initialization to avoid any (non-)accidental modifications.

Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 kernel/module/strict_rwx.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/kernel/module/strict_rwx.c b/kernel/module/strict_rwx.c
index fa701dad4ed1..a3fc8d603750 100644
--- a/kernel/module/strict_rwx.c
+++ b/kernel/module/strict_rwx.c
@@ -120,6 +120,15 @@ static const char *const ro_after_init[] = {
 	 * section, which are marked as such at module load time.
 	 */
 	"__jump_table",
+
+#ifdef CONFIG_HAVE_STATIC_CALL_INLINE
+	/*
+	 * Section .static_call_sites holds data structures that need to be
+	 * sorted and processed at module load time but are never modified
+	 * afterwards.
+	 */
+	".static_call_sites",
+#endif
 };
 
 void module_mark_ro_after_init(const Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
-- 
2.43.0

Re: [PATCH v2 3/3] module: Make .static_call_sites read-only after init

[Christophe Leroy]

Le 06/03/2025 à 14:13, Petr Pavlu a écrit :
> Section .static_call_sites holds data structures that need to be sorted and
> processed only at module load time. This initial processing happens in
> static_call_add_module(), which is invoked as a callback to the
> MODULE_STATE_COMING notification from prepare_coming_module().
> 
> The section is never modified afterwards. Make it therefore read-only after
> module initialization to avoid any (non-)accidental modifications.

Maybe this suggestion is stupid, I didn't investigate the feasability 
but: why don't we group everything that is ro_after_init in a single 
section just like we do in vmlinux ? That would avoid having to add 
every new possible section in the C code.

Like we have in asm-generic/vmlinux.lds.h:

#define RO_AFTER_INIT_DATA						\
	. = ALIGN(8);							\
	__start_ro_after_init = .;					\
	*(.data..ro_after_init)						\
	JUMP_TABLE_DATA							\
	STATIC_CALL_DATA						\
	__end_ro_after_init = .;


> 
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> ---
>   kernel/module/strict_rwx.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/kernel/module/strict_rwx.c b/kernel/module/strict_rwx.c
> index fa701dad4ed1..a3fc8d603750 100644
> --- a/kernel/module/strict_rwx.c
> +++ b/kernel/module/strict_rwx.c
> @@ -120,6 +120,15 @@ static const char *const ro_after_init[] = {
>   	 * section, which are marked as such at module load time.
>   	 */
>   	"__jump_table",
> +
> +#ifdef CONFIG_HAVE_STATIC_CALL_INLINE
> +	/*
> +	 * Section .static_call_sites holds data structures that need to be
> +	 * sorted and processed at module load time but are never modified
> +	 * afterwards.
> +	 */
> +	".static_call_sites",
> +#endif
>   };
>   
>   void module_mark_ro_after_init(const Elf_Ehdr *hdr, Elf_Shdr *sechdrs,

Re: [PATCH v2 3/3] module: Make .static_call_sites read-only after init

[Sami Tolvanen]

On Thu, Mar 06, 2025 at 06:28:58PM +0100, Christophe Leroy wrote:
> 
> 
> Le 06/03/2025 à 14:13, Petr Pavlu a écrit :
> > Section .static_call_sites holds data structures that need to be sorted and
> > processed only at module load time. This initial processing happens in
> > static_call_add_module(), which is invoked as a callback to the
> > MODULE_STATE_COMING notification from prepare_coming_module().
> > 
> > The section is never modified afterwards. Make it therefore read-only after
> > module initialization to avoid any (non-)accidental modifications.
> 
> Maybe this suggestion is stupid, I didn't investigate the feasability but:
> why don't we group everything that is ro_after_init in a single section just
> like we do in vmlinux ? That would avoid having to add every new possible
> section in the C code.
> 
> Like we have in asm-generic/vmlinux.lds.h:
> 
> #define RO_AFTER_INIT_DATA						\
> 	. = ALIGN(8);							\
> 	__start_ro_after_init = .;					\
> 	*(.data..ro_after_init)						\
> 	JUMP_TABLE_DATA							\
> 	STATIC_CALL_DATA						\
> 	__end_ro_after_init = .;

I like this idea. Grouping the sections in the module linker script
feels cleaner than having an array of section names in the code. To be
fair, I think this code predates v5.10, where scripts/module.lds.S was
first added.

Sami

Re: [PATCH v2 3/3] module: Make .static_call_sites read-only after init

[Petr Pavlu]

On 3/7/25 01:12, Sami Tolvanen wrote:
> On Thu, Mar 06, 2025 at 06:28:58PM +0100, Christophe Leroy wrote:
>> Le 06/03/2025 à 14:13, Petr Pavlu a écrit :
>>> Section .static_call_sites holds data structures that need to be sorted and
>>> processed only at module load time. This initial processing happens in
>>> static_call_add_module(), which is invoked as a callback to the
>>> MODULE_STATE_COMING notification from prepare_coming_module().
>>>
>>> The section is never modified afterwards. Make it therefore read-only after
>>> module initialization to avoid any (non-)accidental modifications.
>>
>> Maybe this suggestion is stupid, I didn't investigate the feasability but:
>> why don't we group everything that is ro_after_init in a single section just
>> like we do in vmlinux ? That would avoid having to add every new possible
>> section in the C code.
>>
>> Like we have in asm-generic/vmlinux.lds.h:
>>
>> #define RO_AFTER_INIT_DATA						\
>> 	. = ALIGN(8);							\
>> 	__start_ro_after_init = .;					\
>> 	*(.data..ro_after_init)						\
>> 	JUMP_TABLE_DATA							\
>> 	STATIC_CALL_DATA						\
>> 	__end_ro_after_init = .;
> 
> I like this idea. Grouping the sections in the module linker script
> feels cleaner than having an array of section names in the code. To be
> fair, I think this code predates v5.10, where scripts/module.lds.S was
> first added.

I agree in principle. I like that the information about ro_after_init
sections for vmlinux and modules would be in the same source form, in
linker scripts. This could eventually allow us to share the definition
of ro_after_init sections between vmlinux and modules.

The problem is however how to find the location of the __jump_table and
static_call_sites data. In vmlinux, as a final binary, they are
annotated with start and end symbols. In modules, as relocatable files,
the approach is to rely on them being separate sections, see function
find_module_sections().

I could add start+end symbols for __jump_table and static_call_sites
data in scripts/module.lds.S and use them by the module loader, but this
would create an inconsistency in how various data is looked up. Another
problem is that I can't find a way to tell the linker to add these
symbols only if the specific data is actually present.

-- 
Cheers,
Petr

Re: [PATCH v2 3/3] module: Make .static_call_sites read-only after init

[Sami Tolvanen]

Hi Petr,

On Wed, Mar 12, 2025 at 5:05 AM Petr Pavlu <petr.pavlu@suse.com> wrote:
>
> On 3/7/25 01:12, Sami Tolvanen wrote:
> > On Thu, Mar 06, 2025 at 06:28:58PM +0100, Christophe Leroy wrote:
> >> Le 06/03/2025 à 14:13, Petr Pavlu a écrit :
> >>> Section .static_call_sites holds data structures that need to be sorted and
> >>> processed only at module load time. This initial processing happens in
> >>> static_call_add_module(), which is invoked as a callback to the
> >>> MODULE_STATE_COMING notification from prepare_coming_module().
> >>>
> >>> The section is never modified afterwards. Make it therefore read-only after
> >>> module initialization to avoid any (non-)accidental modifications.
> >>
> >> Maybe this suggestion is stupid, I didn't investigate the feasability but:
> >> why don't we group everything that is ro_after_init in a single section just
> >> like we do in vmlinux ? That would avoid having to add every new possible
> >> section in the C code.
> >>
> >> Like we have in asm-generic/vmlinux.lds.h:
> >>
> >> #define RO_AFTER_INIT_DATA                                           \
> >>      . = ALIGN(8);                                                   \
> >>      __start_ro_after_init = .;                                      \
> >>      *(.data..ro_after_init)                                         \
> >>      JUMP_TABLE_DATA                                                 \
> >>      STATIC_CALL_DATA                                                \
> >>      __end_ro_after_init = .;
> >
> > I like this idea. Grouping the sections in the module linker script
> > feels cleaner than having an array of section names in the code. To be
> > fair, I think this code predates v5.10, where scripts/module.lds.S was
> > first added.
>
> I agree in principle. I like that the information about ro_after_init
> sections for vmlinux and modules would be in the same source form, in
> linker scripts. This could eventually allow us to share the definition
> of ro_after_init sections between vmlinux and modules.
>
> The problem is however how to find the location of the __jump_table and
> static_call_sites data. In vmlinux, as a final binary, they are
> annotated with start and end symbols. In modules, as relocatable files,
> the approach is to rely on them being separate sections, see function
> find_module_sections().
>
> I could add start+end symbols for __jump_table and static_call_sites
> data in scripts/module.lds.S and use them by the module loader, but this
> would create an inconsistency in how various data is looked up.

That's a fair point. Perhaps it makes sense to keep these sections
separate for consistency, and look into cleaning this up later if
needed.

> Another problem is that I can't find a way to tell the linker to add these
> symbols only if the specific data is actually present.

You can use the preprocessor to add the symbols only if the relevant
kernel config is present, similarly to how STATIC_CALL_DATA is defined
in include/asm-generic/vmlinux.lds.h.

In any case, the code looks correct to me. For the series:

Reviewed-by: Sami Tolvanen <samitolvanen@google.com>

Sami

Re: [PATCH v2 3/3] module: Make .static_call_sites read-only after init

[Petr Pavlu]

On 3/13/25 00:21, Sami Tolvanen wrote:
> Hi Petr,
> 
> On Wed, Mar 12, 2025 at 5:05 AM Petr Pavlu <petr.pavlu@suse.com> wrote:
>>
>> On 3/7/25 01:12, Sami Tolvanen wrote:
>>> On Thu, Mar 06, 2025 at 06:28:58PM +0100, Christophe Leroy wrote:
>>>> Le 06/03/2025 à 14:13, Petr Pavlu a écrit :
>>>>> Section .static_call_sites holds data structures that need to be sorted and
>>>>> processed only at module load time. This initial processing happens in
>>>>> static_call_add_module(), which is invoked as a callback to the
>>>>> MODULE_STATE_COMING notification from prepare_coming_module().
>>>>>
>>>>> The section is never modified afterwards. Make it therefore read-only after
>>>>> module initialization to avoid any (non-)accidental modifications.
>>>>
>>>> Maybe this suggestion is stupid, I didn't investigate the feasability but:
>>>> why don't we group everything that is ro_after_init in a single section just
>>>> like we do in vmlinux ? That would avoid having to add every new possible
>>>> section in the C code.
>>>>
>>>> Like we have in asm-generic/vmlinux.lds.h:
>>>>
>>>> #define RO_AFTER_INIT_DATA                                           \
>>>>      . = ALIGN(8);                                                   \
>>>>      __start_ro_after_init = .;                                      \
>>>>      *(.data..ro_after_init)                                         \
>>>>      JUMP_TABLE_DATA                                                 \
>>>>      STATIC_CALL_DATA                                                \
>>>>      __end_ro_after_init = .;
>>>
>>> I like this idea. Grouping the sections in the module linker script
>>> feels cleaner than having an array of section names in the code. To be
>>> fair, I think this code predates v5.10, where scripts/module.lds.S was
>>> first added.
>>
>> I agree in principle. I like that the information about ro_after_init
>> sections for vmlinux and modules would be in the same source form, in
>> linker scripts. This could eventually allow us to share the definition
>> of ro_after_init sections between vmlinux and modules.
>>
>> The problem is however how to find the location of the __jump_table and
>> static_call_sites data. In vmlinux, as a final binary, they are
>> annotated with start and end symbols. In modules, as relocatable files,
>> the approach is to rely on them being separate sections, see function
>> find_module_sections().
>>
>> I could add start+end symbols for __jump_table and static_call_sites
>> data in scripts/module.lds.S and use them by the module loader, but this
>> would create an inconsistency in how various data is looked up.
> 
> That's a fair point. Perhaps it makes sense to keep these sections
> separate for consistency, and look into cleaning this up later if
> needed.
> 
>> Another problem is that I can't find a way to tell the linker to add these
>> symbols only if the specific data is actually present.
> 
> You can use the preprocessor to add the symbols only if the relevant
> kernel config is present, similarly to how STATIC_CALL_DATA is defined
> in include/asm-generic/vmlinux.lds.h.

Right, that works but only statically. Let's say I update module.lds.S
to:

SECTIONS {
	[...]

	.data..ro_after_init {
		*(.data..ro_after_init)
		__start___jump_table = .;
		*(__jump_table)
		__end___jump_table = .;
#ifdef CONFIG_HAVE_STATIC_CALL_INLINE
		__start_static_call_sites = .;
		*(.static_call_sites)
		__end_static_call_sites = .;
#endif
	}
}

What I had in mind is that you can configure the kernel with
CONFIG_HAVE_STATIC_CALL_INLINE, but some modules might not contain any
static calls and so wouldn't have a .static_call_sites section. When
using the above script, such modules would still end up with unnecessary
symbols __start_static_call_sites and __end_static_call_sites defining
an empty range. Worse, if the module doesn't contain any ro_after_init
data, the added symbols would force the linker to create an empty
.data..ro_after_init section.

> 
> In any case, the code looks correct to me. For the series:
> 
> Reviewed-by: Sami Tolvanen <samitolvanen@google.com>

-- 
Thanks,
Petr

Re: [PATCH v2 0/3] module: Make .static_call_sites read-only after init

[Luis Chamberlain]

On Thu, Mar 06, 2025 at 02:13:51PM +0100, Petr Pavlu wrote:
> Section .static_call_sites holds data structures that need to be sorted and
> processed only at module load time. The section is never modified
> afterwards. Make it therefore read-only after module initialization to
> avoid any (non-)accidental modifications.
> 
> Changes since v1 [1]:
> * Rebase the patches. The kernel now has commit 110b1e070f1d ("module:
>   Don't fail module loading when setting ro_after_init section RO failed")
>   which addresses a previous problem with handling ro_after_init sections.
> 
> [1] https://lore.kernel.org/linux-modules/20241223093840.29417-1-petr.pavlu@suse.com/

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [PATCH v2 0/3] module: Make .static_call_sites read-only after init

[Petr Pavlu]

On 3/6/25 14:13, Petr Pavlu wrote:
> Section .static_call_sites holds data structures that need to be sorted and
> processed only at module load time. The section is never modified
> afterwards. Make it therefore read-only after module initialization to
> avoid any (non-)accidental modifications.
> 
> Changes since v1 [1]:
> * Rebase the patches. The kernel now has commit 110b1e070f1d ("module:
>   Don't fail module loading when setting ro_after_init section RO failed")
>   which addresses a previous problem with handling ro_after_init sections.
> 
> [1] https://lore.kernel.org/linux-modules/20241223093840.29417-1-petr.pavlu@suse.com/
> 
> Petr Pavlu (3):
>   module: Constify parameters of module_enforce_rwx_sections()
>   module: Add a separate function to mark sections as read-only after
>     init
>   module: Make .static_call_sites read-only after init
> 
>  kernel/module/internal.h   |  7 ++++--
>  kernel/module/main.c       | 18 +++------------
>  kernel/module/strict_rwx.c | 47 ++++++++++++++++++++++++++++++++++++--
>  3 files changed, 53 insertions(+), 19 deletions(-)

Queued now on modules-next, for 6.16-rc1.

-- Petr