Re: [PATCH] x86/alternative: Consistently patch SMP locks in vmlinux and modules

[Borislav Petkov]

On Tue, Aug 30, 2022 at 09:42:37AM +0200, Julian Pidancet wrote:
> The alternatives_smp_module_add() function restricts patching of SMP
> lock prefixes to the text address range passed as an argument.
> 
> For vmlinux, patching all the instructions located between the _text and
> _etext symbols is allowed. That includes the .text section but also
> other sections such as .text.hot and .text.unlikely.
> 
> As per the comment inside the 'struct smp_alt_module' definition, the
> original purpose of this restriction is to avoid patching the init code
> which may have been deallocated when the alternatives code run.
> 
> For modules, the current code only allows patching instructions located
> inside the .text segment, excluding other sections such as .text.hot or
> .text.unlikely, which may need patching.

Is this something you noticed by inspection or is there a real failure
behind it?

> This change aims to make patching of the kernel core and modules more

Avoid having "This patch" or "This commit" and so on, in the commit
message. It is tautologically useless.

Also, do

$ git grep 'This patch' Documentation/process

for more details.

> consistent, by allowing all text sections of modules except .init.text
> to be patched in module_finalize().
> 
> For that we use mod->core_layout.base/mod->core_layout.text_size as the

Please use passive voice in your commit message: no "we" or "I", etc,
and describe your changes in imperative mood.

Bottom line is: personal pronouns are ambiguous in text, especially with
so many parties/companies/etc developing the kernel so let's avoid them
please.

> address range allowed to be patched, which include all the code sections
> except the init code.
> 
> Signed-off-by: Julian Pidancet <julian.pidancet@xxxxxxxxxx>
> ---
> Public tests: https://gist.github.com/jpidancet/1ee457623426f3e3902a28edaf2c80d0 

Looks like you wrote a module to verify that :)

> Related thread: https://marc.info/?t=130864398400006

Aha, someone else noticed this inconsistency.

>  arch/x86/kernel/module.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
> index b1abf663417c..da22193eb5e0 100644
> --- a/arch/x86/kernel/module.c
> +++ b/arch/x86/kernel/module.c
> @@ -251,14 +251,12 @@ int module_finalize(const Elf_Ehdr *hdr,
>  		    const Elf_Shdr *sechdrs,
>  		    struct module *me)
>  {
> -	const Elf_Shdr *s, *text = NULL, *alt = NULL, *locks = NULL,
> -		*para = NULL, *orc = NULL, *orc_ip = NULL,
> -		*retpolines = NULL, *returns = NULL, *ibt_endbr = NULL;
> +	const Elf_Shdr *s, *alt = NULL, *locks = NULL, *para = NULL,
> +		*orc = NULL, *orc_ip = NULL, *retpolines = NULL,
> +		*returns = NULL, *ibt_endbr = NULL;
>  	char *secstrings = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
>  
>  	for (s = sechdrs; s < sechdrs + hdr->e_shnum; s++) {
> -		if (!strcmp(".text", secstrings + s->sh_name))
> -			text = s;
>  		if (!strcmp(".altinstructions", secstrings + s->sh_name))
>  			alt = s;
>  		if (!strcmp(".smp_locks", secstrings + s->sh_name))
> @@ -302,12 +300,13 @@ int module_finalize(const Elf_Ehdr *hdr,
>  		void *iseg = (void *)ibt_endbr->sh_addr;
>  		apply_ibt_endbr(iseg, iseg + ibt_endbr->sh_size);
>  	}
> -	if (locks && text) {
> +	if (locks) {
>  		void *lseg = (void *)locks->sh_addr;
> -		void *tseg = (void *)text->sh_addr;
> +		void *text = me->core_layout.base;
> +		void *text_end = text + me->core_layout.text_size;
>  		alternatives_smp_module_add(me, me->name,
>  					    lseg, lseg + locks->sh_size,
> -					    tseg, tseg + text->sh_size);
> +					    text, text_end);
>  	}
>  
>  	if (orc && orc_ip)
> -- 

I don't see anything wrong with doing that on a quick glance...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette