[PATCH 0/4] x86: further L1TF / XSA-289 guards

[PATCH 0/4] x86: further L1TF / XSA-289 guards

[Jan Beulich]

This goes alongside Norbert's series, dealing with a few more
places where I happened to know (without any analysis tools)
guest controlled array accesses sit. I've additionally also
checked emul-i8254.c, and I think no adjustments are needed
there (there are a few possible overruns by one, but just like
is the case in patch 2 I don't think they are actual issues).

1: x86emul: avoid speculative out of bounds accesses
2: x86/vMSI: avoid speculative out of bounds accesses
3: x86/vPIC: avoid speculative out of bounds accesses
4: x86/vLAPIC: avoid speculative out of bounds accesses

Jürgen, I've copied you anyway, but I assume your general
Rab-until-RC3 would apply to this series (and perhaps to
further ones, should anyone find time) as much as to Norbert's.

Jan


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

[PATCH 1/4] x86emul: avoid speculative out of bounds accesses

[Jan Beulich]

There are a few array accesses here the indexes of which are (at least
indirectly) driven by the guest. Use array_access_nospec() to bound
such accesses. In the {,_}decode_gpr() cases replace existing guarding
constructs.

To deal with an otherwise occurring #include cycle, drop the inclusion
of asm/x86_emulate.h from asm/processor.h. This include had been
introduced for obtaining the struct cpuid_leaf declaration, which has
since moved into the x86 helper library.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate.c
@@ -102,7 +102,7 @@ int x86emul_read_dr(unsigned int reg, un
     switch ( reg )
     {
     case 0 ... 3:
-        *val = curr->arch.dr[reg];
+        *val = array_access_nospec(curr->arch.dr, reg);
         break;
 
     case 4:
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -2207,10 +2207,7 @@ static void *_decode_gpr(
 
     ASSERT(modrm_reg < ARRAY_SIZE(byte_reg_offsets));
 
-    /* For safety in release builds.  Debug builds will hit the ASSERT() */
-    modrm_reg &= ARRAY_SIZE(byte_reg_offsets) - 1;
-
-    return (void *)regs + byte_reg_offsets[modrm_reg];
+    return (void *)regs + array_access_nospec(byte_reg_offsets, modrm_reg);
 }
 
 static unsigned long *decode_vex_gpr(
@@ -2935,7 +2932,7 @@ x86_decode(
                     b = insn_fetch_type(uint8_t);
                     opcode |= MASK_INSR(0x8f08 + ext - ext_8f08,
                                         X86EMUL_OPC_EXT_MASK);
-                    d = xop_table[ext - ext_8f08];
+                    d = array_access_nospec(xop_table, ext - ext_8f08);
                 }
                 else
                 {
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -642,6 +642,12 @@ int x86_emulate_wrapper(
 #define x86_emulate x86_emulate_wrapper
 #endif
 
+#ifdef __XEN__
+# include <xen/nospec.h>
+#else
+# define array_access_nospec(arr, idx) arr[idx]
+#endif
+
 /* Map GPRs by ModRM encoding to their offset within struct cpu_user_regs. */
 extern const uint8_t cpu_user_regs_gpr_offsets[X86_NR_GPRS];
 
@@ -658,10 +664,7 @@ static inline unsigned long *decode_gpr(
 
     ASSERT(modrm < ARRAY_SIZE(cpu_user_regs_gpr_offsets));
 
-    /* For safety in release builds.  Debug builds will hit the ASSERT() */
-    modrm &= ARRAY_SIZE(cpu_user_regs_gpr_offsets) - 1;
-
-    return (void *)regs + cpu_user_regs_gpr_offsets[modrm];
+    return (void *)regs + array_access_nospec(cpu_user_regs_gpr_offsets, modrm);
 }
 
 /* Unhandleable read, write or instruction fetch */
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -13,7 +13,6 @@
 #include <asm/types.h>
 #include <asm/cpufeature.h>
 #include <asm/desc.h>
-#include <asm/x86_emulate.h>
 #endif
 
 #include <asm/x86-defns.h>





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

Re: [PATCH 1/4] x86emul: avoid speculative out of bounds accesses

[Andrew Cooper]

On 31/01/2019 14:25, Jan Beulich wrote:
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -2207,10 +2207,7 @@ static void *_decode_gpr(
>  
>      ASSERT(modrm_reg < ARRAY_SIZE(byte_reg_offsets));
>  
> -    /* For safety in release builds.  Debug builds will hit the ASSERT() */
> -    modrm_reg &= ARRAY_SIZE(byte_reg_offsets) - 1;
> -
> -    return (void *)regs + byte_reg_offsets[modrm_reg];
> +    return (void *)regs + array_access_nospec(byte_reg_offsets, modrm_reg);
>  }

Actually, the &= here wasn't by accident.  When the array size is an
power of two and known to the compiler, it is a rather lower overhead
alternative to array_access_nospec(), as it avoids the cmp/sbb dance in
the asm volatile statement.

I wonder if there is a sensible way cope with this in
array_access_nospec().  Perhaps something like:

#define array_access_nospec(array, index)
({
    size_t _s = ARRAY_SIZE(array);

    if ( !(_s & (_s - 1)) )
    {
        typeof(index) _i = index & (_s - 1);
        OPTIMIZER_HIDE_VAR(_i);
        (array)[_i];
    }
    else
        (array)[array_index_nospec(index, ARRAY_SIZE(array))];
})

As _s is known at compile time, only one half of the if condition will
be emitted by the compiler.

~Andrew

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

Re: [PATCH 1/4] x86emul: avoid speculative out of bounds accesses

[Jan Beulich]

>>> On 31.01.19 at 15:54, <andrew.cooper3@citrix.com> wrote:
> On 31/01/2019 14:25, Jan Beulich wrote:
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -2207,10 +2207,7 @@ static void *_decode_gpr(
>>  
>>      ASSERT(modrm_reg < ARRAY_SIZE(byte_reg_offsets));
>>  
>> -    /* For safety in release builds.  Debug builds will hit the ASSERT() */
>> -    modrm_reg &= ARRAY_SIZE(byte_reg_offsets) - 1;
>> -
>> -    return (void *)regs + byte_reg_offsets[modrm_reg];
>> +    return (void *)regs + array_access_nospec(byte_reg_offsets, modrm_reg);
>>  }
> 
> Actually, the &= here wasn't by accident.  When the array size is an
> power of two and known to the compiler, it is a rather lower overhead
> alternative to array_access_nospec(), as it avoids the cmp/sbb dance in
> the asm volatile statement.
> 
> I wonder if there is a sensible way cope with this in
> array_access_nospec().  Perhaps something like:
> 
> #define array_access_nospec(array, index)
> ({
>     size_t _s = ARRAY_SIZE(array);
> 
>     if ( !(_s & (_s - 1)) )
>     {
>         typeof(index) _i = index & (_s - 1);
>         OPTIMIZER_HIDE_VAR(_i);
>         (array)[_i];
>     }
>     else
>         (array)[array_index_nospec(index, ARRAY_SIZE(array))];
> })
> 
> As _s is known at compile time, only one half of the if condition will
> be emitted by the compiler.

Except that this won't work as an lvalue anymore, yet we want
to use it as such in some cases. I can't seem to immediately think
of a way to overcome this.

As just said on the call, in the re-based AVX512F gather patch
the respective hunk is now

--- unstable.orig/xen/arch/x86/x86_emulate/x86_emulate.h
+++ unstable/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -662,9 +662,10 @@ static inline unsigned long *decode_gpr(
     BUILD_BUG_ON(ARRAY_SIZE(cpu_user_regs_gpr_offsets) &
                  (ARRAY_SIZE(cpu_user_regs_gpr_offsets) - 1));
 
-    ASSERT(modrm < ARRAY_SIZE(cpu_user_regs_gpr_offsets));
+    /* Note that this also acts as array_access_nospec() stand-in. */
+    modrm &= ARRAY_SIZE(cpu_user_regs_gpr_offsets) - 1;
 
-    return (void *)regs + array_access_nospec(cpu_user_regs_gpr_offsets, modrm);
+    return (void *)regs + cpu_user_regs_gpr_offsets[modrm];
 }
 
 /* Unhandleable read, write or instruction fetch */

I could obviously make the patch here simply insert that comment
instead of adding actual uses of the macro.

Jan



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

Re: [PATCH 1/4] x86emul: avoid speculative out of bounds accesses

[Andrew Cooper]

On 31/01/2019 15:50, Jan Beulich wrote:
>>>> On 31.01.19 at 15:54, <andrew.cooper3@citrix.com> wrote:
>> On 31/01/2019 14:25, Jan Beulich wrote:
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>> @@ -2207,10 +2207,7 @@ static void *_decode_gpr(
>>>  
>>>      ASSERT(modrm_reg < ARRAY_SIZE(byte_reg_offsets));
>>>  
>>> -    /* For safety in release builds.  Debug builds will hit the ASSERT() */
>>> -    modrm_reg &= ARRAY_SIZE(byte_reg_offsets) - 1;
>>> -
>>> -    return (void *)regs + byte_reg_offsets[modrm_reg];
>>> +    return (void *)regs + array_access_nospec(byte_reg_offsets, modrm_reg);
>>>  }
>> Actually, the &= here wasn't by accident.  When the array size is an
>> power of two and known to the compiler, it is a rather lower overhead
>> alternative to array_access_nospec(), as it avoids the cmp/sbb dance in
>> the asm volatile statement.
>>
>> I wonder if there is a sensible way cope with this in
>> array_access_nospec().  Perhaps something like:
>>
>> #define array_access_nospec(array, index)
>> ({
>>     size_t _s = ARRAY_SIZE(array);
>>
>>     if ( !(_s & (_s - 1)) )
>>     {
>>         typeof(index) _i = index & (_s - 1);
>>         OPTIMIZER_HIDE_VAR(_i);
>>         (array)[_i];
>>     }
>>     else
>>         (array)[array_index_nospec(index, ARRAY_SIZE(array))];
>> })
>>
>> As _s is known at compile time, only one half of the if condition will
>> be emitted by the compiler.
> Except that this won't work as an lvalue anymore, yet we want
> to use it as such in some cases. I can't seem to immediately think
> of a way to overcome this.

Does the lvalue use result in safe asm?  Irrespective, this macro can
probably be expressed as a ternary operation and retain its lvalue-ness,
albeit at the expense of readability.

>
> As just said on the call, in the re-based AVX512F gather patch
> the respective hunk is now
>
> --- unstable.orig/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ unstable/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -662,9 +662,10 @@ static inline unsigned long *decode_gpr(
>      BUILD_BUG_ON(ARRAY_SIZE(cpu_user_regs_gpr_offsets) &
>                   (ARRAY_SIZE(cpu_user_regs_gpr_offsets) - 1));
>  
> -    ASSERT(modrm < ARRAY_SIZE(cpu_user_regs_gpr_offsets));
> +    /* Note that this also acts as array_access_nospec() stand-in. */
> +    modrm &= ARRAY_SIZE(cpu_user_regs_gpr_offsets) - 1;
>  
> -    return (void *)regs + array_access_nospec(cpu_user_regs_gpr_offsets, modrm);
> +    return (void *)regs + cpu_user_regs_gpr_offsets[modrm];
>  }
>  
>  /* Unhandleable read, write or instruction fetch */
>
> I could obviously make the patch here simply insert that comment
> instead of adding actual uses of the macro.

Ah ok - this looks fine, however it ends up looking.

~Andrew

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

Re: [PATCH 1/4] x86emul: avoid speculative out of bounds accesses

[Jan Beulich]

>>> On 31.01.19 at 17:14, <andrew.cooper3@citrix.com> wrote:
> On 31/01/2019 15:50, Jan Beulich wrote:
>>>>> On 31.01.19 at 15:54, <andrew.cooper3@citrix.com> wrote:
>>> On 31/01/2019 14:25, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>>> @@ -2207,10 +2207,7 @@ static void *_decode_gpr(
>>>>  
>>>>      ASSERT(modrm_reg < ARRAY_SIZE(byte_reg_offsets));
>>>>  
>>>> -    /* For safety in release builds.  Debug builds will hit the ASSERT() */
>>>> -    modrm_reg &= ARRAY_SIZE(byte_reg_offsets) - 1;
>>>> -
>>>> -    return (void *)regs + byte_reg_offsets[modrm_reg];
>>>> +    return (void *)regs + array_access_nospec(byte_reg_offsets, modrm_reg);
>>>>  }
>>> Actually, the &= here wasn't by accident.  When the array size is an
>>> power of two and known to the compiler, it is a rather lower overhead
>>> alternative to array_access_nospec(), as it avoids the cmp/sbb dance in
>>> the asm volatile statement.
>>>
>>> I wonder if there is a sensible way cope with this in
>>> array_access_nospec().  Perhaps something like:
>>>
>>> #define array_access_nospec(array, index)
>>> ({
>>>     size_t _s = ARRAY_SIZE(array);
>>>
>>>     if ( !(_s & (_s - 1)) )
>>>     {
>>>         typeof(index) _i = index & (_s - 1);
>>>         OPTIMIZER_HIDE_VAR(_i);
>>>         (array)[_i];
>>>     }
>>>     else
>>>         (array)[array_index_nospec(index, ARRAY_SIZE(array))];
>>> })
>>>
>>> As _s is known at compile time, only one half of the if condition will
>>> be emitted by the compiler.
>> Except that this won't work as an lvalue anymore, yet we want
>> to use it as such in some cases. I can't seem to immediately think
>> of a way to overcome this.
> 
> Does the lvalue use result in safe asm?

Hmm, I'm a little puzzled - why would it not? Could you perhaps be
a little more specific about what worries you in that case?

>  Irrespective, this macro can
> probably be expressed as a ternary operation and retain its lvalue-ness,
> albeit at the expense of readability.

Oh, you're right. Not even overly difficult. The readability aspect
could perhaps be addressed by using another helper macro. But
for the purposes here I'll go the comment adjustment route. We
can always add this improvement to the macro later on.

Jan



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

[PATCH v2] x86emul: avoid speculative out of bounds accesses

[Jan Beulich]

There are a few array accesses here the indexes of which are (at least
indirectly) driven by the guest. Use array_access_nospec() to bound
such accesses. In the {,_}decode_gpr() cases replace existing guarding
constructs.

To deal with an otherwise occurring #include cycle, drop the inclusion
of asm/x86_emulate.h from asm/processor.h. This include had been
introduced for obtaining the struct cpuid_leaf declaration, which has
since moved into the x86 helper library.

This is part of the speculative hardening effort.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Instead of altering the code in {,_}decode_gpr() simply alter the
    comment.

--- a/xen/arch/x86/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate.c
@@ -102,7 +102,7 @@ int x86emul_read_dr(unsigned int reg, un
     switch ( reg )
     {
     case 0 ... 3:
-        *val = curr->arch.dr[reg];
+        *val = array_access_nospec(curr->arch.dr, reg);
         break;
 
     case 4:
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -2207,7 +2207,7 @@ static void *_decode_gpr(
 
     ASSERT(modrm_reg < ARRAY_SIZE(byte_reg_offsets));
 
-    /* For safety in release builds.  Debug builds will hit the ASSERT() */
+    /* Note that this also acts as array_access_nospec() stand-in. */
     modrm_reg &= ARRAY_SIZE(byte_reg_offsets) - 1;
 
     return (void *)regs + byte_reg_offsets[modrm_reg];
@@ -2935,7 +2935,7 @@ x86_decode(
                     b = insn_fetch_type(uint8_t);
                     opcode |= MASK_INSR(0x8f08 + ext - ext_8f08,
                                         X86EMUL_OPC_EXT_MASK);
-                    d = xop_table[ext - ext_8f08];
+                    d = array_access_nospec(xop_table, ext - ext_8f08);
                 }
                 else
                 {
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -642,6 +642,12 @@ int x86_emulate_wrapper(
 #define x86_emulate x86_emulate_wrapper
 #endif
 
+#ifdef __XEN__
+# include <xen/nospec.h>
+#else
+# define array_access_nospec(arr, idx) arr[idx]
+#endif
+
 /* Map GPRs by ModRM encoding to their offset within struct cpu_user_regs. */
 extern const uint8_t cpu_user_regs_gpr_offsets[X86_NR_GPRS];
 
@@ -658,7 +664,7 @@ static inline unsigned long *decode_gpr(
 
     ASSERT(modrm < ARRAY_SIZE(cpu_user_regs_gpr_offsets));
 
-    /* For safety in release builds.  Debug builds will hit the ASSERT() */
+    /* Note that this also acts as array_access_nospec() stand-in. */
     modrm &= ARRAY_SIZE(cpu_user_regs_gpr_offsets) - 1;
 
     return (void *)regs + cpu_user_regs_gpr_offsets[modrm];
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -13,7 +13,6 @@
 #include <asm/types.h>
 #include <asm/cpufeature.h>
 #include <asm/desc.h>
-#include <asm/x86_emulate.h>
 #endif
 
 #include <asm/x86-defns.h>





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

Re: [Xen-devel] [PATCH v2] x86emul: avoid speculative out of bounds accesses

[Andrew Cooper]

On 07/02/2019 11:42, Jan Beulich wrote:
> There are a few array accesses here the indexes of which are (at least
> indirectly) driven by the guest. Use array_access_nospec() to bound
> such accesses. In the {,_}decode_gpr() cases replace existing guarding
> constructs.
>
> To deal with an otherwise occurring #include cycle, drop the inclusion
> of asm/x86_emulate.h from asm/processor.h. This include had been
> introduced for obtaining the struct cpuid_leaf declaration, which has
> since moved into the x86 helper library.
>
> This is part of the speculative hardening effort.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Ping: [PATCH v2] x86emul: avoid speculative out of bounds accesses

[Jan Beulich]

>>> On 07.02.19 at 12:42,  wrote:
> There are a few array accesses here the indexes of which are (at least
> indirectly) driven by the guest. Use array_access_nospec() to bound
> such accesses. In the {,_}decode_gpr() cases replace existing guarding
> constructs.
> 
> To deal with an otherwise occurring #include cycle, drop the inclusion
> of asm/x86_emulate.h from asm/processor.h. This include had been
> introduced for obtaining the struct cpuid_leaf declaration, which has
> since moved into the x86 helper library.
> 
> This is part of the speculative hardening effort.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Instead of altering the code in {,_}decode_gpr() simply alter the
>     comment.

This is also meant to be a ping for the 3 other patches v1 of this
was sent with in a series.

Jan



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

[PATCH 2/4] x86/vMSI: avoid speculative out of bounds accesses

[Jan Beulich]

Array indexes used in the MMIO read/write emulation functions are
derived from guest controlled values. Restrict their ranges to limit the
side effects of speculative execution.

Note that the index into .msi_ad[] may also be out of bounds, by exactly
one (indexes 0...3 are possible while the array has just 3 elements).
This is not a problem with the current data layout, as such overrun of
the array would either touch the next element of the parent array or
(for the last entry of the parent array) access the subsequent acc_valid
bit array.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -29,6 +29,7 @@
 #include <xen/xmalloc.h>
 #include <xen/lib.h>
 #include <xen/errno.h>
+#include <xen/nospec.h>
 #include <xen/sched.h>
 #include <xen/softirq.h>
 #include <xen/irq.h>
@@ -231,8 +232,10 @@ static int msixtbl_read(const struct hvm
     {
         nr_entry = (address - entry->gtable) / PCI_MSIX_ENTRY_SIZE;
         index = offset / sizeof(uint32_t);
-        if ( nr_entry >= MAX_MSIX_ACC_ENTRIES ||
-             !acc_bit(test, entry, nr_entry, index) )
+        if ( nr_entry >= ARRAY_SIZE(entry->gentries) )
+            goto out;
+        nr_entry = array_index_nospec(nr_entry, ARRAY_SIZE(entry->gentries));
+        if ( !acc_bit(test, entry, nr_entry, index) )
             goto out;
         *pval = entry->gentries[nr_entry].msi_ad[index];
         if ( len == 8 )
@@ -284,14 +287,18 @@ static int msixtbl_write(struct vcpu *v,
     entry = msixtbl_find_entry(v, address);
     if ( !entry )
         goto out;
-    nr_entry = (address - entry->gtable) / PCI_MSIX_ENTRY_SIZE;
+    nr_entry = array_index_nospec(((address - entry->gtable) /
+                                   PCI_MSIX_ENTRY_SIZE),
+                                  MAX_MSIX_TABLE_ENTRIES);
 
     offset = address & (PCI_MSIX_ENTRY_SIZE - 1);
     if ( offset != PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET )
     {
         index = offset / sizeof(uint32_t);
-        if ( nr_entry < MAX_MSIX_ACC_ENTRIES ) 
+        if ( nr_entry < ARRAY_SIZE(entry->gentries) )
         {
+            nr_entry = array_index_nospec(nr_entry,
+                                          ARRAY_SIZE(entry->gentries));
             entry->gentries[nr_entry].msi_ad[index] = val;
             acc_bit(set, entry, nr_entry, index);
             if ( len == 8 && !index )





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

Re: [Xen-devel] [PATCH 2/4] x86/vMSI: avoid speculative out of bounds accesses

[Andrew Cooper]

On 31/01/2019 14:26, Jan Beulich wrote:
> Array indexes used in the MMIO read/write emulation functions are
> derived from guest controlled values. Restrict their ranges to limit the
> side effects of speculative execution.
>
> Note that the index into .msi_ad[] may also be out of bounds, by exactly

This would be a lot clearer if you explicitly said "may also be
speculatively out of bounds".

> one (indexes 0...3 are possible while the array has just 3 elements).
> This is not a problem with the current data layout, as such overrun of
> the array would either touch the next element of the parent array or
> (for the last entry of the parent array) access the subsequent acc_valid
> bit array.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[PATCH 3/4] x86/vPIC: avoid speculative out of bounds accesses

[Jan Beulich]

Array indexes used in the I/O port read/write emulation functions are
derived from guest controlled values. Where this is not already done,
restrict their ranges to limit the side effects of speculative execution.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vpic.c
+++ b/xen/arch/x86/hvm/vpic.c
@@ -335,7 +335,7 @@ static int vpic_intercept_pic_io(
         return X86EMUL_OKAY;
     }
 
-    vpic = &current->domain->arch.hvm.vpic[port >> 7];
+    vpic = &current->domain->arch.hvm.vpic[!!(port & 0x80)];
 
     if ( dir == IOREQ_WRITE )
         vpic_ioport_write(vpic, port, (uint8_t)*val);
@@ -448,7 +448,7 @@ void vpic_init(struct domain *d)
 
 void vpic_irq_positive_edge(struct domain *d, int irq)
 {
-    struct hvm_hw_vpic *vpic = &d->arch.hvm.vpic[irq >> 3];
+    struct hvm_hw_vpic *vpic = &d->arch.hvm.vpic[!!(irq & 8)];
     uint8_t mask = 1 << (irq & 7);
 
     ASSERT(has_vpic(d));
@@ -466,7 +466,7 @@ void vpic_irq_positive_edge(struct domai
 
 void vpic_irq_negative_edge(struct domain *d, int irq)
 {
-    struct hvm_hw_vpic *vpic = &d->arch.hvm.vpic[irq >> 3];
+    struct hvm_hw_vpic *vpic = &d->arch.hvm.vpic[!!(irq & 8)];
     uint8_t mask = 1 << (irq & 7);
 
     ASSERT(has_vpic(d));





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

Re: [Xen-devel] [PATCH 3/4] x86/vPIC: avoid speculative out of bounds accesses

[Andrew Cooper]

On 31/01/2019 14:26, Jan Beulich wrote:
> Array indexes used in the I/O port read/write emulation functions are
> derived from guest controlled values. Where this is not already done,
> restrict their ranges to limit the side effects of speculative execution.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Amazingly, this makes each of the functions shorter.  I was concerned
that the compiler could optimise away the !!, but these are all external
calls and the compiler can't actually prove the valid ranges of irq.

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

[PATCH 4/4] x86/vLAPIC: avoid speculative out of bounds accesses

[Jan Beulich]

Array indexes used in the MMIO and MSR read/write emulation functions
are derived from guest controlled values. Restrict their ranges to limit
the side effects of speculative execution.

Remove the unused vlapic_lvt_{vector,dm}() instead of adjusting them.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -23,6 +23,7 @@
 #include <xen/domain.h>
 #include <xen/domain_page.h>
 #include <xen/event.h>
+#include <xen/nospec.h>
 #include <xen/trace.h>
 #include <xen/lib.h>
 #include <xen/sched.h>
@@ -44,6 +45,8 @@
 #define VLAPIC_VERSION                  0x00050014
 #define VLAPIC_LVT_NUM                  6
 
+#define VLAPIC_OFFSET_MASK(vlapic) ((ARRAY_SIZE((vlapic)->regs->data) - 1) & ~0xf)
+
 #define LVT_MASK \
     (APIC_LVT_MASKED | APIC_SEND_PENDING | APIC_VECTOR_MASK)
 
@@ -65,12 +68,6 @@ static const unsigned int vlapic_lvt_mas
      LVT_MASK
 };
 
-#define vlapic_lvt_vector(vlapic, lvt_type)                     \
-    (vlapic_get_reg(vlapic, lvt_type) & APIC_VECTOR_MASK)
-
-#define vlapic_lvt_dm(vlapic, lvt_type)                         \
-    (vlapic_get_reg(vlapic, lvt_type) & APIC_MODE_MASK)
-
 #define vlapic_lvtt_period(vlapic)                              \
     ((vlapic_get_reg(vlapic, APIC_LVTT) & APIC_TIMER_MODE_MASK) \
      == APIC_TIMER_MODE_PERIODIC)
@@ -632,7 +629,8 @@ static int vlapic_mmio_read(struct vcpu
      */
     if ( (alignment + len) <= 4 && offset <= (APIC_TDCR + 3) )
     {
-        uint32_t reg = vlapic_read_aligned(vlapic, offset & ~0xf);
+        uint32_t reg = vlapic_read_aligned(vlapic,
+                                           offset & VLAPIC_OFFSET_MASK(vlapic));
 
         switch ( len )
         {
@@ -665,7 +663,7 @@ int guest_rdmsr_x2apic(const struct vcpu
     };
     const struct vlapic *vlapic = vcpu_vlapic(v);
     uint64_t high = 0;
-    uint32_t reg = msr - MSR_X2APIC_FIRST, offset = reg << 4;
+    uint32_t reg = msr - MSR_X2APIC_FIRST, offset;
 
     /*
      * The read side looks as if it might be safe to use outside of current
@@ -675,9 +673,14 @@ int guest_rdmsr_x2apic(const struct vcpu
     ASSERT(v == current);
 
     if ( !vlapic_x2apic_mode(vlapic) ||
-         (reg >= sizeof(readable) * 8) || !test_bit(reg, readable) )
+         (reg >= sizeof(readable) * 8) )
         return X86EMUL_EXCEPTION;
 
+    reg = array_index_nospec(reg, sizeof(readable) * 8);
+    if ( !test_bit(reg, readable) )
+        return X86EMUL_EXCEPTION;
+
+    offset = reg << 4;
     if ( offset == APIC_ICR )
         high = (uint64_t)vlapic_read_aligned(vlapic, APIC_ICR2) << 32;
 
@@ -856,8 +859,8 @@ void vlapic_reg_write(struct vcpu *v, un
     case APIC_LVTERR:       /* LVT Error Reg */
         if ( vlapic_sw_disabled(vlapic) )
             val |= APIC_LVT_MASKED;
-        val &= vlapic_lvt_mask[(reg - APIC_LVTT) >> 4];
-        vlapic_set_reg(vlapic, reg, val);
+        val &= array_access_nospec(vlapic_lvt_mask, (reg - APIC_LVTT) >> 4);
+        vlapic_set_reg(vlapic, reg & VLAPIC_OFFSET_MASK(vlapic), val);
         if ( reg == APIC_LVT0 )
         {
             vlapic_adjust_i8259_target(v->domain);
@@ -901,8 +904,6 @@ static int vlapic_mmio_write(struct vcpu
     unsigned int offset = address - vlapic_base_address(vlapic);
     unsigned int alignment = offset & 0xf;
 
-    offset &= ~0xf;
-
     if ( offset != APIC_EOI )
         HVM_DBG_LOG(DBG_LEVEL_VLAPIC,
                     "offset %#x with length %#x, and value is %#lx",
@@ -915,8 +916,10 @@ static int vlapic_mmio_write(struct vcpu
      * Some processors support smaller accesses, so we allow any access which
      * fully fits within the 32-bit register.
      */
-    if ( (alignment + len) <= 4 && offset <= APIC_TDCR )
+    if ( (alignment + len) <= 4 && offset <= APIC_TDCR + 3 )
     {
+        offset &= VLAPIC_OFFSET_MASK(vlapic);
+
         if ( unlikely(len < 4) )
         {
             uint32_t reg = vlapic_read_aligned(vlapic, offset);
@@ -946,7 +949,7 @@ static int vlapic_mmio_write(struct vcpu
 int vlapic_apicv_write(struct vcpu *v, unsigned int offset)
 {
     struct vlapic *vlapic = vcpu_vlapic(v);
-    uint32_t val = vlapic_get_reg(vlapic, offset);
+    uint32_t val = vlapic_get_reg(vlapic, offset & VLAPIC_OFFSET_MASK(vlapic));
 
     if ( vlapic_x2apic_mode(vlapic) )
     {




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

Re: [Xen-devel] [PATCH 4/4] x86/vLAPIC: avoid speculative out of bounds accesses

[Andrew Cooper]

On 31/01/2019 14:27, Jan Beulich wrote:
> Array indexes used in the MMIO and MSR read/write emulation functions
> are derived from guest controlled values. Restrict their ranges to limit
> the side effects of speculative execution.
>
> Remove the unused vlapic_lvt_{vector,dm}() instead of adjusting them.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

While they are all guest controlled, the MMIO side of things is on the
end of a function pointer call, which has already determined that the
access is within 4k.  I don't think there any safety concerns here.

guest_rdmsr_x2apic() does get values in the range 0x800...0xbff, so I
think this is the only case which needs protecting.

~Andrew

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

Re: [Xen-devel] [PATCH 4/4] x86/vLAPIC: avoid speculative out of bounds accesses

[Jan Beulich]

On 04.07.2019 15:44, Andrew Cooper wrote:
> On 31/01/2019 14:27, Jan Beulich wrote:
>> Array indexes used in the MMIO and MSR read/write emulation functions
>> are derived from guest controlled values. Restrict their ranges to limit
>> the side effects of speculative execution.
>>
>> Remove the unused vlapic_lvt_{vector,dm}() instead of adjusting them.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> While they are all guest controlled, the MMIO side of things is on the
> end of a function pointer call, which has already determined that the
> access is within 4k.  I don't think there any safety concerns here.

I.e. are you suggesting there's no speculation through indirect
calls?

> guest_rdmsr_x2apic() does get values in the range 0x800...0xbff, so I
> think this is the only case which needs protecting.

What about vlapic_apicv_write(), which does get called directly?

And what about the vlapic_lvt_mask[] accesses?

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

Ping: [PATCH 0/4] x86: further L1TF / XSA-289 guards

[Jan Beulich]

>>> On 31.01.19 at 15:07,  wrote:
> This goes alongside Norbert's series, dealing with a few more
> places where I happened to know (without any analysis tools)
> guest controlled array accesses sit. I've additionally also
> checked emul-i8254.c, and I think no adjustments are needed
> there (there are a few possible overruns by one, but just like
> is the case in patch 2 I don't think they are actual issues).
> 
> 1: x86emul: avoid speculative out of bounds accesses

There was a v2 of this sent separately.

Jan

> 2: x86/vMSI: avoid speculative out of bounds accesses
> 3: x86/vPIC: avoid speculative out of bounds accesses
> 4: x86/vLAPIC: avoid speculative out of bounds accesses
> 
> Jürgen, I've copied you anyway, but I assume your general
> Rab-until-RC3 would apply to this series (and perhaps to
> further ones, should anyone find time) as much as to Norbert's.
> 
> Jan
> 
> 




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

[Xen-devel] Ping: [PATCH 0/4] x86: further L1TF / XSA-289 guards

[Jan Beulich]

>>> On 31.01.19 at 15:07,  wrote:
> This goes alongside Norbert's series, dealing with a few more
> places where I happened to know (without any analysis tools)
> guest controlled array accesses sit. I've additionally also
> checked emul-i8254.c, and I think no adjustments are needed
> there (there are a few possible overruns by one, but just like
> is the case in patch 2 I don't think they are actual issues).
> 
> 1: x86emul: avoid speculative out of bounds accesses

There was a v2 of this sent separately.

Jan

> 2: x86/vMSI: avoid speculative out of bounds accesses
> 3: x86/vPIC: avoid speculative out of bounds accesses
> 4: x86/vLAPIC: avoid speculative out of bounds accesses
> 
> Jürgen, I've copied you anyway, but I assume your general
> Rab-until-RC3 would apply to this series (and perhaps to
> further ones, should anyone find time) as much as to Norbert's.
> 
> Jan
> 
> 




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