>>> On 31.01.19 at 20:31, <andrew.coop...@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
