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....@citrix.com> > Signed-off-by: Jan Beulich <jbeul...@suse.com> > --- > 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
