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

Re: [Xen-devel] [PATCH SpectreV1+L1TF v5 1/9] xen/evtchn: block speculative out-of-bound accesses


  • To: Jan Beulich <JBeulich@xxxxxxxx>
  • From: Norbert Manthey <nmanthey@xxxxxxxxx>
  • Date: Fri, 1 Feb 2019 14:45:08 +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>, 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: Fri, 01 Feb 2019 13:45:30 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Openpgp: preference=signencrypt

On 1/31/19 16:05, Jan Beulich wrote:
>>>> On 29.01.19 at 15:43, <nmanthey@xxxxxxxxx> wrote:
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -365,11 +365,16 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, 
>> evtchn_port_t port)
>>      if ( (virq < 0) || (virq >= ARRAY_SIZE(v->virq_to_evtchn)) )
>>          return -EINVAL;
>>  
>> +   /*
>> +    * Make sure the guest controlled value virq is bounded even during
>> +    * speculative execution.
>> +    */
>> +    virq = array_index_nospec(virq, ARRAY_SIZE(v->virq_to_evtchn));
>> +
>>      if ( virq_is_global(virq) && (vcpu != 0) )
>>          return -EINVAL;
>>  
>> -    if ( (vcpu < 0) || (vcpu >= d->max_vcpus) ||
>> -         ((v = d->vcpu[vcpu]) == NULL) )
>> +    if ( (vcpu < 0) || ((v = domain_vcpu(d, vcpu)) == NULL) )
>>          return -ENOENT;
> Is there a reason for the less-than-zero check to survive?
Yes, domain_vcpu uses unsigned integers, and I want to return the proper
error code, in case somebody comes with a vcpu number that would
overflow into the valid range.
>
>> @@ -418,8 +423,7 @@ static long evtchn_bind_ipi(evtchn_bind_ipi_t *bind)
>>      int            port, vcpu = bind->vcpu;
>>      long           rc = 0;
>>  
>> -    if ( (vcpu < 0) || (vcpu >= d->max_vcpus) ||
>> -         (d->vcpu[vcpu] == NULL) )
>> +    if ( (vcpu < 0) || domain_vcpu(d, vcpu) == NULL )
>>          return -ENOENT;
> I'm not sure about this one: We're not after the struct vcpu pointer
> here. Right now subsequent code looks fine, but what if the actual
> "vcpu" local variable was used again in a risky way further down? I
> think here and elsewhere it would be best to eliminate that local
> variable, and use v->vcpu_id only for subsequent consumers (or
> alternatively latch the local variable's value only _after_ the call to
> domain_vcpu(), which might be better especially in cases like).

I agree with getting rid of using the local variable. As discussed
elsewhere, updating such a variable might not fix the problem. However,
in this commit I want to avoid speculative out-of-bound accesses using a
guest controlled variable (vcpu). Hence, I add protection to the
locations where it is used as index. As the domain_vcpu function comes
with protection, I prefer this function over explicitly using
array_index_nospec, if possible.

>
>> @@ -969,8 +980,8 @@ long evtchn_bind_vcpu(unsigned int port, unsigned int 
>> vcpu_id)
>>          unlink_pirq_port(chn, d->vcpu[chn->notify_vcpu_id]);
>>          chn->notify_vcpu_id = vcpu_id;
>>          pirq_set_affinity(d, chn->u.pirq.irq,
>> -                          cpumask_of(d->vcpu[vcpu_id]->processor));
>> -        link_pirq_port(port, chn, d->vcpu[vcpu_id]);
>> +                          cpumask_of(domain_vcpu(d, vcpu_id)->processor));
>> +        link_pirq_port(port, chn, domain_vcpu(d, vcpu_id));
> ... this one, where you then wouldn't need to alter code other than
> that actually checking the vCPU ID.
Instead, I will introduce a struct vcpu variable, assign it in the first
check of the function, and continue using this variable instead of
performing array accesses again in this function.
>
>> @@ -516,14 +517,22 @@ int evtchn_fifo_init_control(struct 
>> evtchn_init_control 
>> *init_control)
>>      gfn     = init_control->control_gfn;
>>      offset  = init_control->offset;
>>  
>> -    if ( vcpu_id >= d->max_vcpus || !d->vcpu[vcpu_id] )
>> +    if ( !domain_vcpu(d, vcpu_id) )
>>          return -ENOENT;
>> -    v = d->vcpu[vcpu_id];
>> +
>> +    v = domain_vcpu(d, vcpu_id);
> Please don't call the function twice.

I will assign the variable as part of the if statement.

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®.