[PATCH][4.17?] x86: also zap secondary time area handles during soft reset

[PATCH][4.17?] x86: also zap secondary time area handles during soft reset

[Jan Beulich]

Just like domain_soft_reset() properly zaps runstate area handles, the
secondary time area ones also need discarding to prevent guest memory
corruption once the guest is re-started.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
To avoid another for_each_vcpu() here, domain_soft_reset() could also
be made call a new arch_vcpu_soft_reset() out of its already present
loop. Yet that would make the change less isolated.

In domain_soft_reset() I wonder whether, just like done here, the
zapping of runstate area handles and vCPU info mappings wouldn't better
be done after all operations which can fail. But perhaps for this to
matter the domain is left in too inconsistent a state anyway if the
function fails ... However, at the very least I wonder whether x86'es
restriction to HVM shouldn't leave PV guests undisturbed if a soft-reset
was attempted on them. Right now they not only have state partially
clobbered, but (if the arch function is reached) they would be crashed
unconditionally.

--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -951,6 +951,7 @@ int arch_domain_soft_reset(struct domain
     struct page_info *page = virt_to_page(d->shared_info), *new_page;
     int ret = 0;
     struct domain *owner;
+    struct vcpu *v;
     mfn_t mfn;
     gfn_t gfn;
     p2m_type_t p2mt;
@@ -1030,7 +1031,12 @@ int arch_domain_soft_reset(struct domain
                "Failed to add a page to replace %pd's shared_info frame %"PRI_gfn"\n",
                d, gfn_x(gfn));
         free_domheap_page(new_page);
+        goto exit_put_gfn;
     }
+
+    for_each_vcpu ( d, v )
+        set_xen_guest_handle(v->arch.time_info_guest, NULL);
+
  exit_put_gfn:
     put_gfn(d, gfn_x(gfn));
  exit_put_page:

Re: [PATCH][4.17?] x86: also zap secondary time area handles during soft reset

[Roger Pau Monné]

On Thu, Oct 13, 2022 at 08:48:21AM +0200, Jan Beulich wrote:
> Just like domain_soft_reset() properly zaps runstate area handles, the
> secondary time area ones also need discarding to prevent guest memory
> corruption once the guest is re-started.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> To avoid another for_each_vcpu() here, domain_soft_reset() could also
> be made call a new arch_vcpu_soft_reset() out of its already present
> loop. Yet that would make the change less isolated.
> 
> In domain_soft_reset() I wonder whether, just like done here, the
> zapping of runstate area handles and vCPU info mappings wouldn't better
> be done after all operations which can fail. But perhaps for this to
> matter the domain is left in too inconsistent a state anyway if the
> function fails ...

We would need some kind of recovery anyway, so given the current code
and lack of recovery it doesn't seem to matter much.

> However, at the very least I wonder whether x86'es
> restriction to HVM shouldn't leave PV guests undisturbed if a soft-reset
> was attempted on them. Right now they not only have state partially
> clobbered, but (if the arch function is reached) they would be crashed
> unconditionally.

It's a toolstack initiated operation by a domctl, so I'm fine with
saying that it's up for the toolstack to prevent soft resets from
being attempted against PV domains.  Would be nice to reject the
operation earlier on the hypervisor, maybe by moving
arch_domain_soft_reset() earlier in domain_soft_reset() so that we
can return without crashing?

In any case it's unlikely for a domain that was attempting a soft
reset to survive the hypervisor rejecting the operation, so it doesn't
matter much whether the domain is crashed by Xen or left as-is I would
think.

Thanks, Roger.

Re: [PATCH][4.17?] x86: also zap secondary time area handles during soft reset

[Jan Beulich]

On 25.10.2022 17:23, Roger Pau Monné wrote:
> On Thu, Oct 13, 2022 at 08:48:21AM +0200, Jan Beulich wrote:
>> Just like domain_soft_reset() properly zaps runstate area handles, the
>> secondary time area ones also need discarding to prevent guest memory
>> corruption once the guest is re-started.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> ---
>> To avoid another for_each_vcpu() here, domain_soft_reset() could also
>> be made call a new arch_vcpu_soft_reset() out of its already present
>> loop. Yet that would make the change less isolated.
>>
>> In domain_soft_reset() I wonder whether, just like done here, the
>> zapping of runstate area handles and vCPU info mappings wouldn't better
>> be done after all operations which can fail. But perhaps for this to
>> matter the domain is left in too inconsistent a state anyway if the
>> function fails ...
> 
> We would need some kind of recovery anyway, so given the current code
> and lack of recovery it doesn't seem to matter much.
> 
>> However, at the very least I wonder whether x86'es
>> restriction to HVM shouldn't leave PV guests undisturbed if a soft-reset
>> was attempted on them. Right now they not only have state partially
>> clobbered, but (if the arch function is reached) they would be crashed
>> unconditionally.
> 
> It's a toolstack initiated operation by a domctl, so I'm fine with
> saying that it's up for the toolstack to prevent soft resets from
> being attempted against PV domains.  Would be nice to reject the
> operation earlier on the hypervisor, maybe by moving
> arch_domain_soft_reset() earlier in domain_soft_reset() so that we
> can return without crashing?

I wasn't sure about moving arch_domain_soft_reset() as a whole, but
yes, if that wouldn't cause other interaction issues this might be
an option.

> In any case it's unlikely for a domain that was attempting a soft
> reset to survive the hypervisor rejecting the operation, so it doesn't
> matter much whether the domain is crashed by Xen or left as-is I would
> think.

I'm sorry, I don't think I understand what you're saying here. For
PV I'd very much expect the guest to survive; it may of course then
be crashed or destroyed by a further tool stack operation.

Jan

Re: [PATCH][4.17?] x86: also zap secondary time area handles during soft reset

[Roger Pau Monné]

On Tue, Oct 25, 2022 at 05:58:10PM +0200, Jan Beulich wrote:
> On 25.10.2022 17:23, Roger Pau Monné wrote:
> > On Thu, Oct 13, 2022 at 08:48:21AM +0200, Jan Beulich wrote:
> I wasn't sure about moving arch_domain_soft_reset() as a whole, but
> yes, if that wouldn't cause other interaction issues this might be
> an option.
> 
> > In any case it's unlikely for a domain that was attempting a soft
> > reset to survive the hypervisor rejecting the operation, so it doesn't
> > matter much whether the domain is crashed by Xen or left as-is I would
> > think.
> 
> I'm sorry, I don't think I understand what you're saying here. For
> PV I'd very much expect the guest to survive; it may of course then
> be crashed or destroyed by a further tool stack operation.

I was expecting a domain that goes to the length of preparing for a
soft reset operation to either perform such soft reset, or die as a
result (and perform a non-soft reset), as recovering into the previous
state won't be feasible.  But maybe I'm wrong.

Thanks, Roger.

RE: [PATCH][4.17?] x86: also zap secondary time area handles during soft reset

[Henry Wang]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Subject: [PATCH][4.17?] x86: also zap secondary time area handles during
> soft reset
> 
> Just like domain_soft_reset() properly zaps runstate area handles, the
> secondary time area ones also need discarding to prevent guest memory
> corruption once the guest is re-started.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry