linux-kernel - RE: [PATCH] tpm/tpm_crb: revamp starting method setting.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5B8DA87D05A7694D9FA63FD143655C1B542F537C@hasmsx108.ger.corp.intel.com>
Date:   Wed, 5 Oct 2016 15:34:02 +0000
From:   "Winkler, Tomas" <tomas.winkler@...el.com>
To:     Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
CC:     "tpmdd-devel@...ts.sourceforge.net" 
        <tpmdd-devel@...ts.sourceforge.net>,
        Jason Gunthorpe <jgunthorpe@...idianresearch.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] tpm/tpm_crb: revamp starting method setting.

> On Wed, Oct 05, 2016 at 04:12:08PM +0300, Tomas Winkler wrote:
> > Encapsulate the start method parsing in a single function and add
> > needed debug printouts.
> > It eliminates small issue with useless double checking for
> > ACPI_TPM2_MEMORY_MAPPED.
> 
> Start method is trival to check from TPM2 dump. I'm not sure which problem is
> this commit trying to solve.

There is not really issue with the implementation, I believe this is just more readable and it helped me with debugging quicker.
Some platforms are coming with discreet TPM (TIS) and some are PTT so this gives me quick answer for that and mostly 
It's coming from the same logging facility you don't have to go to acpi dump. 

Second, it there is this little issue here, were 'sm == ACPI_TPM2_MEMORY_MAPPED' cannot be really hit, as the code has bailed on that before.

if (sm == ACPI_TPM2_COMMAND_BUFFER || sm == ACPI_TPM2_MEMORY_MAPPED ||
	    !strcmp(acpi_device_hid(device), "MSFT0101"))

Last it handles the parsing on one place,  before allocating the private data.

I think this change has some value, but no strong feelings about it.

Tomas



> /Jarkko
> 
> > Signed-off-by: Tomas Winkler <tomas.winkler@...el.com>
> > ---
> >  drivers/char/tpm/tpm_crb.c | 65
> > +++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 50 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > index aa0ef742ac03..aeec313384d7 100644
> > --- a/drivers/char/tpm/tpm_crb.c
> > +++ b/drivers/char/tpm/tpm_crb.c
> > @@ -381,6 +381,52 @@ out:
> >  	return ret;
> >  }
> >
> > +/**
> > + * crb_start_method - parse starting method
> > + *
> > + * @device: acpi device
> > + * @sm: starting method
> > + *
> > + * Return 0 if supported starting method was not found
> > + *    CRB_FL_CRB_START or CRB_FL_ACPI_START otherwise
> > + */
> > +static unsigned int crb_start_method(struct acpi_device *device, u32
> > +sm) {
> > +	struct device *dev = &device->dev;
> > +	u32 flags;
> > +
> > +	switch (sm) {
> > +	/* Should the FIFO driver handle this? */
> > +	case ACPI_TPM2_MEMORY_MAPPED:
> > +		dev_dbg(dev, "starting method[%d]: MM\n", sm);
> > +		return 0;
> > +	case ACPI_TPM2_COMMAND_BUFFER:
> > +		flags = CRB_FL_CRB_START;
> > +		dev_dbg(dev, "starting method[%d]: CB\n", sm);
> > +		break;
> > +	case ACPI_TPM2_START_METHOD:
> > +		flags = CRB_FL_ACPI_START;
> > +		dev_dbg(dev, "starting method[%d]: SM\n", sm);
> > +		break;
> > +	case ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD:
> > +		flags = CRB_FL_ACPI_START;
> > +		dev_dbg(dev, "starting method[%d]: CB w/ SM\n",
> > +			sm);
> > +		break;
> > +	default:
> > +		return 0;
> > +	}
> > +
> > +	/* The reason for the extra quirk is that the PTT in 4th Gen Core CPUs
> > +	 * report only ACPI start but in practice seems to require both
> > +	 * ACPI start and CRB start.
> > +	 */
> > +	if (!strcmp(acpi_device_hid(device), "MSFT0101"))
> > +		flags |= CRB_FL_CRB_START;
> > +
> > +	return flags;
> > +}
> > +
> >  static int crb_acpi_add(struct acpi_device *device)  {
> >  	struct acpi_table_tpm2 *buf;
> > @@ -388,7 +434,7 @@ static int crb_acpi_add(struct acpi_device *device)
> >  	struct tpm_chip *chip;
> >  	struct device *dev = &device->dev;
> >  	acpi_status status;
> > -	u32 sm;
> > +	unsigned int sm;
> >  	int rc;
> >
> >  	status = acpi_get_table(ACPI_SIG_TPM2, 1, @@ -398,26 +444,15 @@
> > static int crb_acpi_add(struct acpi_device *device)
> >  		return -EINVAL;
> >  	}
> >
> > -	/* Should the FIFO driver handle this? */
> > -	sm = buf->start_method;
> > -	if (sm == ACPI_TPM2_MEMORY_MAPPED)
> > +	sm = crb_start_method(device, buf->start_method);
> > +	if (!sm)
> >  		return -ENODEV;
> >
> >  	priv = devm_kzalloc(dev, sizeof(struct crb_priv), GFP_KERNEL);
> >  	if (!priv)
> >  		return -ENOMEM;
> >
> > -	/* The reason for the extra quirk is that the PTT in 4th Gen Core CPUs
> > -	 * report only ACPI start but in practice seems to require both
> > -	 * ACPI start and CRB start.
> > -	 */
> > -	if (sm == ACPI_TPM2_COMMAND_BUFFER || sm ==
> ACPI_TPM2_MEMORY_MAPPED ||
> > -	    !strcmp(acpi_device_hid(device), "MSFT0101"))
> > -		priv->flags |= CRB_FL_CRB_START;
> > -
> > -	if (sm == ACPI_TPM2_START_METHOD ||
> > -	    sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
> > -		priv->flags |= CRB_FL_ACPI_START;
> > +	priv->flags = sm;
> >
> >  	rc = crb_map_io(device, priv, buf);
> >  	if (rc)
> > --
> > 2.7.4
> >