| Message ID | 25f1b0d2-9270-ba42-d110-2bf14e45b7b8@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Arm32: avoid .rodata to be marked as executable | expand |
Hi, On 11/06/2021 11:19, Jan Beulich wrote: > This confuses disassemblers, at the very least. When this data still > lived in .init.*, this probably didn't matter much, albeit the > "#execinstr" would have been suspicious to me already then. But the > latest with their movement to .rodata these attributes should have been > dropped. I don't quite understand why this wasn't really a problem for .init.data but it is a problem for .rodata. Can you expand your thought? > > Fixes: 9cbe093b7b84 ("xen/arm: link: Link proc_info_list in .rodata instead of .init.data") I don't view this commit as the buggy one. I would prefer if there is no Fixes tag. But if you want one, then it should be the patch that introduced #execinstr. > Signed-off-by: Jan Beulich <jbeulich@suse.com> The change below looks good to me. But I don't understand the commit message, so I will wait an answer before acking it. > --- > The PRINT() macro in head.S is another source of such confusion of code > vs data. While in head.o there are mapping symbols guiding disassemblers, > these mapping symbols are gone when looking at xen-syms. But I realize > adr's reach is too limited to allow for a halfway reasonable approach of > moving those strings (e.g. to, at least, have them all together). I have tried it in the past. The solution I have was to differentiate call with MMU on from the one with MMU off. But I considered this wasn't worth the trouble. > > --- a/xen/arch/arm/arm32/proc-v7.S > +++ b/xen/arch/arm/arm32/proc-v7.S > @@ -29,7 +29,7 @@ brahma15mp_init: > mcr CP32(r0, ACTLR) > mov pc, lr > > - .section ".proc.info", #alloc, #execinstr > + .section ".proc.info", #alloc > .type __v7_ca15mp_proc_info, #object > __v7_ca15mp_proc_info: > .long 0x410FC0F0 /* Cortex-A15 */ > @@ -38,7 +38,7 @@ __v7_ca15mp_proc_info: > .long caxx_processor > .size __v7_ca15mp_proc_info, . - __v7_ca15mp_proc_info > > - .section ".proc.info", #alloc, #execinstr > + .section ".proc.info", #alloc > .type __v7_ca7mp_proc_info, #object > __v7_ca7mp_proc_info: > .long 0x410FC070 /* Cortex-A7 */ > @@ -47,7 +47,7 @@ __v7_ca7mp_proc_info: > .long caxx_processor > .size __v7_ca7mp_proc_info, . - __v7_ca7mp_proc_info > > - .section ".proc.info", #alloc, #execinstr > + .section ".proc.info", #alloc > .type __v7_brahma15mp_proc_info, #object > __v7_brahma15mp_proc_info: > .long 0x420F00F0 /* Broadcom Brahma-B15 */ > Cheers,
On 14.06.2021 11:57, Julien Grall wrote: > On 11/06/2021 11:19, Jan Beulich wrote: >> This confuses disassemblers, at the very least. When this data still >> lived in .init.*, this probably didn't matter much, albeit the >> "#execinstr" would have been suspicious to me already then. But the >> latest with their movement to .rodata these attributes should have been >> dropped. > > I don't quite understand why this wasn't really a problem for .init.data > but it is a problem for .rodata. Can you expand your thought? I've said "probably" for a reason, and my thinking here goes along the lines of what I've said on the other patch regarding .init.*: There's perhaps not overly much reason to be picky about the attributes of .init.*, and at least on x86 there is also a case (the EFI binary) where we fold all .init.* into just .init anyway. The alternative to the present description that I see would be to go with just the 1st sentence. But I would be afraid in such a case that you would come back and tell me this is too little of a description. >> Fixes: 9cbe093b7b84 ("xen/arm: link: Link proc_info_list in .rodata instead of .init.data") > I don't view this commit as the buggy one. I would prefer if there is no > Fixes tag. But if you want one, then it should be the patch that > introduced #execinstr. I've dropped the tag. Jan
Hi Jan, On 14/06/2021 12:40, Jan Beulich wrote: > On 14.06.2021 11:57, Julien Grall wrote: >> On 11/06/2021 11:19, Jan Beulich wrote: >>> This confuses disassemblers, at the very least. When this data still >>> lived in .init.*, this probably didn't matter much, albeit the >>> "#execinstr" would have been suspicious to me already then. But the >>> latest with their movement to .rodata these attributes should have been >>> dropped. >> >> I don't quite understand why this wasn't really a problem for .init.data >> but it is a problem for .rodata. Can you expand your thought? > > I've said "probably" for a reason, and my thinking here goes along > the lines of what I've said on the other patch regarding .init.*: > There's perhaps not overly much reason to be picky about the > attributes of .init.*, and at least on x86 there is also a case > (the EFI binary) where we fold all .init.* into just .init anyway. Makese sense. Thanks for the explanation. > > The alternative to the present description that I see would be to > go with just the 1st sentence. But I would be afraid in such a > case that you would come back and tell me this is too little of a > description. How about: "xen/arm: .proc.info doesn't need to be executable The section .proc.info lives in .rodata as it doesn't contain any executable code. However, the section is still marked as executable as the consequence .rodata will also be marked executable. Xen doesn't use the ELF permissions to decide the page-table mapping permission. However, this will confuse disassemblers. #execinstr is now removed on all the pushsection dealing with .proc.info". I can update the commit message on commit. Cheers,
On 14.06.2021 12:54, Julien Grall wrote: > On 14/06/2021 12:40, Jan Beulich wrote: >> On 14.06.2021 11:57, Julien Grall wrote: >>> On 11/06/2021 11:19, Jan Beulich wrote: >>>> This confuses disassemblers, at the very least. When this data still >>>> lived in .init.*, this probably didn't matter much, albeit the >>>> "#execinstr" would have been suspicious to me already then. But the >>>> latest with their movement to .rodata these attributes should have been >>>> dropped. >>> >>> I don't quite understand why this wasn't really a problem for .init.data >>> but it is a problem for .rodata. Can you expand your thought? >> >> I've said "probably" for a reason, and my thinking here goes along >> the lines of what I've said on the other patch regarding .init.*: >> There's perhaps not overly much reason to be picky about the >> attributes of .init.*, and at least on x86 there is also a case >> (the EFI binary) where we fold all .init.* into just .init anyway. > > Makese sense. Thanks for the explanation. > >> >> The alternative to the present description that I see would be to >> go with just the 1st sentence. But I would be afraid in such a >> case that you would come back and tell me this is too little of a >> description. > > How about: > > "xen/arm: .proc.info doesn't need to be executable > > The section .proc.info lives in .rodata as it doesn't contain any > executable code. However, the section is still marked as executable as > the consequence .rodata will also be marked executable. > > Xen doesn't use the ELF permissions to decide the page-table mapping > permission. However, this will confuse disassemblers. > > #execinstr is now removed on all the pushsection dealing with .proc.info". > > I can update the commit message on commit. I'm fine with the new commit message, but I'd prefer the title to remain as is, as that aspect is what did trigger me making this change. Jan
Hi Jan, On 14/06/2021 14:02, Jan Beulich wrote: > On 14.06.2021 12:54, Julien Grall wrote: >> On 14/06/2021 12:40, Jan Beulich wrote: >>> On 14.06.2021 11:57, Julien Grall wrote: >>>> On 11/06/2021 11:19, Jan Beulich wrote: >>>>> This confuses disassemblers, at the very least. When this data still >>>>> lived in .init.*, this probably didn't matter much, albeit the >>>>> "#execinstr" would have been suspicious to me already then. But the >>>>> latest with their movement to .rodata these attributes should have been >>>>> dropped. >>>> >>>> I don't quite understand why this wasn't really a problem for .init.data >>>> but it is a problem for .rodata. Can you expand your thought? >>> >>> I've said "probably" for a reason, and my thinking here goes along >>> the lines of what I've said on the other patch regarding .init.*: >>> There's perhaps not overly much reason to be picky about the >>> attributes of .init.*, and at least on x86 there is also a case >>> (the EFI binary) where we fold all .init.* into just .init anyway. >> >> Makese sense. Thanks for the explanation. >> >>> >>> The alternative to the present description that I see would be to >>> go with just the 1st sentence. But I would be afraid in such a >>> case that you would come back and tell me this is too little of a >>> description. >> >> How about: >> >> "xen/arm: .proc.info doesn't need to be executable >> >> The section .proc.info lives in .rodata as it doesn't contain any >> executable code. However, the section is still marked as executable as >> the consequence .rodata will also be marked executable. >> >> Xen doesn't use the ELF permissions to decide the page-table mapping >> permission. However, this will confuse disassemblers. >> >> #execinstr is now removed on all the pushsection dealing with .proc.info". >> >> I can update the commit message on commit. > > I'm fine with the new commit message, but I'd prefer the title to > remain as is, as that aspect is what did trigger me making this > change. Sure. I will keep your commit title and update the commit message. Cheers,
On 14/06/2021 15:09, Julien Grall wrote: >> I'm fine with the new commit message, but I'd prefer the title to >> remain as is, as that aspect is what did trigger me making this > change. > > Sure. I will keep your commit title and update the commit message. Commited with: Acked-by: Julien Grall <jgrall@amazon.com> Cheers,
--- a/xen/arch/arm/arm32/proc-v7.S +++ b/xen/arch/arm/arm32/proc-v7.S @@ -29,7 +29,7 @@ brahma15mp_init: mcr CP32(r0, ACTLR) mov pc, lr - .section ".proc.info", #alloc, #execinstr + .section ".proc.info", #alloc .type __v7_ca15mp_proc_info, #object __v7_ca15mp_proc_info: .long 0x410FC0F0 /* Cortex-A15 */ @@ -38,7 +38,7 @@ __v7_ca15mp_proc_info: .long caxx_processor .size __v7_ca15mp_proc_info, . - __v7_ca15mp_proc_info - .section ".proc.info", #alloc, #execinstr + .section ".proc.info", #alloc .type __v7_ca7mp_proc_info, #object __v7_ca7mp_proc_info: .long 0x410FC070 /* Cortex-A7 */ @@ -47,7 +47,7 @@ __v7_ca7mp_proc_info: .long caxx_processor .size __v7_ca7mp_proc_info, . - __v7_ca7mp_proc_info - .section ".proc.info", #alloc, #execinstr + .section ".proc.info", #alloc .type __v7_brahma15mp_proc_info, #object __v7_brahma15mp_proc_info: .long 0x420F00F0 /* Broadcom Brahma-B15 */
This confuses disassemblers, at the very least. When this data still lived in .init.*, this probably didn't matter much, albeit the "#execinstr" would have been suspicious to me already then. But the latest with their movement to .rodata these attributes should have been dropped. Fixes: 9cbe093b7b84 ("xen/arm: link: Link proc_info_list in .rodata instead of .init.data") Signed-off-by: Jan Beulich <jbeulich@suse.com> --- The PRINT() macro in head.S is another source of such confusion of code vs data. While in head.o there are mapping symbols guiding disassemblers, these mapping symbols are gone when looking at xen-syms. But I realize adr's reach is too limited to allow for a halfway reasonable approach of moving those strings (e.g. to, at least, have them all together).