[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Arm: avoid .init.data to be marked as executable

  • To: Julien Grall <julien@xxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 14 Jun 2021 14:17:33 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=X118asixY4udkMf49i5xGtdu346NNs10cwM67GSykGs=; b=YJ4das0GbZMo0wudUpXdy6+aJ741I1Z+y5f9QgngZa2a+cwjcfRBLF869LIJNZYu2c1oEJKozBdPtpKO9qHhCJVIBh8fzriPVa7nuoY83pJ5Dm75eD3EQPcnIIOKCiHdlV1Bh7ngU4E+R1Y/NrNCio5QiIv1yslDQTAtPDG78utz82zX9AVbzbqylDzRzFTIrMjIR720deMhgeVQNj9c5wfxva/jKleWM9uh7LSlmFNq5OMrR27Uf7Kns13Fe6ndjEvESiCks6fL31BvMUWX1xyqLJrIJt6b3/tWD9dsSet22mO8u+Vh7z/h0n17UwXPX86ysZnpQgmyNNborK1ZaA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Me0lQkft+IPM+pbg+BG7dkyMrWHdao8AzQ/B2bJsBJkNKfIq63ahpwCGuiTRp/E2f1lWideWr7KOsUH2D46YwX0r4kVv10m/BHK8baRVOcUZMPnX24w3c7g2cP2ALOPnc/W3q2qi6v+zMBQeByrJ6jcTpTeMBOwfrhLvbX/6aqKf7OJUeFa4Fe5mnj/5V8FmzAWM9aBjgD+m1oZLjSiiP36rWk02KZqA1XhLDjuAHhLB6BM0/9b018Nx4myGkd62RHjXgWaOiFLjhKOyAraf4q7WLlFCFfEePjh8R49Pg/IbvH5l7YaIwHNZTRhb3yZ+RhWZyaCfKDkbJZqEd6cKBA==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=suse.com;
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Mon, 14 Jun 2021 12:17:41 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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@xxxxxxxx>
>>>> ---
>>>> 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.




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.