| Message ID | 5c173e92-f615-c95a-21a2-5c894727414d@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] Arm: avoid .init.data to be marked as executable | expand |
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" \ >
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,
--- 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" \
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.