tpmdd-devel.lists.sourceforge.net archive mirror
 help / color / mirror / Atom feed
From: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
To: Josh Zimmerman <joshz-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>,
	Peter Huewe <peterhuewe-Mmb7MZpHnFY@public.gmane.org>,
	Marcel Selhorst <tpmdd-yWjUBOtONefk1uMJSBkQmQ@public.gmane.org>,
	Jason Gunthorpe
	<jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>,
	tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org,
	jmorris-gx6/JNMH7DfYtjvyW6yDsg@public.gmane.org
Subject: Re: [PATCH v6 1/2] tpm: Issue a TPM2_Shutdown for TPM2 devices.
Date: Mon, 19 Jun 2017 01:26:47 +0200	[thread overview]
Message-ID: <1497828407.2552.8.camel@linux.intel.com> (raw)
In-Reply-To: <1497828091.2552.6.camel-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

On Mon, 2017-06-19 at 01:21 +0200, Jarkko Sakkinen wrote:
> Hold on.
> 
> On Fri, 2017-06-16 at 10:25 -0700, Josh Zimmerman wrote:
> > If a TPM2 loses power without a TPM2_Shutdown command being issued
> > (a
> > "disorderly reboot"), it may lose some state that has yet to be
> > persisted to NVRam, and will increment the DA counter. After the DA
> > counter gets sufficiently large, the TPM will lock the user out.
> >  
> > NOTE: This only changes behavior on TPM2 devices. Since TPM1 uses
> 
> sysfs,
> > and sysfs relies on implicit locking on chip->ops, it is not safe
> > to
> > allow this code to run in TPM1, or to add sysfs support to TPM2,
> 
> until
> > that locking is made explicit.
> >  
> > Signed-off-by: Josh Zimmerman <joshz@google.com>
> > Cc: stable@vger.kernel.org
> >  
> > ----
> > v2:
> >    - Properly split changes between this and another commit
> >    - Use proper locking primitive.
> >    - Fix commenting style
> > v3:
> >    - Re-fix commenting style
> > v4:
> >    - Update description and tags (Reviewed-by, Cc).
> > v5:
> >    - Update documentation.
> > v6:
> >    - Call device and/or bus shutdown from tpm_shutdown.
> > ---
> >   drivers/char/tpm/tpm-chip.c  | 31 +++++++++++++++++++++++++++++++
> >   drivers/char/tpm/tpm-sysfs.c |  3 +++
> >   2 files changed, 34 insertions(+)
> >  
> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-
> 
> chip.c
> > index 9dec9f551b83..62691d266f22 100644
> > --- a/drivers/char/tpm/tpm-chip.c
> > +++ b/drivers/char/tpm/tpm-chip.c
> > @@ -142,6 +142,36 @@ static void tpm_devs_release(struct device
> > *dev)
> >   	put_device(&chip->dev);
> >   }
> >   
> > +/**
> > + * tpm_shutdown() - prepare the TPM device for loss of power.
> > + * @dev: device to which the chip is associated.
> > + *
> > + * Issues a TPM2_Shutdown command prior to loss of power, as
> 
> required by the
> > + * TPM 2.0 spec.
> > + *
> > + * XXX: This codepath relies on the fact that sysfs is not enabled
> 
> for
> > + * TPM2: sysfs uses an implicit lock on chip->ops, so this could
> 
> race if TPM2
> > + * has sysfs support enabled before TPM sysfs's implicit locking
> > is
> 
> fixed.
> > + */
> > +static void tpm_shutdown(struct device *dev)
> > +{
> > +	struct tpm_chip *chip = container_of(dev, struct tpm_chip,
> 
> dev);
> > +
> > +	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> > +		down_write(&chip->ops_sem);
> > +		tpm2_shutdown(chip, TPM_SU_CLEAR);
> > +		chip->ops = NULL;
> > +		up_write(&chip->ops_sem);
> > +	}
> > +	// Allow bus- and device-specific code to run. Note: since
> 
> chip->ops
> > +	// is NULL, more-specific shutdown code will not be able
> > to
> 
> issue TPM
> > +	// commands.
> > +	if (dev->bus->shutdown)
> > +		dev->bus->shutdown(dev);
> > +	else if (dev->driver && dev->driver->shutdown)
> > +		dev->driver->shutdown(dev);
> 
> WTF is this part.
> 
> And why we have now duplicate code??
> 
> I'm dropping these patches for now from my master branch. I don't
> understand what is going on...
> 
> /Jarkko


This series is too broken to be merged :-( I expect cover letter for
the next version...

Feels weird that you have to call framework functions like that in the
driver. You must have brilliant reason to do so and that should be very
well documented in the code. This is terrible...

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

  parent reply	other threads:[~2017-06-18 23:26 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-06-16 17:25 [PATCH v6 1/2] tpm: Issue a TPM2_Shutdown for TPM2 devices Josh Zimmerman via tpmdd-devel
     [not found] ` <20170616172531.28464-1-joshz-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2017-06-16 17:25   ` [PATCH v6 2/2] Add "shutdown" to "struct class" Josh Zimmerman via tpmdd-devel
2017-06-18 21:53   ` [PATCH v6 1/2] tpm: Issue a TPM2_Shutdown for TPM2 devices Jarkko Sakkinen
2017-06-18 23:21   ` Jarkko Sakkinen
     [not found]     ` <1497828091.2552.6.camel-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
2017-06-18 23:26       ` Jarkko Sakkinen [this message]
     [not found]         ` <1497828407.2552.8.camel-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
2017-06-19 15:51           ` Jason Gunthorpe
     [not found]             ` <20170619155122.GA10188-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
2017-06-19 22:12               ` Jarkko Sakkinen
     [not found]                 ` <20170619221220.kfk5dldducxvou5b-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
2017-06-19 22:21                   ` Jarkko Sakkinen
     [not found]                     ` <20170619222122.55m5goanayw3gkeo-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
2017-06-19 22:38                       ` Jarkko Sakkinen
     [not found]                         ` <20170619223840.a66extf3r5ylmpnb-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
2017-06-19 22:44                           ` Josh Zimmerman via tpmdd-devel
     [not found]                             ` <CAHSjozDy0dsGPkyXzVsDabjtbM4-QkzoE2fH1gDONXETyA19RA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-06-20 15:05                               ` Jarkko Sakkinen
     [not found]                                 ` <20170620150541.x5b7vkxyb3w55gjq-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
2017-06-21  7:37                                   ` Jarkko Sakkinen
2017-06-20  8:44                           ` Jarkko Sakkinen
  -- strict thread matches above, loose matches on Subject: below --
2017-06-02 21:55 Josh Zimmerman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1497828407.2552.8.camel@linux.intel.com \
    --to=jarkko.sakkinen-vuqaysv1563yd54fqh9/ca@public.gmane.org \
    --cc=jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org \
    --cc=jmorris-gx6/JNMH7DfYtjvyW6yDsg@public.gmane.org \
    --cc=joshz-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org \
    --cc=peterhuewe-Mmb7MZpHnFY@public.gmane.org \
    --cc=tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org \
    --cc=tpmdd-yWjUBOtONefk1uMJSBkQmQ@public.gmane.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).