Re: [PATCH v2 2/5] x86/HVM: split restore state checking from state loading

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/11/2023 1:46 pm, Jan Beulich wrote:
> ..., at least as reasonably feasible without making a check hook
> mandatory (in particular strict vs relaxed/zero-extend length checking
> can't be done early this way).
>
> Note that only one of the two uses of hvm_load() is accompanied with
> hvm_check(). The other directly consumes hvm_save() output, which ought
> to be well-formed. This means that while input data related checks don't
> need repeating in the "load" function when already done by the "check"
> one (albeit assertions to this effect may be desirable), domain state
> related checks (e.g. has_xyz(d)) will be required in both places.
>
> Suggested-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Do we really need all the copying involved in use of _hvm_read_entry()
> (backing hvm_load_entry()? Zero-extending loads are likely easier to
> handle that way, but for strict loads all we gain is a reduced risk of
> unaligned accesses (compared to simply pointing into h->data[]).

Pointless copying is best avoided, but it would mean that we either need
to enforce proper alignment within the buffer (hard, but at least it's
page aligned to start with), or __pack all of the structures so they get
an alignment of 1.

Not that I expect things to break in practice, but UB is UB and in some
copious free time it might be nice to re-activate the unaligned checking
in UBSAN on x86.

> Would the hvm_sr_handlers[] better use array_access_nospec()?

It's control plane only, and we have speculative protections for domU
entering domctls much earlier.  I wouldn't worry.

> --- a/xen/arch/x86/hvm/save.c
> +++ b/xen/arch/x86/hvm/save.c
> @@ -291,9 +369,8 @@ int hvm_load(struct domain *d, hvm_domai
>      if ( !hdr )
>          return -ENODATA;
>  
> -    rc = arch_hvm_load(d, hdr);
> -    if ( rc )
> -        return rc;
> +    ASSERT(!arch_hvm_check(d, hdr));

You're normally the proponent of not having side effects in ASSERT()s
like this.

But our caller did this anyway, so why re-assert it here?

~Andrew