Re: [Xen-devel] [PATCH for-4.10] xen/dom0: Fix latent dom0 construction bugs on all architectures

[Julien Grall]

Hi Andrew,

On 16/10/17 15:38, Andrew Cooper wrote:

  * x86 PV and ARM dom0's must not clear _VPF_down from v->pause_flags until
all state is actually set up.  As it currently stands, d0v0 is eligible for
scheduling before its registers have been set.  This is latent as we also
hold a systemcontroller pause reference at the time which prevents d0 from
being scheduled.

  * x86 PVH dom0's must set v->is_initialised on d0v0, to prevent another vcpu
being able to call VCPUOP_initialise and modify state under the feet of the
running vcpu.  This is latent as PVH dom0 construction don't yet function.

Signed-off-by: Andrew Cooper 


Release-acked-by: Julien Grall 

Cheers,


---
CC: Jan Beulich 
CC: Stefano Stabellini 
CC: Julien Grall 
CC: Wei Liu 
CC: Roger Pau Monné 
---
  xen/arch/arm/domain_build.c   |  6 +++---
  xen/arch/x86/dom0_build.c | 13 +++--
  xen/arch/x86/hvm/dom0_build.c |  1 +
  xen/arch/x86/pv/dom0_build.c  |  6 +++---
  4 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 4636b17..bf29299 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2197,9 +2197,6 @@ int construct_dom0(struct domain *d)
  
  discard_initial_modules();
  
-v->is_initialised = 1;

-clear_bit(_VPF_down, >pause_flags);
-
  memset(regs, 0, sizeof(*regs));
  
  regs->pc = (register_t)kinfo.entry;

@@ -2247,6 +2244,9 @@ int construct_dom0(struct domain *d)
  vcpu_switch_to_aarch64_mode(d->vcpu[i]);
  }
  
+v->is_initialised = 1;

+clear_bit(_VPF_down, >pause_flags);
+
  return 0;
  }
  
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c

index e4bffd5..bf992fe 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -466,6 +466,8 @@ int __init construct_dom0(struct domain *d, const module_t 
*image,
void *(*bootstrap_map)(const module_t *),
char *cmdline)
  {
+int rc;
+
  /* Sanity! */
  BUG_ON(d->domain_id != 0);
  BUG_ON(d->vcpu[0] == NULL);
@@ -481,8 +483,15 @@ int __init construct_dom0(struct domain *d, const module_t 
*image,
  }
  #endif
  
-return (is_hvm_domain(d) ? dom0_construct_pvh : dom0_construct_pv)

-   (d, image, image_headroom, initrd,bootstrap_map, cmdline);
+rc = (is_hvm_domain(d) ? dom0_construct_pvh : dom0_construct_pv)
+ (d, image, image_headroom, initrd, bootstrap_map, cmdline);
+if ( rc )
+return rc;
+
+/* Sanity! */
+BUG_ON(!d->vcpu[0]->is_initialised);
+
+return 0;
  }
  
  /*

diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index e8f746c..a67071c 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -614,6 +614,7 @@ static int __init pvh_setup_cpus(struct domain *d, paddr_t 
entry,
  
  update_domain_wallclock_time(d);
  
+v->is_initialised = 1;

  clear_bit(_VPF_down, >pause_flags);
  
  return 0;

diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index dcbee43..8ad7e3d 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -847,9 +847,6 @@ int __init dom0_construct_pv(struct domain *d,
  
  update_domain_wallclock_time(d);
  
-v->is_initialised = 1;

-clear_bit(_VPF_down, >pause_flags);
-
  /*
   * Initial register values:
   *  DS,ES,FS,GS = FLAT_KERNEL_DS
@@ -883,6 +880,9 @@ int __init dom0_construct_pv(struct domain *d,
  if ( d->domain_id == hardware_domid )
  iommu_hwdom_init(d);
  
+v->is_initialised = 1;

+clear_bit(_VPF_down, >pause_flags);
+
  return 0;
  
  out:




--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.10] xen/dom0: Fix latent dom0 construction bugs on all architectures

[Roger Pau Monné]

On Mon, Oct 16, 2017 at 03:38:03PM +0100, Andrew Cooper wrote:
>  * x86 PV and ARM dom0's must not clear _VPF_down from v->pause_flags until
>all state is actually set up.  As it currently stands, d0v0 is eligible for
>scheduling before its registers have been set.  This is latent as we also
>hold a systemcontroller pause reference at the time which prevents d0 from
>being scheduled.
> 
>  * x86 PVH dom0's must set v->is_initialised on d0v0, to prevent another vcpu
>being able to call VCPUOP_initialise and modify state under the feet of the
>running vcpu.  This is latent as PVH dom0 construction don't yet function.
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Roger Pau Monné 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.10] xen/dom0: Fix latent dom0 construction bugs on all architectures

[Jan Beulich]

>>> On 17.10.17 at 12:38,  wrote:
> On 16/10/17 17:21, Jan Beulich wrote:
> On 16.10.17 at 18:07,  wrote:
>>> On 16/10/17 16:41, Jan Beulich wrote:
  >>> On 16.10.17 at 16:38,  wrote:
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -614,6 +614,7 @@ static int __init pvh_setup_cpus(struct domain *d, 
> paddr_t entry,
>  
>  update_domain_wallclock_time(d);
>  
> +v->is_initialised = 1;
>  clear_bit(_VPF_down, >pause_flags);
 How come this has no counterpart of code being deleted?
>>> Because the bug is that it was never being set before.
>> Oh, I see - I had read that part of the commit message in slightly
>> a wrong way.
>>
>> Reviewed-by: Jan Beulich 
> 
> How about this for an adjusted commit message?
> 
>  * x86 PVH previously was not setting v->is_initialised for d0v0, despite
>setting the vcpu running eventually.  Therefore, a later VCPUOP_initialise
>hypercall will modify state under the feet of the running vcpu.  This is
>latent as PVH dom0 construction don't yet function.

Yes, thanks.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.10] xen/dom0: Fix latent dom0 construction bugs on all architectures

[Andrew Cooper]

On 16/10/17 17:21, Jan Beulich wrote:
 On 16.10.17 at 18:07,  wrote:
>> On 16/10/17 16:41, Jan Beulich wrote:
>>>  >>> On 16.10.17 at 16:38,  wrote:
 --- a/xen/arch/x86/hvm/dom0_build.c
 +++ b/xen/arch/x86/hvm/dom0_build.c
 @@ -614,6 +614,7 @@ static int __init pvh_setup_cpus(struct domain *d, 
 paddr_t entry,
  
  update_domain_wallclock_time(d);
  
 +v->is_initialised = 1;
  clear_bit(_VPF_down, >pause_flags);
>>> How come this has no counterpart of code being deleted?
>> Because the bug is that it was never being set before.
> Oh, I see - I had read that part of the commit message in slightly
> a wrong way.
>
> Reviewed-by: Jan Beulich 

How about this for an adjusted commit message?

 * x86 PVH previously was not setting v->is_initialised for d0v0, despite
   setting the vcpu running eventually.  Therefore, a later VCPUOP_initialise
   hypercall will modify state under the feet of the running vcpu.  This is
   latent as PVH dom0 construction don't yet function.


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.10] xen/dom0: Fix latent dom0 construction bugs on all architectures

[Stefano Stabellini]

On Mon, 16 Oct 2017, Andrew Cooper wrote:
>  * x86 PV and ARM dom0's must not clear _VPF_down from v->pause_flags until
>all state is actually set up.  As it currently stands, d0v0 is eligible for
>scheduling before its registers have been set.  This is latent as we also
>hold a systemcontroller pause reference at the time which prevents d0 from
>being scheduled.
> 
>  * x86 PVH dom0's must set v->is_initialised on d0v0, to prevent another vcpu
>being able to call VCPUOP_initialise and modify state under the feet of the
>running vcpu.  This is latent as PVH dom0 construction don't yet function.
> 
> Signed-off-by: Andrew Cooper 

ARM bits:

Reviewed-by: Stefano Stabellini 


> ---
> CC: Jan Beulich 
> CC: Stefano Stabellini 
> CC: Julien Grall 
> CC: Wei Liu 
> CC: Roger Pau Monné 
> ---
>  xen/arch/arm/domain_build.c   |  6 +++---
>  xen/arch/x86/dom0_build.c | 13 +++--
>  xen/arch/x86/hvm/dom0_build.c |  1 +
>  xen/arch/x86/pv/dom0_build.c  |  6 +++---
>  4 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 4636b17..bf29299 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2197,9 +2197,6 @@ int construct_dom0(struct domain *d)
>  
>  discard_initial_modules();
>  
> -v->is_initialised = 1;
> -clear_bit(_VPF_down, >pause_flags);
> -
>  memset(regs, 0, sizeof(*regs));
>  
>  regs->pc = (register_t)kinfo.entry;
> @@ -2247,6 +2244,9 @@ int construct_dom0(struct domain *d)
>  vcpu_switch_to_aarch64_mode(d->vcpu[i]);
>  }
>  
> +v->is_initialised = 1;
> +clear_bit(_VPF_down, >pause_flags);
> +
>  return 0;
>  }
>  
> diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
> index e4bffd5..bf992fe 100644
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -466,6 +466,8 @@ int __init construct_dom0(struct domain *d, const 
> module_t *image,
>void *(*bootstrap_map)(const module_t *),
>char *cmdline)
>  {
> +int rc;
> +
>  /* Sanity! */
>  BUG_ON(d->domain_id != 0);
>  BUG_ON(d->vcpu[0] == NULL);
> @@ -481,8 +483,15 @@ int __init construct_dom0(struct domain *d, const 
> module_t *image,
>  }
>  #endif
>  
> -return (is_hvm_domain(d) ? dom0_construct_pvh : dom0_construct_pv)
> -   (d, image, image_headroom, initrd,bootstrap_map, cmdline);
> +rc = (is_hvm_domain(d) ? dom0_construct_pvh : dom0_construct_pv)
> + (d, image, image_headroom, initrd, bootstrap_map, cmdline);
> +if ( rc )
> +return rc;
> +
> +/* Sanity! */
> +BUG_ON(!d->vcpu[0]->is_initialised);
> +
> +return 0;
>  }
>  
>  /*
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index e8f746c..a67071c 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -614,6 +614,7 @@ static int __init pvh_setup_cpus(struct domain *d, 
> paddr_t entry,
>  
>  update_domain_wallclock_time(d);
>  
> +v->is_initialised = 1;
>  clear_bit(_VPF_down, >pause_flags);
>  
>  return 0;
> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index dcbee43..8ad7e3d 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -847,9 +847,6 @@ int __init dom0_construct_pv(struct domain *d,
>  
>  update_domain_wallclock_time(d);
>  
> -v->is_initialised = 1;
> -clear_bit(_VPF_down, >pause_flags);
> -
>  /*
>   * Initial register values:
>   *  DS,ES,FS,GS = FLAT_KERNEL_DS
> @@ -883,6 +880,9 @@ int __init dom0_construct_pv(struct domain *d,
>  if ( d->domain_id == hardware_domid )
>  iommu_hwdom_init(d);
>  
> +v->is_initialised = 1;
> +clear_bit(_VPF_down, >pause_flags);
> +
>  return 0;
>  
>  out:
> -- 
> 2.1.4
> ___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.10] xen/dom0: Fix latent dom0 construction bugs on all architectures

[Andrew Cooper]

On 16/10/17 16:51, Roger Pau Monné wrote:
> On Mon, Oct 16, 2017 at 03:38:03PM +0100, Andrew Cooper wrote:
>>  * x86 PV and ARM dom0's must not clear _VPF_down from v->pause_flags until
>>all state is actually set up.  As it currently stands, d0v0 is eligible 
>> for
>>scheduling before its registers have been set.  This is latent as we also
>>hold a systemcontroller pause reference at the time which prevents d0 from
>>being scheduled.
>>
>>  * x86 PVH dom0's must set v->is_initialised on d0v0, to prevent another vcpu
>>being able to call VCPUOP_initialise and modify state under the feet of 
>> the
>>running vcpu.  This is latent as PVH dom0 construction don't yet function.
>>
>> Signed-off-by: Andrew Cooper 
> LGTM, just one question.
>
>> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
>> index e8f746c..a67071c 100644
>> --- a/xen/arch/x86/hvm/dom0_build.c
>> +++ b/xen/arch/x86/hvm/dom0_build.c
>> @@ -614,6 +614,7 @@ static int __init pvh_setup_cpus(struct domain *d, 
>> paddr_t entry,
>>  
>>  update_domain_wallclock_time(d);
>>  
>> +v->is_initialised = 1;
>>  clear_bit(_VPF_down, >pause_flags);
> Don't you want to move this to the end of dom0_construct_pvh? In any
> case, at this point the vCPU state is already setup correctly.

I had considered that, but a) As you say, it only matters when the vcpu
state is set up, and b) it would look odd being anywhere later.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.10] xen/dom0: Fix latent dom0 construction bugs on all architectures

[Jan Beulich]

>>> On 16.10.17 at 18:07,  wrote:
> On 16/10/17 16:41, Jan Beulich wrote:
>>  >>> On 16.10.17 at 16:38,  wrote:
>>> --- a/xen/arch/x86/hvm/dom0_build.c
>>> +++ b/xen/arch/x86/hvm/dom0_build.c
>>> @@ -614,6 +614,7 @@ static int __init pvh_setup_cpus(struct domain *d, 
>>> paddr_t entry,
>>>  
>>>  update_domain_wallclock_time(d);
>>>  
>>> +v->is_initialised = 1;
>>>  clear_bit(_VPF_down, >pause_flags);
>> How come this has no counterpart of code being deleted?
> 
> Because the bug is that it was never being set before.

Oh, I see - I had read that part of the commit message in slightly
a wrong way.

Reviewed-by: Jan Beulich 

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.10] xen/dom0: Fix latent dom0 construction bugs on all architectures

[Andrew Cooper]

On 16/10/17 16:39, Jan Beulich wrote:
 On 16.10.17 at 16:49,  wrote:
>> On 16/10/17 15:44, Wei Liu wrote:
>>> On Mon, Oct 16, 2017 at 03:38:03PM +0100, Andrew Cooper wrote:
  * x86 PV and ARM dom0's must not clear _VPF_down from v->pause_flags until
all state is actually set up.  As it currently stands, d0v0 is eligible 
>> for
scheduling before its registers have been set.  This is latent as we 
 also
hold a systemcontroller pause reference at the time which prevents d0 
>> from
being scheduled.

  * x86 PVH dom0's must set v->is_initialised on d0v0, to prevent another 
 vcpu
being able to call VCPUOP_initialise and modify state under the feet of 
>> the
running vcpu.  This is latent as PVH dom0 construction don't yet 
>> function.
>>> While I think this patch is a good idea, the above paragraph confuses
>>> me: I did boot PVH Dom0 at one point so it did function; I also never
>>> triggered a bug like the one described here.
>> Strictly speaking, this is the PVH v2 dom0 path, not the legacy PVH dom0
>> path.
>>
>> The bottom of dom0_construct_pvh() currently has:
>>
>> ...
>> panic("Building a PVHv2 Dom0 is not yet supported.");
>> return 0;
>> }
>>
>> As for the v->is_initialised, a well behaved dom0 wouldn't hit the
>> issue, because it wouldn't call VCPUOP_initialise against a running
>> vcpu.  Nevertheless, it is relevant to Xen's security that such an
>> attempt doesn't get to the point of actually trying to edit the VMC{S,B}
>> under a running vcpu.
> I don't understand this reply of yours: The changes you make
> are for vCPU 0 only, and even there only for its initial state.

Correct.

> Even if Dom0 decided to bring down and back up that vCPU, it
> would go through a different path.

Correct as well, but unrelated to the bug.

The bug is that, at the moment, d0v1 can make a blind VCPUOP_initialise
call targeting d0v0, while d0v0 is running, and it will go and rewrite
state.  The problem is that d0v0 starts running in a way that
VCPUOP_initialise believes it to be eligible for initialisation.

All other mechanisms of bringing a vcpu down and up cause there to be
proper isolation between playing with a vcpus state, and it being
unscheduled.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.10] xen/dom0: Fix latent dom0 construction bugs on all architectures

[Andrew Cooper]

On 16/10/17 16:41, Jan Beulich wrote:
>  >>> On 16.10.17 at 16:38,  wrote:
>> --- a/xen/arch/x86/hvm/dom0_build.c
>> +++ b/xen/arch/x86/hvm/dom0_build.c
>> @@ -614,6 +614,7 @@ static int __init pvh_setup_cpus(struct domain *d, 
>> paddr_t entry,
>>  
>>  update_domain_wallclock_time(d);
>>  
>> +v->is_initialised = 1;
>>  clear_bit(_VPF_down, >pause_flags);
> How come this has no counterpart of code being deleted?

Because the bug is that it was never being set before.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.10] xen/dom0: Fix latent dom0 construction bugs on all architectures

[Roger Pau Monné]

On Mon, Oct 16, 2017 at 03:38:03PM +0100, Andrew Cooper wrote:
>  * x86 PV and ARM dom0's must not clear _VPF_down from v->pause_flags until
>all state is actually set up.  As it currently stands, d0v0 is eligible for
>scheduling before its registers have been set.  This is latent as we also
>hold a systemcontroller pause reference at the time which prevents d0 from
>being scheduled.
> 
>  * x86 PVH dom0's must set v->is_initialised on d0v0, to prevent another vcpu
>being able to call VCPUOP_initialise and modify state under the feet of the
>running vcpu.  This is latent as PVH dom0 construction don't yet function.
> 
> Signed-off-by: Andrew Cooper 

LGTM, just one question.

> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index e8f746c..a67071c 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -614,6 +614,7 @@ static int __init pvh_setup_cpus(struct domain *d, 
> paddr_t entry,
>  
>  update_domain_wallclock_time(d);
>  
> +v->is_initialised = 1;
>  clear_bit(_VPF_down, >pause_flags);

Don't you want to move this to the end of dom0_construct_pvh? In any
case, at this point the vCPU state is already setup correctly.

Thanks, Roger.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.10] xen/dom0: Fix latent dom0 construction bugs on all architectures

[Jan Beulich]

 >>> On 16.10.17 at 16:38,  wrote:
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -614,6 +614,7 @@ static int __init pvh_setup_cpus(struct domain *d, 
> paddr_t entry,
>  
>  update_domain_wallclock_time(d);
>  
> +v->is_initialised = 1;
>  clear_bit(_VPF_down, >pause_flags);

How come this has no counterpart of code being deleted?

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.10] xen/dom0: Fix latent dom0 construction bugs on all architectures

[Jan Beulich]

>>> On 16.10.17 at 16:49,  wrote:
> On 16/10/17 15:44, Wei Liu wrote:
>> On Mon, Oct 16, 2017 at 03:38:03PM +0100, Andrew Cooper wrote:
>>>  * x86 PV and ARM dom0's must not clear _VPF_down from v->pause_flags until
>>>all state is actually set up.  As it currently stands, d0v0 is eligible 
> for
>>>scheduling before its registers have been set.  This is latent as we also
>>>hold a systemcontroller pause reference at the time which prevents d0 
> from
>>>being scheduled.
>>>
>>>  * x86 PVH dom0's must set v->is_initialised on d0v0, to prevent another 
>>> vcpu
>>>being able to call VCPUOP_initialise and modify state under the feet of 
> the
>>>running vcpu.  This is latent as PVH dom0 construction don't yet 
> function.
>>>
>> While I think this patch is a good idea, the above paragraph confuses
>> me: I did boot PVH Dom0 at one point so it did function; I also never
>> triggered a bug like the one described here.
> 
> Strictly speaking, this is the PVH v2 dom0 path, not the legacy PVH dom0
> path.
> 
> The bottom of dom0_construct_pvh() currently has:
> 
> ...
> panic("Building a PVHv2 Dom0 is not yet supported.");
> return 0;
> }
> 
> As for the v->is_initialised, a well behaved dom0 wouldn't hit the
> issue, because it wouldn't call VCPUOP_initialise against a running
> vcpu.  Nevertheless, it is relevant to Xen's security that such an
> attempt doesn't get to the point of actually trying to edit the VMC{S,B}
> under a running vcpu.

I don't understand this reply of yours: The changes you make
are for vCPU 0 only, and even there only for its initial state.
Even if Dom0 decided to bring down and back up that vCPU, it
would go through a different path.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.10] xen/dom0: Fix latent dom0 construction bugs on all architectures

[Wei Liu]

On Mon, Oct 16, 2017 at 03:49:54PM +0100, Andrew Cooper wrote:
> On 16/10/17 15:44, Wei Liu wrote:
> > On Mon, Oct 16, 2017 at 03:38:03PM +0100, Andrew Cooper wrote:
> >>  * x86 PV and ARM dom0's must not clear _VPF_down from v->pause_flags until
> >>all state is actually set up.  As it currently stands, d0v0 is eligible 
> >> for
> >>scheduling before its registers have been set.  This is latent as we 
> >> also
> >>hold a systemcontroller pause reference at the time which prevents d0 
> >> from
> >>being scheduled.
> >>
> >>  * x86 PVH dom0's must set v->is_initialised on d0v0, to prevent another 
> >> vcpu
> >>being able to call VCPUOP_initialise and modify state under the feet of 
> >> the
> >>running vcpu.  This is latent as PVH dom0 construction don't yet 
> >> function.
> >>
> > While I think this patch is a good idea, the above paragraph confuses
> > me: I did boot PVH Dom0 at one point so it did function; I also never
> > triggered a bug like the one described here.
> 
> Strictly speaking, this is the PVH v2 dom0 path, not the legacy PVH dom0
> path.
> 
> The bottom of dom0_construct_pvh() currently has:
> 
> ...
>     panic("Building a PVHv2 Dom0 is not yet supported.");
>     return 0;
> }
> 

Oh yes, I was using a development branch.

> As for the v->is_initialised, a well behaved dom0 wouldn't hit the
> issue, because it wouldn't call VCPUOP_initialise against a running
> vcpu.  Nevertheless, it is relevant to Xen's security that such an
> attempt doesn't get to the point of actually trying to edit the VMC{S,B}
> under a running vcpu.
> 

Right.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.10] xen/dom0: Fix latent dom0 construction bugs on all architectures

[Wei Liu]

On Mon, Oct 16, 2017 at 03:38:03PM +0100, Andrew Cooper wrote:
>  * x86 PV and ARM dom0's must not clear _VPF_down from v->pause_flags until
>all state is actually set up.  As it currently stands, d0v0 is eligible for
>scheduling before its registers have been set.  This is latent as we also
>hold a systemcontroller pause reference at the time which prevents d0 from
>being scheduled.
> 
>  * x86 PVH dom0's must set v->is_initialised on d0v0, to prevent another vcpu
>being able to call VCPUOP_initialise and modify state under the feet of the
>running vcpu.  This is latent as PVH dom0 construction don't yet function.
> 

While I think this patch is a good idea, the above paragraph confuses
me: I did boot PVH Dom0 at one point so it did function; I also never
triggered a bug like the one described here.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH for-4.10] xen/dom0: Fix latent dom0 construction bugs on all architectures

[Andrew Cooper]

 * x86 PV and ARM dom0's must not clear _VPF_down from v->pause_flags until
   all state is actually set up.  As it currently stands, d0v0 is eligible for
   scheduling before its registers have been set.  This is latent as we also
   hold a systemcontroller pause reference at the time which prevents d0 from
   being scheduled.

 * x86 PVH dom0's must set v->is_initialised on d0v0, to prevent another vcpu
   being able to call VCPUOP_initialise and modify state under the feet of the
   running vcpu.  This is latent as PVH dom0 construction don't yet function.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Stefano Stabellini 
CC: Julien Grall 
CC: Wei Liu 
CC: Roger Pau Monné 
---
 xen/arch/arm/domain_build.c   |  6 +++---
 xen/arch/x86/dom0_build.c | 13 +++--
 xen/arch/x86/hvm/dom0_build.c |  1 +
 xen/arch/x86/pv/dom0_build.c  |  6 +++---
 4 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 4636b17..bf29299 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2197,9 +2197,6 @@ int construct_dom0(struct domain *d)
 
 discard_initial_modules();
 
-v->is_initialised = 1;
-clear_bit(_VPF_down, >pause_flags);
-
 memset(regs, 0, sizeof(*regs));
 
 regs->pc = (register_t)kinfo.entry;
@@ -2247,6 +2244,9 @@ int construct_dom0(struct domain *d)
 vcpu_switch_to_aarch64_mode(d->vcpu[i]);
 }
 
+v->is_initialised = 1;
+clear_bit(_VPF_down, >pause_flags);
+
 return 0;
 }
 
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index e4bffd5..bf992fe 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -466,6 +466,8 @@ int __init construct_dom0(struct domain *d, const module_t 
*image,
   void *(*bootstrap_map)(const module_t *),
   char *cmdline)
 {
+int rc;
+
 /* Sanity! */
 BUG_ON(d->domain_id != 0);
 BUG_ON(d->vcpu[0] == NULL);
@@ -481,8 +483,15 @@ int __init construct_dom0(struct domain *d, const module_t 
*image,
 }
 #endif
 
-return (is_hvm_domain(d) ? dom0_construct_pvh : dom0_construct_pv)
-   (d, image, image_headroom, initrd,bootstrap_map, cmdline);
+rc = (is_hvm_domain(d) ? dom0_construct_pvh : dom0_construct_pv)
+ (d, image, image_headroom, initrd, bootstrap_map, cmdline);
+if ( rc )
+return rc;
+
+/* Sanity! */
+BUG_ON(!d->vcpu[0]->is_initialised);
+
+return 0;
 }
 
 /*
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index e8f746c..a67071c 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -614,6 +614,7 @@ static int __init pvh_setup_cpus(struct domain *d, paddr_t 
entry,
 
 update_domain_wallclock_time(d);
 
+v->is_initialised = 1;
 clear_bit(_VPF_down, >pause_flags);
 
 return 0;
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index dcbee43..8ad7e3d 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -847,9 +847,6 @@ int __init dom0_construct_pv(struct domain *d,
 
 update_domain_wallclock_time(d);
 
-v->is_initialised = 1;
-clear_bit(_VPF_down, >pause_flags);
-
 /*
  * Initial register values:
  *  DS,ES,FS,GS = FLAT_KERNEL_DS
@@ -883,6 +880,9 @@ int __init dom0_construct_pv(struct domain *d,
 if ( d->domain_id == hardware_domid )
 iommu_hwdom_init(d);
 
+v->is_initialised = 1;
+clear_bit(_VPF_down, >pause_flags);
+
 return 0;
 
 out:
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.10] xen/dom0: Fix latent dom0 construction bugs on all architectures

[Andrew Cooper]

On 16/10/17 15:44, Wei Liu wrote:
> On Mon, Oct 16, 2017 at 03:38:03PM +0100, Andrew Cooper wrote:
>>  * x86 PV and ARM dom0's must not clear _VPF_down from v->pause_flags until
>>all state is actually set up.  As it currently stands, d0v0 is eligible 
>> for
>>scheduling before its registers have been set.  This is latent as we also
>>hold a systemcontroller pause reference at the time which prevents d0 from
>>being scheduled.
>>
>>  * x86 PVH dom0's must set v->is_initialised on d0v0, to prevent another vcpu
>>being able to call VCPUOP_initialise and modify state under the feet of 
>> the
>>running vcpu.  This is latent as PVH dom0 construction don't yet function.
>>
> While I think this patch is a good idea, the above paragraph confuses
> me: I did boot PVH Dom0 at one point so it did function; I also never
> triggered a bug like the one described here.

Strictly speaking, this is the PVH v2 dom0 path, not the legacy PVH dom0
path.

The bottom of dom0_construct_pvh() currently has:

...
    panic("Building a PVHv2 Dom0 is not yet supported.");
    return 0;
}

As for the v->is_initialised, a well behaved dom0 wouldn't hit the
issue, because it wouldn't call VCPUOP_initialise against a running
vcpu.  Nevertheless, it is relevant to Xen's security that such an
attempt doesn't get to the point of actually trying to edit the VMC{S,B}
under a running vcpu.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel