On Thu Nov 7, 2024 at 3:44 PM EET, Mimi Zohar wrote:
> > + tpm.disable_pcr_integrity_protection= [HW,TPM]
> > + Do not protect PCR registers from unintended physical
> > + access, or interposers in the bus by the means of
> > + having an encrypted and integrity protected session
>
> "encrypted" isn't needed here.
fixing
>
> > + wrapped around TPM2_PCR_Extend command. Consider this
> > + in a situation where TPM is heavily utilized by
> > + IMA, thus protection causing a major performance hit,
> > + and the space where machines are deployed is by other
> > + means guarded.
> > +
> > tpm_suspend_pcr=[HW,TPM]
> > Format: integer pcr id
> > Specify that at suspend time, the tpm driver
> > diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
> > index cad0048bcc3c..e49a19fea3bd 100644
> > --- a/drivers/char/tpm/tpm-buf.c
> > +++ b/drivers/char/tpm/tpm-buf.c
> > @@ -146,6 +146,26 @@ void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
> > }
> > EXPORT_SYMBOL_GPL(tpm_buf_append_u32);
> >
> > +/**
> > + * tpm_buf_append_handle() - Add a handle
> > + * @chip: &tpm_chip instance
> > + * @buf: &tpm_buf instance
> > + * @handle: a TPM object handle
> > + *
> > + * Add a handle to the buffer, and increase the count tracking the number of
> > + * handles in the command buffer. Works only for command buffers.
> > + */
> > +void tpm_buf_append_handle(struct tpm_chip *chip, struct tpm_buf *buf, u32 handle)
> > +{
> > + if (buf->flags & TPM_BUF_TPM2B) {
> > + dev_err(&chip->dev, "Invalid buffer type (TPM2B)\n");
> > + return;
> > + }
> > +
> > + tpm_buf_append_u32(buf, handle);
> > + buf->handles++;
> > +}
> > +
> > /**
> > * tpm_buf_read() - Read from a TPM buffer
> > * @buf: &tpm_buf instance
> > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> > index 1e856259219e..cc443bcf15e8 100644
> > --- a/drivers/char/tpm/tpm2-cmd.c
> > +++ b/drivers/char/tpm/tpm2-cmd.c
> > @@ -14,6 +14,10 @@
> > #include "tpm.h"
> > #include <crypto/hash_info.h>
> >
> > +static bool disable_pcr_integrity_protection;
> > +module_param(disable_pcr_integrity_protection, bool, 0444);
> > +MODULE_PARM_DESC(disable_pcr_integrity_protection, "Disable TPM2_PCR_Extend encryption");
>
> I like the name 'disable_pcr_integrity_protection. However, the name and
> description doesn't match. Replace 'encryption' with 'integrity protection'.
Weird, I changed that. I don't know how it ended up being like that.
>
> > +
> > static struct tpm2_hash tpm2_hash_map[] = {
> > {HASH_ALGO_SHA1, TPM_ALG_SHA1},
> > {HASH_ALGO_SHA256, TPM_ALG_SHA256},
> > @@ -232,18 +236,26 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
> > int rc;
> > int i;
> >
> > - rc = tpm2_start_auth_session(chip);
> > - if (rc)
> > - return rc;
> > + if (!disable_pcr_integrity_protection) {
> > + rc = tpm2_start_auth_session(chip);
> > + if (rc)
> > + return rc;
> > + }
> >
> > rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
> > if (rc) {
> > - tpm2_end_auth_session(chip);
> > + if (!disable_pcr_integrity_protection)
> > + tpm2_end_auth_session(chip);
> > return rc;
> > }
> >
> > - tpm_buf_append_name(chip, &buf, pcr_idx, NULL);
> > - tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
> > + if (!disable_pcr_integrity_protection) {
> > + tpm_buf_append_name(chip, &buf, pcr_idx);
>
> tpm_buf_append_name() parameters didn't change. Don't remove the 'name' field
> here.
Hmm... weird I'll check this. Maybe I had something left to staging...
>
>
> > + tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
> > + } else {
> > + tpm_buf_append_handle(chip, &buf, pcr_idx);
>
> Or here.
Here I think it is appropriate
>
> > + tpm_buf_append_auth(chip, &buf, 0, NULL, 0);
> > + }
> >
> > tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
> >
> >
>
> Mimi
BR, Jarkko
