Re: [PATCH SpectreV1+L1TF v4 04/11] x86/hvm: block speculative out-of-bound accesses

["Jan Beulich" <JBeulich@suse.com>]

>>> On 31.01.19 at 20:31, <andrew.cooper3@citrix.com> wrote:
> On 23/01/2019 11:51, Norbert Manthey wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -37,6 +37,7 @@
>>  #include <xen/monitor.h>
>>  #include <xen/warning.h>
>>  #include <xen/vpci.h>
>> +#include <xen/nospec.h>
>>  #include <asm/shadow.h>
>>  #include <asm/hap.h>
>>  #include <asm/current.h>
>> @@ -2102,7 +2103,7 @@ int hvm_mov_from_cr(unsigned int cr, unsigned int gpr)
>>      case 2:
>>      case 3:
>>      case 4:
>> -        val = curr->arch.hvm.guest_cr[cr];
>> +        val = array_access_nospec(curr->arch.hvm.guest_cr, cr);
> 
> This is an interesting case - we don't actually need protection here.
> 
> This path is called exclusively from intercepts, so cr is strictly one
> of 0, 2, 3, 4, 8 even under adversarial speculation.  However, as
> guest_cr[] is only 5 entries long, the 8 case can still result in an OoB
> read.
> 
> However, given that the 8 index is in the hw_cr[] array and guaranteed
> to be in the cache by this point, an attacker can't gain any additional
> information by poisoning the switch logic.

Question though is - do we want to make the safety of our
code dependent on such (easily and un-noticeably changeable)
layout considerations? I'm not opposed (and I've used similar
arguments for overruns by 1 elsewhere, albeit in cases where
the layout wasn't as far away from the code in question as it
is here, and where the two fields were adjacent), but perhaps
we'd then want a BUILD_BUG_ON() with a suitable comment
(and carefully coded to avoid potential array-index-out-of-
bounds diagnostics)?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel