The existing tpm_pcr_extend() extends all of a PCR's allocated banks with
the corresponding digest from the provided digests[] argument.
An upcoming code change to IMA will introduce the need to skip over those
banks it does not have a hash algorithm implementation available for.
Introduce tpm_pcr_extend_sel() to support this.
tpm_pcr_extend_sel() also expects a digests[] array, always being the
number of allocated PCR banks in size, just as it's the case for the
existing tpm_pcr_extend(). In addition to that however, it takes a
'banks_skip_mask', and will skip the extension of any bank having its
corresponding bit set there.
Signed-off-by: Nicolai Stange <nstange@suse.de>
---
drivers/char/tpm/tpm-interface.c | 29 +++++++++++++++++++++++++++--
drivers/char/tpm/tpm.h | 3 ++-
drivers/char/tpm/tpm2-cmd.c | 29 +++++++++++++++++++++++++++--
include/linux/tpm.h | 3 +++
4 files changed, 59 insertions(+), 5 deletions(-)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index b1daa0d7b341..88b4496de1df 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -314,6 +314,26 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
*/
int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
struct tpm_digest *digests)
+{
+ return tpm_pcr_extend_sel(chip, pcr_idx, digests, 0);
+}
+EXPORT_SYMBOL_GPL(tpm_pcr_extend);
+
+/**
+ * tpm_pcr_extend_sel - extend a PCR value into selected banks.
+ * @chip: a &struct tpm_chip instance, %NULL for the default chip
+ * @pcr_idx: the PCR to be retrieved
+ * @digests: array of tpm_digest structures used to extend PCRs
+ * @banks_skip_mask: pcr banks to skip
+ *
+ * Note: callers must pass a digest for every allocated PCR bank, in the same
+ * order of the banks in chip->allocated_banks.
+ *
+ * Return: same as with tpm_transmit_cmd()
+ */
+int tpm_pcr_extend_sel(struct tpm_chip *chip, u32 pcr_idx,
+ struct tpm_digest *digests,
+ unsigned long banks_skip_mask)
{
int rc;
int i;
@@ -330,7 +350,13 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
}
if (chip->flags & TPM_CHIP_FLAG_TPM2) {
- rc = tpm2_pcr_extend(chip, pcr_idx, digests);
+ rc = tpm2_pcr_extend(chip, pcr_idx, digests, banks_skip_mask);
+ goto out;
+ }
+
+ /* There's only one SHA1 bank with TPM 1. */
+ if (banks_skip_mask & 1) {
+ rc = 0;
goto out;
}
@@ -341,7 +367,6 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
tpm_put_ops(chip);
return rc;
}
-EXPORT_SYMBOL_GPL(tpm_pcr_extend);
int tpm_auto_startup(struct tpm_chip *chip)
{
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 7bb87fa5f7a1..f4ed49cb4101 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -291,7 +291,8 @@ int tpm2_get_timeouts(struct tpm_chip *chip);
int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
struct tpm_digest *digest, u16 *digest_size_ptr);
int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
- struct tpm_digest *digests);
+ struct tpm_digest *digests,
+ unsigned long banks_skip_mask);
int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,
u32 *value, const char *desc);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index dfdcbd009720..23ded8ea47dc 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -226,16 +226,34 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
* @chip: TPM chip to use.
* @pcr_idx: index of the PCR.
* @digests: list of pcr banks and corresponding digest values to extend.
+ * @banks_skip_mask: pcr banks to skip
*
* Return: Same as with tpm_transmit_cmd.
*/
int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
- struct tpm_digest *digests)
+ struct tpm_digest *digests,
+ unsigned long banks_skip_mask)
{
struct tpm_buf buf;
+ unsigned long skip_mask;
+ u32 banks_count;
int rc;
int i;
+ banks_count = 0;
+ skip_mask = banks_skip_mask;
+ for (i = 0; i < chip->nr_allocated_banks; i++) {
+ const bool skip_bank = skip_mask & 1;
+
+ skip_mask >>= 1;
+ if (skip_bank)
+ continue;
+ banks_count++;
+ }
+
+ if (banks_count == 0)
+ return 0;
+
if (!disable_pcr_integrity) {
rc = tpm2_start_auth_session(chip);
if (rc)
@@ -257,9 +275,16 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
tpm_buf_append_auth(chip, &buf, 0, NULL, 0);
}
- tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
+ tpm_buf_append_u32(&buf, banks_count);
+ skip_mask = banks_skip_mask;
for (i = 0; i < chip->nr_allocated_banks; i++) {
+ const bool skip_bank = skip_mask & 1;
+
+ skip_mask >>= 1;
+ if (skip_bank)
+ continue;
+
tpm_buf_append_u16(&buf, digests[i].alg_id);
tpm_buf_append(&buf, (const unsigned char *)&digests[i].digest,
chip->allocated_banks[i].digest_size);
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 20a40ade8030..7587eecc82fd 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -447,6 +447,9 @@ extern int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
struct tpm_digest *digest);
extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
struct tpm_digest *digests);
+extern int tpm_pcr_extend_sel(struct tpm_chip *chip, u32 pcr_idx,
+ struct tpm_digest *digests,
+ unsigned long banks_skip_mask);
extern int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max);
extern struct tpm_chip *tpm_default_chip(void);
void tpm2_flush_context(struct tpm_chip *chip, u32 handle);
--
2.49.0On Sun, 2025-03-23 at 15:09 +0100, Nicolai Stange wrote:
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index dfdcbd009720..23ded8ea47dc 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -226,16 +226,34 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
> * @chip: TPM chip to use.
> * @pcr_idx: index of the PCR.
> * @digests: list of pcr banks and corresponding digest values to extend.
> + * @banks_skip_mask: pcr banks to skip
> *
> * Return: Same as with tpm_transmit_cmd.
> */
> int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
> - struct tpm_digest *digests)
> + struct tpm_digest *digests,
> + unsigned long banks_skip_mask)
> {
> struct tpm_buf buf;
> + unsigned long skip_mask;
> + u32 banks_count;
> int rc;
> int i;
>
> + banks_count = 0;
> + skip_mask = banks_skip_mask;
> + for (i = 0; i < chip->nr_allocated_banks; i++) {
> + const bool skip_bank = skip_mask & 1;
> +
> + skip_mask >>= 1;
> + if (skip_bank)
> + continue;
> + banks_count++;
> + }
Setting ima_unsupported_pcr_banks_mask used BIT(i). Testing the bit should be
as straight forward here and below.
The first TPM extend after boot is the boot_aggregate. Afterwards the number of
banks being extended should always be the same. Do we really need to re-
calculate the number of banks needing to be extended each time?
> +
> + if (banks_count == 0)
> + return 0;
> +
> if (!disable_pcr_integrity) {
> rc = tpm2_start_auth_session(chip);
> if (rc)
> @@ -257,9 +275,16 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
> tpm_buf_append_auth(chip, &buf, 0, NULL, 0);
> }
>
> - tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
> + tpm_buf_append_u32(&buf, banks_count);
>
> + skip_mask = banks_skip_mask;
> for (i = 0; i < chip->nr_allocated_banks; i++) {
> + const bool skip_bank = skip_mask & 1;
> +
> + skip_mask >>= 1;
> + if (skip_bank)
> + continue;
> +
> tpm_buf_append_u16(&buf, digests[i].alg_id);
> tpm_buf_append(&buf, (const unsigned char *)&digests[i].digest,
> chip->allocated_banks[i].digest_size);
Mimi Zohar <zohar@linux.ibm.com> writes:
> On Sun, 2025-03-23 at 15:09 +0100, Nicolai Stange wrote:
>
>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>> index dfdcbd009720..23ded8ea47dc 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -226,16 +226,34 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
>> * @chip: TPM chip to use.
>> * @pcr_idx: index of the PCR.
>> * @digests: list of pcr banks and corresponding digest values to extend.
>> + * @banks_skip_mask: pcr banks to skip
>> *
>> * Return: Same as with tpm_transmit_cmd.
>> */
>> int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
>> - struct tpm_digest *digests)
>> + struct tpm_digest *digests,
>> + unsigned long banks_skip_mask)
>> {
>> struct tpm_buf buf;
>> + unsigned long skip_mask;
>> + u32 banks_count;
>> int rc;
>> int i;
>>
>> + banks_count = 0;
>> + skip_mask = banks_skip_mask;
>> + for (i = 0; i < chip->nr_allocated_banks; i++) {
>> + const bool skip_bank = skip_mask & 1;
>> +
>> + skip_mask >>= 1;
>> + if (skip_bank)
>> + continue;
>> + banks_count++;
>> + }
>
> Setting ima_unsupported_pcr_banks_mask used BIT(i). Testing the bit should be
> as straight forward here and below.
I opted for not to using BIT(i) here because in theory
->nr_allocated_banks could be > BITS_PER_LONG. Not in practice though,
but I felt it would improve code readabily if there aren't any implict
assumptions. Also I'm not sure static checkers wouldn't complain about
for (i = 0; i < a; i++) { 1ul << i; }
Anyway, I'm realizing now that the code above is effectively just a
popcnt implementation on the lower bits of ~banks_skip_mask.
IMO it would be perhaps even better to do
unsigned long skipped_banks_count, banks_count;
skipped_banks_count = 0;
skip_mask = banks_skip_mask;
for (i = 0; skip_mask && i < chip->nr_allocated_banks; i++) {
skipped_banks_count += skip_mask & 1;
skip_mask >>= 1;
}
banks_count = chip->nr_allocated_banks - skipped_banks_count;
instead. That way it's almost a nop in the common case of a clear
banks_skip_mask, plus there are no conditionals in the body.
> The first TPM extend after boot is the boot_aggregate. Afterwards the number of
> banks being extended should always be the same. Do we really need to re-
> calculate the number of banks needing to be extended each time?
>
Otherwise the number of banks to skip would have to get stored somewhere
and passed around, IIUC. I don't think that's worth it, the total number
of allocated banks is expected to be relatively small and
banks_skip_mask is zero in the common case anyway.
Thanks!
Nicolai
--
SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew McDonald, Werner Knoblich
(HRB 36809, AG Nürnberg)
On Sun, Mar 23, 2025 at 03:09:05PM +0100, Nicolai Stange wrote:
> The existing tpm_pcr_extend() extends all of a PCR's allocated banks with
> the corresponding digest from the provided digests[] argument.
Why not "just" tpm_pcr_extend(). We don't have a concept of
"non-existing tpm_pcr_extend()".
"tpm_pcr_extend() extends the allocated PCR banks ..."
or something.
>
> An upcoming code change to IMA will introduce the need to skip over those
Don't talk about upcoming code changes. Just explain why IMA depends on
the change.
> banks it does not have a hash algorithm implementation available for.
>
> Introduce tpm_pcr_extend_sel() to support this.
>
> tpm_pcr_extend_sel() also expects a digests[] array, always being the
> number of allocated PCR banks in size, just as it's the case for the
> existing tpm_pcr_extend(). In addition to that however, it takes a
> 'banks_skip_mask', and will skip the extension of any bank having its
> corresponding bit set there.
>
> Signed-off-by: Nicolai Stange <nstange@suse.de>
> ---
> drivers/char/tpm/tpm-interface.c | 29 +++++++++++++++++++++++++++--
> drivers/char/tpm/tpm.h | 3 ++-
> drivers/char/tpm/tpm2-cmd.c | 29 +++++++++++++++++++++++++++--
> include/linux/tpm.h | 3 +++
> 4 files changed, 59 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index b1daa0d7b341..88b4496de1df 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -314,6 +314,26 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
> */
> int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
> struct tpm_digest *digests)
> +{
> + return tpm_pcr_extend_sel(chip, pcr_idx, digests, 0);
> +}
> +EXPORT_SYMBOL_GPL(tpm_pcr_extend);
I'd add just an extra argument to tpm_pcr_extend().
BR, Jarkko
Jarkko Sakkinen <jarkko@kernel.org> writes:
> On Sun, Mar 23, 2025 at 03:09:05PM +0100, Nicolai Stange wrote:
>> The existing tpm_pcr_extend() extends all of a PCR's allocated banks with
>> the corresponding digest from the provided digests[] argument.
>
> Why not "just" tpm_pcr_extend(). We don't have a concept of
> "non-existing tpm_pcr_extend()".
>
> "tpm_pcr_extend() extends the allocated PCR banks ..."
>
> or something.
Right.
>>
>> An upcoming code change to IMA will introduce the need to skip over those
>
> Don't talk about upcoming code changes. Just explain why IMA depends on
> the change.
Ok.
>> banks it does not have a hash algorithm implementation available for.
>>
>> Introduce tpm_pcr_extend_sel() to support this.
>>
>> tpm_pcr_extend_sel() also expects a digests[] array, always being the
>> number of allocated PCR banks in size, just as it's the case for the
>> existing tpm_pcr_extend(). In addition to that however, it takes a
>> 'banks_skip_mask', and will skip the extension of any bank having its
>> corresponding bit set there.
>>
>> Signed-off-by: Nicolai Stange <nstange@suse.de>
>> ---
>> drivers/char/tpm/tpm-interface.c | 29 +++++++++++++++++++++++++++--
>> drivers/char/tpm/tpm.h | 3 ++-
>> drivers/char/tpm/tpm2-cmd.c | 29 +++++++++++++++++++++++++++--
>> include/linux/tpm.h | 3 +++
>> 4 files changed, 59 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> index b1daa0d7b341..88b4496de1df 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -314,6 +314,26 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
>> */
>> int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
>> struct tpm_digest *digests)
>> +{
>> + return tpm_pcr_extend_sel(chip, pcr_idx, digests, 0);
>> +}
>> +EXPORT_SYMBOL_GPL(tpm_pcr_extend);
>
> I'd add just an extra argument to tpm_pcr_extend().
Perfect, will do.
Thanks!
Nicolai
--
SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew McDonald, Werner Knoblich
(HRB 36809, AG Nürnberg)
© 2016 - 2025 Red Hat, Inc.