On Wed, May 28, 2025 at 02:35:55PM +0200, Juergen Gross wrote:

In case X86_FEATURE_PSE isn't available (e.g. when running as a Xen PV guest), execmem_arch_setup() will fall back to use PAGE_KERNEL protection for the EXECMEM_MODULE_TEXT range.

This will result in attempts to execute code with the NX bit set in case of ITS mitigation being applied.

Avoid this problem by using PAGE_KERNEL_EXEC protection instead, which will not set the NX bit.

Cc: stable@vger.kernel.org Reported-by: Xin Li xin@zytor.com Fixes: 5185e7f9f3bd ("x86/module: enable ROX caches for module text on 64 bit") Signed-off-by: Juergen Gross jgross@suse.com

---

arch/x86/mm/init.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c index 7456df985d96..f5012ae31d8b 100644 --- a/arch/x86/mm/init.c +++ b/arch/x86/mm/init.c @@ -1089,7 +1089,7 @@ struct execmem_info __init *execmem_arch_setup(void) pgprot = PAGE_KERNEL_ROX; flags = EXECMEM_KASAN_SHADOW | EXECMEM_ROX_CACHE; } else {

• pgprot = PAGE_KERNEL;
  

• pgprot = PAGE_KERNEL_EXEC;
  

Please don't. Everything except ITS can work with PAGE_KENREL so the fix should be on ITS side.

flags = EXECMEM_KASAN_SHADOW;

} -- 2.43.0

On Wed, May 28, 2025 at 08:27:19PM +0300, Mike Rapoport wrote:

On Wed, May 28, 2025 at 02:35:55PM +0200, Juergen Gross wrote:

...

In case X86_FEATURE_PSE isn't available (e.g. when running as a Xen PV guest), execmem_arch_setup() will fall back to use PAGE_KERNEL protection for the EXECMEM_MODULE_TEXT range.

This will result in attempts to execute code with the NX bit set in case of ITS mitigation being applied.

Avoid this problem by using PAGE_KERNEL_EXEC protection instead, which will not set the NX bit.

Cc: stable@vger.kernel.org Reported-by: Xin Li xin@zytor.com Fixes: 5185e7f9f3bd ("x86/module: enable ROX caches for module text on 64 bit") Signed-off-by: Juergen Gross jgross@suse.com

---

arch/x86/mm/init.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c index 7456df985d96..f5012ae31d8b 100644 --- a/arch/x86/mm/init.c +++ b/arch/x86/mm/init.c @@ -1089,7 +1089,7 @@ struct execmem_info __init *execmem_arch_setup(void) pgprot = PAGE_KERNEL_ROX; flags = EXECMEM_KASAN_SHADOW | EXECMEM_ROX_CACHE; } else {

• pgprot = PAGE_KERNEL;
  

• pgprot = PAGE_KERNEL_EXEC;
  

Please don't. Everything except ITS can work with PAGE_KENREL so the fix should be on ITS side.

Well, this is early vs post make_ro again.

Does something like so work for you?

--- diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c index 7456df985d96..f5012ae31d8b 100644 --- a/arch/x86/mm/init.c +++ b/arch/x86/mm/init.c @@ -1089,7 +1089,7 @@ struct execmem_info __init *execmem_arch_setup(void) pgprot = PAGE_KERNEL_ROX; flags = EXECMEM_KASAN_SHADOW | EXECMEM_ROX_CACHE; } else { - pgprot = PAGE_KERNEL; + pgprot = PAGE_KERNEL_EXEC; flags = EXECMEM_KASAN_SHADOW; }

diff --git a/mm/execmem.c b/mm/execmem.c index 6f7a2653b280..dbe2eedea0e6 100644 --- a/mm/execmem.c +++ b/mm/execmem.c @@ -258,6 +258,7 @@ static bool execmem_cache_rox = false;

void execmem_cache_make_ro(void) { + struct execmem_range *module_text = &execmem_info->ranges[EXECMEM_MODULE_TEXT]; struct maple_tree *free_areas = &execmem_cache.free_areas; struct maple_tree *busy_areas = &execmem_cache.busy_areas; MA_STATE(mas_free, free_areas, 0, ULONG_MAX); @@ -269,6 +270,9 @@ void execmem_cache_make_ro(void)

mutex_lock(mutex);

+ if (!(module_text->flags & EXECMEM_ROX_CACHE)) + module_text->pgprot = PAGE_KERNEL; + mas_for_each(&mas_free, area, ULONG_MAX) { unsigned long pages = mas_range_len(&mas_free) >> PAGE_SHIFT; set_memory_ro(mas_free.index, pages);

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID: 1dbf30fdb5e57fb2c39f17f35f2b544d5de34397 Gitweb: https://git.kernel.org/tip/1dbf30fdb5e57fb2c39f17f35f2b544d5de34397 Author: Juergen Gross jgross@suse.com AuthorDate: Tue, 03 Jun 2025 14:14:41 +03:00 Committer: Peter Zijlstra peterz@infradead.org CommitterDate: Wed, 11 Jun 2025 11:20:51 +02:00

x86/mm/pat: don't collapse pages without PSE set

Collapsing pages to a leaf PMD or PUD should be done only if X86_FEATURE_PSE is available, which is not the case when running e.g. as a Xen PV guest.

Fixes: 41d88484c71c ("x86/mm/pat: restore large ROX pages after fragmentation") Signed-off-by: Juergen Gross jgross@suse.com Signed-off-by: Mike Rapoport (Microsoft) rppt@kernel.org Signed-off-by: Peter Zijlstra (Intel) peterz@infradead.org Cc: stable@vger.kernel.org Link: https://lore.kernel.org/r/20250528123557.12847-3-jgross@suse.com --- arch/x86/mm/pat/set_memory.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c index 46edc11..8834c76 100644 --- a/arch/x86/mm/pat/set_memory.c +++ b/arch/x86/mm/pat/set_memory.c @@ -1257,6 +1257,9 @@ static int collapse_pmd_page(pmd_t *pmd, unsigned long addr, pgprot_t pgprot; int i = 0;

+ if (!cpu_feature_enabled(X86_FEATURE_PSE)) + return 0; + addr &= PMD_MASK; pte = pte_offset_kernel(pmd, addr); first = *pte;

On Wed, May 28, 2025 at 02:35:57PM +0200, Juergen Gross wrote:

When allocating memory pages for kernel ITS thunks, make them read-only after having written the last thunk.

This will be needed when X86_FEATURE_PSE isn't available, as the thunk memory will have PAGE_KERNEL_EXEC protection, which is including the write permission.

Cc: stable@vger.kernel.org Fixes: 5185e7f9f3bd ("x86/module: enable ROX caches for module text on 64 bit") Signed-off-by: Juergen Gross jgross@suse.com

---

arch/x86/kernel/alternative.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c index ecfe7b497cad..bd974a0ac88a 100644 --- a/arch/x86/kernel/alternative.c +++ b/arch/x86/kernel/alternative.c @@ -217,6 +217,15 @@ static void *its_alloc(void) return no_free_ptr(page); } +static void its_set_kernel_ro(void *addr) +{ +#ifdef CONFIG_MODULES

• if (its_mod)

• return;
  

+#endif

• execmem_restore_rox(addr, PAGE_SIZE);

+}

static void *its_allocate_thunk(int reg) { int size = 3 + (reg / 8); @@ -234,6 +243,8 @@ static void *its_allocate_thunk(int reg) #endif if (!its_page || (its_offset + size - 1) >= PAGE_SIZE) {

• if (its_page)
  

• 	its_set_kernel_ro(its_page);
  

  its_page = its_alloc(); if (!its_page) { pr_err("ITS page allocation failed\n");

@@ -2338,6 +2349,11 @@ void __init alternative_instructions(void) apply_retpolines(__retpoline_sites, __retpoline_sites_end); apply_returns(__return_sites, __return_sites_end);

• /* Make potential last thunk page read-only. */
• if (its_page)

• its_set_kernel_ro(its_page);
  

• its_page = NULL;
• /*
  • Adjust all CALL instructions to point to func()-10, including
  • those in .altinstr_replacement.

No, this is all sorts of wrong. Execmem API should ensure this.

On Wed, May 28, 2025 at 03:19:24PM +0200, Jürgen Groß wrote:

On 28.05.25 15:10, Peter Zijlstra wrote:

...

On Wed, May 28, 2025 at 02:35:57PM +0200, Juergen Gross wrote:

...

When allocating memory pages for kernel ITS thunks, make them read-only after having written the last thunk.

This will be needed when X86_FEATURE_PSE isn't available, as the thunk memory will have PAGE_KERNEL_EXEC protection, which is including the write permission.

Cc: stable@vger.kernel.org Fixes: 5185e7f9f3bd ("x86/module: enable ROX caches for module text on 64 bit") Signed-off-by: Juergen Gross jgross@suse.com

---

arch/x86/kernel/alternative.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c index ecfe7b497cad..bd974a0ac88a 100644 --- a/arch/x86/kernel/alternative.c +++ b/arch/x86/kernel/alternative.c @@ -217,6 +217,15 @@ static void *its_alloc(void) return no_free_ptr(page); } +static void its_set_kernel_ro(void *addr) +{ +#ifdef CONFIG_MODULES

• if (its_mod)

• return;
  

+#endif

• execmem_restore_rox(addr, PAGE_SIZE);

+}

• static void *its_allocate_thunk(int reg) { int size = 3 + (reg / 8);

@@ -234,6 +243,8 @@ static void *its_allocate_thunk(int reg) #endif if (!its_page || (its_offset + size - 1) >= PAGE_SIZE) {

• if (its_page)
  

• 	its_set_kernel_ro(its_page);
  

  its_page = its_alloc(); if (!its_page) { pr_err("ITS page allocation failed\n");

@@ -2338,6 +2349,11 @@ void __init alternative_instructions(void) apply_retpolines(__retpoline_sites, __retpoline_sites_end); apply_returns(__return_sites, __return_sites_end);

• /* Make potential last thunk page read-only. */
• if (its_page)

• its_set_kernel_ro(its_page);
  

• its_page = NULL;
• /*
  • Adjust all CALL instructions to point to func()-10, including
  • those in .altinstr_replacement.

No, this is all sorts of wrong. Execmem API should ensure this.

You are aware that this patch is basically mirroring the work which is already done for modules in alternative.c?

I am having trouble parsing that -- where does alternative.c do this to modules?

On Wed, May 28, 2025 at 03:30:33PM +0200, Jürgen Groß wrote:

Have a look at its_fini_mod().

Oh, that's what you mean. But this still isn't very nice, you now have restore_rox() without make_temp_rw(), which was the intended usage pattern.

Bah, I hate how execmem works different for !PSE, Mike, you see a sane way to fix this?

Anyway, if we have to do something like this, then I would prefer it shaped something like so:

--- diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c index ecfe7b497cad..33d4d139cb50 100644 --- a/arch/x86/kernel/alternative.c +++ b/arch/x86/kernel/alternative.c @@ -111,9 +111,8 @@ static bool cfi_paranoid __ro_after_init;

#ifdef CONFIG_MITIGATION_ITS

-#ifdef CONFIG_MODULES static struct module *its_mod; -#endif +static struct its_array its_pages; static void *its_page; static unsigned int its_offset;

@@ -151,68 +150,78 @@ static void *its_init_thunk(void *thunk, int reg) return thunk + offset; }

-#ifdef CONFIG_MODULES void its_init_mod(struct module *mod) { if (!cpu_feature_enabled(X86_FEATURE_INDIRECT_THUNK_ITS)) return;

- mutex_lock(&text_mutex); - its_mod = mod; - its_page = NULL; + if (mod) { + mutex_lock(&text_mutex); + its_mod = mod; + its_page = NULL; + } }

void its_fini_mod(struct module *mod) { + struct its_array *pages = &its_pages; + if (!cpu_feature_enabled(X86_FEATURE_INDIRECT_THUNK_ITS)) return;

WARN_ON_ONCE(its_mod != mod);

- its_mod = NULL; - its_page = NULL; - mutex_unlock(&text_mutex); + if (mod) { + pages = &mod->arch.its_pages; + its_mod = NULL; + its_page = NULL; + mutex_unlock(&text_mutex); + }

- for (int i = 0; i < mod->its_num_pages; i++) { - void *page = mod->its_page_array[i]; + for (int i = 0; i < pages->num; i++) { + void *page = pages->pages[i]; execmem_restore_rox(page, PAGE_SIZE); } + + if (!mod) + kfree(pages->pages); }

void its_free_mod(struct module *mod) { + struct its_array *pages = &its_pages; + if (!cpu_feature_enabled(X86_FEATURE_INDIRECT_THUNK_ITS)) return;

- for (int i = 0; i < mod->its_num_pages; i++) { - void *page = mod->its_page_array[i]; + if (mod) + pages = &mod->arch.its_pages; + + for (int i = 0; i < pages->num; i++) { + void *page = pages->pages[i]; execmem_free(page); } - kfree(mod->its_page_array); + kfree(pages->pages); } -#endif /* CONFIG_MODULES */

static void *its_alloc(void) { - void *page __free(execmem) = execmem_alloc(EXECMEM_MODULE_TEXT, PAGE_SIZE); + struct its_array *pages = &its_pages; + void *tmp;

+ void *page __free(execmem) = execmem_alloc(EXECMEM_MODULE_TEXT, PAGE_SIZE); if (!page) return NULL;

-#ifdef CONFIG_MODULES - if (its_mod) { - void *tmp = krealloc(its_mod->its_page_array, - (its_mod->its_num_pages+1) * sizeof(void *), - GFP_KERNEL); - if (!tmp) - return NULL; + tmp = krealloc(pages->pages, (pages->num + 1) * sizeof(void *), GFP_KERNEL); + if (!tmp) + return NULL;

- its_mod->its_page_array = tmp; - its_mod->its_page_array[its_mod->its_num_pages++] = page; + pages->pages = tmp; + pages->pages[pages->num++] = page;

+ if (its_mod) execmem_make_temp_rw(page, PAGE_SIZE); - } -#endif /* CONFIG_MODULES */

return no_free_ptr(page); } @@ -2338,6 +2347,8 @@ void __init alternative_instructions(void) apply_retpolines(__retpoline_sites, __retpoline_sites_end); apply_returns(__return_sites, __return_sites_end);

+ its_fini_mod(NULL); + /* * Adjust all CALL instructions to point to func()-10, including * those in .altinstr_replacement.

On Wed, May 28, 2025 at 05:58:21PM +0200, Peter Zijlstra wrote:

On Wed, May 28, 2025 at 03:30:33PM +0200, Jürgen Groß wrote:

...

Have a look at its_fini_mod().

Oh, that's what you mean. But this still isn't very nice, you now have restore_rox() without make_temp_rw(), which was the intended usage pattern.

Bah, I hate how execmem works different for !PSE, Mike, you see a sane way to fix this?

Not really :(

But just resetting permissions in the end like you did makes perfect sense to me. It's like STRICT_MODULE_RWX, somebody has to set the pages to ROX at some point and running execmem_restore_rox() on something that was already ROX won't cost much, set_memory will bail out early.

Anyway, if we have to do something like this, then I would prefer it shaped something like so:

---

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c index ecfe7b497cad..33d4d139cb50 100644 --- a/arch/x86/kernel/alternative.c +++ b/arch/x86/kernel/alternative.c @@ -111,9 +111,8 @@ static bool cfi_paranoid __ro_after_init; #ifdef CONFIG_MITIGATION_ITS -#ifdef CONFIG_MODULES static struct module *its_mod; -#endif +static struct its_array its_pages; static void *its_page; static unsigned int its_offset; @@ -151,68 +150,78 @@ static void *its_init_thunk(void *thunk, int reg) return thunk + offset; } -#ifdef CONFIG_MODULES void its_init_mod(struct module *mod) { if (!cpu_feature_enabled(X86_FEATURE_INDIRECT_THUNK_ITS)) return;

• mutex_lock(&text_mutex);
• its_mod = mod;
• its_page = NULL;

• if (mod) {

• mutex_lock(&text_mutex);
  

• its_mod = mod;
  

• its_page = NULL;
  

• }

} void its_fini_mod(struct module *mod) {

• struct its_array *pages = &its_pages;
• if (!cpu_feature_enabled(X86_FEATURE_INDIRECT_THUNK_ITS)) return;

WARN_ON_ONCE(its_mod != mod);

• its_mod = NULL;
• its_page = NULL;
• mutex_unlock(&text_mutex);

• if (mod) {

• pages = &mod->arch.its_pages;
  

• its_mod = NULL;
  

• its_page = NULL;
  

• mutex_unlock(&text_mutex);
  

• }

• for (int i = 0; i < mod->its_num_pages; i++) {

• void *page = mod->its_page_array[i];
  

• for (int i = 0; i < pages->num; i++) {

• void *page = pages->pages[i];
  

  execmem_restore_rox(page, PAGE_SIZE); }
• if (!mod)

• kfree(pages->pages);
  

} void its_free_mod(struct module *mod) {

• struct its_array *pages = &its_pages;
• if (!cpu_feature_enabled(X86_FEATURE_INDIRECT_THUNK_ITS)) return;

• for (int i = 0; i < mod->its_num_pages; i++) {

• void *page = mod->its_page_array[i];
  

• if (mod)

• pages = &mod->arch.its_pages;
  

• for (int i = 0; i < pages->num; i++) {

• void *page = pages->pages[i];
  

  execmem_free(page); }

• kfree(mod->its_page_array);

• kfree(pages->pages);

} -#endif /* CONFIG_MODULES */ static void *its_alloc(void) {

• void *page __free(execmem) = execmem_alloc(EXECMEM_MODULE_TEXT, PAGE_SIZE);

• struct its_array *pages = &its_pages;
• void *tmp;

• void *page __free(execmem) = execmem_alloc(EXECMEM_MODULE_TEXT, PAGE_SIZE); if (!page) return NULL;

-#ifdef CONFIG_MODULES

• if (its_mod) {

• void *tmp = krealloc(its_mod->its_page_array,
  

• 		     (its_mod->its_num_pages+1) * sizeof(void *),
  

• 		     GFP_KERNEL);
  

• if (!tmp)
  

• 	return NULL;
  

• tmp = krealloc(pages->pages, (pages->num + 1) * sizeof(void *), GFP_KERNEL);
• if (!tmp)

• return NULL;
  

• its_mod->its_page_array = tmp;
  

• its_mod->its_page_array[its_mod->its_num_pages++] = page;
  

• pages->pages = tmp;
• pages->pages[pages->num++] = page;

• if (its_mod) execmem_make_temp_rw(page, PAGE_SIZE);

• }

-#endif /* CONFIG_MODULES */ return no_free_ptr(page); } @@ -2338,6 +2347,8 @@ void __init alternative_instructions(void) apply_retpolines(__retpoline_sites, __retpoline_sites_end); apply_returns(__return_sites, __return_sites_end);

• its_fini_mod(NULL);
• /*
  • Adjust all CALL instructions to point to func()-10, including
  • those in .altinstr_replacement.

On Wed, May 28, 2025 at 05:58:21PM +0200, Peter Zijlstra wrote:

On Wed, May 28, 2025 at 03:30:33PM +0200, Jürgen Groß wrote:

...

Have a look at its_fini_mod().

Oh, that's what you mean. But this still isn't very nice, you now have restore_rox() without make_temp_rw(), which was the intended usage pattern.

Bah, I hate how execmem works different for !PSE, Mike, you see a sane way to fix this?

Anyway, if we have to do something like this, then I would prefer it shaped something like so:

I expanded this a bit and here's what I've got:

https://lore.kernel.org/lkml/20250603111446.2609381-1-rppt@kernel.org/

---

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c index ecfe7b497cad..33d4d139cb50 100644 --- a/arch/x86/kernel/alternative.c +++ b/arch/x86/kernel/alternative.c @@ -111,9 +111,8 @@ static bool cfi_paranoid __ro_after_init; #ifdef CONFIG_MITIGATION_ITS -#ifdef CONFIG_MODULES static struct module *its_mod; -#endif +static struct its_array its_pages; static void *its_page; static unsigned int its_offset; @@ -151,68 +150,78 @@ static void *its_init_thunk(void *thunk, int reg) return thunk + offset; } -#ifdef CONFIG_MODULES void its_init_mod(struct module *mod) { if (!cpu_feature_enabled(X86_FEATURE_INDIRECT_THUNK_ITS)) return;

• mutex_lock(&text_mutex);
• its_mod = mod;
• its_page = NULL;

• if (mod) {

• mutex_lock(&text_mutex);
  

• its_mod = mod;
  

• its_page = NULL;
  

• }

} void its_fini_mod(struct module *mod) {

• struct its_array *pages = &its_pages;
• if (!cpu_feature_enabled(X86_FEATURE_INDIRECT_THUNK_ITS)) return;

WARN_ON_ONCE(its_mod != mod);

• its_mod = NULL;
• its_page = NULL;
• mutex_unlock(&text_mutex);

• if (mod) {

• pages = &mod->arch.its_pages;
  

• its_mod = NULL;
  

• its_page = NULL;
  

• mutex_unlock(&text_mutex);
  

• }

• for (int i = 0; i < mod->its_num_pages; i++) {

• void *page = mod->its_page_array[i];
  

• for (int i = 0; i < pages->num; i++) {

• void *page = pages->pages[i];
  

  execmem_restore_rox(page, PAGE_SIZE); }
• if (!mod)

• kfree(pages->pages);
  

} void its_free_mod(struct module *mod) {

• struct its_array *pages = &its_pages;
• if (!cpu_feature_enabled(X86_FEATURE_INDIRECT_THUNK_ITS)) return;

• for (int i = 0; i < mod->its_num_pages; i++) {

• void *page = mod->its_page_array[i];
  

• if (mod)

• pages = &mod->arch.its_pages;
  

• for (int i = 0; i < pages->num; i++) {

• void *page = pages->pages[i];
  

  execmem_free(page); }

• kfree(mod->its_page_array);

• kfree(pages->pages);

} -#endif /* CONFIG_MODULES */ static void *its_alloc(void) {

• void *page __free(execmem) = execmem_alloc(EXECMEM_MODULE_TEXT, PAGE_SIZE);

• struct its_array *pages = &its_pages;
• void *tmp;

• void *page __free(execmem) = execmem_alloc(EXECMEM_MODULE_TEXT, PAGE_SIZE); if (!page) return NULL;

-#ifdef CONFIG_MODULES

• if (its_mod) {

• void *tmp = krealloc(its_mod->its_page_array,
  

• 		     (its_mod->its_num_pages+1) * sizeof(void *),
  

• 		     GFP_KERNEL);
  

• if (!tmp)
  

• 	return NULL;
  

• tmp = krealloc(pages->pages, (pages->num + 1) * sizeof(void *), GFP_KERNEL);
• if (!tmp)

• return NULL;
  

• its_mod->its_page_array = tmp;
  

• its_mod->its_page_array[its_mod->its_num_pages++] = page;
  

• pages->pages = tmp;
• pages->pages[pages->num++] = page;

• if (its_mod) execmem_make_temp_rw(page, PAGE_SIZE);

• }

-#endif /* CONFIG_MODULES */ return no_free_ptr(page); } @@ -2338,6 +2347,8 @@ void __init alternative_instructions(void) apply_retpolines(__retpoline_sites, __retpoline_sites_end); apply_returns(__return_sites, __return_sites_end);

• its_fini_mod(NULL);
• /*
  • Adjust all CALL instructions to point to func()-10, including
  • those in .altinstr_replacement.