[PATCH 2/6] tmp: add TPM2_PCR_Allocate command

[Raymond Mao]

Hi Ilias,

On Thu, 16 Jan 2025 at 07:37, Ilias Apalodimas <ilias.apalodimas at linaro.org>
wrote:

> Hi Raymond,
>
> On Wed, 15 Jan 2025 at 22:02, Raymond Mao <raymond.mao at linaro.org> wrote:
> >
> > TPM2_PCR_Allocate command is required to re-configurate
>
>
> reconfigure
>
> > a TPM device
> > to enable or disable algorithms in run-time, thus this patch introduces
> > the implementation of PCR allocate APIs and adds related cmd functions
> > for testing.
> >
> > To test the feature, ensure that TPM is started up.
> > Run pcr_allocate command to turn on/off an algorithm, multiple calls
> > are supported and all changes will be cached:
> > `tpm2 pcr_allocate <algorithm_name> <on|off>`
> > Run startup command with argument 'off' to shutdown the TPM.
> > `tpm2 startup TPM2_SU_CLEAR off`
> > Reboot the board via `reset` to activate the changes.
> >
> > Signed-off-by: Raymond Mao <raymond.mao at linaro.org>
> > ---
> >  cmd/tpm-v2.c     |  94 +++++++++++++++++++++++++++++++++++
> >  include/tpm-v2.h |  29 +++++++++++
> >  lib/tpm-v2.c     | 124 +++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 247 insertions(+)
> >
> > diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c
> > index a6d57ee7c4..2302e8e94c 100644
> > --- a/cmd/tpm-v2.c
> > +++ b/cmd/tpm-v2.c
> > @@ -232,6 +232,86 @@ unmap_data:
> >         return report_return_code(rc);
> >  }
> >
> > +static u32 select_mask(u32 mask, enum tpm2_algorithms algo, bool select)
> > +{
> > +       size_t i;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(hash_algo_list); i++) {
> > +               if (hash_algo_list[i].hash_alg != algo)
> > +                       continue;
> > +
> > +               if (select)
> > +                       mask |= hash_algo_list[i].hash_mask;
> > +               else
> > +                       mask &= ~hash_algo_list[i].hash_mask;
> > +
> > +               break;
> > +       }
> > +
> > +       return mask;
> > +}
> > +
> > +static int do_tpm2_pcrallocate(struct cmd_tbl *cmdtp, int flag, int
> argc,
> > +                              char *const argv[])
> > +{
> > +       struct udevice *dev;
> > +       int ret;
> > +       enum tpm2_algorithms algo;
> > +       const char *pw = (argc < 4) ? NULL : argv[3];
> > +       const ssize_t pw_sz = pw ? strlen(pw) : 0;
> > +       static struct tpml_pcr_selection pcr = { 0 };
> > +       u32 pcr_len = 0;
> > +       bool bon = false;
> > +       static u32 mask;
> > +       int i;
> > +
> > +       /* argv[1]: algorithm (bank), argv[2]: on/off */
> > +       if (argc < 3 || argc > 4)
> > +               return CMD_RET_USAGE;
> > +
> > +       if (!strcasecmp("on", argv[2]))
> > +               bon = true;
> > +       else if (strcasecmp("off", argv[2]))
> > +               return CMD_RET_USAGE;
> > +
> > +       algo = tpm2_name_to_algorithm(argv[1]);
> > +       if (algo == -EINVAL)
> > +               return CMD_RET_USAGE;
> > +
> > +       ret = get_tpm(&dev);
> > +       if (ret)
> > +               return ret;
> > +
> > +       if (!pcr.count) {
>
>
> Ins't this always true?
>
> Not really, pcr is a static variable to cache the latest configurations.
The command itself is designed to turn on/off one algorithm each time for
easy usage, and expects to be called multiple times when the user wants to
change more than one algorithm.
We just need to get the active bank at the first call when the command is
running multiple times.
Since tpm2_get_pcr_info() will not return the real actual active banks
after one PCR allocate command is sent until a reset occurs.


> > +               /*
> > +                * Get current in-use algorithms (banks), PCRs and mask
> via the
> > +                * first call
> > +                */
>
>
> in-use translates to active PCR banks. Isn't that function returning
> the hardware-supported banks?
>
> That returns the active ones only but not includes the inactive ones if
any.


>
> > +               ret = tpm2_get_pcr_info(dev, &pcr);
> > +               if (ret)
> > +                       return ret;
>
>
> So we have the TPM banks the hardware supports here. Does it make
> sense to check if the requested algorithm is supported and exit early?
> In theory, it doesn't matter because the hardware will reject the
> configuration later, but I'd like to have a comment about this.
>
> Ditto. It knows the active algorithms only but not includes the supported
but inactive ones.

Regards,
Raymond


> > +
> > +               for (i = 0; i < pcr.count; i++) {
> > +                       struct tpms_pcr_selection *sel =
> &pcr.selection[i];
> > +
> > +                       if (!tpm2_is_active_bank(sel))
> > +                               continue;
> > +
> > +                       mask = select_mask(mask, sel->hash, true);
> > +                       printf("Active bank[%d] with algo[%#x]\n", i,
> > +                              sel->hash);
> > +               }
> > +       }
> > +
> > +       mask = select_mask(mask, algo, bon);
> > +       ret = tpm2_pcr_config_algo(dev, mask, &pcr, &pcr_len);
> > +
> [...]
>
> Other than that this looks good to me
>
> Thanks
> /Ilias
>