[PATCH v6 0/3] Extend the vTPM proxy driver to pass locality

[PATCH v6 0/3] Extend the vTPM proxy driver to pass locality

[Stefan Berger]

The purpose of this series of patches is to enable the passing of the locality
a command is executing in to a recipient, i.e., TPM emulator. To enable this we
introduce vendor-specific TPM commands for TPM 1.2 and TPM 2 that the driver
sends to the TPM emulator.

v5->v6:
  - Added patch 3/3.

v4->v5:
  - introduce TPM_TRANSMIT_RAW to be able to call tpm_transmit without
    recursing into requesting the locality.

v3->v4:
  - addressed Jarkko's comments: largely a rewrite of the patches

v2->v3:
  - addressed Jarkko's comments

v1->v2:
  - fixed return value from function in patch 3/3


Stefan Berger (3):
  tpm: Introduce flag TPM_TRANSMIT_RAW
  tpm: vtpm_proxy: Implement request_locality function.
  tpm: vtpm_proxy: Prevent userspace from sending driver command

 drivers/char/tpm/tpm-interface.c  |  4 ++-
 drivers/char/tpm/tpm.h            |  1 +
 drivers/char/tpm/tpm_vtpm_proxy.c | 69 +++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/vtpm_proxy.h   |  4 +++
 4 files changed, 77 insertions(+), 1 deletion(-)

-- 
2.4.3

[PATCH v6 1/3] tpm: Introduce flag TPM_TRANSMIT_RAW

[Stefan Berger]

Introduce the flag TPM_TRANSMIT_RAW that allows us to transmit
a command without recursing into the requesting of locality.

Signed-off-by: Stefan Berger <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/char/tpm/tpm-interface.c | 3 ++-
 drivers/char/tpm/tpm.h           | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 4ed08ab..2eacda2 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -416,7 +416,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	/* Store the decision as chip->locality will be changed. */
 	need_locality = chip->locality == -1;
 
-	if (need_locality && chip->ops->request_locality)  {
+	if (!(flags & TPM_TRANSMIT_RAW) &&
+	    need_locality && chip->ops->request_locality)  {
 		rc = chip->ops->request_locality(chip, 0);
 		if (rc < 0)
 			goto out_no_locality;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index e81d8c7..25d9858 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -525,6 +525,7 @@ extern struct idr dev_nums_idr;
 
 enum tpm_transmit_flags {
 	TPM_TRANSMIT_UNLOCKED	= BIT(0),
+	TPM_TRANSMIT_RAW	= BIT(1),
 };
 
 ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
-- 
2.4.3


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

Re: [PATCH v6 1/3] tpm: Introduce flag TPM_TRANSMIT_RAW

[Jarkko Sakkinen]

On Wed, May 24, 2017 at 05:39:39PM -0400, Stefan Berger wrote:
> Introduce the flag TPM_TRANSMIT_RAW that allows us to transmit
> a command without recursing into the requesting of locality.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

I'll rename the constant as TPM_TRANSMIT_NO_LOCALITY if you don't mind.

/Jarkko

> ---
>  drivers/char/tpm/tpm-interface.c | 3 ++-
>  drivers/char/tpm/tpm.h           | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 4ed08ab..2eacda2 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -416,7 +416,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
>  	/* Store the decision as chip->locality will be changed. */
>  	need_locality = chip->locality == -1;
>  
> -	if (need_locality && chip->ops->request_locality)  {
> +	if (!(flags & TPM_TRANSMIT_RAW) &&
> +	    need_locality && chip->ops->request_locality)  {
>  		rc = chip->ops->request_locality(chip, 0);
>  		if (rc < 0)
>  			goto out_no_locality;
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index e81d8c7..25d9858 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -525,6 +525,7 @@ extern struct idr dev_nums_idr;
>  
>  enum tpm_transmit_flags {
>  	TPM_TRANSMIT_UNLOCKED	= BIT(0),
> +	TPM_TRANSMIT_RAW	= BIT(1),
>  };
>  
>  ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
> -- 
> 2.4.3
> 

Re: [PATCH v6 1/3] tpm: Introduce flag TPM_TRANSMIT_RAW

[Stefan Berger]

On 05/24/2017 06:18 PM, Jarkko Sakkinen wrote:
> On Wed, May 24, 2017 at 05:39:39PM -0400, Stefan Berger wrote:
>> Introduce the flag TPM_TRANSMIT_RAW that allows us to transmit
>> a command without recursing into the requesting of locality.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> I'll rename the constant as TPM_TRANSMIT_NO_LOCALITY if you don't mind.

Fine by me.


     Stefan

[PATCH v6 2/3] tpm: vtpm_proxy: Implement request_locality function.

[Stefan Berger]

Implement the request_locality function. To set the locality on the
backend we define vendor-specific TPM 1.2 and TPM 2 ordinals and send
a command to the backend to set the locality for the next commands.

To avoid recursing into requesting the locality, we set the
TPM_TRANSMIT_RAW flag when calling tpm_transmit_cmd. To avoid recursing
into TPM 2 space related commands, we set the space parameter to NULL.

Signed-off-by: Stefan Berger <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 drivers/char/tpm/tpm-interface.c  |  1 +
 drivers/char/tpm/tpm_vtpm_proxy.c | 36 ++++++++++++++++++++++++++++++++++++
 include/uapi/linux/vtpm_proxy.h   |  4 ++++
 3 files changed, 41 insertions(+)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 2eacda2..876d45f 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -537,6 +537,7 @@ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_space *space,
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(tpm_transmit_cmd);
 
 #define TPM_DIGEST_SIZE 20
 #define TPM_RET_CODE_IDX 6
diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
index 751059d..66024bf 100644
--- a/drivers/char/tpm/tpm_vtpm_proxy.c
+++ b/drivers/char/tpm/tpm_vtpm_proxy.c
@@ -371,6 +371,41 @@ static bool vtpm_proxy_tpm_req_canceled(struct tpm_chip  *chip, u8 status)
 	return ret;
 }
 
+static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
+{
+	struct tpm_buf buf;
+	int rc;
+	const struct tpm_output_header *header;
+
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS,
+				  TPM2_CC_SET_LOCALITY);
+	else
+		rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND,
+				  TPM_ORD_SET_LOCALITY);
+	if (rc)
+		return rc;
+	tpm_buf_append_u8(&buf, locality);
+
+	rc = tpm_transmit_cmd(chip, NULL, buf.data, tpm_buf_length(&buf), 0,
+			      TPM_TRANSMIT_UNLOCKED | TPM_TRANSMIT_RAW,
+			      "attempting to set locality");
+	if (rc < 0) {
+		locality = rc;
+		goto out;
+	}
+
+	header = (const struct tpm_output_header *)buf.data;
+	rc = be32_to_cpu(header->return_code);
+	if (rc)
+		locality = -1;
+
+out:
+	tpm_buf_destroy(&buf);
+
+	return locality;
+}
+
 static const struct tpm_class_ops vtpm_proxy_tpm_ops = {
 	.flags = TPM_OPS_AUTO_STARTUP,
 	.recv = vtpm_proxy_tpm_op_recv,
@@ -380,6 +415,7 @@ static const struct tpm_class_ops vtpm_proxy_tpm_ops = {
 	.req_complete_mask = VTPM_PROXY_REQ_COMPLETE_FLAG,
 	.req_complete_val = VTPM_PROXY_REQ_COMPLETE_FLAG,
 	.req_canceled = vtpm_proxy_tpm_req_canceled,
+	.request_locality = vtpm_proxy_request_locality,
 };
 
 /*
diff --git a/include/uapi/linux/vtpm_proxy.h b/include/uapi/linux/vtpm_proxy.h
index a69e991..58ac73c 100644
--- a/include/uapi/linux/vtpm_proxy.h
+++ b/include/uapi/linux/vtpm_proxy.h
@@ -46,4 +46,8 @@ struct vtpm_proxy_new_dev {
 
 #define VTPM_PROXY_IOC_NEW_DEV	_IOWR(0xa1, 0x00, struct vtpm_proxy_new_dev)
 
+/* vendor specific commands to set locality */
+#define TPM2_CC_SET_LOCALITY	0x20001000
+#define TPM_ORD_SET_LOCALITY	0x20001000
+
 #endif /* _UAPI_LINUX_VTPM_PROXY_H */
-- 
2.4.3


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

Re: [PATCH v6 2/3] tpm: vtpm_proxy: Implement request_locality function.

[Jarkko Sakkinen]

On Wed, May 24, 2017 at 05:39:40PM -0400, Stefan Berger wrote:
> Implement the request_locality function. To set the locality on the
> backend we define vendor-specific TPM 1.2 and TPM 2 ordinals and send
> a command to the backend to set the locality for the next commands.
> 
> To avoid recursing into requesting the locality, we set the
> TPM_TRANSMIT_RAW flag when calling tpm_transmit_cmd. To avoid recursing
> into TPM 2 space related commands, we set the space parameter to NULL.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>  drivers/char/tpm/tpm-interface.c  |  1 +
>  drivers/char/tpm/tpm_vtpm_proxy.c | 36 ++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/vtpm_proxy.h   |  4 ++++
>  3 files changed, 41 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 2eacda2..876d45f 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -537,6 +537,7 @@ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_space *space,
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(tpm_transmit_cmd);
>  
>  #define TPM_DIGEST_SIZE 20
>  #define TPM_RET_CODE_IDX 6
> diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
> index 751059d..66024bf 100644
> --- a/drivers/char/tpm/tpm_vtpm_proxy.c
> +++ b/drivers/char/tpm/tpm_vtpm_proxy.c
> @@ -371,6 +371,41 @@ static bool vtpm_proxy_tpm_req_canceled(struct tpm_chip  *chip, u8 status)
>  	return ret;
>  }
>  
> +static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
> +{
> +	struct tpm_buf buf;
> +	int rc;
> +	const struct tpm_output_header *header;
> +
> +	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> +		rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS,
> +				  TPM2_CC_SET_LOCALITY);

I would always go with this branch.

> +	else
> +		rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND,
> +				  TPM_ORD_SET_LOCALITY);
> +	if (rc)
> +		return rc;
> +	tpm_buf_append_u8(&buf, locality);
> +
> +	rc = tpm_transmit_cmd(chip, NULL, buf.data, tpm_buf_length(&buf), 0,
> +			      TPM_TRANSMIT_UNLOCKED | TPM_TRANSMIT_RAW,
> +			      "attempting to set locality");
> +	if (rc < 0) {
> +		locality = rc;
> +		goto out;
> +	}
> +
> +	header = (const struct tpm_output_header *)buf.data;
> +	rc = be32_to_cpu(header->return_code);
> +	if (rc)
> +		locality = -1;
> +
> +out:
> +	tpm_buf_destroy(&buf);
> +
> +	return locality;
> +}
> +
>  static const struct tpm_class_ops vtpm_proxy_tpm_ops = {
>  	.flags = TPM_OPS_AUTO_STARTUP,
>  	.recv = vtpm_proxy_tpm_op_recv,
> @@ -380,6 +415,7 @@ static const struct tpm_class_ops vtpm_proxy_tpm_ops = {
>  	.req_complete_mask = VTPM_PROXY_REQ_COMPLETE_FLAG,
>  	.req_complete_val = VTPM_PROXY_REQ_COMPLETE_FLAG,
>  	.req_canceled = vtpm_proxy_tpm_req_canceled,
> +	.request_locality = vtpm_proxy_request_locality,
>  };
>  
>  /*
> diff --git a/include/uapi/linux/vtpm_proxy.h b/include/uapi/linux/vtpm_proxy.h
> index a69e991..58ac73c 100644
> --- a/include/uapi/linux/vtpm_proxy.h
> +++ b/include/uapi/linux/vtpm_proxy.h
> @@ -46,4 +46,8 @@ struct vtpm_proxy_new_dev {
>  
>  #define VTPM_PROXY_IOC_NEW_DEV	_IOWR(0xa1, 0x00, struct vtpm_proxy_new_dev)
>  
> +/* vendor specific commands to set locality */
> +#define TPM2_CC_SET_LOCALITY	0x20001000
> +#define TPM_ORD_SET_LOCALITY	0x20001000
> +
>  #endif /* _UAPI_LINUX_VTPM_PROXY_H */
> -- 
> 2.4.3

What practical benefit you get from these two constants? I understand
the value range but not so much the redundant code.

/Jarkko

Re: [PATCH v6 2/3] tpm: vtpm_proxy: Implement request_locality function.

[Stefan Berger]

On 05/24/2017 06:21 PM, Jarkko Sakkinen wrote:
> On Wed, May 24, 2017 at 05:39:40PM -0400, Stefan Berger wrote:
>> Implement the request_locality function. To set the locality on the
>> backend we define vendor-specific TPM 1.2 and TPM 2 ordinals and send
>> a command to the backend to set the locality for the next commands.
>>
>> To avoid recursing into requesting the locality, we set the
>> TPM_TRANSMIT_RAW flag when calling tpm_transmit_cmd. To avoid recursing
>> into TPM 2 space related commands, we set the space parameter to NULL.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> ---
>>   drivers/char/tpm/tpm-interface.c  |  1 +
>>   drivers/char/tpm/tpm_vtpm_proxy.c | 36 ++++++++++++++++++++++++++++++++++++
>>   include/uapi/linux/vtpm_proxy.h   |  4 ++++
>>   3 files changed, 41 insertions(+)
>>
>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> index 2eacda2..876d45f 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -537,6 +537,7 @@ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_space *space,
>>   
>>   	return 0;
>>   }
>> +EXPORT_SYMBOL_GPL(tpm_transmit_cmd);
>>   
>>   #define TPM_DIGEST_SIZE 20
>>   #define TPM_RET_CODE_IDX 6
>> diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
>> index 751059d..66024bf 100644
>> --- a/drivers/char/tpm/tpm_vtpm_proxy.c
>> +++ b/drivers/char/tpm/tpm_vtpm_proxy.c
>> @@ -371,6 +371,41 @@ static bool vtpm_proxy_tpm_req_canceled(struct tpm_chip  *chip, u8 status)
>>   	return ret;
>>   }
>>   
>> +static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
>> +{
>> +	struct tpm_buf buf;
>> +	int rc;
>> +	const struct tpm_output_header *header;
>> +
>> +	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>> +		rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS,
>> +				  TPM2_CC_SET_LOCALITY);
> I would always go with this branch.
>
>> +	else
>> +		rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND,
>> +				  TPM_ORD_SET_LOCALITY);
>> +	if (rc)
>> +		return rc;
>> +	tpm_buf_append_u8(&buf, locality);
>> +
>> +	rc = tpm_transmit_cmd(chip, NULL, buf.data, tpm_buf_length(&buf), 0,
>> +			      TPM_TRANSMIT_UNLOCKED | TPM_TRANSMIT_RAW,
>> +			      "attempting to set locality");
>> +	if (rc < 0) {
>> +		locality = rc;
>> +		goto out;
>> +	}
>> +
>> +	header = (const struct tpm_output_header *)buf.data;
>> +	rc = be32_to_cpu(header->return_code);
>> +	if (rc)
>> +		locality = -1;
>> +
>> +out:
>> +	tpm_buf_destroy(&buf);
>> +
>> +	return locality;
>> +}
>> +
>>   static const struct tpm_class_ops vtpm_proxy_tpm_ops = {
>>   	.flags = TPM_OPS_AUTO_STARTUP,
>>   	.recv = vtpm_proxy_tpm_op_recv,
>> @@ -380,6 +415,7 @@ static const struct tpm_class_ops vtpm_proxy_tpm_ops = {
>>   	.req_complete_mask = VTPM_PROXY_REQ_COMPLETE_FLAG,
>>   	.req_complete_val = VTPM_PROXY_REQ_COMPLETE_FLAG,
>>   	.req_canceled = vtpm_proxy_tpm_req_canceled,
>> +	.request_locality = vtpm_proxy_request_locality,
>>   };
>>   
>>   /*
>> diff --git a/include/uapi/linux/vtpm_proxy.h b/include/uapi/linux/vtpm_proxy.h
>> index a69e991..58ac73c 100644
>> --- a/include/uapi/linux/vtpm_proxy.h
>> +++ b/include/uapi/linux/vtpm_proxy.h
>> @@ -46,4 +46,8 @@ struct vtpm_proxy_new_dev {
>>   
>>   #define VTPM_PROXY_IOC_NEW_DEV	_IOWR(0xa1, 0x00, struct vtpm_proxy_new_dev)
>>   
>> +/* vendor specific commands to set locality */
>> +#define TPM2_CC_SET_LOCALITY	0x20001000
>> +#define TPM_ORD_SET_LOCALITY	0x20001000
>> +
>>   #endif /* _UAPI_LINUX_VTPM_PROXY_H */
>> -- 
>> 2.4.3
> What practical benefit you get from these two constants? I understand
> the value range but not so much the redundant code.

If you want to change this, please go ahead and give it a sinle name.
For the code branches above I think that we should at least send a TPM 
1.2 formatted command in case of TPM 1.2 and a TPM 2 formatted one in 
case of TPM 2. It seems just 'proper.'

    Stefan

Re: [PATCH v6 2/3] tpm: vtpm_proxy: Implement request_locality function.

[Jarkko Sakkinen]

On Wed, May 24, 2017 at 07:03:27PM -0400, Stefan Berger wrote:
> On 05/24/2017 06:21 PM, Jarkko Sakkinen wrote:
> > On Wed, May 24, 2017 at 05:39:40PM -0400, Stefan Berger wrote:
> > > Implement the request_locality function. To set the locality on the
> > > backend we define vendor-specific TPM 1.2 and TPM 2 ordinals and send
> > > a command to the backend to set the locality for the next commands.
> > > 
> > > To avoid recursing into requesting the locality, we set the
> > > TPM_TRANSMIT_RAW flag when calling tpm_transmit_cmd. To avoid recursing
> > > into TPM 2 space related commands, we set the space parameter to NULL.
> > > 
> > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > > ---
> > >   drivers/char/tpm/tpm-interface.c  |  1 +
> > >   drivers/char/tpm/tpm_vtpm_proxy.c | 36 ++++++++++++++++++++++++++++++++++++
> > >   include/uapi/linux/vtpm_proxy.h   |  4 ++++
> > >   3 files changed, 41 insertions(+)
> > > 
> > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > > index 2eacda2..876d45f 100644
> > > --- a/drivers/char/tpm/tpm-interface.c
> > > +++ b/drivers/char/tpm/tpm-interface.c
> > > @@ -537,6 +537,7 @@ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_space *space,
> > >   	return 0;
> > >   }
> > > +EXPORT_SYMBOL_GPL(tpm_transmit_cmd);
> > >   #define TPM_DIGEST_SIZE 20
> > >   #define TPM_RET_CODE_IDX 6
> > > diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
> > > index 751059d..66024bf 100644
> > > --- a/drivers/char/tpm/tpm_vtpm_proxy.c
> > > +++ b/drivers/char/tpm/tpm_vtpm_proxy.c
> > > @@ -371,6 +371,41 @@ static bool vtpm_proxy_tpm_req_canceled(struct tpm_chip  *chip, u8 status)
> > >   	return ret;
> > >   }
> > > +static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
> > > +{
> > > +	struct tpm_buf buf;
> > > +	int rc;
> > > +	const struct tpm_output_header *header;
> > > +
> > > +	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> > > +		rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS,
> > > +				  TPM2_CC_SET_LOCALITY);
> > I would always go with this branch.
> > 
> > > +	else
> > > +		rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND,
> > > +				  TPM_ORD_SET_LOCALITY);
> > > +	if (rc)
> > > +		return rc;
> > > +	tpm_buf_append_u8(&buf, locality);
> > > +
> > > +	rc = tpm_transmit_cmd(chip, NULL, buf.data, tpm_buf_length(&buf), 0,
> > > +			      TPM_TRANSMIT_UNLOCKED | TPM_TRANSMIT_RAW,
> > > +			      "attempting to set locality");
> > > +	if (rc < 0) {
> > > +		locality = rc;
> > > +		goto out;
> > > +	}
> > > +
> > > +	header = (const struct tpm_output_header *)buf.data;
> > > +	rc = be32_to_cpu(header->return_code);
> > > +	if (rc)
> > > +		locality = -1;
> > > +
> > > +out:
> > > +	tpm_buf_destroy(&buf);
> > > +
> > > +	return locality;
> > > +}
> > > +
> > >   static const struct tpm_class_ops vtpm_proxy_tpm_ops = {
> > >   	.flags = TPM_OPS_AUTO_STARTUP,
> > >   	.recv = vtpm_proxy_tpm_op_recv,
> > > @@ -380,6 +415,7 @@ static const struct tpm_class_ops vtpm_proxy_tpm_ops = {
> > >   	.req_complete_mask = VTPM_PROXY_REQ_COMPLETE_FLAG,
> > >   	.req_complete_val = VTPM_PROXY_REQ_COMPLETE_FLAG,
> > >   	.req_canceled = vtpm_proxy_tpm_req_canceled,
> > > +	.request_locality = vtpm_proxy_request_locality,
> > >   };
> > >   /*
> > > diff --git a/include/uapi/linux/vtpm_proxy.h b/include/uapi/linux/vtpm_proxy.h
> > > index a69e991..58ac73c 100644
> > > --- a/include/uapi/linux/vtpm_proxy.h
> > > +++ b/include/uapi/linux/vtpm_proxy.h
> > > @@ -46,4 +46,8 @@ struct vtpm_proxy_new_dev {
> > >   #define VTPM_PROXY_IOC_NEW_DEV	_IOWR(0xa1, 0x00, struct vtpm_proxy_new_dev)
> > > +/* vendor specific commands to set locality */
> > > +#define TPM2_CC_SET_LOCALITY	0x20001000
> > > +#define TPM_ORD_SET_LOCALITY	0x20001000
> > > +
> > >   #endif /* _UAPI_LINUX_VTPM_PROXY_H */
> > > -- 
> > > 2.4.3
> > What practical benefit you get from these two constants? I understand
> > the value range but not so much the redundant code.
> 
> If you want to change this, please go ahead and give it a sinle name.
> For the code branches above I think that we should at least send a TPM 1.2
> formatted command in case of TPM 1.2 and a TPM 2 formatted one in case of
> TPM 2. It seems just 'proper.'
> 
>    Stefan

Agreed. I will make these changes no need to send new series.

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

Re: [PATCH v6 2/3] tpm: vtpm_proxy: Implement request_locality function.

[Stefan Berger]

On 05/24/2017 08:09 PM, Jarkko Sakkinen wrote:
> On Wed, May 24, 2017 at 07:03:27PM -0400, Stefan Berger wrote:
>> On 05/24/2017 06:21 PM, Jarkko Sakkinen wrote:
>>> On Wed, May 24, 2017 at 05:39:40PM -0400, Stefan Berger wrote:
>>>> Implement the request_locality function. To set the locality on the
>>>> backend we define vendor-specific TPM 1.2 and TPM 2 ordinals and send
>>>> a command to the backend to set the locality for the next commands.
>>>>
>>>> To avoid recursing into requesting the locality, we set the
>>>> TPM_TRANSMIT_RAW flag when calling tpm_transmit_cmd. To avoid recursing
>>>> into TPM 2 space related commands, we set the space parameter to NULL.
>>>>
>>>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>>> ---
>>>>    drivers/char/tpm/tpm-interface.c  |  1 +
>>>>    drivers/char/tpm/tpm_vtpm_proxy.c | 36 ++++++++++++++++++++++++++++++++++++
>>>>    include/uapi/linux/vtpm_proxy.h   |  4 ++++
>>>>    3 files changed, 41 insertions(+)
>>>>
>>>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>>>> index 2eacda2..876d45f 100644
>>>> --- a/drivers/char/tpm/tpm-interface.c
>>>> +++ b/drivers/char/tpm/tpm-interface.c
>>>> @@ -537,6 +537,7 @@ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_space *space,
>>>>    	return 0;
>>>>    }
>>>> +EXPORT_SYMBOL_GPL(tpm_transmit_cmd);
>>>>    #define TPM_DIGEST_SIZE 20
>>>>    #define TPM_RET_CODE_IDX 6
>>>> diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
>>>> index 751059d..66024bf 100644
>>>> --- a/drivers/char/tpm/tpm_vtpm_proxy.c
>>>> +++ b/drivers/char/tpm/tpm_vtpm_proxy.c
>>>> @@ -371,6 +371,41 @@ static bool vtpm_proxy_tpm_req_canceled(struct tpm_chip  *chip, u8 status)
>>>>    	return ret;
>>>>    }
>>>> +static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
>>>> +{
>>>> +	struct tpm_buf buf;
>>>> +	int rc;
>>>> +	const struct tpm_output_header *header;
>>>> +
>>>> +	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>>>> +		rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS,
>>>> +				  TPM2_CC_SET_LOCALITY);
>>> I would always go with this branch.
>>>
>>>> +	else
>>>> +		rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND,
>>>> +				  TPM_ORD_SET_LOCALITY);
>>>> +	if (rc)
>>>> +		return rc;
>>>> +	tpm_buf_append_u8(&buf, locality);
>>>> +
>>>> +	rc = tpm_transmit_cmd(chip, NULL, buf.data, tpm_buf_length(&buf), 0,
>>>> +			      TPM_TRANSMIT_UNLOCKED | TPM_TRANSMIT_RAW,
>>>> +			      "attempting to set locality");
>>>> +	if (rc < 0) {
>>>> +		locality = rc;
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	header = (const struct tpm_output_header *)buf.data;
>>>> +	rc = be32_to_cpu(header->return_code);
>>>> +	if (rc)
>>>> +		locality = -1;
>>>> +
>>>> +out:
>>>> +	tpm_buf_destroy(&buf);
>>>> +
>>>> +	return locality;
>>>> +}
>>>> +
>>>>    static const struct tpm_class_ops vtpm_proxy_tpm_ops = {
>>>>    	.flags = TPM_OPS_AUTO_STARTUP,
>>>>    	.recv = vtpm_proxy_tpm_op_recv,
>>>> @@ -380,6 +415,7 @@ static const struct tpm_class_ops vtpm_proxy_tpm_ops = {
>>>>    	.req_complete_mask = VTPM_PROXY_REQ_COMPLETE_FLAG,
>>>>    	.req_complete_val = VTPM_PROXY_REQ_COMPLETE_FLAG,
>>>>    	.req_canceled = vtpm_proxy_tpm_req_canceled,
>>>> +	.request_locality = vtpm_proxy_request_locality,
>>>>    };
>>>>    /*
>>>> diff --git a/include/uapi/linux/vtpm_proxy.h b/include/uapi/linux/vtpm_proxy.h
>>>> index a69e991..58ac73c 100644
>>>> --- a/include/uapi/linux/vtpm_proxy.h
>>>> +++ b/include/uapi/linux/vtpm_proxy.h
>>>> @@ -46,4 +46,8 @@ struct vtpm_proxy_new_dev {
>>>>    #define VTPM_PROXY_IOC_NEW_DEV	_IOWR(0xa1, 0x00, struct vtpm_proxy_new_dev)
>>>> +/* vendor specific commands to set locality */
>>>> +#define TPM2_CC_SET_LOCALITY	0x20001000
>>>> +#define TPM_ORD_SET_LOCALITY	0x20001000
>>>> +
>>>>    #endif /* _UAPI_LINUX_VTPM_PROXY_H */
>>>> -- 
>>>> 2.4.3
>>> What practical benefit you get from these two constants? I understand
>>> the value range but not so much the redundant code.
>> If you want to change this, please go ahead and give it a sinle name.
>> For the code branches above I think that we should at least send a TPM 1.2
>> formatted command in case of TPM 1.2 and a TPM 2 formatted one in case of
>> TPM 2. It seems just 'proper.'
>>
>>     Stefan
> Agreed. I will make these changes no need to send new series.

I pushed an update to the test suite, which may help the cause to accept 
this series.

https://github.com/stefanberger/linux-vtpm-tests

Patch 3/3 should probably not return -EFAULT in case of the user trying 
to send the SetLocality command via /dev/tpm%d but rather -EPIPE to get 
the logging of the error suppressed, even though that doesn't seem to be 
the right error code.


   Stefan

>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>
> /Jarkko
>

Re: [PATCH v6 2/3] tpm: vtpm_proxy: Implement request_locality function.

[Jarkko Sakkinen]

On Thu, Jun 01, 2017 at 08:34:31PM -0400, Stefan Berger wrote:
> On 05/24/2017 08:09 PM, Jarkko Sakkinen wrote:
> > On Wed, May 24, 2017 at 07:03:27PM -0400, Stefan Berger wrote:
> > > On 05/24/2017 06:21 PM, Jarkko Sakkinen wrote:
> > > > On Wed, May 24, 2017 at 05:39:40PM -0400, Stefan Berger wrote:
> > > > > Implement the request_locality function. To set the locality on the
> > > > > backend we define vendor-specific TPM 1.2 and TPM 2 ordinals and send
> > > > > a command to the backend to set the locality for the next commands.
> > > > > 
> > > > > To avoid recursing into requesting the locality, we set the
> > > > > TPM_TRANSMIT_RAW flag when calling tpm_transmit_cmd. To avoid recursing
> > > > > into TPM 2 space related commands, we set the space parameter to NULL.
> > > > > 
> > > > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > > > > ---
> > > > >    drivers/char/tpm/tpm-interface.c  |  1 +
> > > > >    drivers/char/tpm/tpm_vtpm_proxy.c | 36 ++++++++++++++++++++++++++++++++++++
> > > > >    include/uapi/linux/vtpm_proxy.h   |  4 ++++
> > > > >    3 files changed, 41 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > > > > index 2eacda2..876d45f 100644
> > > > > --- a/drivers/char/tpm/tpm-interface.c
> > > > > +++ b/drivers/char/tpm/tpm-interface.c
> > > > > @@ -537,6 +537,7 @@ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_space *space,
> > > > >    	return 0;
> > > > >    }
> > > > > +EXPORT_SYMBOL_GPL(tpm_transmit_cmd);
> > > > >    #define TPM_DIGEST_SIZE 20
> > > > >    #define TPM_RET_CODE_IDX 6
> > > > > diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
> > > > > index 751059d..66024bf 100644
> > > > > --- a/drivers/char/tpm/tpm_vtpm_proxy.c
> > > > > +++ b/drivers/char/tpm/tpm_vtpm_proxy.c
> > > > > @@ -371,6 +371,41 @@ static bool vtpm_proxy_tpm_req_canceled(struct tpm_chip  *chip, u8 status)
> > > > >    	return ret;
> > > > >    }
> > > > > +static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
> > > > > +{
> > > > > +	struct tpm_buf buf;
> > > > > +	int rc;
> > > > > +	const struct tpm_output_header *header;
> > > > > +
> > > > > +	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> > > > > +		rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS,
> > > > > +				  TPM2_CC_SET_LOCALITY);
> > > > I would always go with this branch.
> > > > 
> > > > > +	else
> > > > > +		rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND,
> > > > > +				  TPM_ORD_SET_LOCALITY);
> > > > > +	if (rc)
> > > > > +		return rc;
> > > > > +	tpm_buf_append_u8(&buf, locality);
> > > > > +
> > > > > +	rc = tpm_transmit_cmd(chip, NULL, buf.data, tpm_buf_length(&buf), 0,
> > > > > +			      TPM_TRANSMIT_UNLOCKED | TPM_TRANSMIT_RAW,
> > > > > +			      "attempting to set locality");
> > > > > +	if (rc < 0) {
> > > > > +		locality = rc;
> > > > > +		goto out;
> > > > > +	}
> > > > > +
> > > > > +	header = (const struct tpm_output_header *)buf.data;
> > > > > +	rc = be32_to_cpu(header->return_code);
> > > > > +	if (rc)
> > > > > +		locality = -1;
> > > > > +
> > > > > +out:
> > > > > +	tpm_buf_destroy(&buf);
> > > > > +
> > > > > +	return locality;
> > > > > +}
> > > > > +
> > > > >    static const struct tpm_class_ops vtpm_proxy_tpm_ops = {
> > > > >    	.flags = TPM_OPS_AUTO_STARTUP,
> > > > >    	.recv = vtpm_proxy_tpm_op_recv,
> > > > > @@ -380,6 +415,7 @@ static const struct tpm_class_ops vtpm_proxy_tpm_ops = {
> > > > >    	.req_complete_mask = VTPM_PROXY_REQ_COMPLETE_FLAG,
> > > > >    	.req_complete_val = VTPM_PROXY_REQ_COMPLETE_FLAG,
> > > > >    	.req_canceled = vtpm_proxy_tpm_req_canceled,
> > > > > +	.request_locality = vtpm_proxy_request_locality,
> > > > >    };
> > > > >    /*
> > > > > diff --git a/include/uapi/linux/vtpm_proxy.h b/include/uapi/linux/vtpm_proxy.h
> > > > > index a69e991..58ac73c 100644
> > > > > --- a/include/uapi/linux/vtpm_proxy.h
> > > > > +++ b/include/uapi/linux/vtpm_proxy.h
> > > > > @@ -46,4 +46,8 @@ struct vtpm_proxy_new_dev {
> > > > >    #define VTPM_PROXY_IOC_NEW_DEV	_IOWR(0xa1, 0x00, struct vtpm_proxy_new_dev)
> > > > > +/* vendor specific commands to set locality */
> > > > > +#define TPM2_CC_SET_LOCALITY	0x20001000
> > > > > +#define TPM_ORD_SET_LOCALITY	0x20001000
> > > > > +
> > > > >    #endif /* _UAPI_LINUX_VTPM_PROXY_H */
> > > > > -- 
> > > > > 2.4.3
> > > > What practical benefit you get from these two constants? I understand
> > > > the value range but not so much the redundant code.
> > > If you want to change this, please go ahead and give it a sinle name.
> > > For the code branches above I think that we should at least send a TPM 1.2
> > > formatted command in case of TPM 1.2 and a TPM 2 formatted one in case of
> > > TPM 2. It seems just 'proper.'
> > > 
> > >     Stefan
> > Agreed. I will make these changes no need to send new series.
> 
> I pushed an update to the test suite, which may help the cause to accept
> this series.
> 
> https://github.com/stefanberger/linux-vtpm-tests
> 
> Patch 3/3 should probably not return -EFAULT in case of the user trying to
> send the SetLocality command via /dev/tpm%d but rather -EPIPE to get the
> logging of the error suppressed, even though that doesn't seem to be the
> right error code.
> 
> 
>   Stefan
> 
> > 
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > 
> > /Jarkko
> > 

On hold as there are more high priority bug fixes in the queue.

/Jarkko

Re: [PATCH v6 2/3] tpm: vtpm_proxy: Implement request_locality function.

[Jarkko Sakkinen]

On Sun, Jun 04, 2017 at 06:43:32PM +0300, Jarkko Sakkinen wrote:
> On Thu, Jun 01, 2017 at 08:34:31PM -0400, Stefan Berger wrote:
> > On 05/24/2017 08:09 PM, Jarkko Sakkinen wrote:
> > > On Wed, May 24, 2017 at 07:03:27PM -0400, Stefan Berger wrote:
> > > > On 05/24/2017 06:21 PM, Jarkko Sakkinen wrote:
> > > > > On Wed, May 24, 2017 at 05:39:40PM -0400, Stefan Berger wrote:
> > > > > > Implement the request_locality function. To set the locality on the
> > > > > > backend we define vendor-specific TPM 1.2 and TPM 2 ordinals and send
> > > > > > a command to the backend to set the locality for the next commands.
> > > > > > 
> > > > > > To avoid recursing into requesting the locality, we set the
> > > > > > TPM_TRANSMIT_RAW flag when calling tpm_transmit_cmd. To avoid recursing
> > > > > > into TPM 2 space related commands, we set the space parameter to NULL.
> > > > > > 
> > > > > > Signed-off-by: Stefan Berger <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> > > > > > ---
> > > > > >    drivers/char/tpm/tpm-interface.c  |  1 +
> > > > > >    drivers/char/tpm/tpm_vtpm_proxy.c | 36 ++++++++++++++++++++++++++++++++++++
> > > > > >    include/uapi/linux/vtpm_proxy.h   |  4 ++++
> > > > > >    3 files changed, 41 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > > > > > index 2eacda2..876d45f 100644
> > > > > > --- a/drivers/char/tpm/tpm-interface.c
> > > > > > +++ b/drivers/char/tpm/tpm-interface.c
> > > > > > @@ -537,6 +537,7 @@ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_space *space,
> > > > > >    	return 0;
> > > > > >    }
> > > > > > +EXPORT_SYMBOL_GPL(tpm_transmit_cmd);
> > > > > >    #define TPM_DIGEST_SIZE 20
> > > > > >    #define TPM_RET_CODE_IDX 6
> > > > > > diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
> > > > > > index 751059d..66024bf 100644
> > > > > > --- a/drivers/char/tpm/tpm_vtpm_proxy.c
> > > > > > +++ b/drivers/char/tpm/tpm_vtpm_proxy.c
> > > > > > @@ -371,6 +371,41 @@ static bool vtpm_proxy_tpm_req_canceled(struct tpm_chip  *chip, u8 status)
> > > > > >    	return ret;
> > > > > >    }
> > > > > > +static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
> > > > > > +{
> > > > > > +	struct tpm_buf buf;
> > > > > > +	int rc;
> > > > > > +	const struct tpm_output_header *header;
> > > > > > +
> > > > > > +	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> > > > > > +		rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS,
> > > > > > +				  TPM2_CC_SET_LOCALITY);
> > > > > I would always go with this branch.
> > > > > 
> > > > > > +	else
> > > > > > +		rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND,
> > > > > > +				  TPM_ORD_SET_LOCALITY);
> > > > > > +	if (rc)
> > > > > > +		return rc;
> > > > > > +	tpm_buf_append_u8(&buf, locality);
> > > > > > +
> > > > > > +	rc = tpm_transmit_cmd(chip, NULL, buf.data, tpm_buf_length(&buf), 0,
> > > > > > +			      TPM_TRANSMIT_UNLOCKED | TPM_TRANSMIT_RAW,
> > > > > > +			      "attempting to set locality");
> > > > > > +	if (rc < 0) {
> > > > > > +		locality = rc;
> > > > > > +		goto out;
> > > > > > +	}
> > > > > > +
> > > > > > +	header = (const struct tpm_output_header *)buf.data;
> > > > > > +	rc = be32_to_cpu(header->return_code);
> > > > > > +	if (rc)
> > > > > > +		locality = -1;
> > > > > > +
> > > > > > +out:
> > > > > > +	tpm_buf_destroy(&buf);
> > > > > > +
> > > > > > +	return locality;
> > > > > > +}
> > > > > > +
> > > > > >    static const struct tpm_class_ops vtpm_proxy_tpm_ops = {
> > > > > >    	.flags = TPM_OPS_AUTO_STARTUP,
> > > > > >    	.recv = vtpm_proxy_tpm_op_recv,
> > > > > > @@ -380,6 +415,7 @@ static const struct tpm_class_ops vtpm_proxy_tpm_ops = {
> > > > > >    	.req_complete_mask = VTPM_PROXY_REQ_COMPLETE_FLAG,
> > > > > >    	.req_complete_val = VTPM_PROXY_REQ_COMPLETE_FLAG,
> > > > > >    	.req_canceled = vtpm_proxy_tpm_req_canceled,
> > > > > > +	.request_locality = vtpm_proxy_request_locality,
> > > > > >    };
> > > > > >    /*
> > > > > > diff --git a/include/uapi/linux/vtpm_proxy.h b/include/uapi/linux/vtpm_proxy.h
> > > > > > index a69e991..58ac73c 100644
> > > > > > --- a/include/uapi/linux/vtpm_proxy.h
> > > > > > +++ b/include/uapi/linux/vtpm_proxy.h
> > > > > > @@ -46,4 +46,8 @@ struct vtpm_proxy_new_dev {
> > > > > >    #define VTPM_PROXY_IOC_NEW_DEV	_IOWR(0xa1, 0x00, struct vtpm_proxy_new_dev)
> > > > > > +/* vendor specific commands to set locality */
> > > > > > +#define TPM2_CC_SET_LOCALITY	0x20001000
> > > > > > +#define TPM_ORD_SET_LOCALITY	0x20001000
> > > > > > +
> > > > > >    #endif /* _UAPI_LINUX_VTPM_PROXY_H */
> > > > > > -- 
> > > > > > 2.4.3
> > > > > What practical benefit you get from these two constants? I understand
> > > > > the value range but not so much the redundant code.
> > > > If you want to change this, please go ahead and give it a sinle name.
> > > > For the code branches above I think that we should at least send a TPM 1.2
> > > > formatted command in case of TPM 1.2 and a TPM 2 formatted one in case of
> > > > TPM 2. It seems just 'proper.'
> > > > 
> > > >     Stefan
> > > Agreed. I will make these changes no need to send new series.
> > 
> > I pushed an update to the test suite, which may help the cause to accept
> > this series.
> > 
> > https://github.com/stefanberger/linux-vtpm-tests
> > 
> > Patch 3/3 should probably not return -EFAULT in case of the user trying to
> > send the SetLocality command via /dev/tpm%d but rather -EPIPE to get the
> > logging of the error suppressed, even though that doesn't seem to be the
> > right error code.
> > 
> > 
> >   Stefan
> > 
> > > 
> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > > 
> > > /Jarkko
> > > 
> 
> On hold as there are more high priority bug fixes in the queue.
> 
> /Jarkko

Patches are now applied to master (bleeding edge) brach. After they are
tested they can be put also to the next branch.

/Jarkko

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

Re: [PATCH v6 2/3] tpm: vtpm_proxy: Implement request_locality function.

[Stefan Berger]

On 06/04/2017 12:27 PM, Jarkko Sakkinen wrote:
> On Sun, Jun 04, 2017 at 06:43:32PM +0300, Jarkko Sakkinen wrote:
>> On Thu, Jun 01, 2017 at 08:34:31PM -0400, Stefan Berger wrote:
>>> On 05/24/2017 08:09 PM, Jarkko Sakkinen wrote:
>>>> On Wed, May 24, 2017 at 07:03:27PM -0400, Stefan Berger wrote:
>>>>> On 05/24/2017 06:21 PM, Jarkko Sakkinen wrote:
>>>>>> On Wed, May 24, 2017 at 05:39:40PM -0400, Stefan Berger wrote:
>>>>>>> Implement the request_locality function. To set the locality on the
>>>>>>> backend we define vendor-specific TPM 1.2 and TPM 2 ordinals and send
>>>>>>> a command to the backend to set the locality for the next commands.
>>>>>>>
>>>>>>> To avoid recursing into requesting the locality, we set the
>>>>>>> TPM_TRANSMIT_RAW flag when calling tpm_transmit_cmd. To avoid recursing
>>>>>>> into TPM 2 space related commands, we set the space parameter to NULL.
>>>>>>>
>>>>>>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>>>>>> ---
>>>>>>>     drivers/char/tpm/tpm-interface.c  |  1 +
>>>>>>>     drivers/char/tpm/tpm_vtpm_proxy.c | 36 ++++++++++++++++++++++++++++++++++++
>>>>>>>     include/uapi/linux/vtpm_proxy.h   |  4 ++++
>>>>>>>     3 files changed, 41 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>>>>>>> index 2eacda2..876d45f 100644
>>>>>>> --- a/drivers/char/tpm/tpm-interface.c
>>>>>>> +++ b/drivers/char/tpm/tpm-interface.c
>>>>>>> @@ -537,6 +537,7 @@ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_space *space,
>>>>>>>     	return 0;
>>>>>>>     }
>>>>>>> +EXPORT_SYMBOL_GPL(tpm_transmit_cmd);
>>>>>>>     #define TPM_DIGEST_SIZE 20
>>>>>>>     #define TPM_RET_CODE_IDX 6
>>>>>>> diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
>>>>>>> index 751059d..66024bf 100644
>>>>>>> --- a/drivers/char/tpm/tpm_vtpm_proxy.c
>>>>>>> +++ b/drivers/char/tpm/tpm_vtpm_proxy.c
>>>>>>> @@ -371,6 +371,41 @@ static bool vtpm_proxy_tpm_req_canceled(struct tpm_chip  *chip, u8 status)
>>>>>>>     	return ret;
>>>>>>>     }
>>>>>>> +static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
>>>>>>> +{
>>>>>>> +	struct tpm_buf buf;
>>>>>>> +	int rc;
>>>>>>> +	const struct tpm_output_header *header;
>>>>>>> +
>>>>>>> +	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>>>>>>> +		rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS,
>>>>>>> +				  TPM2_CC_SET_LOCALITY);
>>>>>> I would always go with this branch.
>>>>>>
>>>>>>> +	else
>>>>>>> +		rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND,
>>>>>>> +				  TPM_ORD_SET_LOCALITY);
>>>>>>> +	if (rc)
>>>>>>> +		return rc;
>>>>>>> +	tpm_buf_append_u8(&buf, locality);
>>>>>>> +
>>>>>>> +	rc = tpm_transmit_cmd(chip, NULL, buf.data, tpm_buf_length(&buf), 0,
>>>>>>> +			      TPM_TRANSMIT_UNLOCKED | TPM_TRANSMIT_RAW,
>>>>>>> +			      "attempting to set locality");
>>>>>>> +	if (rc < 0) {
>>>>>>> +		locality = rc;
>>>>>>> +		goto out;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	header = (const struct tpm_output_header *)buf.data;
>>>>>>> +	rc = be32_to_cpu(header->return_code);
>>>>>>> +	if (rc)
>>>>>>> +		locality = -1;
>>>>>>> +
>>>>>>> +out:
>>>>>>> +	tpm_buf_destroy(&buf);
>>>>>>> +
>>>>>>> +	return locality;
>>>>>>> +}
>>>>>>> +
>>>>>>>     static const struct tpm_class_ops vtpm_proxy_tpm_ops = {
>>>>>>>     	.flags = TPM_OPS_AUTO_STARTUP,
>>>>>>>     	.recv = vtpm_proxy_tpm_op_recv,
>>>>>>> @@ -380,6 +415,7 @@ static const struct tpm_class_ops vtpm_proxy_tpm_ops = {
>>>>>>>     	.req_complete_mask = VTPM_PROXY_REQ_COMPLETE_FLAG,
>>>>>>>     	.req_complete_val = VTPM_PROXY_REQ_COMPLETE_FLAG,
>>>>>>>     	.req_canceled = vtpm_proxy_tpm_req_canceled,
>>>>>>> +	.request_locality = vtpm_proxy_request_locality,
>>>>>>>     };
>>>>>>>     /*
>>>>>>> diff --git a/include/uapi/linux/vtpm_proxy.h b/include/uapi/linux/vtpm_proxy.h
>>>>>>> index a69e991..58ac73c 100644
>>>>>>> --- a/include/uapi/linux/vtpm_proxy.h
>>>>>>> +++ b/include/uapi/linux/vtpm_proxy.h
>>>>>>> @@ -46,4 +46,8 @@ struct vtpm_proxy_new_dev {
>>>>>>>     #define VTPM_PROXY_IOC_NEW_DEV	_IOWR(0xa1, 0x00, struct vtpm_proxy_new_dev)
>>>>>>> +/* vendor specific commands to set locality */
>>>>>>> +#define TPM2_CC_SET_LOCALITY	0x20001000
>>>>>>> +#define TPM_ORD_SET_LOCALITY	0x20001000
>>>>>>> +
>>>>>>>     #endif /* _UAPI_LINUX_VTPM_PROXY_H */
>>>>>>> -- 
>>>>>>> 2.4.3
>>>>>> What practical benefit you get from these two constants? I understand
>>>>>> the value range but not so much the redundant code.
>>>>> If you want to change this, please go ahead and give it a sinle name.
>>>>> For the code branches above I think that we should at least send a TPM 1.2
>>>>> formatted command in case of TPM 1.2 and a TPM 2 formatted one in case of
>>>>> TPM 2. It seems just 'proper.'
>>>>>
>>>>>      Stefan
>>>> Agreed. I will make these changes no need to send new series.
>>> I pushed an update to the test suite, which may help the cause to accept
>>> this series.
>>>
>>> https://github.com/stefanberger/linux-vtpm-tests
>>>
>>> Patch 3/3 should probably not return -EFAULT in case of the user trying to
>>> send the SetLocality command via /dev/tpm%d but rather -EPIPE to get the
>>> logging of the error suppressed, even though that doesn't seem to be the
>>> right error code.
>>>
>>>
>>>    Stefan
>>>
>>>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>>>
>>>> /Jarkko
>>>>
>> On hold as there are more high priority bug fixes in the queue.
>>
>> /Jarkko
> Patches are now applied to master (bleeding edge) brach. After they are
> tested they can be put also to the next branch.

Thanks. Pulled and tested.

Tested-by: Stefan Berger <stefanb@linux.vnet.ibm.com>

[PATCH v6 3/3] tpm: vtpm_proxy: Prevent userspace from sending driver command

[Stefan Berger]

To prevent userspace from sending the TPM driver command to set
the locality, we need to check every command that is sent from
user space. To distinguish user space commands from internally
sent commands we introduce an additional state flag
STATE_DRIVER_COMMAND that is set while the driver sends this
command. Similar to the TPM 2 space commands we return an error
code when this command is detected.

Signed-off-by: Stefan Berger <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 drivers/char/tpm/tpm_vtpm_proxy.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
index 66024bf..1d877cc 100644
--- a/drivers/char/tpm/tpm_vtpm_proxy.c
+++ b/drivers/char/tpm/tpm_vtpm_proxy.c
@@ -43,6 +43,7 @@ struct proxy_dev {
 #define STATE_OPENED_FLAG        BIT(0)
 #define STATE_WAIT_RESPONSE_FLAG BIT(1)  /* waiting for emulator response */
 #define STATE_REGISTERED_FLAG	 BIT(2)
+#define STATE_DRIVER_COMMAND     BIT(3)  /* sending a driver specific command */
 
 	size_t req_len;              /* length of queued TPM request */
 	size_t resp_len;             /* length of queued TPM response */
@@ -299,6 +300,28 @@ static int vtpm_proxy_tpm_op_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 	return len;
 }
 
+static int vtpm_proxy_is_driver_command(struct tpm_chip *chip,
+					u8 *buf, size_t count)
+{
+	struct tpm_input_header *hdr = (struct tpm_input_header *)buf;
+
+	if (count < sizeof(struct tpm_input_header))
+		return 0;
+
+	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+		switch (be32_to_cpu(hdr->ordinal)) {
+		case TPM2_CC_SET_LOCALITY:
+			return 1;
+		}
+	} else {
+		switch (be32_to_cpu(hdr->ordinal)) {
+		case TPM_ORD_SET_LOCALITY:
+			return 1;
+		}
+	}
+	return 0;
+}
+
 /*
  * Called when core TPM driver forwards TPM requests to 'server side'.
  *
@@ -321,6 +344,10 @@ static int vtpm_proxy_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t count)
 		return -EIO;
 	}
 
+	if (!(proxy_dev->state & STATE_DRIVER_COMMAND) &&
+	    vtpm_proxy_is_driver_command(chip, buf, count))
+		return -EFAULT;
+
 	mutex_lock(&proxy_dev->buf_lock);
 
 	if (!(proxy_dev->state & STATE_OPENED_FLAG)) {
@@ -376,6 +403,7 @@ static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
 	struct tpm_buf buf;
 	int rc;
 	const struct tpm_output_header *header;
+	struct proxy_dev *proxy_dev = dev_get_drvdata(&chip->dev);
 
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS,
@@ -387,9 +415,14 @@ static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
 		return rc;
 	tpm_buf_append_u8(&buf, locality);
 
+	proxy_dev->state |= STATE_DRIVER_COMMAND;
+
 	rc = tpm_transmit_cmd(chip, NULL, buf.data, tpm_buf_length(&buf), 0,
 			      TPM_TRANSMIT_UNLOCKED | TPM_TRANSMIT_RAW,
 			      "attempting to set locality");
+
+	proxy_dev->state &= ~STATE_DRIVER_COMMAND;
+
 	if (rc < 0) {
 		locality = rc;
 		goto out;
-- 
2.4.3


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

Re: [PATCH v6 3/3] tpm: vtpm_proxy: Prevent userspace from sending driver command

[Jarkko Sakkinen]

On Wed, May 24, 2017 at 05:39:41PM -0400, Stefan Berger wrote:
> To prevent userspace from sending the TPM driver command to set
> the locality, we need to check every command that is sent from
> user space. To distinguish user space commands from internally
> sent commands we introduce an additional state flag
> STATE_DRIVER_COMMAND that is set while the driver sends this
> command. Similar to the TPM 2 space commands we return an error
> code when this command is detected.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>  drivers/char/tpm/tpm_vtpm_proxy.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
> index 66024bf..1d877cc 100644
> --- a/drivers/char/tpm/tpm_vtpm_proxy.c
> +++ b/drivers/char/tpm/tpm_vtpm_proxy.c
> @@ -43,6 +43,7 @@ struct proxy_dev {
>  #define STATE_OPENED_FLAG        BIT(0)
>  #define STATE_WAIT_RESPONSE_FLAG BIT(1)  /* waiting for emulator response */
>  #define STATE_REGISTERED_FLAG	 BIT(2)
> +#define STATE_DRIVER_COMMAND     BIT(3)  /* sending a driver specific command */
>  
>  	size_t req_len;              /* length of queued TPM request */
>  	size_t resp_len;             /* length of queued TPM response */
> @@ -299,6 +300,28 @@ static int vtpm_proxy_tpm_op_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>  	return len;
>  }
>  
> +static int vtpm_proxy_is_driver_command(struct tpm_chip *chip,
> +					u8 *buf, size_t count)
> +{
> +	struct tpm_input_header *hdr = (struct tpm_input_header *)buf;
> +
> +	if (count < sizeof(struct tpm_input_header))
> +		return 0;
> +
> +	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> +		switch (be32_to_cpu(hdr->ordinal)) {
> +		case TPM2_CC_SET_LOCALITY:
> +			return 1;
> +		}
> +	} else {
> +		switch (be32_to_cpu(hdr->ordinal)) {
> +		case TPM_ORD_SET_LOCALITY:
> +			return 1;
> +		}
> +	}
> +	return 0;
> +}
> +
>  /*
>   * Called when core TPM driver forwards TPM requests to 'server side'.
>   *
> @@ -321,6 +344,10 @@ static int vtpm_proxy_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t count)
>  		return -EIO;
>  	}
>  
> +	if (!(proxy_dev->state & STATE_DRIVER_COMMAND) &&
> +	    vtpm_proxy_is_driver_command(chip, buf, count))
> +		return -EFAULT;
> +
>  	mutex_lock(&proxy_dev->buf_lock);
>  
>  	if (!(proxy_dev->state & STATE_OPENED_FLAG)) {
> @@ -376,6 +403,7 @@ static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
>  	struct tpm_buf buf;
>  	int rc;
>  	const struct tpm_output_header *header;
> +	struct proxy_dev *proxy_dev = dev_get_drvdata(&chip->dev);
>  
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>  		rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS,
> @@ -387,9 +415,14 @@ static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
>  		return rc;
>  	tpm_buf_append_u8(&buf, locality);
>  
> +	proxy_dev->state |= STATE_DRIVER_COMMAND;
> +
>  	rc = tpm_transmit_cmd(chip, NULL, buf.data, tpm_buf_length(&buf), 0,
>  			      TPM_TRANSMIT_UNLOCKED | TPM_TRANSMIT_RAW,
>  			      "attempting to set locality");
> +
> +	proxy_dev->state &= ~STATE_DRIVER_COMMAND;
> +
>  	if (rc < 0) {
>  		locality = rc;
>  		goto out;
> -- 
> 2.4.3
> 

Otherwise fine except for the redundant code.

/Jarkko

Re: [PATCH v6 3/3] tpm: vtpm_proxy: Prevent userspace from sending driver command

[Jarkko Sakkinen]

On Wed, May 24, 2017 at 03:22:11PM -0700, Jarkko Sakkinen wrote:
> On Wed, May 24, 2017 at 05:39:41PM -0400, Stefan Berger wrote:
> > To prevent userspace from sending the TPM driver command to set
> > the locality, we need to check every command that is sent from
> > user space. To distinguish user space commands from internally
> > sent commands we introduce an additional state flag
> > STATE_DRIVER_COMMAND that is set while the driver sends this
> > command. Similar to the TPM 2 space commands we return an error
> > code when this command is detected.
> > 
> > Signed-off-by: Stefan Berger <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> > ---
> >  drivers/char/tpm/tpm_vtpm_proxy.c | 33 +++++++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> > 
> > diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
> > index 66024bf..1d877cc 100644
> > --- a/drivers/char/tpm/tpm_vtpm_proxy.c
> > +++ b/drivers/char/tpm/tpm_vtpm_proxy.c
> > @@ -43,6 +43,7 @@ struct proxy_dev {
> >  #define STATE_OPENED_FLAG        BIT(0)
> >  #define STATE_WAIT_RESPONSE_FLAG BIT(1)  /* waiting for emulator response */
> >  #define STATE_REGISTERED_FLAG	 BIT(2)
> > +#define STATE_DRIVER_COMMAND     BIT(3)  /* sending a driver specific command */
> >  
> >  	size_t req_len;              /* length of queued TPM request */
> >  	size_t resp_len;             /* length of queued TPM response */
> > @@ -299,6 +300,28 @@ static int vtpm_proxy_tpm_op_recv(struct tpm_chip *chip, u8 *buf, size_t count)
> >  	return len;
> >  }
> >  
> > +static int vtpm_proxy_is_driver_command(struct tpm_chip *chip,
> > +					u8 *buf, size_t count)
> > +{
> > +	struct tpm_input_header *hdr = (struct tpm_input_header *)buf;
> > +
> > +	if (count < sizeof(struct tpm_input_header))
> > +		return 0;
> > +
> > +	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> > +		switch (be32_to_cpu(hdr->ordinal)) {
> > +		case TPM2_CC_SET_LOCALITY:
> > +			return 1;
> > +		}
> > +	} else {
> > +		switch (be32_to_cpu(hdr->ordinal)) {
> > +		case TPM_ORD_SET_LOCALITY:
> > +			return 1;
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> >  /*
> >   * Called when core TPM driver forwards TPM requests to 'server side'.
> >   *
> > @@ -321,6 +344,10 @@ static int vtpm_proxy_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t count)
> >  		return -EIO;
> >  	}
> >  
> > +	if (!(proxy_dev->state & STATE_DRIVER_COMMAND) &&
> > +	    vtpm_proxy_is_driver_command(chip, buf, count))
> > +		return -EFAULT;
> > +
> >  	mutex_lock(&proxy_dev->buf_lock);
> >  
> >  	if (!(proxy_dev->state & STATE_OPENED_FLAG)) {
> > @@ -376,6 +403,7 @@ static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
> >  	struct tpm_buf buf;
> >  	int rc;
> >  	const struct tpm_output_header *header;
> > +	struct proxy_dev *proxy_dev = dev_get_drvdata(&chip->dev);
> >  
> >  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> >  		rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS,
> > @@ -387,9 +415,14 @@ static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
> >  		return rc;
> >  	tpm_buf_append_u8(&buf, locality);
> >  
> > +	proxy_dev->state |= STATE_DRIVER_COMMAND;
> > +
> >  	rc = tpm_transmit_cmd(chip, NULL, buf.data, tpm_buf_length(&buf), 0,
> >  			      TPM_TRANSMIT_UNLOCKED | TPM_TRANSMIT_RAW,
> >  			      "attempting to set locality");
> > +
> > +	proxy_dev->state &= ~STATE_DRIVER_COMMAND;
> > +
> >  	if (rc < 0) {
> >  		locality = rc;
> >  		goto out;
> > -- 
> > 2.4.3
> > 
> 
> Otherwise fine except for the redundant code.
> 
> /Jarkko

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Probably not yet merging and testing this this week though as I'm
travelling in US right now.

/Jarkko

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