[PATCH 2/6] tmp: add TPM2_PCR_Allocate command

[Ilias Apalodimas]

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?

> +               /*
> +                * 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?


> +               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.

> +
> +               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