diff mbox series

[v2] tpm: add support for partial reads

Message ID 154084273717.29396.1021945733871865594.stgit@tstruk-mobl1.jf.intel.com (mailing list archive)
State New, archived
Headers show
Series [v2] tpm: add support for partial reads | expand

Commit Message

Tadeusz Struk Oct. 29, 2018, 7:52 p.m. UTC
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.

The usecase is implemented in this TSS commit:
https://github.com/tpm2-software/tpm2-tss/commit/ce982f67a67dc08e24683d30b05800648d8a264c

Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
---
Changes in v2:
 - Allow writes after only partial response is consumed to maintain
   backwords compatibility.
---
 drivers/char/tpm/tpm-dev-common.c |   38 ++++++++++++++++++++++++++++---------
 drivers/char/tpm/tpm-dev.h        |    2 ++
 2 files changed, 31 insertions(+), 9 deletions(-)

Comments

Jarkko Sakkinen Nov. 5, 2018, 1:39 p.m. UTC | #1
On Mon, Oct 29, 2018 at 12:52:17PM -0700, 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.
> 
> The usecase is implemented in this TSS commit:
> https://github.com/tpm2-software/tpm2-tss/commit/ce982f67a67dc08e24683d30b05800648d8a264c
> 
> Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
> ---
> Changes in v2:
>  - Allow writes after only partial response is consumed to maintain
>    backwords compatibility.

I do not understand what this bullet means. Do you deny writes somehow?

/Jarkko
Jarkko Sakkinen Nov. 5, 2018, 1:44 p.m. UTC | #2
On Mon, Oct 29, 2018 at 12:52:17PM -0700, 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.

You don't explain what the commit does at all.

> The usecase is implemented in this TSS commit:
> https://github.com/tpm2-software/tpm2-tss/commit/ce982f67a67dc08e24683d30b05800648d8a264c

We do not want this as part of the commit message. You should place this
in the beginning of the diffstat section.

> 
> Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
> ---
> Changes in v2:
>  - Allow writes after only partial response is consumed to maintain
>    backwords compatibility.
> ---
>  drivers/char/tpm/tpm-dev-common.c |   38 ++++++++++++++++++++++++++++---------
>  drivers/char/tpm/tpm-dev.h        |    2 ++
>  2 files changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
> index 99b5133a9d05..77e686d35384 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->data_pending = 0;
> +	priv->partial_data = 0;

What is this variable?

Please refine the patch with better documentation.

/Jarkko
Tadeusz Struk Nov. 15, 2018, 6:49 p.m. UTC | #3
Hi Jarkko,
On 11/5/18 5:39 AM, Jarkko Sakkinen wrote:
>> Changes in v2:
>>  - Allow writes after only partial response is consumed to maintain
>>    backwords compatibility.
> I do not understand what this bullet means. Do you deny writes somehow?

No I don't. This comment was wrt the first version, which didn't allow
writes unless the full response was consumed.
In this version things work exactly as before, i.e. a next message
can be send even if only a part of the previous response is consumed.
Thanks,
Tadeusz Struk Nov. 15, 2018, 6:50 p.m. UTC | #4
On 11/5/18 5:44 AM, Jarkko Sakkinen wrote:
> You don't explain what the commit does at all.
> 
>> The usecase is implemented in this TSS commit:
>> https://github.com/tpm2-software/tpm2-tss/commit/ce982f67a67dc08e24683d30b05800648d8a264c
> We do not want this as part of the commit message. You should place this
> in the beginning of the diffstat section.
> 

I will update that and send a v3 soon.
Thanks,
Jarkko Sakkinen Nov. 15, 2018, 8:40 p.m. UTC | #5
On Thu, Nov 15, 2018 at 10:50:54AM -0800, Tadeusz Struk wrote:
> On 11/5/18 5:44 AM, Jarkko Sakkinen wrote:
> > You don't explain what the commit does at all.
> > 
> >> The usecase is implemented in this TSS commit:
> >> https://github.com/tpm2-software/tpm2-tss/commit/ce982f67a67dc08e24683d30b05800648d8a264c
> > We do not want this as part of the commit message. You should place this
> > in the beginning of the diffstat section.
> > 
> 
> I will update that and send a v3 soon.
> Thanks,
> -- 
> Tadeusz

Great!

/Jarkko
diff mbox series

Patch

diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index 99b5133a9d05..77e686d35384 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->data_pending = 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->data_pending || priv->partial_data) {
+		if (*off == 0)
+			priv->partial_data = priv->data_pending;
+
+		ret_size = min_t(ssize_t, size + *off, priv->partial_data);
+		if (ret_size <= 0) {
+			ret_size = 0;
+			priv->data_pending = 0;
+			priv->partial_data = 0;
+			goto out;
+		}
 
-	if (priv->data_pending) {
-		ret_size = min_t(ssize_t, size, priv->data_pending);
-		if (ret_size > 0) {
-			rc = copy_to_user(buf, priv->data_buffer, ret_size);
-			memset(priv->data_buffer, 0, priv->data_pending);
-			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->data_pending = 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,8 @@  ssize_t tpm_common_write(struct file *file, const char __user *buf,
 		goto out;
 	}
 
+	priv->partial_data = 0;
+
 	/*
 	 * If in nonblocking mode schedule an async job to send
 	 * the command return the size.
diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
index a126b575cb8c..a2ca6a7a06f1 100644
--- a/drivers/char/tpm/tpm-dev.h
+++ b/drivers/char/tpm/tpm-dev.h
@@ -11,6 +11,8 @@  struct file_priv {
 
 	/* Holds the amount of data passed or an error code from async op */
 	ssize_t data_pending;
+	/* For partial reads, holds the reminder of a response */
+	ssize_t partial_data;
 	struct mutex buffer_mutex;
 
 	struct timer_list user_read_timer;      /* user needs to claim result */