diff mbox

[v3] x86/HVM: add padding to struct hvm_hw_cpu

Message ID 1453134353-79201-1-git-send-email-roger.pau@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Roger Pau Monne Jan. 18, 2016, 4:25 p.m. UTC 
So that the size of the structure is the same on 32 and 64bit.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
This should fix the issues seen on OSSTest when using a 32bit toolstack on
a 64bit hypervisor to create a Windows 7 HVM guest.
---
Chnges since v2:
 - Fall back to adding a padding field and properly checking for it to be 0.

Changes since v1:
 - Instead of adding padding, change the flags field to be a uint64_t.
---
 xen/arch/x86/hvm/hvm.c                 | 3 +++
 xen/include/public/arch-x86/hvm/save.h | 1 +
 2 files changed, 4 insertions(+)

Comments

Jan Beulich Jan. 19, 2016, 3:13 p.m. UTC | #1 
>>> On 18.01.16 at 17:25, <roger.pau@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1990,6 +1990,9 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
>      if ( hvm_load_entry_zeroextend(CPU, h, &ctxt) != 0 )
>          return -EINVAL;
>  
> +    if ( ctxt.pad0 != 0 )
> +        return -EINVAL;

Right after I had committed and pushed the patch it occurred to
me that this seems to be missing a save side counterpart, which
would constitute both an information leak and a functional bug.
Would you please take another look?

Jan
Roger Pau Monne Jan. 19, 2016, 3:21 p.m. UTC | #2 
El 19/01/16 a les 16.13, Jan Beulich ha escrit:
>>>> On 18.01.16 at 17:25, <roger.pau@citrix.com> wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -1990,6 +1990,9 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
>>      if ( hvm_load_entry_zeroextend(CPU, h, &ctxt) != 0 )
>>          return -EINVAL;
>>  
>> +    if ( ctxt.pad0 != 0 )
>> +        return -EINVAL;
> 
> Right after I had committed and pushed the patch it occurred to
> me that this seems to be missing a save side counterpart, which
> would constitute both an information leak and a functional bug.
> Would you please take another look?

Sure, thanks for realising! Now that you make me look at it
hvm_save_cpu_ctxt should zero hvm_hw_cpu on each iteration, IMHO the
current code is asking for trouble. I will send a patch ASAP.

Roger.
diff mbox

Patch

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index a99edc2..cc5d14b 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1990,6 +1990,9 @@  static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
     if ( hvm_load_entry_zeroextend(CPU, h, &ctxt) != 0 )
         return -EINVAL;
 
+    if ( ctxt.pad0 != 0 )
+        return -EINVAL;
+
     /* Sanity check some control registers. */
     if ( (ctxt.cr0 & HVM_CR0_GUEST_RESERVED_BITS) ||
          !(ctxt.cr0 & X86_CR0_ET) ||
diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h
index b6b1bf8..6862720 100644
--- a/xen/include/public/arch-x86/hvm/save.h
+++ b/xen/include/public/arch-x86/hvm/save.h
@@ -163,6 +163,7 @@  struct hvm_hw_cpu {
 #define _XEN_X86_FPU_INITIALISED        0
 #define XEN_X86_FPU_INITIALISED         (1U<<_XEN_X86_FPU_INITIALISED)
     uint32_t flags;
+    uint32_t pad0;
 };
 
 struct hvm_hw_cpu_compat {