| Message ID | 20250323140911.226137-8-nstange@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ima: get rid of hard dependency on SHA-1 | expand |
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
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. 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
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
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);
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(-)