Re: [PATCH v2] tpm: Opt-in in disable PCR integrity protection

["Jarkko Sakkinen" <jarkko@kernel.org>]

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