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

Re: [Xen-devel] [PATCH SpectreV1+L1TF v6 3/9] x86/hvm: block speculative out-of-bound accesses

  • To: Jan Beulich <JBeulich@xxxxxxxx>
  • From: Norbert Manthey <nmanthey@xxxxxxxxx>
  • Date: Mon, 18 Feb 2019 15:47:06 +0100
  • Autocrypt: addr=nmanthey@xxxxxxxxx; prefer-encrypt=mutual; keydata= mQINBFoJQc0BEADM8Z7hB7AnW6ErbSMsYkKh4HLAPfoM+wt7Fd7axHurcOgFJEBOY2gz0isR /EDiGxYyTgxt5PZHJIfra0OqXRbWuLltbjhJACbu35eaAo8UM4/awgtYx3O1UCbIlvHGsYDg kXjF8bBrVjPu0+g55XizX6ot/YPAgmWTdH8qXoLYVZVWJilKlTqpYEVvarSn/BVgCbIsQIps K93sOTN9eJKDSqHvbkgKl9XG3WsZ703431egIpIZpfN0zZtzumdZONb7LiodcFHJ717vvd89 3Hv2bYv8QLSfYsZcSnyU0NVzbPhb1WtaduwXwNmnX1qHJuExzr8EnRT1pyhVSqouxt+xkKbV QD9r+cWLChumg3g9bDLzyrOTlEfAUNxIqbzSt03CRR43dWgfgGiLDcrqC2b1QR886WDpz4ok xX3fdLaqN492s/3c59qCGNG30ebAj8AbV+v551rsfEba+IWTvvoQnbstc6vKJCc2uG8rom5o eHG/bP1Ug2ht6m/0uWRyFq9C27fpU9+FDhb0ZsT4UwOCbthe35/wBZUg72zDpT/h5lm64G6C 0TRqYRgYcltlP705BJafsymmAXOZ1nTCuXnYAB9G9LzZcKKq5q0rP0kp7KRDbniirCUfp7jK VpPCOUEc3tS1RdCCSeWNuVgzLnJdR8W2h9StuEbb7hW4aFhwRQARAQABtCROb3JiZXJ0IE1h bnRoZXkgPG5tYW50aGV5QGFtYXpvbi5kZT6JAj0EEwEIACcFAloJQc0CGyMFCQlmAYAFCwkI BwIGFQgJCgsCBBYCAwECHgECF4AACgkQZ+8yS8zN62ajmQ/6AlChoY5UlnUaH/jgcabyAfUC XayHgCcpL1SoMKvc2rCA8PF0fza3Ep2Sw0idLqC/LyAYbI6gMYavSZsLcsvY6KYAZKeaEriG 7R6cSdrbmRcKpPjwvv4iY6G0DBTeaqfNjGe1ECY8u522LprDQVquysJIf3YaEyxoK/cLSb0c kjzpqI1P9Vh+8BQb5H9gWpakbhFIwbRGHdAF1roT7tezmEshFS0IURJ2ZFEI+ZgWgtl1MBwN sBt65im7x5gDo25h8A5xC9gLXTc4j3tk+3huaZjUJ9mCbtI12djVtspjNvDyUPQ5Mxw2Jwar C3/ZC+Nkb+VlymmErpnEUZNltcq8gsdYND4TlNbZ2JhD0ibiYFQPkyuCVUiVtimXfh6po9Yt OkE0DIgEngxMYfTTx01Zf6iwrbi49eHd/eQQw3zG5nn+yZsEG8UcP1SCrUma8p93KiKOedoL n43kTg4RscdZMjj4v6JkISBcGTR4uotMYP4M0zwjklnFXPmrZ6/E5huzUpH9B7ZIe/SUu8Ur xww/4dN6rfqbNzMxmya8VGlEQZgUMWcck+cPrRLB09ZOk4zq9i/yaHDEZA1HNOfQ9UCevXV5 7seXSX7PCY6WDAdsT3+FuaoQ7UoWN3rdpb+064QKZ0FsHeGzUd7MZtlgU4EKrh25mTSNZYRs nTz2zT/J33e5Ag0EWglBzQEQAKioD1gSELj3Y47NE11oPkzWWdxKZdVr8B8VMu6nVAAGFRSf Dms4ZmwGY27skMmOH2srnZyTfm9FaTKr8RI+71Fh9nfB9PMmwzA7OIY9nD73/HqPywzTTleG MlALmnuY6xFRSDmqmvxDHgWyzB4TgPWt8+hW3+TJKCx2RgLAdSuULZla4lia+NlS8WNRUDGK sFJCCB3BW5I/cocfpBEUqLbbmnPuD9UKpEnFcYWD9YaDNcBTjSc7iDsvtpdrBXg5VETOz/TQ /CmVs9h/5zug8O4bXxHEEJpCAxs4cGKxowBqx/XJfkwdWeo/LdaeR+LRbXvq4A32HSkyj9sV vygwt2OFEk493JGik8qtAA/oPvuqVPJGacxmZ7zKR12c0mnKCHiexFJzFbC7MSiUhhe8nNiM p6Sl6EZmsTUXhV2bd2M12Bqcss3TTJ1AcW04T4HYHVCSxwl0dVfcf3TIaH0BSPiwFxz0FjMk 10umoRvUhYYoYpPFCz8dujXBlfB8q2tnHltEfoi/EIptt1BMNzTYkHKArj8Fwjf6K+nQ3a8p 1cWfkYpA5bRqbhbplzpa0u1Ex0hZk6pka0qcVgqmH31O2OcSsqeKfUfHkzj3Q6dmuwm1je/f HWH9N1gDPEp1RB5bIxPnOG1Z4SNl9oVQJhc4qoJiqbvkciivYcH7u2CBkboFABEBAAGJAiUE GAEIAA8FAloJQc0CGwwFCQlmAYAACgkQZ+8yS8zN62YU9Q//WTnN28aBX1EhDidVho80Ql2b tV1cDRh/vWTcM4qoM8vzW4+F/Ive6wDVAJ7zwAv8F8WPzy+acxtHLkyYk14M6VZ1eSy0kV0+ RZQdQ+nPtlb1MoDKw2N5zhvs8A+WD8xjDIA9i21hQ/BNILUBINuYKyR19448/41szmYIEhuJ R2fHoLzNdXNKWQnN3/NPTuvpjcrkXKJm2k32qfiys9KBcZX8/GpuMCc9hMuymzOr+YlXo4z4 1xarEJoPOQOXnrmxN4Y30/qmf70KHLZ0GQccIm/o/XSOvNGluaYv0ZVJXHoCnYvTbi0eYvz5 OfOcndqLOfboq9kVHC6Yye1DLNGjIVoShJGSsphxOx2ryGjHwhzqDrLiRkV82gh6dUHKxBWd DXfirT8a4Gz/tY9PMxan67aSxQ5ONpXe7g7FrfrAMe91XRTf50G3rHb8+AqZfxZJFrBn+06i p1cthq7rJSlYCqna2FedTUT+tK1hU9O0aK4ZYYcRzuTRxjd4gKAWDzJ1F/MQ12ftrfCAvs7U sVbXv2TndGIleMnheYv1pIrXEm0+sdz5v91l2/TmvkyyWT8s2ksuZis9luh+OubeLxHq090C hfavI9WxhitfYVsfo2kr3EotGG1MnW+cOkCIX68w+3ZS4nixZyJ/TBa7RcTDNr+gjbiGMtd9 pEddsOqYwOs=
  • Cc: Juergen Gross <jgross@xxxxxxxx>, Tim Deegan <tim@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wei.liu2@xxxxxxxxxx>, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>, George Dunlap <George.Dunlap@xxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>, Dario Faggioli <dfaggioli@xxxxxxxx>, Martin Pohlack <mpohlack@xxxxxxxxx>, wipawel@xxxxxxxxx, Julien Grall <julien.grall@xxxxxxx>, David Woodhouse <dwmw@xxxxxxxxxxxx>, "Martin Mazein\(amazein\)" <amazein@xxxxxxxxx>, xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Julian Stecklina <jsteckli@xxxxxxxxx>, Bjoern Doebel <doebel@xxxxxxxxx>
  • Delivery-date: Mon, 18 Feb 2019 14:47:29 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Openpgp: preference=signencrypt

On 2/15/19 12:46, Jan Beulich wrote:
>>>> On 15.02.19 at 11:50, <nmanthey@xxxxxxxxx> wrote:
>> On 2/15/19 09:55, Jan Beulich wrote:
>>>>>> On 15.02.19 at 09:05, <nmanthey@xxxxxxxxx> wrote:
>>>> On 2/12/19 15:14, Jan Beulich wrote:
>>>>>>>> On 12.02.19 at 15:05, <nmanthey@xxxxxxxxx> wrote:
>>>>>> On 2/12/19 14:25, Jan Beulich wrote:
>>>>>>>>>> On 08.02.19 at 14:44, <nmanthey@xxxxxxxxx> wrote:
>>>>>>>> @@ -4104,6 +4108,12 @@ static int hvmop_set_param(
>>>>>>>>      if ( a.index >= HVM_NR_PARAMS )
>>>>>>>>          return -EINVAL;
>>>>>>>> +    /*
>>>>>>>> +     * Make sure the guest controlled value a.index is bounded even 
>>>>>>>> during
>>>>>>>> +     * speculative execution.
>>>>>>>> +     */
>>>>>>>> +    a.index = array_index_nospec(a.index, HVM_NR_PARAMS);
>>>>>>>> +
>>>>>>>>      d = rcu_lock_domain_by_any_id(a.domid);
>>>>>>>>      if ( d == NULL )
>>>>>>>>          return -ESRCH;
>>>>>>>> @@ -4370,6 +4380,12 @@ static int hvmop_get_param(
>>>>>>>>      if ( a.index >= HVM_NR_PARAMS )
>>>>>>>>          return -EINVAL;
>>>>>>>> +    /*
>>>>>>>> +     * Make sure the guest controlled value a.index is bounded even 
>>>>>>>> during
>>>>>>>> +     * speculative execution.
>>>>>>>> +     */
>>>>>>>> +    a.index = array_index_nospec(a.index, HVM_NR_PARAMS);
>>>>>>> ... the usefulness of these two. To make forward progress it may
>>>>>>> be worthwhile to split off these two changes into a separate patch.
>>>>>>> If you're fine with this, I could strip these two before committing,
>>>>>>> in which case the remaining change is
>>>>>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>>> Taking apart the commit is fine with me. I will submit a follow up
>>>>>> change that does not update the values but fixes the reads.
>>>>> As pointed out during the v5 discussion, I'm unconvinced that if
>>>>> you do so the compiler can't re-introduce the issue via CSE. I'd
>>>>> really like a reliable solution to be determined first.
>>>> I cannot give a guarantee what future compilers might do. Furthermore, I
>>>> do not want to wait until all/most compilers ship with such a
>>>> controllable guarantee.
>>> Guarantee? Future compilers are (hopefully) going to get better at
>>> optimizing, and hence are (again hopefully) going to find more
>>> opportunities for CSE. So the problem is going to get worse rather
>>> than better, and the changes you're proposing to re-instate are
>>> therefore more like false promises.
>> I do not want to dive into compilers future here. I would like to fix
>> the issue for todays compilers now and not wait until compilers evolved
>> one way or another. For this patch, the relevant information is whether
>> it should go in like this, or whether you want me to protect all the
>> reads instead. Is there more data I shall provide to help make this
>> decision?
> I understand that you're not happy with what I've said, and you're
> unlikely to become any happier with what I'll add. But please
> understand that _if_ we make any changes to address issues with
> speculation, the goal has to be that we don't have to come back
> an re-investigate after every new compiler release.
> Even beyond that - if, as you say, we'd limit ourselves to current
> compilers, did you check that all of them at any optimization level
> or with any other flags passed which may affect code generation
> produce non-vulnerable code? And in particular considering the
> case here never recognize CSE potential where we would like them
> not to?
> A code change is, imo, not even worthwhile considering to be put
> in if it is solely based on the observations made with a limited set
> of compilers and/or options. This might indeed help you, if you
> care only about one specific environment. But by putting this in
> (and perhaps even backporting it) we're sort of stating that the
> issue is under control (to the best of our abilities, and for the given
> area of code). For everyone.
I do not see how a fix for problems like the discussed one could enter
the code base given the above conditions. However, for this very
specific fix, there fortunately is a comparison wrt a constant, and
there are many instructions until the potential speculative out-of-bound
access might happen, so that not fixing the two above access is fine for
me. While I cannot guarantee that it is not possible, we did not manage
to come up with a PoC for these two places with the effort we put into this.
> So, to answer your question: From what we know, we simply
> can't take a decision, at least not between the two proposed
> variants of how to change the code. If there was a variant that
> firmly worked, then there would not even be a need for any
> discussion. And again from what we know, there is one
> requirement that need to be fulfilled for a change to be
> considered "firmly working": The index needs to be in a register.
> There must not be a way for the compiler to undermine this,
> be it by CSE or any other means.
> Considering changes done elsewhere, of course this may be
> taken with a grain of salt. In other places we also expect the
> compiler to not emit unreasonable code (e.g. needlessly
> spilling registers to memory just to then reload them). But
> while that's (imo) a fine expectation to have when an array
> index is used just once, it is unavoidably more complicated in
> the case here as well as in the grant table one.

Unless you outline a path forward to fix the above two gadgets, I will
not include the above hunks in the next version of the series.


Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

Xen-devel mailing list



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