[PATCH 0/2] tpm: fix locality and power saving handling

[PATCH 0/2] tpm: fix locality and power saving handling

[Tomas Winkler]

Devices that support locality has failed to transmit due to
reserved order of locality request and cmdReady/goIdle handshake.
The cmdReady/goIdle should be performed on the requested locality.

The first patch corrects the locality and power-save order,
adds required polling for completion of goIdle and locality relinquish.

The second patch removes cmdRead/goIdle handshake from runtime_pm as it's
rather a part of TPM2 protocol  then true device power management,
and on device it's no longer optional.

Tomas Winkler (2):
  tpm: cmd_ready command can be issued only after granting locality
  tpm: separate cmd_ready/go_idle from runtime_pm

 drivers/char/tpm/tpm-interface.c |  21 ++++--
 drivers/char/tpm/tpm_crb.c       | 157 +++++++++++++++++++++++----------------
 drivers/char/tpm/tpm_tis_core.c  |   4 +-
 include/linux/tpm.h              |   4 +-
 4 files changed, 115 insertions(+), 71 deletions(-)

-- 
2.14.3

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2 v2] tpm: cmd_ready command can be issued only after granting locality

[Tomas Winkler]

The correct sequence is to first request locality and only after
that perform cmd_ready  handshake, otherwise the hardware will drop
the subsequent message as from the device point of view the cmd_ready
handshake wasn't performed. Symmetrically locality has to be relinquished
only after going idle handshake has completed, this requires that
go_idle has to poll for the completion and as well locality
relinquish has to poll for completion so it is not overrriden
in back to back commands flow.

The issue is only visible on devices that support multiple localities.

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
V2: poll for locality relinquish completion

 drivers/char/tpm/tpm-interface.c |  14 ++---
 drivers/char/tpm/tpm_crb.c       | 108 +++++++++++++++++++++++++++------------
 drivers/char/tpm/tpm_tis_core.c  |   4 +-
 include/linux/tpm.h              |   2 +-
 4 files changed, 87 insertions(+), 41 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 76df4fbcf089..9fb3d406b078 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -422,8 +422,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	if (!(flags & TPM_TRANSMIT_UNLOCKED))
 		mutex_lock(&chip->tpm_mutex);
 
-	if (chip->dev.parent)
-		pm_runtime_get_sync(chip->dev.parent);
 
 	if (chip->ops->clk_enable != NULL)
 		chip->ops->clk_enable(chip, true);
@@ -439,6 +437,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 		chip->locality = rc;
 	}
 
+	if (chip->dev.parent)
+		pm_runtime_get_sync(chip->dev.parent);
+
 	rc = tpm2_prepare_space(chip, space, ordinal, buf);
 	if (rc)
 		goto out;
@@ -499,17 +500,18 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
 
 out:
+	if (chip->dev.parent)
+		pm_runtime_put_sync(chip->dev.parent);
+
 	if (need_locality && chip->ops->relinquish_locality) {
-		chip->ops->relinquish_locality(chip, chip->locality);
+		rc = chip->ops->relinquish_locality(chip, chip->locality);
 		chip->locality = -1;
 	}
+
 out_no_locality:
 	if (chip->ops->clk_enable != NULL)
 		chip->ops->clk_enable(chip, false);
 
-	if (chip->dev.parent)
-		pm_runtime_put_sync(chip->dev.parent);
-
 	if (!(flags & TPM_TRANSMIT_UNLOCKED))
 		mutex_unlock(&chip->tpm_mutex);
 	return rc ? rc : len;
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 7b3c2a8aa9de..497edd9848cd 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -112,6 +112,25 @@ struct tpm2_crb_smc {
 	u32 smc_func_id;
 };
 
+static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
+				unsigned long timeout)
+{
+	ktime_t start;
+	ktime_t stop;
+
+	start = ktime_get();
+	stop = ktime_add(start, ms_to_ktime(timeout));
+
+	do {
+		if ((ioread32(reg) & mask) == value)
+			return true;
+
+		usleep_range(50, 100);
+	} while (ktime_before(ktime_get(), stop));
+
+	return ((ioread32(reg) & mask) == value);
+}
+
 /**
  * crb_go_idle - request tpm crb device to go the idle state
  *
@@ -128,7 +147,7 @@ struct tpm2_crb_smc {
  *
  * Return: 0 always
  */
-static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv *priv)
+static int crb_go_idle(struct device *dev, struct crb_priv *priv)
 {
 	if ((priv->sm == ACPI_TPM2_START_METHOD) ||
 	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) ||
@@ -136,30 +155,17 @@ static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv *priv)
 		return 0;
 
 	iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
-	/* we don't really care when this settles */
 
+	if (!crb_wait_for_reg_32(&priv->regs_t->ctrl_req,
+				 CRB_CTRL_REQ_GO_IDLE/* mask */,
+				 0, /* value */
+				 TPM2_TIMEOUT_C)) {
+		dev_warn(dev, "goIdle timed out\n");
+		return -ETIME;
+	}
 	return 0;
 }
 
-static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
-				unsigned long timeout)
-{
-	ktime_t start;
-	ktime_t stop;
-
-	start = ktime_get();
-	stop = ktime_add(start, ms_to_ktime(timeout));
-
-	do {
-		if ((ioread32(reg) & mask) == value)
-			return true;
-
-		usleep_range(50, 100);
-	} while (ktime_before(ktime_get(), stop));
-
-	return false;
-}
-
 /**
  * crb_cmd_ready - request tpm crb device to enter ready state
  *
@@ -175,8 +181,7 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
  *
  * Return: 0 on success -ETIME on timeout;
  */
-static int __maybe_unused crb_cmd_ready(struct device *dev,
-					struct crb_priv *priv)
+static int crb_cmd_ready(struct device *dev, struct crb_priv *priv)
 {
 	if ((priv->sm == ACPI_TPM2_START_METHOD) ||
 	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) ||
@@ -195,11 +200,11 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
 	return 0;
 }
 
-static int crb_request_locality(struct tpm_chip *chip, int loc)
+static int __crb_request_locality(struct device *dev,
+				  struct crb_priv *priv, int loc)
 {
-	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
 	u32 value = CRB_LOC_STATE_LOC_ASSIGNED |
-		CRB_LOC_STATE_TPM_REG_VALID_STS;
+		    CRB_LOC_STATE_TPM_REG_VALID_STS;
 
 	if (!priv->regs_h)
 		return 0;
@@ -207,21 +212,45 @@ static int crb_request_locality(struct tpm_chip *chip, int loc)
 	iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h->loc_ctrl);
 	if (!crb_wait_for_reg_32(&priv->regs_h->loc_state, value, value,
 				 TPM2_TIMEOUT_C)) {
-		dev_warn(&chip->dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
+		dev_warn(dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
 		return -ETIME;
 	}
 
 	return 0;
 }
 
-static void crb_relinquish_locality(struct tpm_chip *chip, int loc)
+static int crb_request_locality(struct tpm_chip *chip, int loc)
 {
 	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
 
+	return __crb_request_locality(&chip->dev, priv, loc);
+}
+
+static int __crb_relinquish_locality(struct device *dev,
+				     struct crb_priv *priv, int loc)
+{
+	u32 mask = CRB_LOC_STATE_LOC_ASSIGNED |
+		   CRB_LOC_STATE_TPM_REG_VALID_STS;
+	u32 value = CRB_LOC_STATE_TPM_REG_VALID_STS;
+
 	if (!priv->regs_h)
-		return;
+		return 0;
 
 	iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);
+	if (!crb_wait_for_reg_32(&priv->regs_h->loc_state, mask, value,
+				 TPM2_TIMEOUT_C)) {
+		dev_warn(dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
+		return -ETIME;
+	}
+
+	return 0;
+}
+
+static int crb_relinquish_locality(struct tpm_chip *chip, int loc)
+{
+	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
+
+	return __crb_relinquish_locality(&chip->dev, priv, loc);
 }
 
 static u8 crb_status(struct tpm_chip *chip)
@@ -475,6 +504,10 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 			dev_warn(dev, FW_BUG "Bad ACPI memory layout");
 	}
 
+	ret = __crb_request_locality(dev, priv, 0);
+	if (ret)
+		return ret;
+
 	priv->regs_t = crb_map_res(dev, priv, &io_res, buf->control_address,
 				   sizeof(struct crb_regs_tail));
 	if (IS_ERR(priv->regs_t))
@@ -531,6 +564,8 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 
 	crb_go_idle(dev, priv);
 
+	__crb_relinquish_locality(dev, priv, 0);
+
 	return ret;
 }
 
@@ -588,10 +623,14 @@ static int crb_acpi_add(struct acpi_device *device)
 	chip->acpi_dev_handle = device->handle;
 	chip->flags = TPM_CHIP_FLAG_TPM2;
 
-	rc  = crb_cmd_ready(dev, priv);
+	rc = __crb_request_locality(dev, priv, 0);
 	if (rc)
 		return rc;
 
+	rc  = crb_cmd_ready(dev, priv);
+	if (rc)
+		goto out;
+
 	pm_runtime_get_noresume(dev);
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
@@ -601,12 +640,15 @@ static int crb_acpi_add(struct acpi_device *device)
 		crb_go_idle(dev, priv);
 		pm_runtime_put_noidle(dev);
 		pm_runtime_disable(dev);
-		return rc;
+		goto out;
 	}
 
-	pm_runtime_put(dev);
+	pm_runtime_put_sync(dev);
 
-	return 0;
+out:
+	__crb_relinquish_locality(dev, priv, 0);
+
+	return rc;
 }
 
 static int crb_acpi_remove(struct acpi_device *device)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 183a5f54d875..a22b12adbdfd 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -143,11 +143,13 @@ static bool check_locality(struct tpm_chip *chip, int l)
 	return false;
 }
 
-static void release_locality(struct tpm_chip *chip, int l)
+static int release_locality(struct tpm_chip *chip, int l)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 
 	tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
+
+	return 0;
 }
 
 static int request_locality(struct tpm_chip *chip, int l)
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index bcdd3790e94d..06639fb6ab85 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -44,7 +44,7 @@ struct tpm_class_ops {
 	bool (*update_timeouts)(struct tpm_chip *chip,
 				unsigned long *timeout_cap);
 	int (*request_locality)(struct tpm_chip *chip, int loc);
-	void (*relinquish_locality)(struct tpm_chip *chip, int loc);
+	int (*relinquish_locality)(struct tpm_chip *chip, int loc);
 	void (*clk_enable)(struct tpm_chip *chip, bool value);
 };
 
-- 
2.14.3

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2 v2] tpm: cmd_ready command can be issued only after granting locality

[Jason Gunthorpe]

On Sun, Jan 28, 2018 at 09:51:00AM +0200, Tomas Winkler wrote:

> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index bcdd3790e94d..06639fb6ab85 100644
> +++ b/include/linux/tpm.h
> @@ -44,7 +44,7 @@ struct tpm_class_ops {
>  	bool (*update_timeouts)(struct tpm_chip *chip,
>  				unsigned long *timeout_cap);
>  	int (*request_locality)(struct tpm_chip *chip, int loc);
> -	void (*relinquish_locality)(struct tpm_chip *chip, int loc);
> +	int (*relinquish_locality)(struct tpm_chip *chip, int loc);

This seems wrong.. What is the core code supposed to do if relinquish
fails?

Just returning an error code from transmit doesn't really do anything
helpful from a broad subsytem perspective.

I think if a driver can fail reliquish then it needs some kind of
strategy to recover.

Suggest trying the reliquish again on every next request until
success, otherwise fail request locality, potentially permanently.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2 v2] tpm: cmd_ready command can be issued only after granting locality

[Winkler, Tomas]

> 
> On Sun, Jan 28, 2018 at 09:51:00AM +0200, Tomas Winkler wrote:
> 
> > diff --git a/include/linux/tpm.h b/include/linux/tpm.h index
> > bcdd3790e94d..06639fb6ab85 100644
> > +++ b/include/linux/tpm.h
> > @@ -44,7 +44,7 @@ struct tpm_class_ops {
> >  	bool (*update_timeouts)(struct tpm_chip *chip,
> >  				unsigned long *timeout_cap);
> >  	int (*request_locality)(struct tpm_chip *chip, int loc);
> > -	void (*relinquish_locality)(struct tpm_chip *chip, int loc);
> > +	int (*relinquish_locality)(struct tpm_chip *chip, int loc);
> 
> This seems wrong.. What is the core code supposed to do if relinquish fails?

Not much just propage the error to the caller and leave the policy decision to it.

> Just returning an error code from transmit doesn't really do anything helpful
> from a broad subsytem perspective.

Yes, you are right, but I'm not sure even if the subsystem is broad enough to understand
the system setup,  or in another direction specific enough to behave upon hw limitations. 
> 
> I think if a driver can fail reliquish then it needs some kind of strategy to
> recover.
Maybe some driver can and some not, but if it doesn't succeed it should return an error.
> 
> Suggest trying the reliquish again on every next request until success,
> otherwise fail request locality, potentially permanently.

This is something I rather prevent because it leaves the HW in kind of undefined state 
( and we should probably work on that a bit more later).
As far as I've debugged the flow now, the driver just fails, and the error goes up 
user space caller or the internal flow is stopped.
A user can reboot the system or whatever it helps in his/her particular setup.

Make sense?

Anyhow I will dig to it more how fatal is that relinquish failure. 

Thanks
Tomas

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2 v2] tpm: cmd_ready command can be issued only after granting locality

[Jason Gunthorpe]

On Sun, Jan 28, 2018 at 09:17:53PM +0000, Winkler, Tomas wrote:

> > I think if a driver can fail reliquish then it needs some kind of strategy to
> > recover.

> Maybe some driver can and some not, but if it doesn't succeed it
> should return an error.

But you can't just leave the driver in some inconsistent state..

Every time I've audited something to do with 'add error codes to
destroy/free/release' I find driver design issues..

> > Suggest trying the reliquish again on every next request until success,
> > otherwise fail request locality, potentially permanently.
> 
> This is something I rather prevent because it leaves the HW in kind of undefined state 
> ( and we should probably work on that a bit more later).
> As far as I've debugged the flow now, the driver just fails, and the error goes up 
> user space caller or the internal flow is stopped.

But tranmist_command will be called again - then what does the driver
do? The driver needs an answer for that..

If you don't want to retry then I'd rather see request_locality
permanently fail then adding a return code to release.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2 v2] tpm: cmd_ready command can be issued only after granting locality

[Winkler, Tomas]

> On Sun, Jan 28, 2018 at 09:17:53PM +0000, Winkler, Tomas wrote:
> 
> > > I think if a driver can fail reliquish then it needs some kind of
> > > strategy to recover.
> 
> > Maybe some driver can and some not, but if it doesn't succeed it
> > should return an error.
> 
> But you can't just leave the driver in some inconsistent state..
> 
> Every time I've audited something to do with 'add error codes to
> destroy/free/release' I find driver design issues..

I'm sure of it, but from this particular point the driver itself is stateless, 
it's just reading HW state via registers. It's not going through driver state changes.

> > > Suggest trying the reliquish again on every next request until
> > > success, otherwise fail request locality, potentially permanently.
> >
> > This is something I rather prevent because it leaves the HW in kind of
> > undefined state ( and we should probably work on that a bit more later).
> > As far as I've debugged the flow now, the driver just fails, and the
> > error goes up user space caller or the internal flow is stopped.
> 
> But tranmist_command will be called again - then what does the driver do?
> The driver needs an answer for that..
It will just fail again
> 
> If you don't want to retry then I'd rather see request_locality permanently
> fail then adding a return code to release.

What do you mean exactly mean by permanently fail,  
My current assumption is that  it will fail permanently because the HW is not responsive
Or indicate error on any subsequent command, unless the hw recover somehow. 
Currently I'm not aware of any possibility to reset the device except rebooting the system.


Thanks
Tomas

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2 v2] tpm: cmd_ready command can be issued only after granting locality

[Jarkko Sakkinen]

On Sun, Jan 28, 2018 at 09:17:53PM +0000, Winkler, Tomas wrote:
> 
> > 
> > On Sun, Jan 28, 2018 at 09:51:00AM +0200, Tomas Winkler wrote:
> > 
> > > diff --git a/include/linux/tpm.h b/include/linux/tpm.h index
> > > bcdd3790e94d..06639fb6ab85 100644
> > > +++ b/include/linux/tpm.h
> > > @@ -44,7 +44,7 @@ struct tpm_class_ops {
> > >  	bool (*update_timeouts)(struct tpm_chip *chip,
> > >  				unsigned long *timeout_cap);
> > >  	int (*request_locality)(struct tpm_chip *chip, int loc);
> > > -	void (*relinquish_locality)(struct tpm_chip *chip, int loc);
> > > +	int (*relinquish_locality)(struct tpm_chip *chip, int loc);
> > 
> > This seems wrong.. What is the core code supposed to do if relinquish fails?
> 
> Not much just propage the error to the caller and leave the policy
> decision to it.

Your patch set must either cover this or keep it as void.

A better idea is to print an error to klog.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2 v2] tpm: cmd_ready command can be issued only after granting locality

[Winkler, Tomas]

> 
> On Sun, Jan 28, 2018 at 09:17:53PM +0000, Winkler, Tomas wrote:
> >
> > >
> > > On Sun, Jan 28, 2018 at 09:51:00AM +0200, Tomas Winkler wrote:
> > >
> > > > diff --git a/include/linux/tpm.h b/include/linux/tpm.h index
> > > > bcdd3790e94d..06639fb6ab85 100644
> > > > +++ b/include/linux/tpm.h
> > > > @@ -44,7 +44,7 @@ struct tpm_class_ops {
> > > >  	bool (*update_timeouts)(struct tpm_chip *chip,
> > > >  				unsigned long *timeout_cap);
> > > >  	int (*request_locality)(struct tpm_chip *chip, int loc);
> > > > -	void (*relinquish_locality)(struct tpm_chip *chip, int loc);
> > > > +	int (*relinquish_locality)(struct tpm_chip *chip, int loc);
> > >
> > > This seems wrong.. What is the core code supposed to do if relinquish
> fails?
> >
> > Not much just propage the error to the caller and leave the policy
> > decision to it.
> 
> Your patch set must either cover this or keep it as void.


How the code is covering other failures in the transmit functions,  
how is this one different from for example request_locality failure?
Why we should not propage this error up?

> 
> A better idea is to print an error to klog.
We can do that in addition.


Thanks
Tomas

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2 v2] tpm: cmd_ready command can be issued only after granting locality

[Jarkko Sakkinen]

On Tue, Feb 06, 2018 at 09:26:15PM +0000, Winkler, Tomas wrote:
> > 
> > On Sun, Jan 28, 2018 at 09:17:53PM +0000, Winkler, Tomas wrote:
> > >
> > > >
> > > > On Sun, Jan 28, 2018 at 09:51:00AM +0200, Tomas Winkler wrote:
> > > >
> > > > > diff --git a/include/linux/tpm.h b/include/linux/tpm.h index
> > > > > bcdd3790e94d..06639fb6ab85 100644
> > > > > +++ b/include/linux/tpm.h
> > > > > @@ -44,7 +44,7 @@ struct tpm_class_ops {
> > > > >  	bool (*update_timeouts)(struct tpm_chip *chip,
> > > > >  				unsigned long *timeout_cap);
> > > > >  	int (*request_locality)(struct tpm_chip *chip, int loc);
> > > > > -	void (*relinquish_locality)(struct tpm_chip *chip, int loc);
> > > > > +	int (*relinquish_locality)(struct tpm_chip *chip, int loc);
> > > >
> > > > This seems wrong.. What is the core code supposed to do if relinquish
> > fails?
> > >
> > > Not much just propage the error to the caller and leave the policy
> > > decision to it.
> > 
> > Your patch set must either cover this or keep it as void.
> 
> 
> How the code is covering other failures in the transmit functions,  
> how is this one different from for example request_locality failure?
> Why we should not propage this error up?
> 
> > 
> > A better idea is to print an error to klog.
> We can do that in addition.

I guess you are right. This can be propagated to the user space so that
it knows that there is problem. To make the root more visible the klog
message would make sense.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2 v2] tpm: cmd_ready command can be issued only after granting locality

[Winkler, Tomas]

> 
> On Tue, Feb 06, 2018 at 09:26:15PM +0000, Winkler, Tomas wrote:
> > >
> > > On Sun, Jan 28, 2018 at 09:17:53PM +0000, Winkler, Tomas wrote:
> > > >
> > > > >
> > > > > On Sun, Jan 28, 2018 at 09:51:00AM +0200, Tomas Winkler wrote:
> > > > >
> > > > > > diff --git a/include/linux/tpm.h b/include/linux/tpm.h index
> > > > > > bcdd3790e94d..06639fb6ab85 100644
> > > > > > +++ b/include/linux/tpm.h
> > > > > > @@ -44,7 +44,7 @@ struct tpm_class_ops {
> > > > > >  	bool (*update_timeouts)(struct tpm_chip *chip,
> > > > > >  				unsigned long *timeout_cap);
> > > > > >  	int (*request_locality)(struct tpm_chip *chip, int loc);
> > > > > > -	void (*relinquish_locality)(struct tpm_chip *chip, int loc);
> > > > > > +	int (*relinquish_locality)(struct tpm_chip *chip, int loc);
> > > > >
> > > > > This seems wrong.. What is the core code supposed to do if
> > > > > relinquish
> > > fails?
> > > >
> > > > Not much just propage the error to the caller and leave the policy
> > > > decision to it.
> > >
> > > Your patch set must either cover this or keep it as void.
> >
> >
> > How the code is covering other failures in the transmit functions, how
> > is this one different from for example request_locality failure?
> > Why we should not propage this error up?
> >
> > >
> > > A better idea is to print an error to klog.
> > We can do that in addition.
> 
> I guess you are right. This can be propagated to the user space so that it
> knows that there is problem. To make the root more visible the klog
> message would make sense.

Thanks, will add an error message.
Tomas


--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2 v2] tpm: separate cmd_ready/go_idle from runtime_pm

[Tomas Winkler]

We cannot use go_idle cmd_ready commands via runtime_pm handles
as with the introduction of localities this is no longer an optional
feature, while runtime pm can be not enabled.
Though cmd_ready/go_idle provides power saving feature, it's also part of
TPM2 protocol and should be called explicitly.
This patch exposes cmd_read/go_idle via tpm class ops and removes
runtime pm support as it is not used by any driver.

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
V2: resent
 drivers/char/tpm/tpm-interface.c | 15 ++++++---
 drivers/char/tpm/tpm_crb.c       | 69 +++++++++++++++++-----------------------
 include/linux/tpm.h              |  2 ++
 3 files changed, 42 insertions(+), 44 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 9fb3d406b078..0c493f885a9e 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -29,7 +29,6 @@
 #include <linux/mutex.h>
 #include <linux/spinlock.h>
 #include <linux/freezer.h>
-#include <linux/pm_runtime.h>
 #include <linux/tpm_eventlog.h>
 
 #include "tpm.h"
@@ -437,8 +436,11 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 		chip->locality = rc;
 	}
 
-	if (chip->dev.parent)
-		pm_runtime_get_sync(chip->dev.parent);
+	if (chip->ops->cmd_ready) {
+		rc = chip->ops->cmd_ready(chip);
+		if (rc)
+			goto out;
+	}
 
 	rc = tpm2_prepare_space(chip, space, ordinal, buf);
 	if (rc)
@@ -500,8 +502,11 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
 
 out:
-	if (chip->dev.parent)
-		pm_runtime_put_sync(chip->dev.parent);
+	if (chip->ops->go_idle) {
+		rc = chip->ops->go_idle(chip);
+		if (rc)
+			goto out;
+	}
 
 	if (need_locality && chip->ops->relinquish_locality) {
 		rc = chip->ops->relinquish_locality(chip, chip->locality);
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 497edd9848cd..acfcdc6f31af 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -132,7 +132,7 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
 }
 
 /**
- * crb_go_idle - request tpm crb device to go the idle state
+ * __crb_go_idle - request tpm crb device to go the idle state
  *
  * @dev:  crb device
  * @priv: crb private data
@@ -147,7 +147,7 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
  *
  * Return: 0 always
  */
-static int crb_go_idle(struct device *dev, struct crb_priv *priv)
+static int __crb_go_idle(struct device *dev, struct crb_priv *priv)
 {
 	if ((priv->sm == ACPI_TPM2_START_METHOD) ||
 	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) ||
@@ -166,8 +166,16 @@ static int crb_go_idle(struct device *dev, struct crb_priv *priv)
 	return 0;
 }
 
+static int crb_go_idle(struct tpm_chip *chip)
+{
+	struct device *dev = &chip->dev;
+	struct crb_priv *priv = dev_get_drvdata(dev);
+
+	return __crb_go_idle(dev, priv);
+}
+
 /**
- * crb_cmd_ready - request tpm crb device to enter ready state
+ * __crb_cmd_ready - request tpm crb device to enter ready state
  *
  * @dev:  crb device
  * @priv: crb private data
@@ -181,7 +189,7 @@ static int crb_go_idle(struct device *dev, struct crb_priv *priv)
  *
  * Return: 0 on success -ETIME on timeout;
  */
-static int crb_cmd_ready(struct device *dev, struct crb_priv *priv)
+static int __crb_cmd_ready(struct device *dev, struct crb_priv *priv)
 {
 	if ((priv->sm == ACPI_TPM2_START_METHOD) ||
 	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) ||
@@ -200,6 +208,14 @@ static int crb_cmd_ready(struct device *dev, struct crb_priv *priv)
 	return 0;
 }
 
+static int crb_cmd_ready(struct tpm_chip *chip)
+{
+	struct device *dev = &chip->dev;
+	struct crb_priv *priv = dev_get_drvdata(dev);
+
+	return __crb_cmd_ready(dev, priv);
+}
+
 static int __crb_request_locality(struct device *dev,
 				  struct crb_priv *priv, int loc)
 {
@@ -401,6 +417,8 @@ static const struct tpm_class_ops tpm_crb = {
 	.send = crb_send,
 	.cancel = crb_cancel,
 	.req_canceled = crb_req_canceled,
+	.go_idle  = crb_go_idle,
+	.cmd_ready = crb_cmd_ready,
 	.request_locality = crb_request_locality,
 	.relinquish_locality = crb_relinquish_locality,
 	.req_complete_mask = CRB_DRV_STS_COMPLETE,
@@ -517,7 +535,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	 * PTT HW bug w/a: wake up the device to access
 	 * possibly not retained registers.
 	 */
-	ret = crb_cmd_ready(dev, priv);
+	ret = __crb_cmd_ready(dev, priv);
 	if (ret)
 		return ret;
 
@@ -562,7 +580,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	if (!ret)
 		priv->cmd_size = cmd_size;
 
-	crb_go_idle(dev, priv);
+	__crb_go_idle(dev, priv);
 
 	__crb_relinquish_locality(dev, priv, 0);
 
@@ -627,23 +645,13 @@ static int crb_acpi_add(struct acpi_device *device)
 	if (rc)
 		return rc;
 
-	rc  = crb_cmd_ready(dev, priv);
+	rc  = __crb_cmd_ready(dev, priv);
 	if (rc)
 		goto out;
 
-	pm_runtime_get_noresume(dev);
-	pm_runtime_set_active(dev);
-	pm_runtime_enable(dev);
-
 	rc = tpm_chip_register(chip);
-	if (rc) {
-		crb_go_idle(dev, priv);
-		pm_runtime_put_noidle(dev);
-		pm_runtime_disable(dev);
-		goto out;
-	}
 
-	pm_runtime_put_sync(dev);
+	__crb_go_idle(dev, priv);
 
 out:
 	__crb_relinquish_locality(dev, priv, 0);
@@ -658,43 +666,27 @@ static int crb_acpi_remove(struct acpi_device *device)
 
 	tpm_chip_unregister(chip);
 
-	pm_runtime_disable(dev);
-
 	return 0;
 }
 
-static int __maybe_unused crb_pm_runtime_suspend(struct device *dev)
-{
-	struct tpm_chip *chip = dev_get_drvdata(dev);
-	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
-
-	return crb_go_idle(dev, priv);
-}
-
-static int __maybe_unused crb_pm_runtime_resume(struct device *dev)
-{
-	struct tpm_chip *chip = dev_get_drvdata(dev);
-	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
-
-	return crb_cmd_ready(dev, priv);
-}
-
 static int __maybe_unused crb_pm_suspend(struct device *dev)
 {
+	struct tpm_chip *chip = dev_get_drvdata(dev);
 	int ret;
 
 	ret = tpm_pm_suspend(dev);
 	if (ret)
 		return ret;
 
-	return crb_pm_runtime_suspend(dev);
+	return crb_go_idle(chip);
 }
 
 static int __maybe_unused crb_pm_resume(struct device *dev)
 {
+	struct tpm_chip *chip = dev_get_drvdata(dev);
 	int ret;
 
-	ret = crb_pm_runtime_resume(dev);
+	ret = crb_cmd_ready(chip);
 	if (ret)
 		return ret;
 
@@ -703,7 +695,6 @@ static int __maybe_unused crb_pm_resume(struct device *dev)
 
 static const struct dev_pm_ops crb_pm = {
 	SET_SYSTEM_SLEEP_PM_OPS(crb_pm_suspend, crb_pm_resume)
-	SET_RUNTIME_PM_OPS(crb_pm_runtime_suspend, crb_pm_runtime_resume, NULL)
 };
 
 static const struct acpi_device_id crb_device_ids[] = {
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 06639fb6ab85..8eb5e5ebe136 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -43,6 +43,8 @@ struct tpm_class_ops {
 	u8 (*status) (struct tpm_chip *chip);
 	bool (*update_timeouts)(struct tpm_chip *chip,
 				unsigned long *timeout_cap);
+	int (*go_idle)(struct tpm_chip *chip);
+	int (*cmd_ready)(struct tpm_chip *chip);
 	int (*request_locality)(struct tpm_chip *chip, int loc);
 	int (*relinquish_locality)(struct tpm_chip *chip, int loc);
 	void (*clk_enable)(struct tpm_chip *chip, bool value);
-- 
2.14.3

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2 v2] tpm: separate cmd_ready/go_idle from runtime_pm

[Jarkko Sakkinen]

On Sun, Jan 28, 2018 at 09:51:01AM +0200, Tomas Winkler wrote:
> We cannot use go_idle cmd_ready commands via runtime_pm handles
> as with the introduction of localities this is no longer an optional
> feature, while runtime pm can be not enabled.
> Though cmd_ready/go_idle provides power saving feature, it's also part of
> TPM2 protocol and should be called explicitly.
> This patch exposes cmd_read/go_idle via tpm class ops and removes
> runtime pm support as it is not used by any driver.
> 
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>

Makes sense so that only 1/2 woud need to be backported to stable
(1/2 should probably have cc for stable).

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html