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

Re: [Xen-devel] [PATCH SpectreV1+L1TF v4 09/11] x86/vioapic: block speculative out-of-bound accesses


  • To: Jan Beulich <JBeulich@xxxxxxxx>
  • From: Norbert Manthey <nmanthey@xxxxxxxxx>
  • Date: Mon, 28 Jan 2019 12:03:39 +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: 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>, 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, 28 Jan 2019 11:04:16 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Openpgp: preference=signencrypt

On 1/25/19 17:34, Jan Beulich wrote:
>>>> On 23.01.19 at 12:57, <nmanthey@xxxxxxxxx> wrote:
>> @@ -66,6 +67,9 @@ static struct hvm_vioapic *gsi_vioapic(const struct domain 
>> *d,
>>  {
>>      unsigned int i;
>>  
>> +    /* Make sure the compiler does not optimize the initialization */
>> +    OPTIMIZER_HIDE_VAR(pin);
> Since there's no initialization here, I think it would help to add "done
> in the callers". Perhaps also "optimize away" or "delete"?
>
> And then I think you mean *pin.
True, I will adapt both the comment and the OPTIMIZER_HIDE_VAR call.
>
>> @@ -212,7 +217,12 @@ static void vioapic_write_redirent(
>>      struct hvm_irq *hvm_irq = hvm_domain_irq(d);
>>      union vioapic_redir_entry *pent, ent;
>>      int unmasked = 0;
>> -    unsigned int gsi = vioapic->base_gsi + idx;
>> +    unsigned int gsi;
>> +
>> +    /* Make sure no out-of-bound value for idx can be used */
>> +    idx = array_index_nospec(idx, vioapic->nr_pins);
>> +
>> +    gsi = vioapic->base_gsi + idx;
> I dislike the disconnect from the respective bounds check: There's
> only one caller, so the construct could be moved there, or
> otherwise I'd like to see an ASSERT() added documenting that the
> bounds check is expected to have happened in the caller.
I agree that the idx value is used as an array index in this function
only once. However, the gsi value also uses the value of idx, and as
that is passed to other functions, I want to bound the gsi variable as
well. Therefore, I chose to have a separate assignment for the idx variable.
>
>> @@ -378,7 +388,8 @@ static inline int pit_channel0_enabled(void)
>>  
>>  static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int pin)
>>  {
>> -    uint16_t dest = vioapic->redirtbl[pin].fields.dest_id;
>> +    uint16_t dest = vioapic->redirtbl
>> +               [pin = array_index_nospec(pin, 
>> vioapic->nr_pins)].fields.dest_id;
>>      uint8_t dest_mode = vioapic->redirtbl[pin].fields.dest_mode;
>>      uint8_t delivery_mode = vioapic->redirtbl[pin].fields.delivery_mode;
>>      uint8_t vector = vioapic->redirtbl[pin].fields.vector;
> I'm sorry, but despite prior discussions I'm still not happy about
> this change - all of the callers pass known good values:
> - vioapic_write_redirent() gets adjusted above,
> - vioapic_irq_positive_edge() gets the value passed into here
>   from gsi_vioapic(), which you also take care of,
> - vioapic_update_EOI() loops over all pins, so only passes in-
>   range values.
> Therefore I still don't see what protection this change adds.
> As per above, if it was to stay, some sort of connection to the
> range check(s) it guards would otherwise be nice to establish,
> but I realize that adding an ASSERT() here would go against
> a certain aspect of review comments I gave on earlier versions.
I will drop this change. As you called out, all callers are bound
checked already. Hence, I will not add an assert.
>
>> @@ -463,7 +474,7 @@ static void vioapic_deliver(struct hvm_vioapic *vioapic, 
>> unsigned int pin)
>>  
>>  void vioapic_irq_positive_edge(struct domain *d, unsigned int irq)
>>  {
>> -    unsigned int pin;
>> +    unsigned int pin = 0; /* See gsi_vioapic */
>>      struct hvm_vioapic *vioapic = gsi_vioapic(d, irq, &pin);
>>      union vioapic_redir_entry *ent;
>>  
>> @@ -560,7 +571,7 @@ int vioapic_get_vector(const struct domain *d, unsigned 
>> int gsi)
>>  
>>  int vioapic_get_trigger_mode(const struct domain *d, unsigned int gsi)
>>  {
>> -    unsigned int pin;
>> +    unsigned int pin = 0; /* See gsi_vioapic */
>>      const struct hvm_vioapic *vioapic = gsi_vioapic(d, gsi, &pin);
>>  
>>      if ( !vioapic )
> Since there are more callers of gsi_vioapic(), justification should be
> added to the description why only some need adjustment (or
> otherwise, just to be on the safe side as well as for consistency
> all of them should be updated, in which case it would still be nice
> to call out the ones which really [don't] need updating).

I will extend the explanation in the commit message.

Best,
Norbert

>
> Jan
>
>




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
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

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