| Message ID | 20201204054415.579042-9-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Generalize memory encryption models | expand |
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 - 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) 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)"? 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. > + > /* > * With securable guest memory, the host can't see the real > * contents of RAM, so there's no point in it trying to merge
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).
On Thu, 17 Dec 2020 16:38:20 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > 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. Yes, that should align with what QEMU is doing elsewhere. > > > 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). Yes, that makes sense.
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); + /* * With securable guest memory, the host can't see the real * contents of RAM, so there's no point in it trying to merge diff --git a/include/exec/securable-guest-memory.h b/include/exec/securable-guest-memory.h index 7325b504ba..20cf13777b 100644 --- a/include/exec/securable-guest-memory.h +++ b/include/exec/securable-guest-memory.h @@ -36,6 +36,8 @@ struct SecurableGuestMemory { Object parent; + + bool ready; }; typedef struct SecurableGuestMemoryClass { diff --git a/target/i386/sev.c b/target/i386/sev.c index 7333a60dc0..022ce5fc3a 100644 --- a/target/i386/sev.c +++ b/target/i386/sev.c @@ -701,6 +701,8 @@ int sev_kvm_init(SecurableGuestMemory *sgm, Error **errp) qemu_add_machine_init_done_notifier(&sev_machine_done_notify); qemu_add_vm_change_state_handler(sev_vm_state_change, sev); + sgm->ready = true; + return 0; err: sev_guest = NULL;
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(+)