Re: [PATCH 07/10] tpm2_key_protector: Support NV index handles

[Gary Lin]

On Thu, Jan 02, 2025 at 02:54:51PM -0500, Stefan Berger wrote:
> 
> 
> On 12/19/24 3:12 AM, Gary Lin wrote:
> > Previously, NV index mode only supported persistent handles which are
> > only for the TPM objects. Without introducing new parameters, it is
> 
> for TPM objects.
> 
Will fix it in v2.

> > difficult to support authorized policy.
> 
> I am not sure how this sentence relates to the patch. Remove it?
> 
Okay. My original intension is to express the limitation of persistent
handles, but it seems not necessary anyway.

> > 
> > On the other hand, the "NV index" handle allows the user-defined data,
> > so it can be an alternative to the key file and support TPM 2.0 Key> File
> format immediately.
> > 
> > The following tpm2-tools commands stores the given key file, sealed.tpm,
> 
> /stores/store
> 
> > in either TPM 2.0 Key File format or the raw format into the NV index
> > handle, 0x1000000.
> 
> remove ','
> 
Will fix them in v2.

> > 
> >    # tpm2_nvdefine -C o \
> >        -a "ownerread|policywrite|ownerwrite" \
> >        -s $(stat -c %s sealed.tpm) \
> >        0x1000000
> >    # tpm2_nvwrite -C o -i sealed.tpm 0x1000000
> > 
> > To unseal the key in GRUB, add the 'tpm2_key_protector_init' command to
> > grub.cfg:
> > 
> >    tpm2_key_protector_init --mode=nv --nvindex=0x1000000
> 
> This works for as long as NO password in the owner hierarchy is set.
> 
The password of owner hierarchy is already addressed in grub.texi, so I
assume that there is no password for the owner hierarchy.

> >    cryptomount -u <UUID> --protector tpm2
> > 
> > To remove the NV index handle:
> > 
> >    # tpm2_nvundefine -C o 0x1000000
> > > Signed-off-by: Gary Lin <glin@suse.com>
> > ---
> >   .../commands/tpm2_key_protector/module.c      | 72 ++++++++++++++++---
> >   1 file changed, 61 insertions(+), 11 deletions(-)
> > 
> > diff --git a/grub-core/commands/tpm2_key_protector/module.c 
> > b/grub-core/commands/tpm2_key_protector/module.c
> > index e58da6f7a..1b3b89df8 100644
> > --- a/grub-core/commands/tpm2_key_protector/module.c
> > +++ b/grub-core/commands/tpm2_key_protector/module.c
> > @@ -1133,10 +1133,9 @@ tpm2_protector_srk_recover (const 
> > tpm2_protector_context_t *ctx,
> >   }
> >   static grub_err_t
> > -tpm2_protector_nv_recover (const tpm2_protector_context_t *ctx,
> > -                      grub_uint8_t **key, grub_size_t *key_size)
> > +tpm2_protector_unseal_persistent (const tpm2_protector_context_t *ctx, 
> > TPM_HANDLE_t sealed_handle,
> > +                             grub_uint8_t **key, grub_size_t *key_size)
> >   {
> > -  TPM_HANDLE_t sealed_handle = ctx->nv;
> >     tpm2key_policy_t policy_seq = NULL;
> >     bool dump_pcr = false;
> >     grub_err_t err;
> > @@ -1163,6 +1162,51 @@ tpm2_protector_nv_recover (const 
> > tpm2_protector_context_t *ctx,
> >     return err;
> >   }
> > +static grub_err_t
> > +tpm2_protector_unseal_nvindex (const tpm2_protector_context_t *ctx, 
> > TPM_HANDLE_t nvindex,
> > +                          grub_uint8_t **key, grub_size_t *key_size)
> > +{
> > +  TPMS_AUTH_COMMAND_t authCmd = {0};
> > +  TPM2B_NV_PUBLIC_t nv_public;
> > +  TPM2B_NAME_t nv_name;
> > +  grub_uint16_t data_size;
> > +  TPM2B_MAX_NV_BUFFER_t data;
> > +  TPM_RC_t rc;
> > +
> > +  /* Get the data size in the NV index handle */
> > +  rc = grub_tpm2_nv_readpublic (nvindex, NULL, &nv_public, &nv_name);
> > +  if (rc != TPM_RC_SUCCESS)
> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "failed to retrieve info 
> > from 0x%x (TPM2_NV_ReadPublic: 0x%x)", nvindex, rc);
> > +
> > +  data_size = nv_public.nvPublic.dataSize;
> > +  if (data_size > TPM_MAX_NV_BUFFER_SIZE)
> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "insufficient data buffer");
> > +
> > +  /* Read the data from the NV index handle */
> > +  authCmd.sessionHandle = TPM_RS_PW;
> > +  rc = grub_tpm2_nv_read (TPM_RH_OWNER, nvindex, &authCmd, data_size, 0, 
> > &data);
> > +  if (rc != TPM_RC_SUCCESS)
> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "failed to read data from 
> > 0x%x (TPM2_NV_Read: 0x%x)", nvindex, rc);
> > +
> > +  return tpm2_protector_unseal_buffer (ctx, data.buffer, data_size, key, 
> > key_size);
> > +}
> > +
> > +static grub_err_t
> > +tpm2_protector_nv_recover (const tpm2_protector_context_t *ctx,
> > +                      grub_uint8_t **key, grub_size_t *key_size)
> > +{
> > +  grub_err_t err;
> > +
> > +  if (TPM_HT_IS_PERSISTENT (ctx->nv) == true)
> 
> you could omit '== true' here as well and in all other cases, unless this is
> mandatory in GRUB
> 
I'll keep it since '== true' is requested in the previous review.

> > +    err = tpm2_protector_unseal_persistent (ctx, ctx->nv, key, key_size);
> > +  else if (TPM_HT_IS_NVINDEX (ctx->nv) == true)
> > +    err = tpm2_protector_unseal_nvindex (ctx, ctx->nv, key, key_size);
> > +  else
> > +    err = GRUB_ERR_BAD_ARGUMENT;
> > +
> > +  return err;
> > +}
> > +
> >   static grub_err_t
> >   tpm2_protector_recover (const tpm2_protector_context_t *ctx,
> >                     grub_uint8_t **key, grub_size_t *key_size)
> > @@ -1207,22 +1251,23 @@ tpm2_protector_check_args (tpm2_protector_context_t 
> > *ctx)
> >       return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("in SRK mode, please 
> > specify a key file with only --tpm2key/-T or --keyfile/-k"));
> >     if (ctx->mode == TPM2_PROTECTOR_MODE_SRK && ctx->nv != 0)
> > -    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("in SRK mode, an NV Index 
> > cannot be specified"));
> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("in SRK mode, a NV Index 
> > cannot be specified"));
> 
> 'an' was correct because it's spoken like 'envy', so starts with a vowel
> 
Ah, thanks.

> >     /* Checks for NV mode */
> >     if (ctx->mode == TPM2_PROTECTOR_MODE_NV && ctx->nv == 0)
> > -    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("in NV Index mode, an NV 
> > Index must be specified: --nvindex or -n"));
> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("in NV Index mode, a NV 
> > Index must be specified: --nvindex or -n"));
> 
> same
> 
> >     if (ctx->mode == TPM2_PROTECTOR_MODE_NV &&
> >         (ctx->tpm2key != NULL || ctx->keyfile != NULL))
> >       return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("in NV Index mode, a 
> > keyfile cannot be specified"));
> 
> a key file cannot be specified when using NV Index mode
> 
> >   > -  if (ctx->mode == TPM2_PROTECTOR_MODE_NV && ctx->srk != 0)
> > -    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("in NV Index mode, an SRK 
> > cannot be specified"));
> > +  if (ctx->mode == TPM2_PROTECTOR_MODE_NV && TPM_HT_IS_PERSISTENT 
> > (ctx->nv) == true &&
> > +      (ctx->srk != 0 || ctx->srk_type.type != TPM_ALG_ERROR))
> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("in NV Index mode with a 
> > persistent handle, a SRK cannot be specified"));
> 
> an SRK cannot be specified when using NV Index mode with a persistent handle
> 
> 
> >     if (ctx->mode == TPM2_PROTECTOR_MODE_NV &&
> > -      ctx->srk_type.type != TPM_ALG_ERROR)
> > -    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("in NV Index mode, an 
> > asymmetric key type cannot be specified"));
> > +      (TPM_HT_IS_PERSISTENT (ctx->nv) == false && TPM_HT_IS_NVINDEX 
> > (ctx->nv) == false))
> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("in NV Index mode, a NV 
> > index must be a persistent or NV index handle"));
> 
> an NV index must be either a persistent handle or an NV index handle when
> using NV index mode
> 
Will fix the sentances above in v2.

> >     /* Defaults assignment */
> >     if (ctx->bank == TPM_ALG_ERROR)
> > @@ -1234,8 +1279,13 @@ tpm2_protector_check_args (tpm2_protector_context_t 
> > *ctx)
> >         ctx->pcr_count = 1;
> >       }
> > -  if (ctx->mode == TPM2_PROTECTOR_MODE_SRK &&
> > -      ctx->srk_type.type == TPM_ALG_ERROR)
> > +  /*
> > +   * Set ECC_NIS_P256 as the default SRK when using SRK mode or NV mode 
> > with
> 
> ECC_NIST_P256
> 
Oops, thanks for spotting the typo.

> > +   * a NV index handle
> 
> an NV index handle
> 
Will fix it in v2.

Gary Lin

> > +   */
> > +  if (ctx->srk_type.type == TPM_ALG_ERROR &&
> > +      (ctx->mode == TPM2_PROTECTOR_MODE_SRK ||
> > +       (ctx->mode == TPM2_PROTECTOR_MODE_NV && TPM_HT_IS_NVINDEX (ctx->nv) 
> > == true)))
> >       {
> >         ctx->srk_type.type = TPM_ALG_ECC;
> >         ctx->srk_type.detail.ecc_curve = TPM_ECC_NIST_P256;
>