* [PATCH] Arm: avoid .init.data to be marked as executable @ 2021-06-11 9:39 Jan Beulich 2021-06-14 9:41 ` Julien Grall 0 siblings, 1 reply; 6+ messages in thread From: Jan Beulich @ 2021-06-11 9:39 UTC (permalink / raw) To: xen-devel; +Cc: Julien Grall, Stefano Stabellini This confuses disassemblers, at the very least. Move .altinstr_replacement to .init.text, dropping the redundant ALIGN(). 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> --- I'm uncertain whether having .altinstr_replacement inside or outside the [_sinittext,_einittext) region is better; I simply followed what we have on the x86 side right now. --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -147,6 +147,7 @@ SECTIONS .init.text : { _sinittext = .; *(.init.text) + *(.altinstr_replacement) _einittext = .; } :text . = ALIGN(PAGE_SIZE); @@ -169,8 +170,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] 6+ messages in thread
* Re: [PATCH] Arm: avoid .init.data to be marked as executable 2021-06-11 9:39 [PATCH] Arm: avoid .init.data to be marked as executable Jan Beulich @ 2021-06-14 9:41 ` Julien Grall 2021-06-14 10:02 ` Jan Beulich 0 siblings, 1 reply; 6+ messages in thread From: Julien Grall @ 2021-06-14 9:41 UTC (permalink / raw) To: Jan Beulich, xen-devel; +Cc: Stefano Stabellini Hi Jan, On 11/06/2021 11:39, Jan Beulich wrote: > This confuses disassemblers, at the very least. Move > .altinstr_replacement to .init.text, The alternative code was borrowed from Linux. The code has now changed to cater very large kernel. They used to keep the .altinstr_replacement and altinstructions close to each other (albeit they were both in .init.text). I am not entirely why, but I am a bit worry to separate them. What sort of test did you do? > dropping the redundant ALIGN(). > > 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> > --- > I'm uncertain whether having .altinstr_replacement inside or outside the > [_sinittext,_einittext) region is better; I simply followed what we have > on the x86 side right now. This means the altinstructions will be marked executable in the page-table. They technically should not be executable, so I would move them outside _einittext and make sure the section is aligned to a PAGE_SIZE. Cheers, -- Julien Grall ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Arm: avoid .init.data to be marked as executable 2021-06-14 9:41 ` Julien Grall @ 2021-06-14 10:02 ` Jan Beulich 2021-06-14 10:32 ` Julien Grall 0 siblings, 1 reply; 6+ messages in thread From: Jan Beulich @ 2021-06-14 10:02 UTC (permalink / raw) To: Julien Grall; +Cc: Stefano Stabellini, xen-devel On 14.06.2021 11:41, Julien Grall wrote: > On 11/06/2021 11:39, Jan Beulich wrote: >> This confuses disassemblers, at the very least. Move >> .altinstr_replacement to .init.text, > > The alternative code was borrowed from Linux. The code has now changed > to cater very large kernel. They used to keep the .altinstr_replacement > and altinstructions close to each other (albeit they were both in > .init.text). > > I am not entirely why, but I am a bit worry to separate them. What sort > of test did you do? Well, just build tests, on the assumption that relocation overflows would be reported by the linker if the sections ended up too far apart. >> dropping the redundant ALIGN(). >> >> 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> >> --- >> I'm uncertain whether having .altinstr_replacement inside or outside the >> [_sinittext,_einittext) region is better; I simply followed what we have >> on the x86 side right now. > > This means the altinstructions will be marked executable in the > page-table. They technically should not be executable, so I would move > them outside _einittext and make sure the section is aligned to a PAGE_SIZE. Hmm, are you saying you bother getting attributes right for .init.* in the page tables? I ask because we don't on x86, and because it would seem wasteful to me to pad to PAGE_SIZE just for this. But you're the maintainer, i.e. I'm merely double checking ... Jan ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Arm: avoid .init.data to be marked as executable 2021-06-14 10:02 ` Jan Beulich @ 2021-06-14 10:32 ` Julien Grall 2021-06-14 12:17 ` Jan Beulich 0 siblings, 1 reply; 6+ messages in thread From: Julien Grall @ 2021-06-14 10:32 UTC (permalink / raw) To: Jan Beulich; +Cc: Stefano Stabellini, xen-devel On 14/06/2021 12:02, Jan Beulich wrote: > On 14.06.2021 11:41, Julien Grall wrote: >> On 11/06/2021 11:39, Jan Beulich wrote: >>> This confuses disassemblers, at the very least. Move >>> .altinstr_replacement to .init.text, >> >> The alternative code was borrowed from Linux. The code has now changed >> to cater very large kernel. They used to keep the .altinstr_replacement >> and altinstructions close to each other (albeit they were both in >> .init.text). >> >> I am not entirely why, but I am a bit worry to separate them. What sort >> of test did you do? > > Well, just build tests, on the assumption that relocation overflows > would be reported by the linker if the sections ended up too far > apart. Hmmm, fair point. They should also not be further than the original instruction. So there ought to be fine. > >>> dropping the redundant ALIGN(). >>> >>> 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> >>> --- >>> I'm uncertain whether having .altinstr_replacement inside or outside the >>> [_sinittext,_einittext) region is better; I simply followed what we have >>> on the x86 side right now. >> >> This means the altinstructions will be marked executable in the >> page-table. They technically should not be executable, so I would move >> them outside _einittext and make sure the section is aligned to a PAGE_SIZE. > > Hmm, are you saying you bother getting attributes right for .init.* > in the page tables? I ask because we don't on x86, and because it > would seem wasteful to me to pad to PAGE_SIZE just for this. But > you're the maintainer, i.e. I'm merely double checking ... So this is a defense in depth. Your assumption is .init.text is going to disappear after boot. However, if there is a bug that would leave .init.text present then this may add more attack surface. So I think it is a good practice to keep the permission correct. However... looking the alternative code again, there is another reason to move this change out of the range _sinitext - _einittext. The function branch_insn_requires_update() will forbid branch target in another alternative instructions. This is first checking that the target is part of an active text. With this change, this will return true because alternative instruction replacement will be between _sinittext and _einittext. So .altinstructions_replacement must outside of the region [_stinittext, _einittext[. Cheers, -- Julien Grall ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Arm: avoid .init.data to be marked as executable 2021-06-14 10:32 ` Julien Grall @ 2021-06-14 12:17 ` Jan Beulich 2021-06-14 12:56 ` Julien Grall 0 siblings, 1 reply; 6+ messages in thread From: Jan Beulich @ 2021-06-14 12:17 UTC (permalink / raw) To: Julien Grall; +Cc: Stefano Stabellini, xen-devel On 14.06.2021 12:32, Julien Grall wrote: > > > On 14/06/2021 12:02, Jan Beulich wrote: >> On 14.06.2021 11:41, Julien Grall wrote: >>> On 11/06/2021 11:39, Jan Beulich wrote: >>>> This confuses disassemblers, at the very least. Move >>>> .altinstr_replacement to .init.text, >>> >>> The alternative code was borrowed from Linux. The code has now changed >>> to cater very large kernel. They used to keep the .altinstr_replacement >>> and altinstructions close to each other (albeit they were both in >>> .init.text). >>> >>> I am not entirely why, but I am a bit worry to separate them. What sort >>> of test did you do? >> >> Well, just build tests, on the assumption that relocation overflows >> would be reported by the linker if the sections ended up too far >> apart. > > Hmmm, fair point. They should also not be further than the original > instruction. So there ought to be fine. > >> >>>> dropping the redundant ALIGN(). >>>> >>>> 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> >>>> --- >>>> I'm uncertain whether having .altinstr_replacement inside or outside the >>>> [_sinittext,_einittext) region is better; I simply followed what we have >>>> on the x86 side right now. >>> >>> This means the altinstructions will be marked executable in the >>> page-table. They technically should not be executable, so I would move >>> them outside _einittext and make sure the section is aligned to a PAGE_SIZE. >> >> Hmm, are you saying you bother getting attributes right for .init.* >> in the page tables? I ask because we don't on x86, and because it >> would seem wasteful to me to pad to PAGE_SIZE just for this. But >> you're the maintainer, i.e. I'm merely double checking ... > > So this is a defense in depth. Your assumption is .init.text is going to > disappear after boot. However, if there is a bug that would leave > .init.text present then this may add more attack surface. So I think it > is a good practice to keep the permission correct. > > However... looking the alternative code again, there is another reason > to move this change out of the range _sinitext - _einittext. The > function branch_insn_requires_update() will forbid branch target in > another alternative instructions. > > This is first checking that the target is part of an active text. With > this change, this will return true because alternative instruction > replacement will be between _sinittext and _einittext. > > So .altinstructions_replacement must outside of the region [_stinittext, > _einittext[. I see. But I'm not sure about the defense-in-depth aspect: By putting it outside [_sinittext,_einittext) it'll get mapped r/w, while I think you were implying that it would become r/o. Not even .init.rodata gets mapped r/o. As a result I'm not convinced yet that you really want me to make the change. Otoh your arguments will make me put together an x86-side change moving this section past _einittext. Jan ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Arm: avoid .init.data to be marked as executable 2021-06-14 12:17 ` Jan Beulich @ 2021-06-14 12:56 ` Julien Grall 0 siblings, 0 replies; 6+ messages in thread From: Julien Grall @ 2021-06-14 12:56 UTC (permalink / raw) To: Jan Beulich; +Cc: Stefano Stabellini, xen-devel Hi Jan, On 14/06/2021 14:17, Jan Beulich wrote: > On 14.06.2021 12:32, Julien Grall wrote: >> >> >> On 14/06/2021 12:02, Jan Beulich wrote: >>> On 14.06.2021 11:41, Julien Grall wrote: >>>> On 11/06/2021 11:39, Jan Beulich wrote: >>>>> This confuses disassemblers, at the very least. Move >>>>> .altinstr_replacement to .init.text, >>>> >>>> The alternative code was borrowed from Linux. The code has now changed >>>> to cater very large kernel. They used to keep the .altinstr_replacement >>>> and altinstructions close to each other (albeit they were both in >>>> .init.text). >>>> >>>> I am not entirely why, but I am a bit worry to separate them. What sort >>>> of test did you do? >>> >>> Well, just build tests, on the assumption that relocation overflows >>> would be reported by the linker if the sections ended up too far >>> apart. >> >> Hmmm, fair point. They should also not be further than the original >> instruction. So there ought to be fine. >> >>> >>>>> dropping the redundant ALIGN(). >>>>> >>>>> 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> >>>>> --- >>>>> I'm uncertain whether having .altinstr_replacement inside or outside the >>>>> [_sinittext,_einittext) region is better; I simply followed what we have >>>>> on the x86 side right now. >>>> >>>> This means the altinstructions will be marked executable in the >>>> page-table. They technically should not be executable, so I would move >>>> them outside _einittext and make sure the section is aligned to a PAGE_SIZE. >>> >>> Hmm, are you saying you bother getting attributes right for .init.* >>> in the page tables? I ask because we don't on x86, and because it >>> would seem wasteful to me to pad to PAGE_SIZE just for this. But >>> you're the maintainer, i.e. I'm merely double checking ... >> >> So this is a defense in depth. Your assumption is .init.text is going to >> disappear after boot. However, if there is a bug that would leave >> .init.text present then this may add more attack surface. So I think it >> is a good practice to keep the permission correct. >> >> However... looking the alternative code again, there is another reason >> to move this change out of the range _sinitext - _einittext. The >> function branch_insn_requires_update() will forbid branch target in >> another alternative instructions. >> >> This is first checking that the target is part of an active text. With >> this change, this will return true because alternative instruction >> replacement will be between _sinittext and _einittext. >> >> So .altinstructions_replacement must outside of the region [_stinittext, >> _einittext[. > > I see. But I'm not sure about the defense-in-depth aspect: By putting > it outside [_sinittext,_einittext) it'll get mapped r/w, while I think > you were implying that it would become r/o. Not even .init.rodata gets > mapped r/o. Yes it is no r/o and that should be fixed at some point. However, I feel that r/w is better than allowing execution because some the instructions can lead to a DoS if executed on platform not supporting them. But that's a matter of opinion and I think this confused the messaging here. > > As a result I'm not convinced yet that you really want me to make the > change. I wrote "must", so I am not sure what else I could say to convince you that I really want to make this change... To re-iterate, this code will break runtime check in the alternative patching code. So the .altinstruction_replacement **should** be placed after _einittext. Cheers, -- Julien Grall ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-06-14 12:57 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-11 9:39 [PATCH] Arm: avoid .init.data to be marked as executable Jan Beulich 2021-06-14 9:41 ` Julien Grall 2021-06-14 10:02 ` Jan Beulich 2021-06-14 10:32 ` Julien Grall 2021-06-14 12:17 ` Jan Beulich 2021-06-14 12:56 ` Julien Grall
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).