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

From: Julien Grall <julien@xen.org>
To: Jan Beulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH] Arm32: avoid .rodata to be marked as executable
Date: Mon, 14 Jun 2021 12:54:02 +0200	[thread overview]
Message-ID: <7a57d3df-94d0-5ee6-1ceb-bf4eddec1392@xen.org> (raw)
In-Reply-To: <4143bdfd-ca78-d7ce-4ed0-2b6271c48ecf@suse.com>

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