Re: [Xen-devel] [PATCH for-4.10] xen/dom0: Fix latent dom0 construction bugs on all architectures
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 CooperRelease-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
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 CooperReviewed-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
>>> 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
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
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 CooperARM 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
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
>>> 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
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
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
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 CooperLGTM, 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
>>> 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
>>> 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
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
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
Re: [Xen-devel] [PATCH for-4.10] xen/dom0: Fix latent dom0 construction bugs on all architectures
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