[PATCH 2/6] tmp: add TPM2_PCR_Allocate command

[Ilias Apalodimas]

On Thu, 16 Jan 2025 at 20:14, Raymond Mao <raymond.mao at linaro.org> wrote:
>
> 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.

I completely ignored the static in that variable.

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

It returns the hardware available PCR banks regardless of them being
active or not

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