Re: [for-6.0 v5 08/13] securable guest memory: Introduce sgm "ready" flag

[David Gibson]

On Mon, Dec 14, 2020 at 06:00:36PM +0100, Cornelia Huck wrote:
> On Fri,  4 Dec 2020 16:44:10 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > The platform specific details of mechanisms for implementing securable
> > guest memory may require setup at various points during initialization.
> > Thus, it's not really feasible to have a single sgm initialization hook,
> > but instead each mechanism needs its own initialization calls in arch or
> > machine specific code.
> > 
> > However, to make it harder to have a bug where a mechanism isn't properly
> > initialized under some circumstances, we want to have a common place,
> > relatively late in boot, where we verify that sgm has been initialized if
> > it was requested.
> > 
> > This patch introduces a ready flag to the SecurableGuestMemory base type
> > to accomplish this, which we verify just before the machine specific
> > initialization function.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/core/machine.c                     | 8 ++++++++
> >  include/exec/securable-guest-memory.h | 2 ++
> >  target/i386/sev.c                     | 2 ++
> >  3 files changed, 12 insertions(+)
> > 
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 816ea3ae3e..a67a27d03c 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -1155,6 +1155,14 @@ void machine_run_board_init(MachineState *machine)
> >      }
> >  
> >      if (machine->sgm) {
> > +        /*
> > +         * Where securable guest memory is initialized depends on the
> > +         * specific mechanism in use.  But, we need to make sure it's
> > +         * ready by now.  If it isn't, that's a bug in the
> > +         * implementation of that sgm mechanism.
> > +         */
> > +        assert(machine->sgm->ready);
> 
> Under which circumstances might we arrive here with 'ready' not set?
> 
> - programming error, setup is happening too late -> assert() seems
>   appropriate

Yes, this is designed to catch programming errors.  In particular I'm
concerned about:
  * Re-arranging the init code, and either entirely forgetting the sgm
    setup, or accidentally moving it too late
  * The sgm setup is buried in the machine setup code, conditional on
    various things, and changes mean we no longer either call it or
    (correctly) fail
  * User has specified an sgm scheme designed for a machine type other
    than the one they selected.  The arch/machine init code hasn't
    correctly accounted for that possibility and ignores it, instead
    of correctly throwing an error
 
> - we tried to set it up, but some error happened -> should we rely on
>   the setup code to error out first? (i.e. we won't end up here, unless
>   there's a programming error, in which case the assert() looks
>   fine)

Yes, that's my intention.

>   Is there a possible use case for "we could not set it up, but we
>   support an unsecured guest (as long as it is clear what happens)"?

I don't think so.  My feeling is that if you specify that you want the
feature, qemu needs to either give it to you, or fail, not silently
degrade the features presented to the guest.

>   Likely only for guests that transition themselves, but one could
>   argue that QEMU should simply be invoked a second time without the
>   sgm stuff being specified in the error case.

Right - I think whatever error we give here is likely to be easier to
diagnose than the guest itself throwing an error when it fails to
transition to secure mode (plus we should catch it always, rather than
only if we run a guest which tries to go secure).

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature