[PATCH] Arm32: avoid .rodata to be marked as executable

[PATCH] Arm32: avoid .rodata to be marked as executable

[Jan Beulich]

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).

--- 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 */

Re: [PATCH] Arm32: avoid .rodata to be marked as executable

[Julien Grall]

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,

-- 
Julien Grall

Re: [PATCH] Arm32: avoid .rodata to be marked as executable

[Jan Beulich]

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

Re: [PATCH] Arm32: avoid .rodata to be marked as executable

[Julien Grall]

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,

-- 
Julien Grall

Re: [PATCH] Arm32: avoid .rodata to be marked as executable

[Jan Beulich]

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

Re: [PATCH] Arm32: avoid .rodata to be marked as executable

[Julien Grall]

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,


-- 
Julien Grall

Re: [PATCH] Arm32: avoid .rodata to be marked as executable

[Julien Grall]

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,

-- 
Julien Grall