Re: [PATCH v4 4/6] machine/nitro-enclave: Add built-in Nitro Secure Module device

[Dorjoy Chowdhury]

Hi Daniel,

On Mon, Aug 19, 2024 at 4:37 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Sun, Aug 18, 2024 at 05:42:55PM +0600, Dorjoy Chowdhury wrote:
> > AWS Nitro Enclaves have built-in Nitro Secure Module (NSM) device which
> > is used for stripped down TPM functionality like attestation. This commit
> > adds the built-in NSM device in the nitro-enclave machine type.
> >
> > In Nitro Enclaves, all the PCRs start in a known zero state and the first
> > 16 PCRs are locked from boot and reserved. The PCR0, PCR1, PCR2 and PCR8
> > contain the SHA384 hashes related to the EIF file used to boot the
> > VM for validation.
> >
> > Some optional nitro-enclave machine options have been added:
> >     - 'id': Enclave identifier, reflected in the module-id of the NSM
> > device. If not provided, a default id will be set.
> >     - 'parent-role': Parent instance IAM role ARN, reflected in PCR3
> > of the NSM device.
> >     - 'parent-id': Parent instance identifier, reflected in PCR4 of the
> > NSM device.
> >
> > Signed-off-by: Dorjoy Chowdhury <dorjoychy111@gmail.com>
> > ---
> >  crypto/meson.build              |   2 +-
> >  crypto/x509-utils.c             |  73 +++++++++++
> >  include/crypto/x509-utils.h     |  22 ++++
>
> Preferrably add these 3 in a standlone commit, since its is good practice
> to separate commits adding infra, from commits adding usage of infra.
>
> >  hw/core/eif.c                   | 225 +++++++++++++++++++++++++++++---
> >  hw/core/eif.h                   |   5 +-
> >  hw/core/meson.build             |   4 +-
> >  hw/i386/Kconfig                 |   1 +
> >  hw/i386/nitro_enclave.c         | 141 +++++++++++++++++++-
> >  include/hw/i386/nitro_enclave.h |  26 ++++
> >  9 files changed, 479 insertions(+), 20 deletions(-)
> >  create mode 100644 crypto/x509-utils.c
> >  create mode 100644 include/crypto/x509-utils.h
> >

[...]

> > +
> > +    /* First 16 PCRs are locked from boot and reserved for nitro enclave */
> > +    for (int i = 0; i < 16; ++i) {
> > +        ne_state->vnsm->lock_pcr(ne_state->vnsm, i);
> > +    }
>
> Worth  NITRO_PCR_MAX constant ?
>

I kept the 16 for now.

> >  static void nitro_enclave_machine_initfn(Object *obj)
> >  {
> >      MicrovmMachineState *mms = MICROVM_MACHINE(obj);
> >      X86MachineState *x86ms = X86_MACHINE(obj);
> > +    NitroEnclaveMachineState *nems = NITRO_ENCLAVE_MACHINE(obj);
> > +
> > +    nems->id = g_strdup("i-234-enc5678");
>
> Is this some magic string that the Nitro spec requires ? If so, probably
> worth a #define constant for it with a comment about rationale.
>

It's just some default we provide, not related to Nitro spec. So kept
it as is for now.

Regards,
Dorjoy