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] Arm: avoid .init.data to be marked as executable
Date: Mon, 14 Jun 2021 12:32:11 +0200 [thread overview]
Message-ID: <1b44cb6d-dda6-5297-893b-a53fe7d123d9@xen.org> (raw)
In-Reply-To: <8c5ec03f-5ea1-99f8-a521-3552d0015ac4@suse.com>
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
next prev parent reply other threads:[~2021-06-14 10:32 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2021-06-14 12:17 ` Jan Beulich
2021-06-14 12:56 ` Julien Grall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1b44cb6d-dda6-5297-893b-a53fe7d123d9@xen.org \
--to=julien@xen.org \
--cc=jbeulich@suse.com \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).