Re: [tpmdd-devel] [PATCH] tpm: don't destroy chip device prematurely

[Jason Gunthorpe]

On Wed, Oct 05, 2016 at 08:09:17PM +, Winkler, Tomas wrote:

> It could, but that patch was not merged yet, and I believe even if
> the issue is exposed only with runtime_pm currently, we have a bug
> in design even w/o runtime pm.

Please don't make changes without any justification :(

> > These are all fine, obviously. Todays kernel retains those values across
> > device_del and we set those values in tpmm_chip_alloc/etc. So correct values
> > are present as long as the chip memory exists. tpm continues to hold a kref 
> > on
> > the chip so the memory will be around.
> 
> I'm not saying they are not, but calling deep into the tpm stack and
> even to the parent device with unutilized device is not sane.

You keep asserting that, but it just isn't true at all.

For a long time the tpm subsystem didn't even have a
'struct device'. That is something Jarkko and I added.

The *ONLY* thing it does is act as the anchor for user space - eg it
holds the sysfs, contains the 'dev' file for the cdev, etc, etc. This
was an important clean up.

Internally to the tpm core, and the drivers, the chip->dev does
*NOTHING* except hold the few variables you pointed out. That is it.

We could go back to the old code, without the 'dev' and things
could still work correctly.

This is why your assertion the struct device needs to be registered
makes no sense.

If the runtime_pm patches change this, then we have a very serious
problem. Removing this assumption is much harder than a one line patch
moving device_del.

I actually have no idea how you'd do it, since we call all sorts of
tpm ops between device_init and device_add - again device_del is the
least of the problems if runtime pm insists the chip->dev be
registered when running transmit_cmd.

So, I again, strongly advise you to give up on this idea, it is too
hard for TPM, and does not seem technically needed at this time. Even
it it does seem to make some kind of intuitive sense.

> > Is there some other PM path where dev->parent becomes invovled?
> 
> Of course, the power management utilize the device hierarch, it
> assumes there is power dependencies between parents and child
> devices, such as bus controllers and the devices on that bus.

Sure, but that relationship only maters if something does a PM call on
the chip->dev, and AFAIK, nothing does that.

Do you know differently?

You pointed at something that isn't even run and said it is the source
of the problem.. You really need to set up here and explain exactly
what is happening.

> > Are you just guessing this solves a problem, or were you able to reproduce
> > Jarkko's report?
> 
> No, guesses are not my style :), this solves the issue, as you see
> this was also validated by Jarrko on his setup.

In the thread you pointed to Jarkko said he could not reproduce the
original issue. Jarkko can you clarify??

> > Even if that pm_runtime_put is happening, why doesn't the
> > 
> > +pm_runtime_get_sync(chip->dev.parent);
> > 
> > The runtime_pm patch adds to tpm_transmit take care of bringing the device
> > back?
> 
> Unfortunately not because, because it gets out of sync and what is
> actually happening is that idle callback is called and device is put
> to idle between send and receive.

What? As far as I understand this PM stuff, I would call that a very
serious bug.

If a PM transition during transmit_cmd causes the TPM to abort/fail
command execution then it *MUST* be prevented. Period.

pm_runtime_get_sync appears to be the correct thing to get the
guarentee, so I'm very confused by your statement.

> Now we can find a trick to fix this, but this would be rather w/o
> while we know what the real issue is.

don't understand this.. Are you saying that going into idle during
tpm_transmit is not a bug?

Sounds like there is some sort of race condition with the pm stuff
that needs fixing.

I still don't see what your actual bug is, other than what I already
knew - somehow PM causes transmit_cmd to fail.

Jason

--
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

Re: [tpmdd-devel] [PATCH v2 1/2] Documentation: tpm: add the IBM Virtual TPM device tree binding documentation

[Nayna]

On 09/29/2016 04:34 PM, Jarkko Sakkinen wrote:
> On Wed, Sep 28, 2016 at 04:30:40AM -0400, Nayna Jain wrote:
>> Virtual TPM, which is being used on IBM POWER7+ and POWER8 systems running
>> POWERVM, is currently supported by tpm device driver but lacks the
>> documentation. This patch adds the missing documentation for the existing
>> support.
>>
>> Suggested-by: Jason Gunthorpe 
>> Signed-off-by: Nayna Jain 
>> ---
>> Changelog v2:
>>
>> - New Patch
>>
>>   .../devicetree/bindings/security/tpm/ibmvtpm.txt   | 41 
>> ++
>>   1 file changed, 41 insertions(+)
>>   create mode 100644 
>> Documentation/devicetree/bindings/security/tpm/ibmvtpm.txt
>>
>> diff --git a/Documentation/devicetree/bindings/security/tpm/ibmvtpm.txt 
>> b/Documentation/devicetree/bindings/security/tpm/ibmvtpm.txt
>> new file mode 100644
>> index 000..d89f999
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/security/tpm/ibmvtpm.txt
>> @@ -0,0 +1,41 @@
>> +* Device Tree Bindings for IBM Virtual Trusted Platform Module(vtpm)
>> +
>> +Required properties:
>> +
>> +- compatible: property name that conveys the platform 
>> architecture
>> +  identifiers, as 'IBM,vtpm'
>> +- device_type   : specifies type of virtual device
>
> A generic device tree question. What is the difference between
> these fields? Why the I2C one does have 'device_type'?

Please find the details as below:

compatible - Standard property name as per IEEE 1275, specifying the 
interface compatible with this device. This property is consumed by 
linux kernel for selection of device driver.

device_type - Standard property name as per IEEE 1275, specifying the 
device type. This property MAY be used by linux kernel for device driver 
selection. It is used in the case of IBM virtual TPM driver.

/**
  * vio_register_device_node: - Register a new vio device.
  * @of_node:The OF node for this device.
  *
  * Creates and initializes a vio_dev structure from the data in
  * of_node and adds it to the list of virtual devices.
  * Returns a pointer to the created vio_dev or NULL if node has
  * NULL device_type or compatible fields.
  */
struct vio_dev *vio_register_device_node(struct device_node *of_node)

and vtpm device table being defined as below:

static struct vio_device_id tpm_ibmvtpm_device_table[] = {
 { "IBM,vtpm", "IBM,vtpm"}, 
---> type,compat
 { "", "" }
};

So, vio (virtual) devices uses both device_type and compatible property 
for device registration and driver selection.
In case of physical TPM, it is only compatible property being used for 
device driver selection

Also, please note that device_type property is now deprecated in latest 
Device Tree specs.

Thanks & Regards,
- Nayna


>
> /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

Re: [tpmdd-devel] [PATCH] tpm: don't destroy chip device prematurely

[Winkler, Tomas]

> 
> On Wed, Oct 05, 2016 at 07:48:59AM +, Winkler, Tomas wrote:
> > > > down_write(>ops_sem);
> > > > chip->ops = NULL;
> > > > up_write(>ops_sem);
> > >
> > > No, that is wrong as well, another thread can issue a TPM command
> > > between the device_del and the ops = NULL. Presumably that will fail
> > > the same as tpm2_shutdown does.
> >
> > Right, but that's why we need the TPM_CHIP_FLAG_REGISTERED bit to
> > stay.
> 
> How does that help?
> 
> We already null ops to signal the driver is removed, but we don't check ops in
> all places today. Notably sysfs doesn't do the test, and relies on the hard 
> fence
> device_del provides.
> 
> So at a minimum to go forward with this approach you have to fix sysfs to be
> safe - which I don't think is worthwhile..

Yes, if this approach is taken this has to go across the whole stack, there is 
no question about it. 


> > Second, tmp2_shutdown only assure that the tpm state is saved, we are
> > taking too hardline here.  If another command is issued, this is a
> > problem of the upper layers and it has to be fixed in the upper layer.
> 
> We can't fix it at the upper layers, this is classic removal race. Whatever
> happens it must not cause the kernel to malfunction.
> 
> > This is the problem statement form the commit message:
> >
> > ' But with the introduction of runtime power management it will result
> > in shutting down the parent device while it still in use.'
> 
> > https://sourceforge.net/p/tpmdd/mailman/message/35395799/
> 
> Great, your commit message should include the klog message this patch is
> fixing.
>

It could, but that  patch was not merged yet, and I believe even if the issue 
is exposed only with runtime_pm currently, we have a bug in design even w/o 
runtime pm.

> > > But again, the real bug is in design, where a device is used after
> > > device_del()  is called.
> 
> No, I don't think so..

I do :)
> 
> > > What code actually fails? I don't see anything in the runtime pm
> > > patch that relies on chip->dev at all.
> >
> >  "chip-dev.parent''
> >  dev_get_drvdata(>dev);
> 
> Also chip->dev.name because we can call dev_log/etc(>dev..)

Yes, of course but this was not the point of disagreement
 
> These are all fine, obviously. Todays kernel retains those values across
> device_del and we set those values in tpmm_chip_alloc/etc. So correct values
> are present as long as the chip memory exists. tpm continues to hold a kref on
> the chip so the memory will be around.

I'm not saying they are not, but calling deep into the tpm stack and even to 
the parent device with unutilized device is not sane. 

> 'dev' values are only being used for clarity and convenience, if ever the 
> kernel
> changes behavior and nulls those values during device_del then we will copy
> the values into chip and stop using dev. No algorithm needs to change, and we
> don't need a registered 'dev'
> to function.

Frankly I would suggest to stay with the device, it let you add tpm spec 
attributes (sysfs) if needed to the stack, you cannot hang those on the air.
You should distinguish between chip->dev and cdev though, those are not the 
same things. 

> However, I find such a possible change to be deeply unlikely. If you look
> around the kernel I think you will find that many subsystems subtly depend on
> these same invariants for corectness during various unregister races.

Yes, but the code around is aware that it's unregistering, you have device_del, 
put_device  pair enclosing device removal in the same function., unlike 
tmp2_shutdown which requires most of the stack functional. 
 
> > > What code fails and why?
> >
> >device_del(dev)
> >   bus_remove_device(dev)
> > device_release_driver(dev)
> >__device_release_driver(dev)
> >   pm_runtime_reinit(dev) {
> > if (dev->parent)
> >
> > pm_runtime_put(dev->parent);
> 
> Eh?? The full code is:
> 
> void pm_runtime_reinit(struct device *dev) {  if (!pm_runtime_enabled(dev)) {
>  if (dev->power.irq_safe) {
>  if (dev->parent)
>   pm_runtime_put(dev->parent);
> 
> And irq_safe is only set by pm_runtime_irq_safe(). I can't find any place that
> looks like that is called on chip->dev
> 
> Is there some other PM path where dev->parent becomes invovled?

Of course, the power management utilize the device hierarch, it assumes there 
is power dependencies between parents and child devices, such as bus 
controllers and the devices on that bus. 

> Are you just guessing this solves a problem, or were you able to reproduce
> Jarkko's report?

No, guesses are not my style :), this solves the issue, as you see this was 
also validated by Jarrko on his setup.

> Considering that Jarkko cannot reliably reproduce the original bug, I'm deeply
> skeptical this patch actually does anything more than fiddle with timing
> around some kind of undiscovered race condition 

Re: [tpmdd-devel] [PATCH] tpm/tpm_crb: revamp starting method setting.

[Winkler, Tomas]

> 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 
> > ---
> >  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 = >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 = >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
> >

--
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

Re: [tpmdd-devel] [PATCH] tpm: don't destroy chip device prematurely

[Jarkko Sakkinen]

On Wed, Oct 05, 2016 at 07:48:59AM +, Winkler, Tomas wrote:
> > >
> > > > On Tue, Oct 04, 2016 at 08:19:46AM +0300, Jarkko Sakkinen wrote:
> > > >
> > > > > > Make the driver uncallable first. The worst race that can happen
> > > > > > is that open("/dev/tpm0", ...) returns -EPIPE. I do not consider
> > > > > > this fatal at all.
> > > > >
> > > > > No responses for this reasonable proposal so I'll show what I mean:
> > > >
> > > > How is this any better than what Thomas proposed? It seems much
> > > > worse to me since now we have even more stuff in the wrong order.
> > > >
> > > > There are three purposes to the ordering as it stands today
> > > >  1) To guarantee that tpm2_shutdown is the last command delivered to
> > > > the TPM. When it is issued all other ways to access the device
> > > > are hard fenced off.
> > >
> > > I'm not sure where are you taking this requirements from simple bit is
> > > just enough to make the HW inaccessible if the interface is designed
> > > right.
> > 
> > I'm having a hard time understanding the english in your email. (Jarkko do 
> > you
> > know what Tomas is talking about??)
> 
> Will try to do better. 

Jason, sorry your question slipped while going through the dicussion
:-)

I think I'll take the standpoint that I'll wait for the next version.

The important thing is to notice that runtime PM requires the device
to be "alive" and in the device hierarchy. It's a constraint...

/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

Re: [tpmdd-devel] [PATCH] tpm/tpm_crb: revamp starting method setting.

[Jarkko Sakkinen]

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.

/Jarkko

> Signed-off-by: Tomas Winkler 
> ---
>  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 = >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 = >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
> 

--
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

[tpmdd-devel] [PATCH] tpm/tpm_crb: revamp starting method setting.

[Tomas Winkler]

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.

Signed-off-by: Tomas Winkler 
---
 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 = >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 = >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


--
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

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jarkko Sakkinen]

On Tue, Oct 04, 2016 at 11:12:31AM -0600, Jason Gunthorpe wrote:
> On Tue, Oct 04, 2016 at 08:26:51AM +0300, Jarkko Sakkinen wrote:
> > On Mon, Oct 03, 2016 at 03:11:29PM -0600, Jason Gunthorpe wrote:
> > > On Mon, Oct 03, 2016 at 11:22:30PM +0300, Jarkko Sakkinen wrote:
> > > 
> > > > > Sort of, the typical race is broadly
> > > > > 
> > > > > CPU0   CPU1
> > > > > 
> > > > > fops->open()
> > > > > securityfs_remove()
> > > > >   kref_put(chip)
> > > > >   kfree(chip)
> > > > > kref_get(data->chip.kref)
> 
> > > You need to actually race open and securityfs_remove to see the
> > > kref_get() loose its race and then use-after-free.
> > 
> > So you are worried that get_device() might come when the chip is already
> > gone?
> 
> Yes, I'm worried that securityfs_remove doesn not guarentee that
> all threads running open() have completed and that no new threads can
> start an open(). If that is guarenteed then we are fine once the
> get_device is added.
> 
> There might be some tricky thing guaranteeing that but I haven't found
> it..

Great, thanks for time and patience explaining. This will help me a lot
to properly review the next revisions of this series.

/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

Re: [tpmdd-devel] [PATCH] tpm: don't destroy chip device prematurely

[Winkler, Tomas]

> >
> > > On Tue, Oct 04, 2016 at 08:19:46AM +0300, Jarkko Sakkinen wrote:
> > >
> > > > > Make the driver uncallable first. The worst race that can happen
> > > > > is that open("/dev/tpm0", ...) returns -EPIPE. I do not consider
> > > > > this fatal at all.
> > > >
> > > > No responses for this reasonable proposal so I'll show what I mean:
> > >
> > > How is this any better than what Thomas proposed? It seems much
> > > worse to me since now we have even more stuff in the wrong order.
> > >
> > > There are three purposes to the ordering as it stands today
> > >  1) To guarantee that tpm2_shutdown is the last command delivered to
> > > the TPM. When it is issued all other ways to access the device
> > > are hard fenced off.
> >
> > I'm not sure where are you taking this requirements from simple bit is
> > just enough to make the HW inaccessible if the interface is designed
> > right.
> 
> I'm having a hard time understanding the english in your email. (Jarkko do you
> know what Tomas is talking about??)

Will try to do better. 

> 
> > The ordering can be resolved, like this
> >
> > down_write(>ops_sem);
> > if (chip->flags & TPM_CHIP_FLAG_TPM2)
> > tpm2_shutdown(chip, TPM2_SU_CLEAR);
> > up_write(>ops_sem);
> >
> > device_del(>dev);
> >
> > down_write(>ops_sem);
> > chip->ops = NULL;
> > up_write(>ops_sem);
> 
> No, that is wrong as well, another thread can issue a TPM command between
> the device_del and the ops = NULL. Presumably that will fail the same as
> tpm2_shutdown does.
>

Right, but that's why we need the TPM_CHIP_FLAG_REGISTERED bit to stay.
Second, tmp2_shutdown only assure that the tpm state is saved,  we are taking 
too hardline here.  If another command is issued, this is a problem of the 
upper layers and it has to be fixed in the upper layer.
On the other hand it is much worse if tpm2_shutdown is not sent at all.

> > > I still haven't heard an explanation why Thomas's other patches need
> > > this, or why trying to change this ordering makes any sense at all
> > > considering how the subsystem is constructed.
> >
> > I thought it's quite clear form the commit message, the device_del
> 
> Not clear at all the commit message describes the 'solution' not the problem.
> This doesn't help..

This is the problem statement form the commit message: 

' But with the introduction of runtime power management it will result in
shutting down the parent device while it still in use.' 

We are talking about 
https://lkml.org/lkml/2016/9/12/352 
https://sourceforge.net/p/tpmdd/mailman/message/35395799/

But again, the real bug is in design, where a device is used after device_del() 
 is called.

> 
> > naturally toggles runtime_pm of the parent device, it tries to resume
> > the parent device so it can perform denationalization and then suspend
> > the parent device back which caused tpm2_shutdown to fail.
> 
> What code actually fails? I don't see anything in the runtime pm patch that
> relies on chip->dev at all.

 "chip-dev.parent''
 dev_get_drvdata(>dev);

> 
> What code fails and why?

   device_del(dev)
  bus_remove_device(dev)
device_release_driver(dev)
   __device_release_driver(dev)
  pm_runtime_reinit(dev) {
if (dev->parent)
   pm_runtime_put(dev->parent);
   }

 
> > I general we can not to implement power management via runtime_pm and
> > resolve the issue within tpm_crb driver but it's not abouth tpm_crb.
> > tpm2_shutdown is a tpm stack call it's not tpm_crb function, it uses
> > tpm_transmit_cmd and friends it should have valid tpm_chip initialized
> > and valid.  I'm not sure what could be more clearer than that.
> 
> I'll say it again, the tpm_transmit_cmd path must not require a registered 
> chip-
> >dev.

I rephrase it again as well, this requirement is  just abusing of the device 
interface.

> device_del only unregisters the dev, it does not deinitialize it, nor does it 
> free
> any memory. 

Someone will do a legitimate fix in device_del and all you house will crash on 
you, the device should not be used after device_del is called. 
There is nothing in the interface that promises that nothing is destructed. 

I still don't understand how this has any impact on the pm stuff
> when all the pm stuff is attached only to the pdev.

There is device hierarchy which is important for power management, please read 
the code.

> > I have to admit that I'm not sure what the vtpm does yet, but I have a
> > feeling that a simple flag can fix this.
> 
> What flag? Fix what?

TPM_CHIP_FLAG_REGISTERED

Tomas


--
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___