Hi Daniel,
On Mon, Aug 19, 2024 at 4:37 PM Daniel P. Berrangé <berra...@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 <dorjoychy...@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
