[PATCH 1/2] tpm: rename data_pending to transmit_result

[PATCH 1/2] tpm: rename data_pending to transmit_result

[Tadeusz Struk]

The file_priv->data_pending name is not adequate as it
doesn't contain any data, but the result from the last
successful call to tpm_transmit() function, so rename it
to transmit_result. Also its type should be size_t instead
of ssize_t as it will never be negative. Change it as well.

Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
---
 drivers/char/tpm/tpm-dev-common.c |   20 ++++++++++----------
 drivers/char/tpm/tpm-dev.h        |    4 ++--
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index 99b5133a9d05..67a70e2fde7f 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -40,7 +40,7 @@ static void tpm_async_work(struct work_struct *work)
 
 	tpm_put_ops(priv->chip);
 	if (ret > 0) {
-		priv->data_pending = ret;
+		priv->transmit_result = ret;
 		mod_timer(&priv->user_read_timer, jiffies + (120 * HZ));
 	}
 	mutex_unlock(&priv->buffer_mutex);
@@ -63,7 +63,7 @@ static void tpm_timeout_work(struct work_struct *work)
 					      timeout_work);
 
 	mutex_lock(&priv->buffer_mutex);
-	priv->data_pending = 0;
+	priv->transmit_result = 0;
 	memset(priv->data_buffer, 0, sizeof(priv->data_buffer));
 	mutex_unlock(&priv->buffer_mutex);
 	wake_up_interruptible(&priv->async_wait);
@@ -94,16 +94,16 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
 	flush_work(&priv->timeout_work);
 	mutex_lock(&priv->buffer_mutex);
 
-	if (priv->data_pending) {
-		ret_size = min_t(ssize_t, size, priv->data_pending);
+	if (priv->transmit_result) {
+		ret_size = min_t(ssize_t, size, priv->transmit_result);
 		if (ret_size > 0) {
 			rc = copy_to_user(buf, priv->data_buffer, ret_size);
-			memset(priv->data_buffer, 0, priv->data_pending);
+			memset(priv->data_buffer, 0, priv->transmit_result);
 			if (rc)
 				ret_size = -EFAULT;
 		}
 
-		priv->data_pending = 0;
+		priv->transmit_result = 0;
 	}
 
 	mutex_unlock(&priv->buffer_mutex);
@@ -125,7 +125,7 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
 	 * tpm_read or a user_read_timer timeout. This also prevents split
 	 * buffered writes from blocking here.
 	 */
-	if (priv->data_pending != 0 || priv->command_enqueued) {
+	if (priv->transmit_result != 0 || priv->command_enqueued) {
 		ret = -EBUSY;
 		goto out;
 	}
@@ -168,7 +168,7 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
 	tpm_put_ops(priv->chip);
 
 	if (ret > 0) {
-		priv->data_pending = ret;
+		priv->transmit_result = ret;
 		mod_timer(&priv->user_read_timer, jiffies + (120 * HZ));
 		ret = size;
 	}
@@ -184,7 +184,7 @@ __poll_t tpm_common_poll(struct file *file, poll_table *wait)
 
 	poll_wait(file, &priv->async_wait, wait);
 
-	if (priv->data_pending)
+	if (priv->transmit_result)
 		mask = EPOLLIN | EPOLLRDNORM;
 	else
 		mask = EPOLLOUT | EPOLLWRNORM;
@@ -201,7 +201,7 @@ void tpm_common_release(struct file *file, struct file_priv *priv)
 	del_singleshot_timer_sync(&priv->user_read_timer);
 	flush_work(&priv->timeout_work);
 	file->private_data = NULL;
-	priv->data_pending = 0;
+	priv->transmit_result = 0;
 }
 
 int __init tpm_dev_common_init(void)
diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
index a126b575cb8c..3ff1dc9f3d75 100644
--- a/drivers/char/tpm/tpm-dev.h
+++ b/drivers/char/tpm/tpm-dev.h
@@ -9,8 +9,8 @@ struct file_priv {
 	struct tpm_chip *chip;
 	struct tpm_space *space;
 
-	/* Holds the amount of data passed or an error code from async op */
-	ssize_t data_pending;
+	/* Holds the resul of the last successful call to tpm_transmit() */
+	size_t transmit_result;
 	struct mutex buffer_mutex;
 
 	struct timer_list user_read_timer;      /* user needs to claim result */

[PATCH v4 2/2] tpm: add support for partial reads

[Tadeusz Struk]

Currently to read a response from the TPM device an application needs
provide big enough buffer for the whole response and read it in one go.
The application doesn't know how big the response it beforehand so it
always needs to maintain a 4K buffer and read the max (4K).
In case if the user of the TSS library doesn't provide big enough
buffer the TCTI spec says that the library should set the required
size and return TSS2_TCTI_RC_INSUFFICIENT_BUFFER error code so that the
application could allocate a bigger buffer and call receive again.
To make it possible in the TSS library, this requires being able to do
partial reads from the driver.
The library would read the 10 bytes header first to get the actual size
of the response from the header, and then read the rest of the response.

This patch adds support for partial reads, i.e. the user can read the
response in one or multiple reads, until the whole response is consumed.
The user can also read only part of the response and ignore
the rest by issuing a new write to send a new command.

Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
---
The usecase is implemented in this TSS commit:
https://github.com/tpm2-software/tpm2-tss/commit/ce982f67a67dc08e24683d30b05800648d8a264c

Changes in v4:
 - Use unsigned type for response_pending as it will never be negative.
 - Rebased on top of name change data_pending to transmit_result patch.

Changes in v3:
 - Remove link to usecase implemented in TSS out of the commit message.
 - Update the conddition in tpm_common_poll() to take into account
   the partial_data also.

Changes in v2:
 - Allow writes after only partial response is consumed to maintain
   backwords compatibility.
---
 drivers/char/tpm/tpm-dev-common.c |   41 ++++++++++++++++++++++++++++---------
 drivers/char/tpm/tpm-dev.h        |    2 ++
 2 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index 67a70e2fde7f..0544733e1b6d 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -64,6 +64,7 @@ static void tpm_timeout_work(struct work_struct *work)
 
 	mutex_lock(&priv->buffer_mutex);
 	priv->transmit_result = 0;
+	priv->partial_data = 0;
 	memset(priv->data_buffer, 0, sizeof(priv->data_buffer));
 	mutex_unlock(&priv->buffer_mutex);
 	wake_up_interruptible(&priv->async_wait);
@@ -90,22 +91,39 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
 	ssize_t ret_size = 0;
 	int rc;
 
-	del_singleshot_timer_sync(&priv->user_read_timer);
-	flush_work(&priv->timeout_work);
 	mutex_lock(&priv->buffer_mutex);
+	if (priv->transmit_result || priv->partial_data) {
+		if (*off == 0)
+			priv->partial_data = priv->transmit_result;
+
+		ret_size = min_t(ssize_t, size, priv->partial_data);
+		if (ret_size <= 0) {
+			ret_size = 0;
+			priv->transmit_result = 0;
+			priv->partial_data = 0;
+			goto out;
+		}
 
-	if (priv->transmit_result) {
-		ret_size = min_t(ssize_t, size, priv->transmit_result);
-		if (ret_size > 0) {
-			rc = copy_to_user(buf, priv->data_buffer, ret_size);
-			memset(priv->data_buffer, 0, priv->transmit_result);
-			if (rc)
-				ret_size = -EFAULT;
+		rc = copy_to_user(buf, priv->data_buffer + *off, ret_size);
+		if (rc) {
+			memset(priv->data_buffer, 0, TPM_BUFSIZE);
+			priv->partial_data = 0;
+			ret_size = -EFAULT;
+		} else {
+			memset(priv->data_buffer + *off, 0, ret_size);
+			priv->partial_data -= ret_size;
+			*off += ret_size;
 		}
 
 		priv->transmit_result = 0;
 	}
 
+out:
+	if (!priv->partial_data) {
+		*off = 0;
+		del_singleshot_timer_sync(&priv->user_read_timer);
+		flush_work(&priv->timeout_work);
+	}
 	mutex_unlock(&priv->buffer_mutex);
 	return ret_size;
 }
@@ -150,6 +168,9 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
 		goto out;
 	}
 
+	priv->partial_data = 0;
+	*off = 0;
+
 	/*
 	 * If in nonblocking mode schedule an async job to send
 	 * the command return the size.
@@ -184,7 +205,7 @@ __poll_t tpm_common_poll(struct file *file, poll_table *wait)
 
 	poll_wait(file, &priv->async_wait, wait);
 
-	if (priv->transmit_result)
+	if (priv->transmit_result || priv->partial_data)
 		mask = EPOLLIN | EPOLLRDNORM;
 	else
 		mask = EPOLLOUT | EPOLLWRNORM;
diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
index 3ff1dc9f3d75..714a4419d392 100644
--- a/drivers/char/tpm/tpm-dev.h
+++ b/drivers/char/tpm/tpm-dev.h
@@ -11,6 +11,8 @@ struct file_priv {
 
 	/* Holds the resul of the last successful call to tpm_transmit() */
 	size_t transmit_result;
+	/* Holds the count how much of the response is still unread */
+	size_t partial_data;
 	struct mutex buffer_mutex;
 
 	struct timer_list user_read_timer;      /* user needs to claim result */

Re: [PATCH 1/2] tpm: rename data_pending to transmit_result

[Jarkko Sakkinen]

On Fri, Nov 16, 2018 at 09:51:13AM -0800, Tadeusz Struk wrote:
> The file_priv->data_pending name is not adequate as it
> doesn't contain any data, but the result from the last
> successful call to tpm_transmit() function, so rename it
> to transmit_result. Also its type should be size_t instead
> of ssize_t as it will never be negative. Change it as well.
> 
> Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
> ---
>  drivers/char/tpm/tpm-dev-common.c |   20 ++++++++++----------
>  drivers/char/tpm/tpm-dev.h        |    4 ++--
>  2 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
> index 99b5133a9d05..67a70e2fde7f 100644
> --- a/drivers/char/tpm/tpm-dev-common.c
> +++ b/drivers/char/tpm/tpm-dev-common.c
> @@ -40,7 +40,7 @@ static void tpm_async_work(struct work_struct *work)
>  
>  	tpm_put_ops(priv->chip);
>  	if (ret > 0) {
> -		priv->data_pending = ret;
> +		priv->transmit_result = ret;

I don't know if you saw my later response before sending this but
since you made the remark that it does not carry the error code
more proper name is response_length because that is exactly the
data it contains.

/Jarkko

Re: [PATCH v4 2/2] tpm: add support for partial reads

[Jarkko Sakkinen]

On Fri, Nov 16, 2018 at 09:51:51AM -0800, Tadeusz Struk wrote:
> Currently to read a response from the TPM device an application needs
> provide big enough buffer for the whole response and read it in one go.
> The application doesn't know how big the response it beforehand so it
> always needs to maintain a 4K buffer and read the max (4K).
> In case if the user of the TSS library doesn't provide big enough
> buffer the TCTI spec says that the library should set the required
> size and return TSS2_TCTI_RC_INSUFFICIENT_BUFFER error code so that the
> application could allocate a bigger buffer and call receive again.
> To make it possible in the TSS library, this requires being able to do
> partial reads from the driver.
> The library would read the 10 bytes header first to get the actual size
> of the response from the header, and then read the rest of the response.
> 
> This patch adds support for partial reads, i.e. the user can read the
> response in one or multiple reads, until the whole response is consumed.
> The user can also read only part of the response and ignore
> the rest by issuing a new write to send a new command.
> 
> Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
> ---
> The usecase is implemented in this TSS commit:
> https://github.com/tpm2-software/tpm2-tss/commit/ce982f67a67dc08e24683d30b05800648d8a264c
> 
> Changes in v4:
>  - Use unsigned type for response_pending as it will never be negative.
>  - Rebased on top of name change data_pending to transmit_result patch.
> 
> Changes in v3:
>  - Remove link to usecase implemented in TSS out of the commit message.
>  - Update the conddition in tpm_common_poll() to take into account
>    the partial_data also.
> 
> Changes in v2:
>  - Allow writes after only partial response is consumed to maintain
>    backwords compatibility.
> ---
>  drivers/char/tpm/tpm-dev-common.c |   41 ++++++++++++++++++++++++++++---------
>  drivers/char/tpm/tpm-dev.h        |    2 ++
>  2 files changed, 33 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
> index 67a70e2fde7f..0544733e1b6d 100644
> --- a/drivers/char/tpm/tpm-dev-common.c
> +++ b/drivers/char/tpm/tpm-dev-common.c
> @@ -64,6 +64,7 @@ static void tpm_timeout_work(struct work_struct *work)
>  
>  	mutex_lock(&priv->buffer_mutex);
>  	priv->transmit_result = 0;
> +	priv->partial_data = 0;
>  	memset(priv->data_buffer, 0, sizeof(priv->data_buffer));
>  	mutex_unlock(&priv->buffer_mutex);
>  	wake_up_interruptible(&priv->async_wait);
> @@ -90,22 +91,39 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
>  	ssize_t ret_size = 0;
>  	int rc;
>  
> -	del_singleshot_timer_sync(&priv->user_read_timer);
> -	flush_work(&priv->timeout_work);
>  	mutex_lock(&priv->buffer_mutex);
> +	if (priv->transmit_result || priv->partial_data) {
> +		if (*off == 0)
> +			priv->partial_data = priv->transmit_result;
> +
> +		ret_size = min_t(ssize_t, size, priv->partial_data);
> +		if (ret_size <= 0) {

When ret_size < 0? Shouldn't this be just "if (!ret_size)"?

> +			ret_size = 0;
> +			priv->transmit_result = 0;
> +			priv->partial_data = 0;
> +			goto out;
> +		}
>  
> -	if (priv->transmit_result) {
> -		ret_size = min_t(ssize_t, size, priv->transmit_result);
> -		if (ret_size > 0) {
> -			rc = copy_to_user(buf, priv->data_buffer, ret_size);
> -			memset(priv->data_buffer, 0, priv->transmit_result);
> -			if (rc)
> -				ret_size = -EFAULT;
> +		rc = copy_to_user(buf, priv->data_buffer + *off, ret_size);
> +		if (rc) {
> +			memset(priv->data_buffer, 0, TPM_BUFSIZE);
> +			priv->partial_data = 0;
> +			ret_size = -EFAULT;
> +		} else {
> +			memset(priv->data_buffer + *off, 0, ret_size);
> +			priv->partial_data -= ret_size;
> +			*off += ret_size;
>  		}
>  
>  		priv->transmit_result = 0;
>  	}
>  
> +out:
> +	if (!priv->partial_data) {
> +		*off = 0;
> +		del_singleshot_timer_sync(&priv->user_read_timer);
> +		flush_work(&priv->timeout_work);
> +	}
>  	mutex_unlock(&priv->buffer_mutex);
>  	return ret_size;
>  }
> @@ -150,6 +168,9 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
>  		goto out;
>  	}
>  
> +	priv->partial_data = 0;
> +	*off = 0;
> +
>  	/*
>  	 * If in nonblocking mode schedule an async job to send
>  	 * the command return the size.
> @@ -184,7 +205,7 @@ __poll_t tpm_common_poll(struct file *file, poll_table *wait)
>  
>  	poll_wait(file, &priv->async_wait, wait);
>  
> -	if (priv->transmit_result)
> +	if (priv->transmit_result || priv->partial_data)
>  		mask = EPOLLIN | EPOLLRDNORM;
>  	else
>  		mask = EPOLLOUT | EPOLLWRNORM;
> diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
> index 3ff1dc9f3d75..714a4419d392 100644
> --- a/drivers/char/tpm/tpm-dev.h
> +++ b/drivers/char/tpm/tpm-dev.h
> @@ -11,6 +11,8 @@ struct file_priv {
>  
>  	/* Holds the resul of the last successful call to tpm_transmit() */
>  	size_t transmit_result;
> +	/* Holds the count how much of the response is still unread */
> +	size_t partial_data;

I'm otherwise happy how this look like but why call it partial_data.
You cannot really tell from the name anything about its contents as
data is very abstract term.

BTW, why you need the new variable anyway and not just decrease the
variable where the length is original stored?

/Jarkko

Re: [PATCH v4 2/2] tpm: add support for partial reads

[Tadeusz Struk]

On 11/17/18 11:48 PM, Jarkko Sakkinen wrote:
>> +	if (priv->transmit_result || priv->partial_data) {
>> +		if (*off == 0)
>> +			priv->partial_data = priv->transmit_result;
>> +
>> +		ret_size = min_t(ssize_t, size, priv->partial_data);
>> +		if (ret_size <= 0) {
> When ret_size < 0? Shouldn't this be just "if (!ret_size)"?

What we want to check here is if ret_size is positive, which is a valid
value, or if it is negative or zero, which is an invalid value, so in
this case (!ret_size) will not work.

> 
>> 	/* Holds the resul of the last successful call to tpm_transmit() */
>>  	size_t transmit_result;
>> +	/* Holds the count how much of the response is still unread */
>> +	size_t partial_data;
> I'm otherwise happy how this look like but why call it partial_data.
> You cannot really tell from the name anything about its contents as
> data is very abstract term.
 
so I will rename these two to response_length and response_length_rem,
how does this sound?

> BTW, why you need the new variable anyway and not just decrease the
> variable where the length is original stored?

We need to have two variables, otherwise how do we tell if some part of
response was consumed to allow sending a new command?
The transmit_result is used for that. If it is zero then one can transmit
a new command even if the whole response is not consumed. The new variable
tracks how much of the response is still to be read. 

Thanks,
-- 
Tadeusz

Re: [PATCH v4 2/2] tpm: add support for partial reads

[Jarkko Sakkinen]

On Sun, Nov 18, 2018 at 07:05:19PM -0800, Tadeusz Struk wrote:
> > When ret_size < 0? Shouldn't this be just "if (!ret_size)"?
> 
> What we want to check here is if ret_size is positive, which is a valid
> value, or if it is negative or zero, which is an invalid value, so in
> this case (!ret_size) will not work.

Please explain a scenario where "!ret_size" would no work given that
both size and partial_data have always positive value?

> >> 	/* Holds the resul of the last successful call to tpm_transmit() */
> >>  	size_t transmit_result;
> >> +	/* Holds the count how much of the response is still unread */
> >> +	size_t partial_data;
> > I'm otherwise happy how this look like but why call it partial_data.
> > You cannot really tell from the name anything about its contents as
> > data is very abstract term.
>  
> so I will rename these two to response_length and response_length_rem,
> how does this sound?

Yes, assuming that there would be a hard requirement to even have two
variables in the first place.

> > BTW, why you need the new variable anyway and not just decrease the
> > variable where the length is original stored?
> 
> We need to have two variables, otherwise how do we tell if some part of
> response was consumed to allow sending a new command?

I don't understand. In order to maintain backwards compatibility you can
send a new command at any time.

> The transmit_result is used for that. If it is zero then one can transmit
> a new command even if the whole response is not consumed. The new variable
> tracks how much of the response is still to be read. 

AFAIK you only need to track the latter, not both.

/Jarkko

Re: [PATCH v4 2/2] tpm: add support for partial reads

[Tadeusz Struk]

On 11/19/18 5:58 AM, Jarkko Sakkinen wrote:
> Please explain a scenario where "!ret_size" would no work given that
> both size and partial_data have always positive value?

Right, I only looked at the one line above before responding.
I'll change it to !ret_size

> 
> I don't understand. In order to maintain backwards compatibility you can
> send a new command at any time.

No, currently it is not possible to send a new command until the previous
response is consumed. -EBUSY is returned if one sends a new command before
reading the previous response (or at least part of it). See:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/char/tpm/tpm-dev-common.c#n128

-- 
Tadeusz

Re: [PATCH v4 2/2] tpm: add support for partial reads

[Jarkko Sakkinen]

On Mon, Nov 19, 2018 at 08:44:49AM -0800, Tadeusz Struk wrote:
> On 11/19/18 5:58 AM, Jarkko Sakkinen wrote:
> > Please explain a scenario where "!ret_size" would no work given that
> > both size and partial_data have always positive value?
> 
> Right, I only looked at the one line above before responding.
> I'll change it to !ret_size
> 
> > 
> > I don't understand. In order to maintain backwards compatibility you can
> > send a new command at any time.
> 
> No, currently it is not possible to send a new command until the previous
> response is consumed. -EBUSY is returned if one sends a new command before
> reading the previous response (or at least part of it). See:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/char/tpm/tpm-dev-common.c#n128

Ah, you are correct.

You should add a boolean flag instead of introducing a new variable for
holding amount that has been read because obviously one read operation
is enough for backwards compatibility.

The code could read the code to data_pending and then set

   priv->data_read = false;

We do not need the original amount for anything.

/Jarkko

Re: [PATCH v4 2/2] tpm: add support for partial reads

[Tadeusz Struk]

On 11/19/18 9:28 AM, Jarkko Sakkinen wrote:
> Ah, you are correct.
> 
> You should add a boolean flag instead of introducing a new variable for
> holding amount that has been read because obviously one read operation
> is enough for backwards compatibility.
> 
> The code could read the code to data_pending and then set
> 
>    priv->data_read = false;
> 
> We do not need the original amount for anything.

but we still need to keep track of how much of the response is left unconsumed.

-- 
Tadeusz