Re: [PATCH 3/3] x86/alternative: make kernel ITS thunks read-only

[Mike Rapoport <rppt@kernel.org>]

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.

-- 
Sincerely yours,
Mike.