From: Julien Grall <julien@xen.org>
To: Bertrand Marquis <Bertrand.Marquis@arm.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
"security@xenproject.org" <security@xenproject.org>,
Paul Durrant <paul@xen.org>,
Xen-devel <xen-devel@lists.xenproject.org>,
Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
Andre Przywara <Andre.Przywara@arm.com>,
Julien Grall <jgrall@amazon.com>, nd <nd@arm.com>
Subject: Re: [PATCH 1/2] xen/arm: entry: Place a speculation barrier following an ret instruction
Date: Tue, 18 Aug 2020 18:34:02 +0100 [thread overview]
Message-ID: <5dceeedf-9982-37c5-553e-76f22d9d6db2@xen.org> (raw)
In-Reply-To: <7EAB4E0A-338C-4DCF-80A4-A426BC95C051@arm.com>
Hi Bertrand,
On 18/08/2020 18:06, Bertrand Marquis wrote:
>
>
>> On 18 Aug 2020, at 17:43, Julien Grall <julien@xen.org> wrote:
>>
>>
>>
>> On 18/08/2020 17:35, Bertrand Marquis wrote:
>>> Hi Julien,
>>
>> Hi Bertrand,
>>
>>> Somehow we stopped on this thread and you did already most of the work so I think we should try to finish what you started
>>
>> Sorry this fell-through the cracks. I have a new version for patch #1, but not yet patch #2.
>
> No problem this came back while trying to reduce my todolist stack :-)
>
>>
>> I am still debating with myself where the speculation barrier should be added after the SMC :).
>
> I think that we should unless the SMC is in the context switch path (as all other calls should not have a performance impact).
I will introduce *_unsafe() helpers that will not contain the
speculation barrier. It could then be used in place where we think the
barrier is unnecessary.
>
>>
>>>> On 4 Jul 2020, at 17:07, Julien Grall <julien@xen.org> wrote:
>>>>
>>>> On 17/06/2020 17:23, Julien Grall wrote:
>>>>> Hi,
>>>>> On 16/06/2020 22:24, Stefano Stabellini wrote:
>>>>>> On Tue, 16 Jun 2020, Julien Grall wrote:
>>>>>>> From: Julien Grall <jgrall@amazon.com>
>>>>>>>
>>>>>>> Some CPUs can speculate past a RET instruction and potentially perform
>>>>>>> speculative accesses to memory before processing the return.
>>>>>>>
>>>>>>> There is no known gadget available after the RET instruction today.
>>>>>>> However some of the registers (such as in check_pending_guest_serror())
>>>>>>> may contain a value provided the guest.
>>>>>> ^ by
>>>>>>
>>>>>>
>>>>>>> In order to harden the code, it would be better to add a speculation
>>>>>>> barrier after each RET instruction. The performance is meant to be
>>>>>>> negligeable as the speculation barrier is not meant to be archicturally
>>>>>>> executed.
>>>>>>>
>>>>>>> Note that on arm32, the ldmia instruction will act as a return from the
>>>>>>> function __context_switch(). While the whitepaper doesn't suggest it is
>>>>>>> possible to speculate after the instruction, add preventively a
>>>>>>> speculation barrier after it as well.
>>>>>>>
>>>>>>> This is part of the work to mitigate straight-line speculation.
>>>>>>>
>>>>>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>>>>>
>>>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>>>>>
>>>>>> I did a compile-test on the patch too.
>>>>>>
>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> I am still unsure whether we preventively should add a speculation barrier
>>>>>>> preventively after all the RET instructions in arm*/lib/. The smc call be
>>>>>>> taken care in a follow-up patch.
>>>>>>
>>>>>> SMC is great to have but it seems to be overkill to do the ones under
>>>>>> lib/.
>>>>> From my understanding, the compiler will add a speculation barrier preventively after each 'ret' when the mitigation are turned on.So it feels to me we want to follow the same approach.
>>>>> Obviously, we can avoid them but I would like to have a justification for not adding them (nothing is overkilled against speculation ;)).
>>>>
>>>> I finally found some time to look at arm*/lib in more details. Some of the helpers can definitely be called with guest inputs.
>>>>
>>>> For instance, memchr() is called from hypfs_get_path_user() with the 3rd argument controlled by the guest. In both 32-bit and 64-bit implementation, you will reach the end of the function memchr() with r2/w2 and r3/w3 (it contains a character from the buffer) controlled by the guest.
>>>>
>>>> As this is the only function in the unit, we don't know what will be the instructions right after RET. So it would be safer to add a speculation barrier there too.
>>> How about adding a speculation barrier directly in the ENDPROC macro ?
>>
>> This would unfortunately not cover all the cases because you can return in the middle of the function. I will have a look to see if we can leverage it.
>
> I agree that it would not solve all of them but a big part would be solved by it.
> An other solution might be to have a RETURN macro encoded as "mov pc,lr; sb" and "ret; sb”.
This is a bit messy on Arm32 because not all the return are using "mov
pc,lr". Anyway, I will explore the two approaches.
>
> The patch sounds good, i just need to find a way to analyse if you missed a ret or not which is not easy with such a patch :-)
I did struggle to find all the instances. The directory lib/ is actually
quite difficult to go through on 32-bit because they are multiple way to
implement a return.
Finding a way to reduce manual speculation barrier would definitely be
helpful. I will try to revise the patch during this week.
Cheers,
--
Julien Grall
next prev parent reply other threads:[~2020-08-18 17:34 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-16 17:59 [PATCH 0/2] xen/arm: Mitigate straight-line speculation Julien Grall
2020-06-16 17:59 ` [PATCH 1/2] xen/arm: entry: Place a speculation barrier following an ret instruction Julien Grall
2020-06-16 21:24 ` Stefano Stabellini
2020-06-17 16:23 ` Julien Grall
2020-07-04 16:07 ` Julien Grall
2020-08-18 16:35 ` Bertrand Marquis
2020-08-18 16:43 ` Julien Grall
2020-08-18 17:06 ` Bertrand Marquis
2020-08-18 17:34 ` Julien Grall [this message]
2020-08-19 7:59 ` Bertrand Marquis
2020-08-19 8:02 ` Jan Beulich
2020-08-19 8:50 ` Julien Grall
2020-08-19 8:58 ` Jan Beulich
2020-08-19 9:56 ` Julien Grall
2020-06-16 17:59 ` [PATCH 2/2] xen/arm: Mitigate straight-line speculation for SMC call Julien Grall
2020-06-16 21:34 ` Stefano Stabellini
2020-06-16 21:57 ` Julien Grall
2020-06-16 23:16 ` Andrew Cooper
2020-06-16 23:27 ` Stefano Stabellini
2020-06-18 17:46 ` 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=5dceeedf-9982-37c5-553e-76f22d9d6db2@xen.org \
--to=julien@xen.org \
--cc=Andre.Przywara@arm.com \
--cc=Bertrand.Marquis@arm.com \
--cc=Volodymyr_Babchuk@epam.com \
--cc=jgrall@amazon.com \
--cc=nd@arm.com \
--cc=paul@xen.org \
--cc=security@xenproject.org \
--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).