Linux-Integrity Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/3] Enable vTPM 2.0 for the IBM vTPM driver
@ 2020-02-04 13:27 Stefan Berger
  2020-02-04 13:27 ` [PATCH 1/3] tpm: of: Handle IBM,vtpm20 case when getting log parameters Stefan Berger
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Stefan Berger @ 2020-02-04 13:27 UTC (permalink / raw)
  To: linux-integrity; +Cc: aik, david, linux-kernel, nayna, gcwilson, Stefan Berger

From: Stefan Berger <stefanb@linux.ibm.com>

QEMU 5.0 will support the PAPR vTPM device model for TPM 1.2 and TPM 2.0.
This series of patches enables vTPM 2.0 support for the IBM vTPM driver.

Regards,
   Stefan

Stefan Berger (3):
  tpm: of: Handle IBM,vtpm20 case when getting log parameters
  tpm: ibmvtpm: Wait for buffer to be set before proceeding
  tpm: ibmvtpm: Add support for TPM 2

 drivers/char/tpm/eventlog/of.c |  3 ++-
 drivers/char/tpm/tpm_ibmvtpm.c | 17 ++++++++++++++++-
 drivers/char/tpm/tpm_ibmvtpm.h |  1 +
 3 files changed, 19 insertions(+), 2 deletions(-)

-- 
2.23.0


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/3] tpm: of: Handle IBM,vtpm20 case when getting log parameters
  2020-02-04 13:27 [PATCH 0/3] Enable vTPM 2.0 for the IBM vTPM driver Stefan Berger
@ 2020-02-04 13:27 ` Stefan Berger
  2020-02-13 17:46   ` Nayna
  2020-02-04 13:27 ` [PATCH 2/3] tpm: ibmvtpm: Wait for buffer to be set before proceeding Stefan Berger
  2020-02-04 13:27 ` [PATCH 3/3] tpm: ibmvtpm: Add support for TPM 2 Stefan Berger
  2 siblings, 1 reply; 17+ messages in thread
From: Stefan Berger @ 2020-02-04 13:27 UTC (permalink / raw)
  To: linux-integrity; +Cc: aik, david, linux-kernel, nayna, gcwilson, Stefan Berger

From: Stefan Berger <stefanb@linux.ibm.com>

A vTPM 2.0 is identified by 'IBM,vtpm20' in the 'compatible' node in
the device tree. Handle it in the same way as 'IBM,vtpm'.

The vTPM 2.0's log is written in little endian format so that for this
aspect we can rely on existing code.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 drivers/char/tpm/eventlog/of.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
index af347c190819..a9ce66d09a75 100644
--- a/drivers/char/tpm/eventlog/of.c
+++ b/drivers/char/tpm/eventlog/of.c
@@ -51,7 +51,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
 	 * endian format. For this reason, vtpm doesn't need conversion
 	 * but physical tpm needs the conversion.
 	 */
-	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0) {
+	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
+	    of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
 		size = be32_to_cpup((__force __be32 *)sizep);
 		base = be64_to_cpup((__force __be64 *)basep);
 	} else {
-- 
2.23.0


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 2/3] tpm: ibmvtpm: Wait for buffer to be set before proceeding
  2020-02-04 13:27 [PATCH 0/3] Enable vTPM 2.0 for the IBM vTPM driver Stefan Berger
  2020-02-04 13:27 ` [PATCH 1/3] tpm: of: Handle IBM,vtpm20 case when getting log parameters Stefan Berger
@ 2020-02-04 13:27 ` Stefan Berger
  2020-02-13 17:53   ` Nayna
  2020-02-04 13:27 ` [PATCH 3/3] tpm: ibmvtpm: Add support for TPM 2 Stefan Berger
  2 siblings, 1 reply; 17+ messages in thread
From: Stefan Berger @ 2020-02-04 13:27 UTC (permalink / raw)
  To: linux-integrity; +Cc: aik, david, linux-kernel, nayna, gcwilson, Stefan Berger

From: Stefan Berger <stefanb@linux.ibm.com>

Synchronize with the results from the CRQs before continuing with
the initialization. This avoids trying to send TPM commands while
the rtce buffer has not been allocated, yet.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 drivers/char/tpm/tpm_ibmvtpm.c | 9 +++++++++
 drivers/char/tpm/tpm_ibmvtpm.h | 1 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index 78cc52690177..eee566eddb35 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -571,6 +571,7 @@ static irqreturn_t ibmvtpm_interrupt(int irq, void *vtpm_instance)
 	 */
 	while ((crq = ibmvtpm_crq_get_next(ibmvtpm)) != NULL) {
 		ibmvtpm_crq_process(crq, ibmvtpm);
+		wake_up_interruptible(&ibmvtpm->crq_queue.wq);
 		crq->valid = 0;
 		smp_wmb();
 	}
@@ -618,6 +619,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
 	}
 
 	crq_q->num_entry = CRQ_RES_BUF_SIZE / sizeof(*crq_q->crq_addr);
+	init_waitqueue_head(&crq_q->wq);
 	ibmvtpm->crq_dma_handle = dma_map_single(dev, crq_q->crq_addr,
 						 CRQ_RES_BUF_SIZE,
 						 DMA_BIDIRECTIONAL);
@@ -670,6 +672,13 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
 	if (rc)
 		goto init_irq_cleanup;
 
+	if (!wait_event_timeout(ibmvtpm->crq_queue.wq,
+				ibmvtpm->rtce_buf != NULL,
+				HZ)) {
+		dev_err(dev, "Initialization failed\n");
+		goto init_irq_cleanup;
+	}
+
 	return tpm_chip_register(chip);
 init_irq_cleanup:
 	do {
diff --git a/drivers/char/tpm/tpm_ibmvtpm.h b/drivers/char/tpm/tpm_ibmvtpm.h
index 7983f1a33267..b92aa7d3e93e 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.h
+++ b/drivers/char/tpm/tpm_ibmvtpm.h
@@ -26,6 +26,7 @@ struct ibmvtpm_crq_queue {
 	struct ibmvtpm_crq *crq_addr;
 	u32 index;
 	u32 num_entry;
+	wait_queue_head_t wq;
 };
 
 struct ibmvtpm_dev {
-- 
2.23.0


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 3/3] tpm: ibmvtpm: Add support for TPM 2
  2020-02-04 13:27 [PATCH 0/3] Enable vTPM 2.0 for the IBM vTPM driver Stefan Berger
  2020-02-04 13:27 ` [PATCH 1/3] tpm: of: Handle IBM,vtpm20 case when getting log parameters Stefan Berger
  2020-02-04 13:27 ` [PATCH 2/3] tpm: ibmvtpm: Wait for buffer to be set before proceeding Stefan Berger
@ 2020-02-04 13:27 ` Stefan Berger
  2020-02-13 17:53   ` Nayna
  2 siblings, 1 reply; 17+ messages in thread
From: Stefan Berger @ 2020-02-04 13:27 UTC (permalink / raw)
  To: linux-integrity; +Cc: aik, david, linux-kernel, nayna, gcwilson, Stefan Berger

From: Stefan Berger <stefanb@linux.ibm.com>

Support TPM 2 in the IBM vTPM driver. The hypervisor tells us what
version of TPM is connected through the vio_device_id.

In case a TPM 2 is found, we set the TPM_OPS_AUTO_STARTUP flag to
have properly initialize the TPM and driver.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 drivers/char/tpm/tpm_ibmvtpm.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index eee566eddb35..d479d64a65aa 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -29,6 +29,7 @@ static const char tpm_ibmvtpm_driver_name[] = "tpm_ibmvtpm";
 
 static const struct vio_device_id tpm_ibmvtpm_device_table[] = {
 	{ "IBM,vtpm", "IBM,vtpm"},
+	{ "IBM,vtpm", "IBM,vtpm20"},
 	{ "", "" }
 };
 MODULE_DEVICE_TABLE(vio, tpm_ibmvtpm_device_table);
@@ -443,7 +444,7 @@ static bool tpm_ibmvtpm_req_canceled(struct tpm_chip *chip, u8 status)
 	return (status == 0);
 }
 
-static const struct tpm_class_ops tpm_ibmvtpm = {
+static struct tpm_class_ops tpm_ibmvtpm = {
 	.recv = tpm_ibmvtpm_recv,
 	.send = tpm_ibmvtpm_send,
 	.cancel = tpm_ibmvtpm_cancel,
@@ -672,6 +673,11 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
 	if (rc)
 		goto init_irq_cleanup;
 
+	if (!strcmp(id->compat, "IBM,vtpm20")) {
+		chip->flags |= TPM_CHIP_FLAG_TPM2;
+		tpm_ibmvtpm.flags = TPM_OPS_AUTO_STARTUP;
+	}
+
 	if (!wait_event_timeout(ibmvtpm->crq_queue.wq,
 				ibmvtpm->rtce_buf != NULL,
 				HZ)) {
-- 
2.23.0


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/3] tpm: of: Handle IBM,vtpm20 case when getting log parameters
  2020-02-04 13:27 ` [PATCH 1/3] tpm: of: Handle IBM,vtpm20 case when getting log parameters Stefan Berger
@ 2020-02-13 17:46   ` Nayna
  2020-02-13 19:16     ` Stefan Berger
  0 siblings, 1 reply; 17+ messages in thread
From: Nayna @ 2020-02-13 17:46 UTC (permalink / raw)
  To: Stefan Berger, linux-integrity
  Cc: aik, david, linux-kernel, gcwilson, Stefan Berger


On 2/4/20 8:27 AM, Stefan Berger wrote:
> From: Stefan Berger <stefanb@linux.ibm.com>
>
> A vTPM 2.0 is identified by 'IBM,vtpm20' in the 'compatible' node in
> the device tree. Handle it in the same way as 'IBM,vtpm'.
>
> The vTPM 2.0's log is written in little endian format so that for this
> aspect we can rely on existing code.
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>   drivers/char/tpm/eventlog/of.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
> index af347c190819..a9ce66d09a75 100644
> --- a/drivers/char/tpm/eventlog/of.c
> +++ b/drivers/char/tpm/eventlog/of.c
> @@ -51,7 +51,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
>   	 * endian format. For this reason, vtpm doesn't need conversion
>   	 * but physical tpm needs the conversion.
>   	 */
> -	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0) {
> +	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
> +	    of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {

How about changing this to use of_device_compatible_match() ?

Thanks & Regards,

       - Nayna


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] tpm: ibmvtpm: Add support for TPM 2
  2020-02-04 13:27 ` [PATCH 3/3] tpm: ibmvtpm: Add support for TPM 2 Stefan Berger
@ 2020-02-13 17:53   ` Nayna
  2020-02-13 18:20     ` Stefan Berger
  0 siblings, 1 reply; 17+ messages in thread
From: Nayna @ 2020-02-13 17:53 UTC (permalink / raw)
  To: Stefan Berger, linux-integrity
  Cc: aik, david, linux-kernel, gcwilson, Stefan Berger


On 2/4/20 8:27 AM, Stefan Berger wrote:
> From: Stefan Berger <stefanb@linux.ibm.com>
>
> Support TPM 2 in the IBM vTPM driver. The hypervisor tells us what
> version of TPM is connected through the vio_device_id.
>
> In case a TPM 2 is found, we set the TPM_OPS_AUTO_STARTUP flag to
> have properly initialize the TPM and driver.
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>   drivers/char/tpm/tpm_ibmvtpm.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
> index eee566eddb35..d479d64a65aa 100644
> --- a/drivers/char/tpm/tpm_ibmvtpm.c
> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> @@ -29,6 +29,7 @@ static const char tpm_ibmvtpm_driver_name[] = "tpm_ibmvtpm";
>
>   static const struct vio_device_id tpm_ibmvtpm_device_table[] = {
>   	{ "IBM,vtpm", "IBM,vtpm"},
> +	{ "IBM,vtpm", "IBM,vtpm20"},
>   	{ "", "" }
>   };
>   MODULE_DEVICE_TABLE(vio, tpm_ibmvtpm_device_table);
> @@ -443,7 +444,7 @@ static bool tpm_ibmvtpm_req_canceled(struct tpm_chip *chip, u8 status)
>   	return (status == 0);
>   }
>
> -static const struct tpm_class_ops tpm_ibmvtpm = {
> +static struct tpm_class_ops tpm_ibmvtpm = {
>   	.recv = tpm_ibmvtpm_recv,
>   	.send = tpm_ibmvtpm_send,
>   	.cancel = tpm_ibmvtpm_cancel,
> @@ -672,6 +673,11 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
>   	if (rc)
>   		goto init_irq_cleanup;
>
> +	if (!strcmp(id->compat, "IBM,vtpm20")) {
> +		chip->flags |= TPM_CHIP_FLAG_TPM2;
> +		tpm_ibmvtpm.flags = TPM_OPS_AUTO_STARTUP;

TPM_OPS_AUTO_STARTUP flag isn't set for vTPM 1.2. What is different in 
case of vTPM 2.0 ?

Thanks & Regards,

      - Nayna


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/3] tpm: ibmvtpm: Wait for buffer to be set before proceeding
  2020-02-04 13:27 ` [PATCH 2/3] tpm: ibmvtpm: Wait for buffer to be set before proceeding Stefan Berger
@ 2020-02-13 17:53   ` Nayna
  2020-02-13 18:11     ` Stefan Berger
  0 siblings, 1 reply; 17+ messages in thread
From: Nayna @ 2020-02-13 17:53 UTC (permalink / raw)
  To: Stefan Berger, linux-integrity
  Cc: aik, david, linux-kernel, gcwilson, Stefan Berger


On 2/4/20 8:27 AM, Stefan Berger wrote:
> From: Stefan Berger <stefanb@linux.ibm.com>
>
> Synchronize with the results from the CRQs before continuing with
> the initialization. This avoids trying to send TPM commands while
> the rtce buffer has not been allocated, yet.

Seems like it is not specific to vtpm 2.0.  Shall it be clarified that 
it is a fix for vtpm 1.2 also ?

Thanks & Regards,

     - Nayna


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/3] tpm: ibmvtpm: Wait for buffer to be set before proceeding
  2020-02-13 17:53   ` Nayna
@ 2020-02-13 18:11     ` Stefan Berger
  0 siblings, 0 replies; 17+ messages in thread
From: Stefan Berger @ 2020-02-13 18:11 UTC (permalink / raw)
  To: Nayna, Stefan Berger, linux-integrity; +Cc: aik, david, linux-kernel, gcwilson

On 2/13/20 12:53 PM, Nayna wrote:
>
> On 2/4/20 8:27 AM, Stefan Berger wrote:
>> From: Stefan Berger <stefanb@linux.ibm.com>
>>
>> Synchronize with the results from the CRQs before continuing with
>> the initialization. This avoids trying to send TPM commands while
>> the rtce buffer has not been allocated, yet.
>
> Seems like it is not specific to vtpm 2.0.  Shall it be clarified that 
> it is a fix for vtpm 1.2 also ?

That's why it says 'TPM commands' and not 'TPM 2.0 commands'.


   Stefan

>
> Thanks & Regards,
>
>     - Nayna
>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] tpm: ibmvtpm: Add support for TPM 2
  2020-02-13 17:53   ` Nayna
@ 2020-02-13 18:20     ` Stefan Berger
  2020-02-13 18:35       ` Jason Gunthorpe
  0 siblings, 1 reply; 17+ messages in thread
From: Stefan Berger @ 2020-02-13 18:20 UTC (permalink / raw)
  To: Nayna, Stefan Berger, linux-integrity; +Cc: aik, david, linux-kernel, gcwilson

On 2/13/20 12:53 PM, Nayna wrote:
>
> On 2/4/20 8:27 AM, Stefan Berger wrote:
>> From: Stefan Berger <stefanb@linux.ibm.com>
>>
>> Support TPM 2 in the IBM vTPM driver. The hypervisor tells us what
>> version of TPM is connected through the vio_device_id.
>>
>> In case a TPM 2 is found, we set the TPM_OPS_AUTO_STARTUP flag to
>> have properly initialize the TPM and driver.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> ---
>>   drivers/char/tpm/tpm_ibmvtpm.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c 
>> b/drivers/char/tpm/tpm_ibmvtpm.c
>> index eee566eddb35..d479d64a65aa 100644
>> --- a/drivers/char/tpm/tpm_ibmvtpm.c
>> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
>> @@ -29,6 +29,7 @@ static const char tpm_ibmvtpm_driver_name[] = 
>> "tpm_ibmvtpm";
>>
>>   static const struct vio_device_id tpm_ibmvtpm_device_table[] = {
>>       { "IBM,vtpm", "IBM,vtpm"},
>> +    { "IBM,vtpm", "IBM,vtpm20"},
>>       { "", "" }
>>   };
>>   MODULE_DEVICE_TABLE(vio, tpm_ibmvtpm_device_table);
>> @@ -443,7 +444,7 @@ static bool tpm_ibmvtpm_req_canceled(struct 
>> tpm_chip *chip, u8 status)
>>       return (status == 0);
>>   }
>>
>> -static const struct tpm_class_ops tpm_ibmvtpm = {
>> +static struct tpm_class_ops tpm_ibmvtpm = {
>>       .recv = tpm_ibmvtpm_recv,
>>       .send = tpm_ibmvtpm_send,
>>       .cancel = tpm_ibmvtpm_cancel,
>> @@ -672,6 +673,11 @@ static int tpm_ibmvtpm_probe(struct vio_dev 
>> *vio_dev,
>>       if (rc)
>>           goto init_irq_cleanup;
>>
>> +    if (!strcmp(id->compat, "IBM,vtpm20")) {
>> +        chip->flags |= TPM_CHIP_FLAG_TPM2;
>> +        tpm_ibmvtpm.flags = TPM_OPS_AUTO_STARTUP;
>
> TPM_OPS_AUTO_STARTUP flag isn't set for vTPM 1.2. What is different in 
> case of vTPM 2.0 ?


I don't want side effects for the TPM 1.2 case here, so I am only 
modifying the flag for the case where the new TPM 2 is being used.  
Here's the code where it shows the effect.

int tpm_auto_startup(struct tpm_chip *chip)
{
     int rc;

     if (!(chip->ops->flags & TPM_OPS_AUTO_STARTUP))
         return 0;

     if (chip->flags & TPM_CHIP_FLAG_TPM2)
         rc = tpm2_auto_startup(chip);
     else
         rc = tpm1_auto_startup(chip);

     return rc;
}

In the TPM 2 case we then get timeouts, do the TPM self test, send 
TPM2_STARTUP if necessary and get attributes of the TPM 2 command from 
the device. All necessary to start it up.


https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/tpm2-cmd.c#L719

Does this answer your question ?


    Stefan




>
> Thanks & Regards,
>
>      - Nayna
>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] tpm: ibmvtpm: Add support for TPM 2
  2020-02-13 18:20     ` Stefan Berger
@ 2020-02-13 18:35       ` Jason Gunthorpe
  2020-02-13 19:04         ` Stefan Berger
  0 siblings, 1 reply; 17+ messages in thread
From: Jason Gunthorpe @ 2020-02-13 18:35 UTC (permalink / raw)
  To: Stefan Berger
  Cc: Nayna, Stefan Berger, linux-integrity, aik, david, linux-kernel,
	gcwilson

On Thu, Feb 13, 2020 at 01:20:12PM -0500, Stefan Berger wrote:

> I don't want side effects for the TPM 1.2 case here, so I am only modifying
> the flag for the case where the new TPM 2 is being used.  Here's the code
> where it shows the effect.

I'm surprised this driver is using AUTO_STARTUP, it was intended for
embedded cases where their is no firmware to boot the TPM.

Chips using AUTO_STARTUP are basically useless for PCRs/etc.

I'd expect somthing called vtpm to have been started and PCRs working
before Linux is started??

Jason

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] tpm: ibmvtpm: Add support for TPM 2
  2020-02-13 18:35       ` Jason Gunthorpe
@ 2020-02-13 19:04         ` Stefan Berger
  2020-02-13 19:11           ` Jason Gunthorpe
  0 siblings, 1 reply; 17+ messages in thread
From: Stefan Berger @ 2020-02-13 19:04 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Nayna, Stefan Berger, linux-integrity, aik, david, linux-kernel,
	gcwilson

On 2/13/20 1:35 PM, Jason Gunthorpe wrote:
> On Thu, Feb 13, 2020 at 01:20:12PM -0500, Stefan Berger wrote:
>
>> I don't want side effects for the TPM 1.2 case here, so I am only modifying
>> the flag for the case where the new TPM 2 is being used.  Here's the code
>> where it shows the effect.
> I'm surprised this driver is using AUTO_STARTUP, it was intended for
> embedded cases where their is no firmware to boot the TPM.


The TIS is also using it on any device.

static const struct tpm_class_ops tpm_tis = {
     .flags = TPM_OPS_AUTO_STARTUP,
     .status = tpm_tis_status,

https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/tpm_tis_core.c#L917


>
> Chips using AUTO_STARTUP are basically useless for PCRs/etc.
>
> I'd expect somthing called vtpm to have been started and PCRs working
> before Linux is started??

Yes, there's supposed to be firmware.

I only see one caller to tpm2_get_cc_attrs_tbl(chip), which is necessary 
to call. This caller happens to be in tpm2_auto_startup.


    Stefan


>
> Jason



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] tpm: ibmvtpm: Add support for TPM 2
  2020-02-13 19:04         ` Stefan Berger
@ 2020-02-13 19:11           ` Jason Gunthorpe
  2020-02-13 19:15             ` Stefan Berger
  0 siblings, 1 reply; 17+ messages in thread
From: Jason Gunthorpe @ 2020-02-13 19:11 UTC (permalink / raw)
  To: Stefan Berger
  Cc: Nayna, Stefan Berger, linux-integrity, aik, david, linux-kernel,
	gcwilson

On Thu, Feb 13, 2020 at 02:04:12PM -0500, Stefan Berger wrote:
> On 2/13/20 1:35 PM, Jason Gunthorpe wrote:
> > On Thu, Feb 13, 2020 at 01:20:12PM -0500, Stefan Berger wrote:
> > 
> > > I don't want side effects for the TPM 1.2 case here, so I am only modifying
> > > the flag for the case where the new TPM 2 is being used.  Here's the code
> > > where it shows the effect.
> > I'm surprised this driver is using AUTO_STARTUP, it was intended for
> > embedded cases where their is no firmware to boot the TPM.
> 
> The TIS is also using it on any device.

TIS is a generic driver, and can run on TPMs without firmware
support. It doesn't know either way

> > Chips using AUTO_STARTUP are basically useless for PCRs/etc.
> > 
> > I'd expect somthing called vtpm to have been started and PCRs working
> > before Linux is started??
> 
> Yes, there's supposed to be firmware.
> 
> I only see one caller to tpm2_get_cc_attrs_tbl(chip), which is necessary to
> call. This caller happens to be in tpm2_auto_startup.

That seems to be a mistake, proper startup of the driver should never
require auto_startup.

Jason

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] tpm: ibmvtpm: Add support for TPM 2
  2020-02-13 19:11           ` Jason Gunthorpe
@ 2020-02-13 19:15             ` Stefan Berger
  2020-02-13 19:39               ` Jason Gunthorpe
  0 siblings, 1 reply; 17+ messages in thread
From: Stefan Berger @ 2020-02-13 19:15 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Nayna, Stefan Berger, linux-integrity, aik, david, linux-kernel,
	gcwilson

On 2/13/20 2:11 PM, Jason Gunthorpe wrote:
> On Thu, Feb 13, 2020 at 02:04:12PM -0500, Stefan Berger wrote:
>> On 2/13/20 1:35 PM, Jason Gunthorpe wrote:
>>> On Thu, Feb 13, 2020 at 01:20:12PM -0500, Stefan Berger wrote:
>>>
>>>> I don't want side effects for the TPM 1.2 case here, so I am only modifying
>>>> the flag for the case where the new TPM 2 is being used.  Here's the code
>>>> where it shows the effect.
>>> I'm surprised this driver is using AUTO_STARTUP, it was intended for
>>> embedded cases where their is no firmware to boot the TPM.
>> The TIS is also using it on any device.
> TIS is a generic driver, and can run on TPMs without firmware
> support. It doesn't know either way

The following drivers are all using it:


drivers/char/tpm/st33zp24/st33zp24.c, line 493
drivers/char/tpm/tpm-interface.c, line 374
drivers/char/tpm/tpm_crb.c, line 421
drivers/char/tpm/tpm_ftpm_tee.c, line 184
drivers/char/tpm/tpm_i2c_atmel.c, line 139
drivers/char/tpm/tpm_i2c_infineon.c, line 602
drivers/char/tpm/tpm_i2c_nuvoton.c, line 465
drivers/char/tpm/tpm_tis_core.c, line 917
drivers/char/tpm/tpm_vtpm_proxy.c, line 435

https://elixir.bootlin.com/linux/latest/ident/TPM_OPS_AUTO_STARTUP


>
>>> Chips using AUTO_STARTUP are basically useless for PCRs/etc.
>>>
>>> I'd expect somthing called vtpm to have been started and PCRs working
>>> before Linux is started??
>> Yes, there's supposed to be firmware.
>>
>> I only see one caller to tpm2_get_cc_attrs_tbl(chip), which is necessary to
>> call. This caller happens to be in tpm2_auto_startup.
> That seems to be a mistake, proper startup of the driver should never
> require auto_startup.

Is this IBM vTPM driver special that it should do things differently 
than all those drivers listed above? From looking at the code is seems 
it is to be set for the TPM 2.0 case.


   Stefan



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/3] tpm: of: Handle IBM,vtpm20 case when getting log parameters
  2020-02-13 17:46   ` Nayna
@ 2020-02-13 19:16     ` Stefan Berger
  0 siblings, 0 replies; 17+ messages in thread
From: Stefan Berger @ 2020-02-13 19:16 UTC (permalink / raw)
  To: Nayna, Stefan Berger, linux-integrity; +Cc: aik, david, linux-kernel, gcwilson

On 2/13/20 12:46 PM, Nayna wrote:
>
> On 2/4/20 8:27 AM, Stefan Berger wrote:
>> From: Stefan Berger <stefanb@linux.ibm.com>
>>
>> A vTPM 2.0 is identified by 'IBM,vtpm20' in the 'compatible' node in
>> the device tree. Handle it in the same way as 'IBM,vtpm'.
>>
>> The vTPM 2.0's log is written in little endian format so that for this
>> aspect we can rely on existing code.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> ---
>>   drivers/char/tpm/eventlog/of.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/tpm/eventlog/of.c 
>> b/drivers/char/tpm/eventlog/of.c
>> index af347c190819..a9ce66d09a75 100644
>> --- a/drivers/char/tpm/eventlog/of.c
>> +++ b/drivers/char/tpm/eventlog/of.c
>> @@ -51,7 +51,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
>>        * endian format. For this reason, vtpm doesn't need conversion
>>        * but physical tpm needs the conversion.
>>        */
>> -    if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0) {
>> +    if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
>> +        of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
>
> How about changing this to use of_device_compatible_match() ?


I can change it.

    Stefan



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] tpm: ibmvtpm: Add support for TPM 2
  2020-02-13 19:15             ` Stefan Berger
@ 2020-02-13 19:39               ` Jason Gunthorpe
  2020-02-13 19:45                 ` Stefan Berger
  0 siblings, 1 reply; 17+ messages in thread
From: Jason Gunthorpe @ 2020-02-13 19:39 UTC (permalink / raw)
  To: Stefan Berger
  Cc: Nayna, Stefan Berger, linux-integrity, aik, david, linux-kernel,
	gcwilson

On Thu, Feb 13, 2020 at 02:15:03PM -0500, Stefan Berger wrote:
> On 2/13/20 2:11 PM, Jason Gunthorpe wrote:
> > On Thu, Feb 13, 2020 at 02:04:12PM -0500, Stefan Berger wrote:
> > > On 2/13/20 1:35 PM, Jason Gunthorpe wrote:
> > > > On Thu, Feb 13, 2020 at 01:20:12PM -0500, Stefan Berger wrote:
> > > > 
> > > > > I don't want side effects for the TPM 1.2 case here, so I am only modifying
> > > > > the flag for the case where the new TPM 2 is being used.  Here's the code
> > > > > where it shows the effect.
> > > > I'm surprised this driver is using AUTO_STARTUP, it was intended for
> > > > embedded cases where their is no firmware to boot the TPM.
> > > The TIS is also using it on any device.
> > TIS is a generic driver, and can run on TPMs without firmware
> > support. It doesn't know either way
> 
> The following drivers are all using it:
> 
> 
> drivers/char/tpm/st33zp24/st33zp24.c, line 493
> drivers/char/tpm/tpm-interface.c, line 374
> drivers/char/tpm/tpm_crb.c, line 421
> drivers/char/tpm/tpm_ftpm_tee.c, line 184
> drivers/char/tpm/tpm_i2c_atmel.c, line 139
> drivers/char/tpm/tpm_i2c_infineon.c, line 602
> drivers/char/tpm/tpm_i2c_nuvoton.c, line 465
> drivers/char/tpm/tpm_tis_core.c, line 917
> drivers/char/tpm/tpm_vtpm_proxy.c, line 435
> 
> https://elixir.bootlin.com/linux/latest/ident/TPM_OPS_AUTO_STARTUP

These are all general purpose drivers.

Though perhaps vtpm_proxy shouldn't include it, not sure.

> > > > Chips using AUTO_STARTUP are basically useless for PCRs/etc.
> > > > 
> > > > I'd expect somthing called vtpm to have been started and PCRs working
> > > > before Linux is started??
> > > Yes, there's supposed to be firmware.
> > > 
> > > I only see one caller to tpm2_get_cc_attrs_tbl(chip), which is necessary to
> > > call. This caller happens to be in tpm2_auto_startup.
> > That seems to be a mistake, proper startup of the driver should never
> > require auto_startup.
> 
> Is this IBM vTPM driver special that it should do things differently than
> all those drivers listed above? From looking at the code is seems it is to
> be set for the TPM 2.0 case.

Any driver that knows the TPM must be started prior to Linux
booting should not use the flag.  vtpm drivers in general would seem
to be the case where we can make this statement.

If it was mandatory then it would not be a flag the driver has to specify.

Jason

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] tpm: ibmvtpm: Add support for TPM 2
  2020-02-13 19:39               ` Jason Gunthorpe
@ 2020-02-13 19:45                 ` Stefan Berger
  2020-02-13 19:50                   ` Jason Gunthorpe
  0 siblings, 1 reply; 17+ messages in thread
From: Stefan Berger @ 2020-02-13 19:45 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Nayna, Stefan Berger, linux-integrity, aik, david, linux-kernel,
	gcwilson

On 2/13/20 2:39 PM, Jason Gunthorpe wrote:
> On Thu, Feb 13, 2020 at 02:15:03PM -0500, Stefan Berger wrote:
>> On 2/13/20 2:11 PM, Jason Gunthorpe wrote:
>>> On Thu, Feb 13, 2020 at 02:04:12PM -0500, Stefan Berger wrote:
>>>> On 2/13/20 1:35 PM, Jason Gunthorpe wrote:
>>>>> On Thu, Feb 13, 2020 at 01:20:12PM -0500, Stefan Berger wrote:
>>>>>
>>>>>> I don't want side effects for the TPM 1.2 case here, so I am only modifying
>>>>>> the flag for the case where the new TPM 2 is being used.  Here's the code
>>>>>> where it shows the effect.
>>>>> I'm surprised this driver is using AUTO_STARTUP, it was intended for
>>>>> embedded cases where their is no firmware to boot the TPM.
>>>> The TIS is also using it on any device.
>>> TIS is a generic driver, and can run on TPMs without firmware
>>> support. It doesn't know either way
>> The following drivers are all using it:
>>
>>
>> drivers/char/tpm/st33zp24/st33zp24.c, line 493
>> drivers/char/tpm/tpm-interface.c, line 374
>> drivers/char/tpm/tpm_crb.c, line 421
>> drivers/char/tpm/tpm_ftpm_tee.c, line 184
>> drivers/char/tpm/tpm_i2c_atmel.c, line 139
>> drivers/char/tpm/tpm_i2c_infineon.c, line 602
>> drivers/char/tpm/tpm_i2c_nuvoton.c, line 465
>> drivers/char/tpm/tpm_tis_core.c, line 917
>> drivers/char/tpm/tpm_vtpm_proxy.c, line 435
>>
>> https://elixir.bootlin.com/linux/latest/ident/TPM_OPS_AUTO_STARTUP
> These are all general purpose drivers.
>
> Though perhaps vtpm_proxy shouldn't include it, not sure.
>
>>>>> Chips using AUTO_STARTUP are basically useless for PCRs/etc.
>>>>>
>>>>> I'd expect somthing called vtpm to have been started and PCRs working
>>>>> before Linux is started??
>>>> Yes, there's supposed to be firmware.
>>>>
>>>> I only see one caller to tpm2_get_cc_attrs_tbl(chip), which is necessary to
>>>> call. This caller happens to be in tpm2_auto_startup.
>>> That seems to be a mistake, proper startup of the driver should never
>>> require auto_startup.
>> Is this IBM vTPM driver special that it should do things differently than
>> all those drivers listed above? From looking at the code is seems it is to
>> be set for the TPM 2.0 case.
> Any driver that knows the TPM must be started prior to Linux
> booting should not use the flag.  vtpm drivers in general would seem
> to be the case where we can make this statement.

Wouldn't this statement apply to all systems, including embedded ones?  
Basically all firmwares should implement the CRTM and do the TPM 
initialization.


>
> If it was mandatory then it would not be a flag the driver has to specify.

I'll add a case where we call into  tpm2_get_cc_attrs_tbl(chip) in case 
the auto startup flag is not set. I believe this is the only part that's 
missing for TPM 2 to work if not autostarted.


    Stefan


>
> Jason



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] tpm: ibmvtpm: Add support for TPM 2
  2020-02-13 19:45                 ` Stefan Berger
@ 2020-02-13 19:50                   ` Jason Gunthorpe
  0 siblings, 0 replies; 17+ messages in thread
From: Jason Gunthorpe @ 2020-02-13 19:50 UTC (permalink / raw)
  To: Stefan Berger
  Cc: Nayna, Stefan Berger, linux-integrity, aik, david, linux-kernel,
	gcwilson

On Thu, Feb 13, 2020 at 02:45:49PM -0500, Stefan Berger wrote:
> > Any driver that knows the TPM must be started prior to Linux
> > booting should not use the flag.  vtpm drivers in general would seem
> > to be the case where we can make this statement.
> 
> Wouldn't this statement apply to all systems, including embedded ones? 
> Basically all firmwares should implement the CRTM and do the TPM
> initialization.

It is not mandatory that systems with TPMs start it in the
firmware. That is only required if the TPM PCR feature is going to be
used. A TPM can quite happily be used for key storage without FW
support.

Arguably this is sort of done wrong. eg if the platform has provided
TPM information through uEFI or something then we shouldn't try to
auto start the system TPM. At least for TPM1 detecting a non-started
TPM was harmless, so nobody really cared. I wonder if TPM2 is much
different..

But certainly auto startup should *not* be required to have a working
TPM driver, that is just fundamentally wrong.

Jason

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, back to index

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-04 13:27 [PATCH 0/3] Enable vTPM 2.0 for the IBM vTPM driver Stefan Berger
2020-02-04 13:27 ` [PATCH 1/3] tpm: of: Handle IBM,vtpm20 case when getting log parameters Stefan Berger
2020-02-13 17:46   ` Nayna
2020-02-13 19:16     ` Stefan Berger
2020-02-04 13:27 ` [PATCH 2/3] tpm: ibmvtpm: Wait for buffer to be set before proceeding Stefan Berger
2020-02-13 17:53   ` Nayna
2020-02-13 18:11     ` Stefan Berger
2020-02-04 13:27 ` [PATCH 3/3] tpm: ibmvtpm: Add support for TPM 2 Stefan Berger
2020-02-13 17:53   ` Nayna
2020-02-13 18:20     ` Stefan Berger
2020-02-13 18:35       ` Jason Gunthorpe
2020-02-13 19:04         ` Stefan Berger
2020-02-13 19:11           ` Jason Gunthorpe
2020-02-13 19:15             ` Stefan Berger
2020-02-13 19:39               ` Jason Gunthorpe
2020-02-13 19:45                 ` Stefan Berger
2020-02-13 19:50                   ` Jason Gunthorpe

Linux-Integrity Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-integrity/0 linux-integrity/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-integrity linux-integrity/ https://lore.kernel.org/linux-integrity \
		linux-integrity@vger.kernel.org
	public-inbox-index linux-integrity

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-integrity


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git