| Message ID | 20170427182659.403-1-joshz@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 27, 2017 at 11:26:59AM -0700, Josh Zimmerman wrote: > If a TPM2 loses power without a TPM2_Shutdown command being issued, it > may lose some state that has yet to be persisted to NVRam, and will > increment the DA counter (meaning that after too many disorderly > reboots, the TPM will lock the user out). > > This is a variant of https://patchwork.kernel.org/patch/9516631/. This has all the same problems of that patch, just now adds the code to tis driver as well.. There is not much reason to accept this and reject the above patch. > It differs in that: > * It only changes behavior on TPM2 devices, to avoid invoking the > unbounded-waiting sysfs codepath that was discussed on that patch. Hum. If that is a sensible work around for now then lets just do that at the core code level please. If you do this, then tpm_sysfs_add_device really needs a comment warning that tpm2 can not have sysfs until the problems are fixed with shutdown. > * It modifies tpm_tis rather than tpm_i2c_infineon, so that it can > change behavior for all TPM2 devices. There are many drivers that support TPM2 beyond tpm_tis, this just fixes a few tis drivers, re-enforcing my point in the original patch that this stuff has no buisness being in the low level drivers and should be in core code. Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Thu, Apr 27, 2017 at 11:26:59AM -0700, Josh Zimmerman wrote: > If a TPM2 loses power without a TPM2_Shutdown command being issued, it > may lose some state that has yet to be persisted to NVRam, and will > increment the DA counter (meaning that after too many disorderly > reboots, the TPM will lock the user out). > > This is a variant of https://patchwork.kernel.org/patch/9516631/. > It differs in that: > * It only changes behavior on TPM2 devices, to avoid invoking the > unbounded-waiting sysfs codepath that was discussed on that patch. > * It modifies tpm_tis rather than tpm_i2c_infineon, so that it can > change behavior for all TPM2 devices. > > Signed-off-by: Josh Zimmerman <joshz@google.com> > --- > drivers/char/tpm/tpm_tis.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c > index c7e1384f1b08..bd9c70b305ab 100644 > --- a/drivers/char/tpm/tpm_tis.c > +++ b/drivers/char/tpm/tpm_tis.c > @@ -216,11 +216,23 @@ static void tpm_tis_pnp_remove(struct pnp_dev *dev) > tpm_tis_remove(chip); > } > > +static void tpm_tis_pnp_shutdown(struct pnp_dev *dev) > +{ > + struct tpm_chip *chip = pnp_get_drvdata(dev); > + // TPM 2.0 requires that the TPM2_Shutdown() command be issued prior to loss of power. > + // If it is not, the DA counter will be incremented and, eventually, the user will be > + // locked out of their TPM. > + if (chip->flags & TPM_CHIP_FLAG_TPM2) { > + tpm_tis_pnp_remove(dev); > + } > +} > + What about acpi driver? Doy you think this should be in stable? I wonder if we could move in tpm_tis to nowadays common infrastructure where you just have platform_driver and use acpi_match_table. I'm referring to similar infra as I have in my other project: http://git.infradead.org/users/jjs/linux-isgx.git/blob/HEAD:/drivers/platform/x86/intel_sgx/sgx_main.c I guess we have a problem in tis such that some of the devices in some environments are not available through ACPI. There's also of_match_table but I'm wondering if ACPI and OF would cover everything we are using pnp_driver for. I'm just thinking that since you are fixing the hooks maybe it would make sense to make things right at the same time. Currently the whole things is a mess. I talked about this with Rafael a while ago. He said that for some reason it is wrong in the first place to root to ACPI device but I cannot recall the exact reason. /Jarkko > static struct pnp_driver tis_pnp_driver = { > .name = "tpm_tis", > .id_table = tpm_pnp_tbl, > .probe = tpm_tis_pnp_init, > .remove = tpm_tis_pnp_remove, > + .shutdown = tpm_tis_pnp_shutdown, > .driver = { > .pm = &tpm_tis_pm, > }, > -- > 2.13.0.rc0.306.g87b477812d-goog > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Thu, Apr 27, 2017 at 02:49:12PM -0600, Jason Gunthorpe wrote: > On Thu, Apr 27, 2017 at 11:26:59AM -0700, Josh Zimmerman wrote: > > If a TPM2 loses power without a TPM2_Shutdown command being issued, it > > may lose some state that has yet to be persisted to NVRam, and will > > increment the DA counter (meaning that after too many disorderly > > reboots, the TPM will lock the user out). > > > > This is a variant of https://patchwork.kernel.org/patch/9516631/. > > This has all the same problems of that patch, just now adds the code > to tis driver as well.. > > There is not much reason to accept this and reject the above patch. > > > It differs in that: > > * It only changes behavior on TPM2 devices, to avoid invoking the > > unbounded-waiting sysfs codepath that was discussed on that patch. > > Hum. If that is a sensible work around for now then lets just do that > at the core code level please. > > If you do this, then tpm_sysfs_add_device really needs a comment > warning that tpm2 can not have sysfs until the problems are fixed with > shutdown. > > > * It modifies tpm_tis rather than tpm_i2c_infineon, so that it can > > change behavior for all TPM2 devices. > > There are many drivers that support TPM2 beyond tpm_tis, this just > fixes a few tis drivers, re-enforcing my point in the original patch > that this stuff has no buisness being in the low level drivers and > should be in core code. > > Jason Let me note that this does not even cover all of the tpm_tis. /Jarkko ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Fri, Apr 28, 2017 at 03:12:26PM +0300, Jarkko Sakkinen wrote: > I guess we have a problem in tis such that some of the devices in some > environments are not available through ACPI. There's also of_match_table > but I'm wondering if ACPI and OF would cover everything we are using > pnp_driver for. How do you recover the acpi handle like pnp_acpi_device does from a platform_device? If you know how to do that then we should use acpi_match_table and merge the platform and acpi devices.. Does the acpi_match_table encompass all of the pnp subsystem as well, or do we still need to have pnp_driver to handle legacy pre-acpi PNP systems? Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
It sounds like there are several intertwined issues here. I'd like to keep this patch relatively minimal, so here are my current thoughts on a path forward: * Refactor references to chip->ops to go through tpm_try_get_ops, as mentioned in the previous patch * Have a common tpm_shutdown method in tpm-chip.c that device specific drivers call (or that is just registered automatically for all devices?) that nulls out chip->ops (to prevent any commands being issued after tpm2_shutdown) and sends tpm2_shutdown This would leave further device refactoring for a future patch. Before I spend too much time on this, does this seem like a reasonable approach? Anything I'm missing? Thanks, Josh ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Fri, Apr 28, 2017 at 11:27:16AM -0700, Josh Zimmerman wrote: > It sounds like there are several intertwined issues here. I'd like to > keep this patch relatively minimal, so here are my current thoughts on > a path forward: > > * Refactor references to chip->ops to go through tpm_try_get_ops, as > mentioned in the previous patch As I said, if you rely on the fact that sysfs does not exist for TPM2 then this should already be done for the TPM2 case and an incremental later patch could solve this problem in sysfs to turn shutdown on for tpm1. As long as this logic is clearly documented it is probably an OK step for now. > * Have a common tpm_shutdown method in tpm-chip.c that device specific > drivers call (or that is just registered automatically for all > devices?) that nulls out chip->ops (to prevent any commands being > issued after tpm2_shutdown) and sends tpm2_shutdown I think the original patch got a bit stuck on exactly how to do this, but yes, somehow the common shutdown method is called. Either from the driver core (preferred, IHMO) or via patching every single TPM driver. > Before I spend too much time on this, does this seem like a reasonable > approach? Anything I'm missing? Don't think so, but suggest re-reading that huge original thread to be sure. Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Fri, Apr 28, 2017 at 11:43 AM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > On Fri, Apr 28, 2017 at 11:27:16AM -0700, Josh Zimmerman wrote: >> It sounds like there are several intertwined issues here. I'd like to >> keep this patch relatively minimal, so here are my current thoughts on >> a path forward: >> >> * Refactor references to chip->ops to go through tpm_try_get_ops, as >> mentioned in the previous patch > > As I said, if you rely on the fact that sysfs does not exist for TPM2 > then this should already be done for the TPM2 case and an incremental > later patch could solve this problem in sysfs to turn shutdown on for > tpm1. As long as this logic is clearly documented it is probably an OK > step for now. This I'm not actually sure about: it seems that there are other places than sysfs where chip->ops is referenced without being guarded by tpm_try_get_ops. If we rely on just NULLing out chip->ops this could be dangerous. I'll look at sending a patch for this first. >> * Have a common tpm_shutdown method in tpm-chip.c that device specific >> drivers call (or that is just registered automatically for all >> devices?) that nulls out chip->ops (to prevent any commands being >> issued after tpm2_shutdown) and sends tpm2_shutdown > > I think the original patch got a bit stuck on exactly how to do this, > but yes, somehow the common shutdown method is called. Either from the > driver core (preferred, IHMO) or via patching every single TPM driver. I think the original thread was leaning towards (and I agree with) moving it into the core. Just to make sure I'm understanding the code structure correctly, tpm-chip.c is part of that core and a reasonable place for shared functionality, correct? >> Before I spend too much time on this, does this seem like a reasonable >> approach? Anything I'm missing? > > Don't think so, but suggest re-reading that huge original thread to be > sure. Will do. > Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Fri, Apr 28, 2017 at 02:32:31PM -0700, Josh Zimmerman wrote: > On Fri, Apr 28, 2017 at 11:43 AM, Jason Gunthorpe > <jgunthorpe@obsidianresearch.com> wrote: > > On Fri, Apr 28, 2017 at 11:27:16AM -0700, Josh Zimmerman wrote: > >> It sounds like there are several intertwined issues here. I'd like to > >> keep this patch relatively minimal, so here are my current thoughts on > >> a path forward: > >> > >> * Refactor references to chip->ops to go through tpm_try_get_ops, as > >> mentioned in the previous patch > > > > As I said, if you rely on the fact that sysfs does not exist for TPM2 > > then this should already be done for the TPM2 case and an incremental > > later patch could solve this problem in sysfs to turn shutdown on for > > tpm1. As long as this logic is clearly documented it is probably an OK > > step for now. > > This I'm not actually sure about: it seems that there are other places > than sysfs where chip->ops is referenced without being guarded by > tpm_try_get_ops. If we rely on just NULLing out chip->ops this could > be dangerous. I'll look at sending a patch for this first. If that is the case then we have an existing bug.. The only other user entry point is via the cdev, and those call try_get_ops around all calls down into the tpm code. Everything coming in via the kapi must call tpm_chip_find_get, which does try_get_ops. > > I think the original patch got a bit stuck on exactly how to do this, > > but yes, somehow the common shutdown method is called. Either from the > > driver core (preferred, IHMO) or via patching every single TPM driver. > > I think the original thread was leaning towards (and I agree with) > moving it into the core. Just to make sure I'm understanding the code > structure correctly, tpm-chip.c is part of that core and a reasonable > place for shared functionality, correct? Yes.. But I went I said driver core, I ment by adding tpm_class->shutdown that is called by drivers/base/... 'or something like that' The trouble is how to consistently get the machine shutdown callback from the driver core. Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Fri, Apr 28, 2017 at 11:39 PM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > On Fri, Apr 28, 2017 at 02:32:31PM -0700, Josh Zimmerman wrote: >> On Fri, Apr 28, 2017 at 11:43 AM, Jason Gunthorpe >> <jgunthorpe@obsidianresearch.com> wrote: >> > On Fri, Apr 28, 2017 at 11:27:16AM -0700, Josh Zimmerman wrote: >> >> It sounds like there are several intertwined issues here. I'd like to >> >> keep this patch relatively minimal, so here are my current thoughts on >> >> a path forward: >> >> >> >> * Refactor references to chip->ops to go through tpm_try_get_ops, as >> >> mentioned in the previous patch >> > >> > As I said, if you rely on the fact that sysfs does not exist for TPM2 >> > then this should already be done for the TPM2 case and an incremental >> > later patch could solve this problem in sysfs to turn shutdown on for >> > tpm1. As long as this logic is clearly documented it is probably an OK >> > step for now. >> >> This I'm not actually sure about: it seems that there are other places >> than sysfs where chip->ops is referenced without being guarded by >> tpm_try_get_ops. If we rely on just NULLing out chip->ops this could >> be dangerous. I'll look at sending a patch for this first. > > If that is the case then we have an existing bug.. > > The only other user entry point is via the cdev, and those call > try_get_ops around all calls down into the tpm code. > > Everything coming in via the kapi must call tpm_chip_find_get, which > does try_get_ops. > >> > I think the original patch got a bit stuck on exactly how to do this, >> > but yes, somehow the common shutdown method is called. Either from the >> > driver core (preferred, IHMO) or via patching every single TPM driver. >> >> I think the original thread was leaning towards (and I agree with) >> moving it into the core. Just to make sure I'm understanding the code >> structure correctly, tpm-chip.c is part of that core and a reasonable >> place for shared functionality, correct? > > Yes.. > > But I went I said driver core, I ment by adding tpm_class->shutdown > that is called by drivers/base/... 'or something like that' > > The trouble is how to consistently get the machine shutdown callback > from the driver core. It is not particularly clear what you mean here. Care to elaborate? Thanks, Rafael ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Fri, Apr 28, 2017 at 2:39 PM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > On Fri, Apr 28, 2017 at 02:32:31PM -0700, Josh Zimmerman wrote: >> On Fri, Apr 28, 2017 at 11:43 AM, Jason Gunthorpe >> <jgunthorpe@obsidianresearch.com> wrote: >> > On Fri, Apr 28, 2017 at 11:27:16AM -0700, Josh Zimmerman wrote: >> >> It sounds like there are several intertwined issues here. I'd like to >> >> keep this patch relatively minimal, so here are my current thoughts on >> >> a path forward: >> >> >> >> * Refactor references to chip->ops to go through tpm_try_get_ops, as >> >> mentioned in the previous patch >> > >> > As I said, if you rely on the fact that sysfs does not exist for TPM2 >> > then this should already be done for the TPM2 case and an incremental >> > later patch could solve this problem in sysfs to turn shutdown on for >> > tpm1. As long as this logic is clearly documented it is probably an OK >> > step for now. >> >> This I'm not actually sure about: it seems that there are other places >> than sysfs where chip->ops is referenced without being guarded by >> tpm_try_get_ops. If we rely on just NULLing out chip->ops this could >> be dangerous. I'll look at sending a patch for this first. > > If that is the case then we have an existing bug.. > > The only other user entry point is via the cdev, and those call > try_get_ops around all calls down into the tpm code. > > Everything coming in via the kapi must call tpm_chip_find_get, which > does try_get_ops. OK, then I'm confused about the problem with sysfs. If everything is already grabbing a ref first, why can't we just grab a ref and NULL out chip->ops? My understanding from reading the other thread was that sysfs (and, possibly, some other places--see the paragraph starting "I went through the places that access chip->ops to see what's" in the original thread) doesn't grab the ref, which was part of the reason this was tricky. Josh ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Fri, Apr 28, 2017 at 11:45:53PM +0200, Rafael J. Wysocki wrote: > > The trouble is how to consistently get the machine shutdown callback > > from the driver core. > > It is not particularly clear what you mean here. Care to elaborate? The request is to have a way for the generic tpm code to be called before struct device_driver -> shutdown() Presumably by adding shutdown() to struct class like the suspend/resume PM methods? Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Fri, Apr 28, 2017 at 03:07:02PM -0700, Josh Zimmerman wrote: > On Fri, Apr 28, 2017 at 2:39 PM, Jason Gunthorpe > <jgunthorpe@obsidianresearch.com> wrote: > > On Fri, Apr 28, 2017 at 02:32:31PM -0700, Josh Zimmerman wrote: > >> On Fri, Apr 28, 2017 at 11:43 AM, Jason Gunthorpe > >> <jgunthorpe@obsidianresearch.com> wrote: > >> > On Fri, Apr 28, 2017 at 11:27:16AM -0700, Josh Zimmerman wrote: > >> >> It sounds like there are several intertwined issues here. I'd like to > >> >> keep this patch relatively minimal, so here are my current thoughts on > >> >> a path forward: > >> >> > >> >> * Refactor references to chip->ops to go through tpm_try_get_ops, as > >> >> mentioned in the previous patch > >> > > >> > As I said, if you rely on the fact that sysfs does not exist for TPM2 > >> > then this should already be done for the TPM2 case and an incremental > >> > later patch could solve this problem in sysfs to turn shutdown on for > >> > tpm1. As long as this logic is clearly documented it is probably an OK > >> > step for now. > >> > >> This I'm not actually sure about: it seems that there are other places > >> than sysfs where chip->ops is referenced without being guarded by > >> tpm_try_get_ops. If we rely on just NULLing out chip->ops this could > >> be dangerous. I'll look at sending a patch for this first. > > > > If that is the case then we have an existing bug.. > > > > The only other user entry point is via the cdev, and those call > > try_get_ops around all calls down into the tpm code. > > > > Everything coming in via the kapi must call tpm_chip_find_get, which > > does try_get_ops. > > OK, then I'm confused about the problem with sysfs. If everything > is already grabbing a ref first, why can't we just grab a ref and > NULL out chip->ops? sysfs relies on an implicit ref that is held across the duration of the sysfs file's lifetime. That ref is not given up until device_del returns, which serializes everything. See the comment in tpm_sysfs_add_device This is fine so long as ops is only ever null'd after device_del, but this shutdown work proposes to change that invariant.. To compensate, the ops lock would have to be pushed down into each sysfs method (for tpm1), or just not use sysfs (like tpm2 does) Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Fri, Apr 28, 2017 at 3:16 PM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > On Fri, Apr 28, 2017 at 11:45:53PM +0200, Rafael J. Wysocki wrote: > >> > The trouble is how to consistently get the machine shutdown callback >> > from the driver core. >> >> It is not particularly clear what you mean here. Care to elaborate? > > The request is to have a way for the generic tpm code to be called > before struct device_driver -> shutdown() To clarify just a bit: I believe TPM2_Shutdown must be the *last* command sent to the TPM, so it's not as simple as "must be called before device-specific shutdown": if the device has a command it needs to send, it must do so before the core sends tpm2_shutdown (or at least must send another tpm2_shutdown when it's done). Josh ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Fri, Apr 28, 2017 at 09:27:48AM -0600, Jason Gunthorpe wrote: > On Fri, Apr 28, 2017 at 03:12:26PM +0300, Jarkko Sakkinen wrote: > > > I guess we have a problem in tis such that some of the devices in some > > environments are not available through ACPI. There's also of_match_table > > but I'm wondering if ACPI and OF would cover everything we are using > > pnp_driver for. > > How do you recover the acpi handle like pnp_acpi_device does from a > platform_device? > > If you know how to do that then we should use acpi_match_table and > merge the platform and acpi devices.. > > Does the acpi_match_table encompass all of the pnp subsystem as well, > or do we still need to have pnp_driver to handle legacy pre-acpi PNP > systems? > > Jason AFAIK with ACPI_COMPANION as in http://lxr.free-electrons.com/source/net/rfkill/rfkill-gpio.c /Jarkko ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Fri, Apr 28, 2017 at 11:27:16AM -0700, Josh Zimmerman wrote: > It sounds like there are several intertwined issues here. I'd like to > keep this patch relatively minimal, so here are my current thoughts on > a path forward: > > * Refactor references to chip->ops to go through tpm_try_get_ops, as > mentioned in the previous patch > * Have a common tpm_shutdown method in tpm-chip.c that device specific > drivers call (or that is just registered automatically for all > devices?) that nulls out chip->ops (to prevent any commands being > issued after tpm2_shutdown) and sends tpm2_shutdown > > This would leave further device refactoring for a future patch. > > Before I spend too much time on this, does this seem like a reasonable > approach? Anything I'm missing? > > Thanks, > Josh I think this sounds like a plan. /Jarkko ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Fri, Apr 28, 2017 at 03:39:05PM -0600, Jason Gunthorpe wrote: > On Fri, Apr 28, 2017 at 02:32:31PM -0700, Josh Zimmerman wrote: > > On Fri, Apr 28, 2017 at 11:43 AM, Jason Gunthorpe > > <jgunthorpe@obsidianresearch.com> wrote: > > > On Fri, Apr 28, 2017 at 11:27:16AM -0700, Josh Zimmerman wrote: > > >> It sounds like there are several intertwined issues here. I'd like to > > >> keep this patch relatively minimal, so here are my current thoughts on > > >> a path forward: > > >> > > >> * Refactor references to chip->ops to go through tpm_try_get_ops, as > > >> mentioned in the previous patch > > > > > > As I said, if you rely on the fact that sysfs does not exist for TPM2 > > > then this should already be done for the TPM2 case and an incremental > > > later patch could solve this problem in sysfs to turn shutdown on for > > > tpm1. As long as this logic is clearly documented it is probably an OK > > > step for now. > > > > This I'm not actually sure about: it seems that there are other places > > than sysfs where chip->ops is referenced without being guarded by > > tpm_try_get_ops. If we rely on just NULLing out chip->ops this could > > be dangerous. I'll look at sending a patch for this first. > > If that is the case then we have an existing bug.. > > The only other user entry point is via the cdev, and those call > try_get_ops around all calls down into the tpm code. > > Everything coming in via the kapi must call tpm_chip_find_get, which > does try_get_ops. See the comment in tpm_sysfs_add_device. /Jarkko ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Fri, Apr 28, 2017 at 04:24:39PM -0600, Jason Gunthorpe wrote: > On Fri, Apr 28, 2017 at 03:07:02PM -0700, Josh Zimmerman wrote: > > On Fri, Apr 28, 2017 at 2:39 PM, Jason Gunthorpe > > <jgunthorpe@obsidianresearch.com> wrote: > > > On Fri, Apr 28, 2017 at 02:32:31PM -0700, Josh Zimmerman wrote: > > >> On Fri, Apr 28, 2017 at 11:43 AM, Jason Gunthorpe > > >> <jgunthorpe@obsidianresearch.com> wrote: > > >> > On Fri, Apr 28, 2017 at 11:27:16AM -0700, Josh Zimmerman wrote: > > >> >> It sounds like there are several intertwined issues here. I'd like to > > >> >> keep this patch relatively minimal, so here are my current thoughts on > > >> >> a path forward: > > >> >> > > >> >> * Refactor references to chip->ops to go through tpm_try_get_ops, as > > >> >> mentioned in the previous patch > > >> > > > >> > As I said, if you rely on the fact that sysfs does not exist for TPM2 > > >> > then this should already be done for the TPM2 case and an incremental > > >> > later patch could solve this problem in sysfs to turn shutdown on for > > >> > tpm1. As long as this logic is clearly documented it is probably an OK > > >> > step for now. > > >> > > >> This I'm not actually sure about: it seems that there are other places > > >> than sysfs where chip->ops is referenced without being guarded by > > >> tpm_try_get_ops. If we rely on just NULLing out chip->ops this could > > >> be dangerous. I'll look at sending a patch for this first. > > > > > > If that is the case then we have an existing bug.. > > > > > > The only other user entry point is via the cdev, and those call > > > try_get_ops around all calls down into the tpm code. > > > > > > Everything coming in via the kapi must call tpm_chip_find_get, which > > > does try_get_ops. > > > > OK, then I'm confused about the problem with sysfs. If everything > > is already grabbing a ref first, why can't we just grab a ref and > > NULL out chip->ops? > > > sysfs relies on an implicit ref that is held across the duration of > the sysfs file's lifetime. That ref is not given up until device_del > returns, which serializes everything. See the comment in > tpm_sysfs_add_device > > This is fine so long as ops is only ever null'd after device_del, but > this shutdown work proposes to change that invariant.. > > To compensate, the ops lock would have to be pushed down into each > sysfs method (for tpm1), or just not use sysfs (like tpm2 does) > > Jason Ah. Ignore my last response. /Jarkko ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Sat Apr 29 17, Jarkko Sakkinen wrote: >On Fri, Apr 28, 2017 at 09:27:48AM -0600, Jason Gunthorpe wrote: >> On Fri, Apr 28, 2017 at 03:12:26PM +0300, Jarkko Sakkinen wrote: >> >> > I guess we have a problem in tis such that some of the devices in some >> > environments are not available through ACPI. There's also of_match_table >> > but I'm wondering if ACPI and OF would cover everything we are using >> > pnp_driver for. >> >> How do you recover the acpi handle like pnp_acpi_device does from a >> platform_device? >> >> If you know how to do that then we should use acpi_match_table and >> merge the platform and acpi devices.. >> >> Does the acpi_match_table encompass all of the pnp subsystem as well, >> or do we still need to have pnp_driver to handle legacy pre-acpi PNP >> systems? >> >> Jason > >AFAIK with ACPI_COMPANION as in > >http://lxr.free-electrons.com/source/net/rfkill/rfkill-gpio.c > >/Jarkko What about ACPI_HANDLE like is done in thie bit of sdhci_acpi_probe()? static int sdhci_acpi_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; acpi_handle handle = ACPI_HANDLE(dev); > >------------------------------------------------------------------------------ >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 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Sat, Apr 29, 2017 at 08:30:41AM -0700, Jerry Snitselaar wrote: > On Sat Apr 29 17, Jarkko Sakkinen wrote: > > On Fri, Apr 28, 2017 at 09:27:48AM -0600, Jason Gunthorpe wrote: > > > On Fri, Apr 28, 2017 at 03:12:26PM +0300, Jarkko Sakkinen wrote: > > > > > > > I guess we have a problem in tis such that some of the devices in some > > > > environments are not available through ACPI. There's also of_match_table > > > > but I'm wondering if ACPI and OF would cover everything we are using > > > > pnp_driver for. > > > > > > How do you recover the acpi handle like pnp_acpi_device does from a > > > platform_device? > > > > > > If you know how to do that then we should use acpi_match_table and > > > merge the platform and acpi devices.. > > > > > > Does the acpi_match_table encompass all of the pnp subsystem as well, > > > or do we still need to have pnp_driver to handle legacy pre-acpi PNP > > > systems? > > > > > > Jason > > > > AFAIK with ACPI_COMPANION as in > > > > http://lxr.free-electrons.com/source/net/rfkill/rfkill-gpio.c > > > > /Jarkko > > What about ACPI_HANDLE like is done in thie bit of sdhci_acpi_probe()? > > static int sdhci_acpi_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > acpi_handle handle = ACPI_HANDLE(dev); Yes. That can be used for getting the handle. ACPI_HANDLE just wraps ACPI_COMPANION in a way that it extracts the handle from struct acpi_device. /Jarkko ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Fri, Apr 28, 2017 at 3:16 PM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > On Fri, Apr 28, 2017 at 11:45:53PM +0200, Rafael J. Wysocki wrote: > >> > The trouble is how to consistently get the machine shutdown callback >> > from the driver core. >> >> It is not particularly clear what you mean here. Care to elaborate? > > The request is to have a way for the generic tpm code to be called > before struct device_driver -> shutdown() > > Presumably by adding shutdown() to struct class like the > suspend/resume PM methods? I've seen some reference to the existing `suspend` method as being legacy. e.g., drivers/base/power/main.c:__device_suspend: if (dev->class) { if (dev->class->pm) { info = "class "; callback = pm_op(dev->class->pm, state); goto Run; } else if (dev->class->suspend) { pm_dev_dbg(dev, state, "legacy class "); error = legacy_suspend(dev, state, dev->class->suspend, "legacy class "); goto End; } } In addition, it's not clear to me that the tpm codebase is using class->suspend right now; it looks like for the most part each device uses STATIC_DEV_PM_OPS and sets up the common suspend function to be called independently. (this is based on grepping the tpm driver for "suspend" so it's possible I'm missing some clever macro hacks, but I did dig in somewhat thoroughly and don't believe I'm missing anything like that) Is there some reason a similar approach of having each driver type register its own shutdown function pointer (that might just all be the same function, much like tpm_pm_suspend) would not be acceptable? Josh ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Fri, May 05, 2017 at 03:39:01PM -0700, Josh Zimmerman wrote: > I've seen some reference to the existing `suspend` method as being > legacy. e.g., drivers/base/power/main.c:__device_suspend: I think this is because class->pm is the modern way to do class->suspend, not because class methods are legacy. > In addition, it's not clear to me that the tpm codebase is using > class->suspend right now; it looks like for the most part each > device Yes, but that doesn't mean this is the right way to do things, it just means this old code hasn't really been updated. The tpm subsystem was a total mess just a few years ago, not everything is cleaned up yet.. eg it would probably make much more sense to hook class->pm to do tpm_pm_suspend rather than have each driver include the same STATIC_DEV_PM_OPS. > Is there some reason a similar approach of having each driver type > register its own shutdown function pointer (that might just all be the > same function, much like tpm_pm_suspend) would not be acceptable? Now that we have so many tpm drivers it is a very good idea not to introduce boilerplate into every driver, we should be going the other way :) Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hm, according to include/linux/pm.h,
* It is allowed to unregister devices while the above callbacks are being
* executed. However, a callback routine MUST NOT try to
unregister the device
* it was called for, although it may unregister children of that device (for
* example, if it detects that a child was unplugged while the system was
* asleep).
So, it seems if we want to add shutdown to class->pm, we'll need to do
the refactoring for sysfs now to avoid the implicit lock in order to
safely NULL out chip->ops. (Otherwise, I believe we'd need to
unregister.)\\
I'll start work on that, but I wanted to send this email first to
double-check that my understanding was correct and I wasn't missing an
easier path.
Thanks,
Josh
On Fri, May 5, 2017 at 3:46 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Fri, May 05, 2017 at 03:39:01PM -0700, Josh Zimmerman wrote:
>
>> I've seen some reference to the existing `suspend` method as being
>> legacy. e.g., drivers/base/power/main.c:__device_suspend:
>
> I think this is because class->pm is the modern way to do
> class->suspend, not because class methods are legacy.
>
>> In addition, it's not clear to me that the tpm codebase is using
>> class->suspend right now; it looks like for the most part each
>> device
>
> Yes, but that doesn't mean this is the right way to do things, it just
> means this old code hasn't really been updated. The tpm subsystem was
> a total mess just a few years ago, not everything is cleaned up yet..
>
> eg it would probably make much more sense to hook class->pm to do
> tpm_pm_suspend rather than have each driver include the same
> STATIC_DEV_PM_OPS.
>
>> Is there some reason a similar approach of having each driver type
>> register its own shutdown function pointer (that might just all be the
>> same function, much like tpm_pm_suspend) would not be acceptable?
>
> Now that we have so many tpm drivers it is a very good idea not to
> introduce boilerplate into every driver, we should be going the other
> way :)
>
> Jason
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Mon, May 08, 2017 at 11:16:56AM -0700, Josh Zimmerman wrote: > Hm, according to include/linux/pm.h, > > * It is allowed to unregister devices while the above callbacks are being > * executed. However, a callback routine MUST NOT try to > unregister the device > * it was called for, although it may unregister children of that device (for > * example, if it detects that a child was unplugged while the system was > * asleep). > > So, it seems if we want to add shutdown to class->pm, we'll need to do > the refactoring for sysfs now to avoid the implicit lock in order to > safely NULL out chip->ops. (Otherwise, I believe we'd need to > unregister.)\\ > > I'll start work on that, but I wanted to send this email first to > double-check that my understanding was correct and I wasn't missing an > easier path. Like I said, if you guard shutdown with a 'if TPM2' then the sysfs case cannot occur.. It eventually needs to be fixed for TPM1, but a TPM2 only first step would be OK too. Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Ah, yes, right. Too early on Monday still :) Josh On Mon, May 8, 2017 at 11:24 AM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > On Mon, May 08, 2017 at 11:16:56AM -0700, Josh Zimmerman wrote: >> Hm, according to include/linux/pm.h, >> >> * It is allowed to unregister devices while the above callbacks are being >> * executed. However, a callback routine MUST NOT try to >> unregister the device >> * it was called for, although it may unregister children of that device (for >> * example, if it detects that a child was unplugged while the system was >> * asleep). >> >> So, it seems if we want to add shutdown to class->pm, we'll need to do >> the refactoring for sysfs now to avoid the implicit lock in order to >> safely NULL out chip->ops. (Otherwise, I believe we'd need to >> unregister.)\\ >> >> I'll start work on that, but I wanted to send this email first to >> double-check that my understanding was correct and I wasn't missing an >> easier path. > > Like I said, if you guard shutdown with a 'if TPM2' then the sysfs > case cannot occur.. > > It eventually needs to be fixed for TPM1, but a TPM2 only first step > would be OK too. > > Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index c7e1384f1b08..bd9c70b305ab 100644 --- a/drivers/char/tpm/tpm_tis.c +++ b/drivers/char/tpm/tpm_tis.c @@ -216,11 +216,23 @@ static void tpm_tis_pnp_remove(struct pnp_dev *dev) tpm_tis_remove(chip); } +static void tpm_tis_pnp_shutdown(struct pnp_dev *dev) +{ + struct tpm_chip *chip = pnp_get_drvdata(dev); + // TPM 2.0 requires that the TPM2_Shutdown() command be issued prior to loss of power. + // If it is not, the DA counter will be incremented and, eventually, the user will be + // locked out of their TPM. + if (chip->flags & TPM_CHIP_FLAG_TPM2) { + tpm_tis_pnp_remove(dev); + } +} + static struct pnp_driver tis_pnp_driver = { .name = "tpm_tis", .id_table = tpm_pnp_tbl, .probe = tpm_tis_pnp_init, .remove = tpm_tis_pnp_remove, + .shutdown = tpm_tis_pnp_shutdown, .driver = { .pm = &tpm_tis_pm, },
If a TPM2 loses power without a TPM2_Shutdown command being issued, it may lose some state that has yet to be persisted to NVRam, and will increment the DA counter (meaning that after too many disorderly reboots, the TPM will lock the user out). This is a variant of https://patchwork.kernel.org/patch/9516631/. It differs in that: * It only changes behavior on TPM2 devices, to avoid invoking the unbounded-waiting sysfs codepath that was discussed on that patch. * It modifies tpm_tis rather than tpm_i2c_infineon, so that it can change behavior for all TPM2 devices. Signed-off-by: Josh Zimmerman <joshz@google.com> --- drivers/char/tpm/tpm_tis.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)