* [PATCH v2] Arm: avoid .init.data to be marked as executable
@ 2021-06-14 13:52 Jan Beulich
2021-06-14 13:54 ` Julien Grall
0 siblings, 1 reply; 3+ messages in thread
From: Jan Beulich @ 2021-06-14 13:52 UTC (permalink / raw)
To: xen-devel; +Cc: Julien Grall, Stefano Stabellini
This confuses disassemblers, at the very least. Move
.altinstr_replacement to .init.text. The previously redundant ALIGN()
now gets converted to page alignment, such that the hypervisor mapping
won't have this as executable (it'll instead get mapped r/w, which I'm
told is intended to be adjusted at some point).
Note that for the actual patching logic's purposes this part of
.init.text _has_ to live after _einittext (or before _sinittext), or
else branch_insn_requires_update() would produce wrong results.
Also, to have .altinstr_replacement have consistent attributes in the
object files, add "x" to the one instance where it was missing.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Put past _einittext.
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -148,6 +148,8 @@ SECTIONS
_sinittext = .;
*(.init.text)
_einittext = .;
+ . = ALIGN(PAGE_SIZE); /* Avoid mapping alt insns executable */
+ *(.altinstr_replacement)
} :text
. = ALIGN(PAGE_SIZE);
.init.data : {
@@ -169,8 +171,6 @@ SECTIONS
__alt_instructions = .;
*(.altinstructions)
__alt_instructions_end = .;
- . = ALIGN(4);
- *(.altinstr_replacement)
#ifdef CONFIG_DEBUG_LOCK_PROFILE
. = ALIGN(POINTER_ALIGN);
--- a/xen/include/asm-arm/alternative.h
+++ b/xen/include/asm-arm/alternative.h
@@ -67,7 +67,7 @@ int apply_alternatives(const struct alt_
ALTINSTR_ENTRY(feature,cb) \
".popsection\n" \
" .if " __stringify(cb) " == 0\n" \
- ".pushsection .altinstr_replacement, \"a\"\n" \
+ ".pushsection .altinstr_replacement, \"ax\"\n" \
"663:\n\t" \
newinstr "\n" \
"664:\n\t" \
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] Arm: avoid .init.data to be marked as executable
2021-06-14 13:52 [PATCH v2] Arm: avoid .init.data to be marked as executable Jan Beulich
@ 2021-06-14 13:54 ` Julien Grall
2021-06-14 16:54 ` Julien Grall
0 siblings, 1 reply; 3+ messages in thread
From: Julien Grall @ 2021-06-14 13:54 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Stefano Stabellini
Hi Jan,
On 14/06/2021 15:52, Jan Beulich wrote:
> This confuses disassemblers, at the very least. Move
> .altinstr_replacement to .init.text. The previously redundant ALIGN()
> now gets converted to page alignment, such that the hypervisor mapping
> won't have this as executable (it'll instead get mapped r/w, which I'm
> told is intended to be adjusted at some point).
>
> Note that for the actual patching logic's purposes this part of
> .init.text _has_ to live after _einittext (or before _sinittext), or
> else branch_insn_requires_update() would produce wrong results.
>
> Also, to have .altinstr_replacement have consistent attributes in the
> object files, add "x" to the one instance where it was missing.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Julien Grall <jgrall@amazon.com>
Cheers,
> ---
> v2: Put past _einittext.
>
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -148,6 +148,8 @@ SECTIONS
> _sinittext = .;
> *(.init.text)
> _einittext = .;
> + . = ALIGN(PAGE_SIZE); /* Avoid mapping alt insns executable */
> + *(.altinstr_replacement)
> } :text
> . = ALIGN(PAGE_SIZE);
> .init.data : {
> @@ -169,8 +171,6 @@ SECTIONS
> __alt_instructions = .;
> *(.altinstructions)
> __alt_instructions_end = .;
> - . = ALIGN(4);
> - *(.altinstr_replacement)
>
> #ifdef CONFIG_DEBUG_LOCK_PROFILE
> . = ALIGN(POINTER_ALIGN);
> --- a/xen/include/asm-arm/alternative.h
> +++ b/xen/include/asm-arm/alternative.h
> @@ -67,7 +67,7 @@ int apply_alternatives(const struct alt_
> ALTINSTR_ENTRY(feature,cb) \
> ".popsection\n" \
> " .if " __stringify(cb) " == 0\n" \
> - ".pushsection .altinstr_replacement, \"a\"\n" \
> + ".pushsection .altinstr_replacement, \"ax\"\n" \
> "663:\n\t" \
> newinstr "\n" \
> "664:\n\t" \
>
--
Julien Grall
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] Arm: avoid .init.data to be marked as executable
2021-06-14 13:54 ` Julien Grall
@ 2021-06-14 16:54 ` Julien Grall
0 siblings, 0 replies; 3+ messages in thread
From: Julien Grall @ 2021-06-14 16:54 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Stefano Stabellini
On 14/06/2021 15:54, Julien Grall wrote:
> Hi Jan,
>
> On 14/06/2021 15:52, Jan Beulich wrote:
>> This confuses disassemblers, at the very least. Move
>> .altinstr_replacement to .init.text. The previously redundant ALIGN()
>> now gets converted to page alignment, such that the hypervisor mapping
>> won't have this as executable (it'll instead get mapped r/w, which I'm
>> told is intended to be adjusted at some point).
>>
>> Note that for the actual patching logic's purposes this part of
>> .init.text _has_ to live after _einittext (or before _sinittext), or
>> else branch_insn_requires_update() would produce wrong results.
>>
>> Also, to have .altinstr_replacement have consistent attributes in the
>> object files, add "x" to the one instance where it was missing.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> Acked-by: Julien Grall <jgrall@amazon.com>
Comitted.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-06-14 16:54 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-14 13:52 [PATCH v2] Arm: avoid .init.data to be marked as executable Jan Beulich
2021-06-14 13:54 ` Julien Grall
2021-06-14 16:54 ` Julien Grall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.