Re: [PATCH] tpm: Revert "tpm: fix invalid locking in NONBLOCKING mode"

[Tadeusz Struk]

On 5/27/20 5:30 PM, Jarkko Sakkinen wrote:
>> This won't help if the message is read by an async tcti. If the problem lies
>> in the chip get locality code, perhaps this could help to debug the 
>> root-cause
>> instead of masking it out in the upper layer code:
> What is TCTI and async TCTI? Not following.

TPM Command Transmission Interface (TCTI) as defined by TCG in
https://trustedcomputinggroup.org/resource/tss-tcti-specification/

the reason we added the O_NONBLOCK mode was to satisfy the TCG spec for async 
TCTI.

Thanks,
Tadeusz

Re: [PATCH] tpm: Revert "tpm: fix invalid locking in NONBLOCKING mode"

[Tadeusz Struk]

On 5/26/20 1:00 PM, James Bottomley wrote:
> I don't think there is a root cause other than a TIS TPM is getting
> annoyed by us cycling localities too rapidly because we don't do an
> actual TPM operation between request and relinquish.  Since the first
> request/relinquish seems unnecessary for the async case, moving the ops
> get eliminates the problem.

Could be, so maybe we could try both patches.
More debug info on the error path won't hurt.
Thanks,
Tadeusz

Re: [PATCH] tpm: Revert "tpm: fix invalid locking in NONBLOCKING mode"

[Tadeusz Struk]

On 5/26/20 12:14 PM, James Bottomley wrote:
> + /* atomic tpm command send and result receive. We only hold the ops
> +  * lock during this period so that the tpm can be unregistered even if
> +  * the char dev is held open.
> +  */
> + if (tpm_try_get_ops(priv->chip)) {
> + ret = -EPIPE;
> + goto out;
> + }
> +
Hi James,
This won't help if the message is read by an async tcti. If the problem lies
in the chip get locality code, perhaps this could help to debug the root-cause
instead of masking it out in the upper layer code:

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 2435216bd10a..da5ecd0376bf 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -202,20 +202,22 @@ static int request_locality(struct tpm_chip *chip, int l)
return rc;
 
stop = jiffies + chip->timeout_a;
+   timeout = stop - jiffies;
 
if (chip->flags & TPM_CHIP_FLAG_IRQ) {
 again:
timeout = stop - jiffies;
if ((long)timeout <= 0)
-   return -1;
+   goto out;
+
rc = wait_event_interruptible_timeout(priv->int_queue,
- (check_locality
-  (chip, l)),
+ check_locality(chip, l),
  timeout);
if (rc > 0)
return l;
if (rc == -ERESTARTSYS && freezing(current)) {
clear_thread_flag(TIF_SIGPENDING);
+   timeout = stop - jiffies;
goto again;
}
} else {
@@ -226,6 +228,10 @@ static int request_locality(struct tpm_chip *chip, int l)
tpm_msleep(TPM_TIMEOUT);
} while (time_before(jiffies, stop));
}
+out:
+   dev_warn(&chip->dev, "%s: failed to request locality %d after %lu ms\n",
+__func__, l, timeout * HZ / 1000);
+
return -1;
 }
 

[PATCH] tpm: add check after commands attribs tab allocation

[Tadeusz Struk]

devm_kcalloc() can fail and return NULL so we need to check for that.

Fixes: 58472f5cd4f6f ("tpm: validate TPM 2.0 commands")
Signed-off-by: Tadeusz Struk 
---
 drivers/char/tpm/tpm2-cmd.c |4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index ba9acae83bff..5817dfe5c5d2 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -939,6 +939,10 @@ static int tpm2_get_cc_attrs_tbl(struct tpm_chip *chip)
 
chip->cc_attrs_tbl = devm_kcalloc(&chip->dev, 4, nr_commands,
  GFP_KERNEL);
+   if (!chip->cc_attrs_tbl) {
+   rc = -ENOMEM;
+   goto out;
+   }
 
rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_GET_CAPABILITY);
if (rc)

Re: [PATCH v4] tpm: fix an invalid condition in tpm_common_poll

[Tadeusz Struk]

On 3/28/19 5:34 AM, Jarkko Sakkinen wrote:
> Thank you, it is applied.

Thank you Jarkko.

-- 
Tadeusz

[PATCH v4] tpm: fix an invalid condition in tpm_common_poll

[Tadeusz Struk]

The poll condition should only check response_length,
because reads should only be issued if there is data to read.
The response_read flag only prevents double writes.
The problem was that the write set the response_read to false,
enqued a tpm job, and returned. Then application called poll
which checked the response_read flag and returned EPOLLIN.
Then the application called read, but got nothing.
After all that the async_work kicked in.
Added also mutex_lock around the poll check to prevent
other possible race conditions.

Cc: sta...@vger.kernel.org
Fixes: 9488585b21bef0df12 ("tpm: add support for partial reads")
Reported-by: Mantas Mikulėnas 
Tested-by: Mantas Mikulėnas 
Signed-off-by: Tadeusz Struk 
---
 drivers/char/tpm/tpm-dev-common.c |9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c 
b/drivers/char/tpm/tpm-dev-common.c
index 5eecad233ea1..744b0237300a 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -203,12 +203,19 @@ __poll_t tpm_common_poll(struct file *file, poll_table 
*wait)
__poll_t mask = 0;
 
poll_wait(file, &priv->async_wait, wait);
+   mutex_lock(&priv->buffer_mutex);
 
-   if (!priv->response_read || priv->response_length)
+   /*
+* The response_length indicates if there is still response
+* (or part of it) to be consumed. Partial reads decrease it
+* by the number of bytes read, and write resets it the zero.
+*/
+   if (priv->response_length)
mask = EPOLLIN | EPOLLRDNORM;
else
mask = EPOLLOUT | EPOLLWRNORM;
 
+   mutex_unlock(&priv->buffer_mutex);
return mask;
 }
 

Re: [PATCH RESEND v3] tpm: fix an invalid condition in tpm_common_poll

[Tadeusz Struk]

Hi Jarkko,
On 3/25/19 7:09 AM, Jarkko Sakkinen wrote:
> It is still missing the comment I asked to add. Otherwise, it is good.
> 

Sorry, I didn't see your email with the suggestion earlier.
To be honest I'm not sure if this comment adds much value, or if it is
even correct. The poll doesn't "succeed" or "fail". It just returns
a mask indicating if there is any data to read or if the user can write.

Isn't the commit message + 'git blame' enough to remember why it was
done this way?

Thanks,
-- 
Tadeusz

[PATCH RESEND v3] tpm: fix an invalid condition in tpm_common_poll

[Tadeusz Struk]

The poll condition should only check response_length,
because reads should only be issued if there is data to read.
The response_read flag only prevents double writes.
The problem was that the write set the response_read to false,
enqued a tpm job, and returned. Then application called poll
which checked the response_read flag and returned EPOLLIN.
Then the application called read, but got nothing.
After all that the async_work kicked in.
Added also mutex_lock around the poll check to prevent
other possible race conditions.

Cc: sta...@vger.kernel.org
Fixes: 9488585b21bef0df12 ("tpm: add support for partial reads")
Reported-by: Mantas Mikulėnas 
Tested-by: Mantas Mikulėnas 
Signed-off-by: Tadeusz Struk 
---
 drivers/char/tpm/tpm-dev-common.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c 
b/drivers/char/tpm/tpm-dev-common.c
index 5eecad233ea1..7312d3214381 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -203,12 +203,14 @@ __poll_t tpm_common_poll(struct file *file, poll_table 
*wait)
__poll_t mask = 0;
 
poll_wait(file, &priv->async_wait, wait);
+   mutex_lock(&priv->buffer_mutex);
 
-   if (!priv->response_read || priv->response_length)
+   if (priv->response_length)
mask = EPOLLIN | EPOLLRDNORM;
else
mask = EPOLLOUT | EPOLLWRNORM;
 
+   mutex_unlock(&priv->buffer_mutex);
return mask;
 }
 

[PATCH v3] tpm: fix an invalid condition in tpm_common_poll

[Tadeusz Struk]

The poll condition should only check response_length,
because reads should only be issued if there is data to read.
The response_read flag only prevents double writes.
The problem was that the write set the response_read to false,
enqued a tpm job, and returned. Then application called poll
which checked the response_read flag and returned EPOLLIN.
Then the application called read, but got nothing.
After all that the async_work kicked in.
Added also mutex_lock around the poll check to prevent
other possible race conditions.

Fixes: 9488585b21bef0df12 ("tpm: add support for partial reads")
Reported-by: Mantas Mikulėnas 
Tested-by: Mantas Mikulėnas 
Signed-off-by: Tadeusz Struk 
---
 drivers/char/tpm/tpm-dev-common.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c 
b/drivers/char/tpm/tpm-dev-common.c
index 5eecad233ea1..7312d3214381 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -203,12 +203,14 @@ __poll_t tpm_common_poll(struct file *file, poll_table 
*wait)
__poll_t mask = 0;
 
poll_wait(file, &priv->async_wait, wait);
+   mutex_lock(&priv->buffer_mutex);
 
-   if (!priv->response_read || priv->response_length)
+   if (priv->response_length)
mask = EPOLLIN | EPOLLRDNORM;
else
mask = EPOLLOUT | EPOLLWRNORM;
 
+   mutex_unlock(&priv->buffer_mutex);
return mask;
 }
 

Re: [PATCH v2] tpm: fix an invalid condition in tpm_common_poll

[Tadeusz Struk]

On 3/20/19 11:51 AM, Mantas Mikulėnas wrote:
> Thanks, this patch seems to work, and I apologize for not responding
> to test the patches earlier.

Thanks for testing.

> 
> Any chance it'll be submitted for stable 5.0.x as well?

Yes, it's a regression. I included the "Fixes" tag so
it should be applied to all affected versions.
In this case it's 5.0 only.
Thanks,
-- 
Tadeusz

Re: [PATCH v2] tpm: fix an invalid condition in tpm_common_poll

[Tadeusz Struk]

On 3/20/19 7:30 AM, James Bottomley wrote:
> Just an observation on this: the mutex is now no-longer necessary
> because a read on a size_t quantity is always atomic.

True, that's why it wasn't there at the beginning, but
then things changed and I forgot to add it, so let's
put it there just in case.

Thanks,
-- 
Tadeusz

[PATCH v2] tpm: fix an invalid condition in tpm_common_poll

[Tadeusz Struk]

The poll condition should only check response_length,
because reads should only be issued if there is data to read.
The response_read flag only prevents double writes.
The problem was that the write set the response_read to false,
enqued a tpm job, and returned. Then application called poll
which checked the response_read flag and returned EPOLLIN.
Then the application called read, but got nothing.
After all that the async_work kicked in.
Added also mutex_lock around the poll check to prevent
other possible race conditions.

Fixes: 9488585b21bef0df12 ("tpm: add support for partial reads")
Reported-by: Mantas Mikulėnas 
Signed-off-by: Tadeusz Struk 
---
 drivers/char/tpm/tpm-dev-common.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c 
b/drivers/char/tpm/tpm-dev-common.c
index 5eecad233ea1..7312d3214381 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -203,12 +203,14 @@ __poll_t tpm_common_poll(struct file *file, poll_table 
*wait)
__poll_t mask = 0;
 
poll_wait(file, &priv->async_wait, wait);
+   mutex_lock(&priv->buffer_mutex);
 
-   if (!priv->response_read || priv->response_length)
+   if (priv->response_length)
mask = EPOLLIN | EPOLLRDNORM;
else
mask = EPOLLOUT | EPOLLWRNORM;
 
+   mutex_unlock(&priv->buffer_mutex);
return mask;
 }
 

Re: [PATCH] tpm: fix a race between poll and write in tpm-dev-common

[Tadeusz Struk]

On 3/18/19 4:19 PM, James Bottomley wrote:
>> @@ -203,12 +203,14 @@ __poll_t tpm_common_poll(struct file *file,
>> poll_table *wait)
>>  __poll_t mask = 0;
>>  
>>  poll_wait(file, &priv->async_wait, wait);
>> +mutex_lock(&priv->buffer_mutex);
>>  
>>  if (!priv->response_read || priv->response_length)
>>  mask = EPOLLIN | EPOLLRDNORM;
>>  else
>>  mask = EPOLLOUT | EPOLLWRNORM;
>>  
>> +mutex_unlock(&priv->buffer_mutex);
> This doesn't do anything to address the theory that the queued work
> hasn't run before the poll wakes up, does it?  If you have an
> alternative theory, could you explain it?

Right, it needs to be guarded by the mutex and also the condition
should only check priv->response_length, because we only care
about if there is data to read. The response_read flag only
prevents double writes, without reading in the middle (or a timeout)
which clean it. I will send a v2 soon.

Thanks,
-- 
Tadeusz

[PATCH] tpm: fix a race between poll and write in tpm-dev-common

[Tadeusz Struk]

Since the poll returns EPOLLIN base on the state of two
variables, the response_read being false and the
response_length > 0 the poll needs to take the buffer_mutex
after it is woken up.

Fixes: 9488585b21bef0df12 ("tpm: add support for partial reads")
Reported-by: Mantas Mikulėnas 
Signed-off-by: Tadeusz Struk 
---
 drivers/char/tpm/tpm-dev-common.c |2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/char/tpm/tpm-dev-common.c 
b/drivers/char/tpm/tpm-dev-common.c
index 5eecad233ea1..61e458d6f652 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -203,12 +203,14 @@ __poll_t tpm_common_poll(struct file *file, poll_table 
*wait)
__poll_t mask = 0;
 
poll_wait(file, &priv->async_wait, wait);
+   mutex_lock(&priv->buffer_mutex);
 
if (!priv->response_read || priv->response_length)
mask = EPOLLIN | EPOLLRDNORM;
else
mask = EPOLLOUT | EPOLLWRNORM;
 
+   mutex_unlock(&priv->buffer_mutex);
return mask;
 }
 

Re: [PATCH v3] selftests/tpm2: Extend tests to cover partial reads

[Tadeusz Struk]

On 2/13/19 9:19 AM, Jarkko Sakkinen wrote:
> OK, great, thank you.
> 
> They are in my master branch now.

Thanks Jarkko. Are you planning to push it also to 5.1?

[PATCH v3] selftests/tpm2: Extend tests to cover partial reads

[Tadeusz Struk]

Three new tests added:
1. Send get random cmd, read header in 1st read, read the rest in second
   read - expect success
2. Send get random cmd, read only part of the response, send another
   get random command, read the response - expect success
3. Send get random cmd followed by another get random cmd, without
   reading the first response - expect the second cmd to fail with -EBUSY

Signed-off-by: Tadeusz Struk 
---
v3:
- Remove python docstrings

v2:
- Removed extra logging
- Changed subject tag to selftest/tpm2:
---
 tools/testing/selftests/tpm2/tpm2.py   |1 
 tools/testing/selftests/tpm2/tpm2_tests.py |   63 
 2 files changed, 64 insertions(+)

diff --git a/tools/testing/selftests/tpm2/tpm2.py 
b/tools/testing/selftests/tpm2/tpm2.py
index c2b9f2b1a0ac..828c18584624 100644
--- a/tools/testing/selftests/tpm2/tpm2.py
+++ b/tools/testing/selftests/tpm2/tpm2.py
@@ -22,6 +22,7 @@ TPM2_CC_UNSEAL = 0x015E
 TPM2_CC_FLUSH_CONTEXT = 0x0165
 TPM2_CC_START_AUTH_SESSION = 0x0176
 TPM2_CC_GET_CAPABILITY = 0x017A
+TPM2_CC_GET_RANDOM = 0x017B
 TPM2_CC_PCR_READ = 0x017E
 TPM2_CC_POLICY_PCR = 0x017F
 TPM2_CC_PCR_EXTEND = 0x0182
diff --git a/tools/testing/selftests/tpm2/tpm2_tests.py 
b/tools/testing/selftests/tpm2/tpm2_tests.py
index 3bb066fea4a0..d4973be53493 100644
--- a/tools/testing/selftests/tpm2/tpm2_tests.py
+++ b/tools/testing/selftests/tpm2/tpm2_tests.py
@@ -158,6 +158,69 @@ class SmokeTest(unittest.TestCase):
 pass
 self.assertEqual(rejected, True)
 
+def test_read_partial_resp(self):
+try:
+fmt = '>HIIH'
+cmd = struct.pack(fmt,
+  tpm2.TPM2_ST_NO_SESSIONS,
+  struct.calcsize(fmt),
+  tpm2.TPM2_CC_GET_RANDOM,
+  0x20)
+self.client.tpm.write(cmd)
+hdr = self.client.tpm.read(10)
+sz = struct.unpack('>I', hdr[2:6])[0]
+rsp = self.client.tpm.read()
+except:
+pass
+self.assertEqual(sz, 10 + 2 + 32)
+self.assertEqual(len(rsp), 2 + 32)
+
+def test_read_partial_overwrite(self):
+try:
+fmt = '>HIIH'
+cmd = struct.pack(fmt,
+  tpm2.TPM2_ST_NO_SESSIONS,
+  struct.calcsize(fmt),
+  tpm2.TPM2_CC_GET_RANDOM,
+  0x20)
+self.client.tpm.write(cmd)
+# Read part of the respone
+rsp1 = self.client.tpm.read(15)
+
+# Send a new cmd
+self.client.tpm.write(cmd)
+
+# Read the whole respone
+rsp2 = self.client.tpm.read()
+except:
+pass
+self.assertEqual(len(rsp1), 15)
+self.assertEqual(len(rsp2), 10 + 2 + 32)
+
+def test_send_two_cmds(self):
+rejected = False
+try:
+fmt = '>HIIH'
+cmd = struct.pack(fmt,
+  tpm2.TPM2_ST_NO_SESSIONS,
+  struct.calcsize(fmt),
+  tpm2.TPM2_CC_GET_RANDOM,
+  0x20)
+self.client.tpm.write(cmd)
+
+# expect the second one to raise -EBUSY error
+self.client.tpm.write(cmd)
+rsp = self.client.tpm.read()
+
+except IOError, e:
+# read the response
+rsp = self.client.tpm.read()
+rejected = True
+pass
+except:
+pass
+self.assertEqual(rejected, True)
+
 class SpaceTest(unittest.TestCase):
 def setUp(self):
 logging.basicConfig(filename='SpaceTest.log', level=logging.DEBUG)

[PATCH v2] selftests/tpm2: Open tpm dev in unbuffered mode

[Tadeusz Struk]

In order to have control over how many bytes are read or written
the device needs to be opened in unbuffered mode.

Signed-off-by: Tadeusz Struk 
---

v2:
- Changed subject tags to selftests/tpm2:
---
 tools/testing/selftests/tpm2/tpm2.py |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/tpm2/tpm2.py 
b/tools/testing/selftests/tpm2/tpm2.py
index 40ea95ce2ead..c2b9f2b1a0ac 100644
--- a/tools/testing/selftests/tpm2/tpm2.py
+++ b/tools/testing/selftests/tpm2/tpm2.py
@@ -357,9 +357,9 @@ class Client:
 self.flags = flags
 
 if (self.flags & Client.FLAG_SPACE) == 0:
-self.tpm = open('/dev/tpm0', 'r+b')
+self.tpm = open('/dev/tpm0', 'r+b', buffering=0)
 else:
-self.tpm = open('/dev/tpmrm0', 'r+b')
+self.tpm = open('/dev/tpmrm0', 'r+b', buffering=0)
 
 def close(self):
 self.tpm.close()

Re: [PATCH v2 2/2] selftests/tpm2: Extend tests to cover partial reads

[Tadeusz Struk]

On 2/13/19 7:13 AM, Jarkko Sakkinen wrote:
> On Tue, 2019-02-12 at 15:42 -0800, Tadeusz Struk wrote:
>> Three new tests added:
>> 1. Send get random cmd, read header in 1st read, read the rest in second
>>read - expect success
>> 2. Send get random cmd, read only part of the response, send another
>>get random command, read the response - expect success
>> 3. Send get random cmd followed by another get random cmd, without
>>reading the first response - expect the second cmd to fail with -EBUSY
>>
>> Signed-off-by: Tadeusz Struk 
> 
> Getting still some garbage in the output:
> 
> $ sudo ./test_smoke.sh 
> [sudo] password for jsakkine: 
> test_read_partial_overwrite (tpm2_tests.SmokeTest)
> Reads only part of the response and issue a new cmd ... ok
> test_read_partial_resp (tpm2_tests.SmokeTest)
> Reads random in two subsequent reads ... ok
> test_seal_with_auth (tpm2_tests.SmokeTest) ... ok
> test_seal_with_policy (tpm2_tests.SmokeTest) ... ok
> test_seal_with_too_long_auth (tpm2_tests.SmokeTest) ... ok
> test_send_two_cmds (tpm2_tests.SmokeTest)
> Send two cmds without reading a response ... ok
> test_too_short_cmd (tpm2_tests.SmokeTest) ... ok
> test_unseal_with_wrong_auth (tpm2_tests.SmokeTest) ... ok
> test_unseal_with_wrong_policy (tpm2_tests.SmokeTest) ... ok

Looks like python unittest prints out the docstrings:

+def test_read_partial_resp(self):
+"""Reads random in two subsequent reads"""

I can remove them if it makes more difficult to process the output.
Thanks,
-- 
Tadeusz

[PATCH v2 2/2] selftests/tpm2: Extend tests to cover partial reads

[Tadeusz Struk]

Three new tests added:
1. Send get random cmd, read header in 1st read, read the rest in second
   read - expect success
2. Send get random cmd, read only part of the response, send another
   get random command, read the response - expect success
3. Send get random cmd followed by another get random cmd, without
   reading the first response - expect the second cmd to fail with -EBUSY

Signed-off-by: Tadeusz Struk 
---

v2:
- Removed extra logging
- Changed subject tag to selftest/tpm2:
---
 tools/testing/selftests/tpm2/tpm2.py   |1 
 tools/testing/selftests/tpm2/tpm2_tests.py |   66 
 2 files changed, 67 insertions(+)

diff --git a/tools/testing/selftests/tpm2/tpm2.py 
b/tools/testing/selftests/tpm2/tpm2.py
index c2b9f2b1a0ac..828c18584624 100644
--- a/tools/testing/selftests/tpm2/tpm2.py
+++ b/tools/testing/selftests/tpm2/tpm2.py
@@ -22,6 +22,7 @@ TPM2_CC_UNSEAL = 0x015E
 TPM2_CC_FLUSH_CONTEXT = 0x0165
 TPM2_CC_START_AUTH_SESSION = 0x0176
 TPM2_CC_GET_CAPABILITY = 0x017A
+TPM2_CC_GET_RANDOM = 0x017B
 TPM2_CC_PCR_READ = 0x017E
 TPM2_CC_POLICY_PCR = 0x017F
 TPM2_CC_PCR_EXTEND = 0x0182
diff --git a/tools/testing/selftests/tpm2/tpm2_tests.py 
b/tools/testing/selftests/tpm2/tpm2_tests.py
index 3bb066fea4a0..e82d84043c3f 100644
--- a/tools/testing/selftests/tpm2/tpm2_tests.py
+++ b/tools/testing/selftests/tpm2/tpm2_tests.py
@@ -158,6 +158,72 @@ class SmokeTest(unittest.TestCase):
 pass
 self.assertEqual(rejected, True)
 
+def test_read_partial_resp(self):
+"""Reads random in two subsequent reads"""
+try:
+fmt = '>HIIH'
+cmd = struct.pack(fmt,
+  tpm2.TPM2_ST_NO_SESSIONS,
+  struct.calcsize(fmt),
+  tpm2.TPM2_CC_GET_RANDOM,
+  0x20)
+self.client.tpm.write(cmd)
+hdr = self.client.tpm.read(10)
+sz = struct.unpack('>I', hdr[2:6])[0]
+rsp = self.client.tpm.read()
+except:
+pass
+self.assertEqual(sz, 10 + 2 + 32)
+self.assertEqual(len(rsp), 2 + 32)
+
+def test_read_partial_overwrite(self):
+"""Reads only part of the response and issue a new cmd"""
+try:
+fmt = '>HIIH'
+cmd = struct.pack(fmt,
+  tpm2.TPM2_ST_NO_SESSIONS,
+  struct.calcsize(fmt),
+  tpm2.TPM2_CC_GET_RANDOM,
+  0x20)
+self.client.tpm.write(cmd)
+# Read part of the respone
+rsp1 = self.client.tpm.read(15)
+
+# Send a new cmd
+self.client.tpm.write(cmd)
+
+# Read the whole respone
+rsp2 = self.client.tpm.read()
+except:
+pass
+self.assertEqual(len(rsp1), 15)
+self.assertEqual(len(rsp2), 10 + 2 + 32)
+
+def test_send_two_cmds(self):
+"""Send two cmds without reading a response"""
+rejected = False
+try:
+fmt = '>HIIH'
+cmd = struct.pack(fmt,
+  tpm2.TPM2_ST_NO_SESSIONS,
+  struct.calcsize(fmt),
+  tpm2.TPM2_CC_GET_RANDOM,
+  0x20)
+self.client.tpm.write(cmd)
+
+# expect the second one to raise -EBUSY error
+self.client.tpm.write(cmd)
+rsp = self.client.tpm.read()
+
+except IOError, e:
+# read the response
+rsp = self.client.tpm.read()
+rejected = True
+pass
+except:
+pass
+self.assertEqual(rejected, True)
+
 class SpaceTest(unittest.TestCase):
 def setUp(self):
 logging.basicConfig(filename='SpaceTest.log', level=logging.DEBUG)

[PATCH v2 1/2] selftests/tpm2: Open tpm dev in unbuffered mode

[Tadeusz Struk]

In order to have control over how many bytes are read or written
the device needs to be opened in unbuffered mode.

Signed-off-by: Tadeusz Struk 
---

v2:
- Changed subject tags to selftests/tpm2:
---
 tools/testing/selftests/tpm2/tpm2.py |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/tpm2/tpm2.py 
b/tools/testing/selftests/tpm2/tpm2.py
index 40ea95ce2ead..c2b9f2b1a0ac 100644
--- a/tools/testing/selftests/tpm2/tpm2.py
+++ b/tools/testing/selftests/tpm2/tpm2.py
@@ -357,9 +357,9 @@ class Client:
 self.flags = flags
 
 if (self.flags & Client.FLAG_SPACE) == 0:
-self.tpm = open('/dev/tpm0', 'r+b')
+self.tpm = open('/dev/tpm0', 'r+b', buffering=0)
 else:
-self.tpm = open('/dev/tpmrm0', 'r+b')
+self.tpm = open('/dev/tpmrm0', 'r+b', buffering=0)
 
 def close(self):
 self.tpm.close()

Re: [PATCH 2/2] selftests: tpm2: Extend tests to cover partial reads

[Tadeusz Struk]

On 2/11/19 8:48 AM, Jarkko Sakkinen wrote:
> You are missing a cover letter from this patch set. Please have it in
> v2. Also use tag "selftests/tpm2" instead of having two tags in the
> short summaries. Now they look a bit weird.

Since when is the cover letter mandatory?
I understand that is helps for a complicated patch set
to explain the problem and solution in the cover letter,
but for this simple test case addition what's the point?
And there is nothing forcing a cover letter in
https://www.kernel.org/doc/html/v4.20/process/submitting-patches.html

Also double tags seams to be quite common for selftest.
See git log tools/testing/selftests/
Thanks,
-- 
Tadeusz

[PATCH 2/2] selftests: tpm2: Extend tests to cover partial reads

[Tadeusz Struk]

Three new tests added:
1. Send get random cmd, read header in 1st read, read the rest in second
   read - expect success
2. Send get random cmd, read only part of the response, send another
   get random command, read the response - expect success
3. Send get random cmd followed by another get random cmd, without
   reading the first response - expect the second cmd to fail with -EBUSY

Signed-off-by: Tadeusz Struk 
---
 tools/testing/selftests/tpm2/tpm2.py   |1 
 tools/testing/selftests/tpm2/tpm2_tests.py |   82 
 2 files changed, 83 insertions(+)

diff --git a/tools/testing/selftests/tpm2/tpm2.py 
b/tools/testing/selftests/tpm2/tpm2.py
index c2b9f2b1a0ac..828c18584624 100644
--- a/tools/testing/selftests/tpm2/tpm2.py
+++ b/tools/testing/selftests/tpm2/tpm2.py
@@ -22,6 +22,7 @@ TPM2_CC_UNSEAL = 0x015E
 TPM2_CC_FLUSH_CONTEXT = 0x0165
 TPM2_CC_START_AUTH_SESSION = 0x0176
 TPM2_CC_GET_CAPABILITY = 0x017A
+TPM2_CC_GET_RANDOM = 0x017B
 TPM2_CC_PCR_READ = 0x017E
 TPM2_CC_POLICY_PCR = 0x017F
 TPM2_CC_PCR_EXTEND = 0x0182
diff --git a/tools/testing/selftests/tpm2/tpm2_tests.py 
b/tools/testing/selftests/tpm2/tpm2_tests.py
index 3bb066fea4a0..3f6a65cbdd39 100644
--- a/tools/testing/selftests/tpm2/tpm2_tests.py
+++ b/tools/testing/selftests/tpm2/tpm2_tests.py
@@ -158,6 +158,88 @@ class SmokeTest(unittest.TestCase):
 pass
 self.assertEqual(rejected, True)
 
+def test_read_partial_resp(self):
+"""Reads random in two subsequent reads"""
+log = logging.getLogger(__name__)
+try:
+fmt = '>HIIH'
+cmd = struct.pack(fmt,
+  tpm2.TPM2_ST_NO_SESSIONS,
+  struct.calcsize(fmt),
+  tpm2.TPM2_CC_GET_RANDOM,
+  0x20)
+self.client.tpm.write(cmd)
+hdr = self.client.tpm.read(10)
+sz = struct.unpack('>I', hdr[2:6])[0]
+rsp = self.client.tpm.read()
+log.debug("header:")
+log.debug(tpm2.hex_dump(hdr))
+log.debug("resp:")
+log.debug(tpm2.hex_dump(rsp))
+except:
+log.debug("Error:", sys.exc_info()[0])
+raise
+
+self.assertEqual(sz, 10 + 2 + 32)
+self.assertEqual(len(rsp), 2 + 32)
+
+def test_read_partial_overwrite(self):
+"""Reads only part of the response and issue a new cmd"""
+log = logging.getLogger(__name__)
+try:
+fmt = '>HIIH'
+cmd = struct.pack(fmt,
+  tpm2.TPM2_ST_NO_SESSIONS,
+  struct.calcsize(fmt),
+  tpm2.TPM2_CC_GET_RANDOM,
+  0x20)
+self.client.tpm.write(cmd)
+# Read part of the respone
+rsp1 = self.client.tpm.read(15)
+log.debug("rsp 1:")
+log.debug(tpm2.hex_dump(rsp1))
+
+# Send a new cmd
+self.client.tpm.write(cmd)
+
+# Read the whole respone
+rsp2 = self.client.tpm.read()
+log.debug("rsp 2:")
+log.debug(tpm2.hex_dump(rsp2))
+
+except:
+log.debug("Error:", sys.exc_info()[0])
+raise
+
+self.assertEqual(len(rsp1), 15)
+self.assertEqual(len(rsp2), 10 + 2 + 32)
+
+def test_send_two_cmds(self):
+"""Send two cmds without reading a response"""
+rejected = False
+try:
+fmt = '>HIIH'
+cmd = struct.pack(fmt,
+  tpm2.TPM2_ST_NO_SESSIONS,
+  struct.calcsize(fmt),
+  tpm2.TPM2_CC_GET_RANDOM,
+  0x20)
+self.client.tpm.write(cmd)
+
+# expect the second one to raise -EBUSY error
+self.client.tpm.write(cmd)
+rsp = self.client.tpm.read()
+
+except IOError, e:
+# read the response
+rsp = self.client.tpm.read()
+rejected = True
+pass
+except:
+raise
+
+self.assertEqual(rejected, True)
+
 class SpaceTest(unittest.TestCase):
 def setUp(self):
 logging.basicConfig(filename='SpaceTest.log', level=logging.DEBUG)

[PATCH 1/2] selftests: tpm2: Open tpm dev in unbuffered mode

[Tadeusz Struk]

In order to have control over how many bytes are read or written
the device needs to be opened in unbuffered mode.

Signed-off-by: Tadeusz Struk 
---
 tools/testing/selftests/tpm2/tpm2.py |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/tpm2/tpm2.py 
b/tools/testing/selftests/tpm2/tpm2.py
index 40ea95ce2ead..c2b9f2b1a0ac 100644
--- a/tools/testing/selftests/tpm2/tpm2.py
+++ b/tools/testing/selftests/tpm2/tpm2.py
@@ -357,9 +357,9 @@ class Client:
 self.flags = flags
 
 if (self.flags & Client.FLAG_SPACE) == 0:
-self.tpm = open('/dev/tpm0', 'r+b')
+self.tpm = open('/dev/tpm0', 'r+b', buffering=0)
 else:
-self.tpm = open('/dev/tpmrm0', 'r+b')
+self.tpm = open('/dev/tpmrm0', 'r+b', buffering=0)
 
 def close(self):
 self.tpm.close()

Re: [PATCH] selftests: add TPM 2.0 tests

[Tadeusz Struk]

On 11/27/18 2:10 PM, Jarkko Sakkinen wrote:
> Added the tests that I've been using for testing TPM 2.0 functionality
> for long time but have out-of-tree so far residing in
> 
> https://github.com/jsakkine-intel/tpm2-scripts
> 
> Cc: Tadeusz Struk 
> Signed-off-by: Jarkko Sakkinen 
> ---
> Tadeusz: does not include your test. Better that you send them after
> this has been accepted to get proper SOB info.

Sure I can follow up after this is merged.
Thanks,
-- 
Tadeusz

[PATCH v6] 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 
---
The usecase is implemented in this TSS commit:
https://github.com/tpm2-software/tpm2-tss/commit/ce982f67a67dc08e24683d30b05800648d8a264c

Changes in v6:
 - Remove needless comments in struct file_priv

Changes in v5:
 - Merge the two patches back into one.
 - Change variable 'partial_data' to a new flag 'response_read'.
 - Update the logic around partial_data to reflect the new flag.
 - Add a new variable response_length that keeps track of how much
   of the response is left.

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 |   51 +
 drivers/char/tpm/tpm-dev.h|5 +---
 2 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c 
b/drivers/char/tpm/tpm-dev-common.c
index 99b5133a9d05..344739223451 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->response_length = ret;
mod_timer(&priv->user_read_timer, jiffies + (120 * HZ));
}
mutex_unlock(&priv->buffer_mutex);
@@ -63,7 +63,8 @@ static void tpm_timeout_work(struct work_struct *work)
  timeout_work);
 
mutex_lock(&priv->buffer_mutex);
-   priv->data_pending = 0;
+   priv->response_read = true;
+   priv->response_length = 0;
memset(priv->data_buffer, 0, sizeof(priv->data_buffer));
mutex_unlock(&priv->buffer_mutex);
wake_up_interruptible(&priv->async_wait);
@@ -74,6 +75,7 @@ void tpm_common_open(struct file *file, struct tpm_chip *chip,
 {
priv->chip = chip;
priv->space = space;
+   priv->response_read = true;
 
mutex_init(&priv->buffer_mutex);
timer_setup(&priv->user_read_timer, user_reader_timeout, 0);
@@ -90,22 +92,35 @@ 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) {
-   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;
+   if (priv->response_length) {
+   priv->response_read = true;
+
+   ret_size = min_t(ssize_t, size, priv->response_length);
+   if (!ret_size) {
+   priv->response_length = 0;
+   goto out;
}
 
-   priv->data_pending = 0;
+   rc = copy_to_user(buf, priv->data_buffer + *off, ret_size);
+   if (rc) {
+   memset(priv->data_buffer, 0, TPM_BUFSIZE);
+   priv->response_length = 0;
+   ret_size = -EFAULT;
+   } else {
+   memset(priv->data_buffer + *off, 0, r

Re: [PATCH v5] tpm: add support for partial reads

[Tadeusz Struk]

On 11/20/18 3:07 PM, Jarkko Sakkinen wrote:
 +  /* Holds the resul of the last successful call to tpm_transmit() */
>>> This comment is cruft.
>> Do you want me to remove it? This is the comment you proposed.
> As I explained before it made sense before you made the remark that
> it can only get positive values i.e. the length.
> 
So what do you want me to do with this one?

-- 
Tadeusz

Re: [PATCH v5] tpm: add support for partial reads

[Tadeusz Struk]

On 11/20/18 4:48 AM, Jarkko Sakkinen wrote:
>> +/* Holds the resul of the last successful call to tpm_transmit() */
> This comment is cruft.

Do you want me to remove it? This is the comment you proposed.

> 
>> +size_t response_length;
> data_pending would be now perfectly fine name now that we concluded
> that the original length is not needed to be stored. Better than this
> as once you decrease it the variable name and contents mismatch.
> 

Can we finally agree on something? We have changed three times already.
response_length is exactly what it is and data_pending is a bit vague.

>> +/* Becomes true when the response (or part of it) is consumed */
>> +bool response_read;
> /* Tell whether response been consumed (read at least once). */
> 
Ok I'll update the comment.

-- 
Tadeusz

Re: [PATCH v5] tpm: add support for partial reads

[Tadeusz Struk]

On 11/20/18 4:48 AM, Jarkko Sakkinen wrote:
>> The usecase is implemented in this TSS commit:
>> https://github.com/tpm2-software/tpm2-tss/commit/ce982f67a67dc08e24683d30b05800648d8a264c
> Can you implement test for this to
> 
> https://github.com/jsakkine-intel/tpm2-scripts

Just created a PR for you
https://github.com/jsakkine-intel/tpm2-scripts/pull/4

-- 
Tadeusz

[PATCH v5] 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 
---
The usecase is implemented in this TSS commit:
https://github.com/tpm2-software/tpm2-tss/commit/ce982f67a67dc08e24683d30b05800648d8a264c

Changes in v5:
 - Merge the two patches back into one.
 - Change variable 'partial_data' to a new flag 'response_read'.
 - Update the logic around partial_data to reflect the new flag.
 - Add a new variable response_length that keeps track of how much
   of the response is left.

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 |   51 +
 drivers/char/tpm/tpm-dev.h|7 +++--
 2 files changed, 39 insertions(+), 19 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c 
b/drivers/char/tpm/tpm-dev-common.c
index 99b5133a9d05..344739223451 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->response_length = ret;
mod_timer(&priv->user_read_timer, jiffies + (120 * HZ));
}
mutex_unlock(&priv->buffer_mutex);
@@ -63,7 +63,8 @@ static void tpm_timeout_work(struct work_struct *work)
  timeout_work);
 
mutex_lock(&priv->buffer_mutex);
-   priv->data_pending = 0;
+   priv->response_read = true;
+   priv->response_length = 0;
memset(priv->data_buffer, 0, sizeof(priv->data_buffer));
mutex_unlock(&priv->buffer_mutex);
wake_up_interruptible(&priv->async_wait);
@@ -74,6 +75,7 @@ void tpm_common_open(struct file *file, struct tpm_chip *chip,
 {
priv->chip = chip;
priv->space = space;
+   priv->response_read = true;
 
mutex_init(&priv->buffer_mutex);
timer_setup(&priv->user_read_timer, user_reader_timeout, 0);
@@ -90,22 +92,35 @@ 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) {
-   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;
+   if (priv->response_length) {
+   priv->response_read = true;
+
+   ret_size = min_t(ssize_t, size, priv->response_length);
+   if (!ret_size) {
+   priv->response_length = 0;
+   goto out;
}
 
-   priv->data_pending = 0;
+   rc = copy_to_user(buf, priv->data_buffer + *off, ret_size);
+   if (rc) {
+   memset(priv->data_buffer, 0, TPM_BUFSIZE);
+   priv->response_length = 0;
+   ret_size = -EFAULT;
+   } else {
+   memset(priv->data_buffer + *off, 0, ret_size);
+   priv->response_length -= ret_size;

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

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

[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

[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 
---
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)
 

[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 
---
 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 */

Re: [PATCH v3] tpm: add support for partial reads

[Tadeusz Struk]

On 11/15/18 3:31 PM, Jarkko Sakkinen wrote:
> You could drop these memset() calls and also one from
> tpm_timeout_work(). The call could be done once in the beginning of
> tpm_common_write() instead of having three different call sites.
> 

Don't we want to clean the buffer as the response is read?
When we will only memset it in write and if one would send
just one command there will only be one response.
The data will sit in the buffer until the next command.
There will be a quite bit time window when the data can leak.

> 
> Naming becomes a mess and the comment for data_pending variable is
> incorrect as it is also used for synchronous operation.
> 
> Maybe add a prepending commit to rename it as
> 
>   /* Holds the resul of the tpm_transmit() last call. */
>   ssize_t transmit_result;

Agree, will do that.

> 
> That is at least clear and obvious on what it contains.
> 
> The comment for partial_data is incorrect as the variable does not
> contain any data.

Yes, I will change it.

> 
> We could use declare:
> 
>   /* Holds the count how much of the response is still unread. */
>   size_t response_pending;
> 
> Observe another remark from your commit: there is no reaso to ssize_t as
> the type as the value should never be a negative number.

Yes, in fact now the priv->data_pending can also be only positive.
I'll change it and send a new version soon.
Thanks,
-- 
Tadeusz

[PATCH v3] 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 
---
The usecase is implemented in this TSS commit:
https://github.com/tpm2-software/tpm2-tss/commit/ce982f67a67dc08e24683d30b05800648d8a264c

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 99b5133a9d05..5d43b0c28565 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, 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,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->data_pending)
+   if (priv->data_pending || 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 a126b57

Re: [PATCH v2] tpm: add support for partial reads

[Tadeusz Struk]

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

Re: [PATCH v2] tpm: add support for partial reads

[Tadeusz Struk]

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

[PATCH v2] 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.

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

Signed-off-by: Tadeusz Struk 
---
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;
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 */

Re: [PATCH v6 0/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

On 9/16/18 5:03 AM, Jarkko Sakkinen wrote:
> I tried to test this but I get 404 from 
> https://github.com/tstruk/tpm2-tss/tree/async

This has been already merged to tss upstream 
https://github.com/tpm2-software/tpm2-tss
To enable it you need to configure tss with --enable-tcti-device-async=yes
Thanks,
Tadeusz

Re: [PATCH v5 0/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

On 08/31/2018 01:58 AM, Jarkko Sakkinen wrote:
> Just the change to the commit message. Mislooked patchwork, the typo was
> in my response :-) I'll do recheck for 2/2. Check those comments before
> v6 if there is anything else.

Hi,
I have done the changes you requested and ran the "checkpatch.pl --strict"
on it and it says that "no obvious style problems and is ready for submission".
Thanks,
-- 
Tadeusz

[PATCH v6 1/2] tpm: add ptr to the tpm_space struct to file_priv

[Tadeusz Struk]

Add a ptr to struct tpm_space to the file_priv and consolidate
of the write operations for the two interfaces.

Tested-by: Philip Tricca 
Signed-off-by: Tadeusz Struk 
---
 drivers/char/tpm/tpm-dev-common.c |8 +---
 drivers/char/tpm/tpm-dev.c|   10 ++
 drivers/char/tpm/tpm-dev.h|5 +++--
 drivers/char/tpm/tpmrm-dev.c  |   14 ++
 4 files changed, 12 insertions(+), 25 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c 
b/drivers/char/tpm/tpm-dev-common.c
index e4a04b2d3c32..f0c033b69b62 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -43,9 +43,11 @@ static void timeout_work(struct work_struct *work)
 }
 
 void tpm_common_open(struct file *file, struct tpm_chip *chip,
-struct file_priv *priv)
+struct file_priv *priv, struct tpm_space *space)
 {
priv->chip = chip;
+   priv->space = space;
+
mutex_init(&priv->buffer_mutex);
timer_setup(&priv->user_read_timer, user_reader_timeout, 0);
INIT_WORK(&priv->work, timeout_work);
@@ -79,7 +81,7 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
 }
 
 ssize_t tpm_common_write(struct file *file, const char __user *buf,
-size_t size, loff_t *off, struct tpm_space *space)
+size_t size, loff_t *off)
 {
struct file_priv *priv = file->private_data;
size_t in_size = size;
@@ -119,7 +121,7 @@ ssize_t tpm_common_write(struct file *file, const char 
__user *buf,
mutex_unlock(&priv->buffer_mutex);
return -EPIPE;
}
-   out_size = tpm_transmit(priv->chip, space, priv->data_buffer,
+   out_size = tpm_transmit(priv->chip, priv->space, priv->data_buffer,
sizeof(priv->data_buffer), 0);
 
tpm_put_ops(priv->chip);
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index ebd74ab5abef..98b9630c3a36 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -39,7 +39,7 @@ static int tpm_open(struct inode *inode, struct file *file)
if (priv == NULL)
goto out;
 
-   tpm_common_open(file, chip, priv);
+   tpm_common_open(file, chip, priv, NULL);
 
return 0;
 
@@ -48,12 +48,6 @@ static int tpm_open(struct inode *inode, struct file *file)
return -ENOMEM;
 }
 
-static ssize_t tpm_write(struct file *file, const char __user *buf,
-size_t size, loff_t *off)
-{
-   return tpm_common_write(file, buf, size, off, NULL);
-}
-
 /*
  * Called on file close
  */
@@ -73,6 +67,6 @@ const struct file_operations tpm_fops = {
.llseek = no_llseek,
.open = tpm_open,
.read = tpm_common_read,
-   .write = tpm_write,
+   .write = tpm_common_write,
.release = tpm_release,
 };
diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
index b24cfb4d3ee1..4048677bbd78 100644
--- a/drivers/char/tpm/tpm-dev.h
+++ b/drivers/char/tpm/tpm-dev.h
@@ -6,6 +6,7 @@
 
 struct file_priv {
struct tpm_chip *chip;
+   struct tpm_space *space;
 
/* Data passed to and from the tpm via the read/write calls */
size_t data_pending;
@@ -18,11 +19,11 @@ struct file_priv {
 };
 
 void tpm_common_open(struct file *file, struct tpm_chip *chip,
-struct file_priv *priv);
+struct file_priv *priv, struct tpm_space *space);
 ssize_t tpm_common_read(struct file *file, char __user *buf,
size_t size, loff_t *off);
 ssize_t tpm_common_write(struct file *file, const char __user *buf,
-size_t size, loff_t *off, struct tpm_space *space);
+size_t size, loff_t *off);
 void tpm_common_release(struct file *file, struct file_priv *priv);
 
 #endif
diff --git a/drivers/char/tpm/tpmrm-dev.c b/drivers/char/tpm/tpmrm-dev.c
index 1a0e97a5da5a..96006c6b9696 100644
--- a/drivers/char/tpm/tpmrm-dev.c
+++ b/drivers/char/tpm/tpmrm-dev.c
@@ -28,7 +28,7 @@ static int tpmrm_open(struct inode *inode, struct file *file)
return -ENOMEM;
}
 
-   tpm_common_open(file, chip, &priv->priv);
+   tpm_common_open(file, chip, &priv->priv, &priv->space);
 
return 0;
 }
@@ -45,21 +45,11 @@ static int tpmrm_release(struct inode *inode, struct file 
*file)
return 0;
 }
 
-static ssize_t tpmrm_write(struct file *file, const char __user *buf,
-  size_t size, loff_t *off)
-{
-   struct file_priv *fpriv = file->private_data;
-   struct tpmrm_priv *priv = container_of(fpriv, struct tpmrm_priv, priv);
-
-   return tpm_common_write(file, buf, size, off, &priv->space);
-}
-
 const struct file_operations tpmrm_fops = {
.owner = THIS_MODULE,
.llseek = no_llseek,
.open = 

[PATCH v6 0/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

The TCG SAPI specification [1] defines a set of functions, which allow
applications to use the TPM device in either blocking or non-blocking fashion.
Each command defined by the specification has a corresponding
Tss2_Sys__Prepare() and Tss2_Sys__Complete() call, which
together with Tss2_Sys_ExecuteAsync() is designed to allow asynchronous
mode of operation. Currently the TPM driver supports only blocking calls,
which doesn't allow asynchronous IO operations.
This patch changes it and adds support for nonblocking write and a new poll
function to enable applications, which want to take advantage of this feature.
The new functionality can be tested using standard TPM tools implemented
in [2], together with modified TCTI from [3], and an example application
by Philip Tricca [4]. Here is a short description from Philip:

"The example application `glib-tss2-event` uses a glib main event loop
to create an RSA 2048 primary key in the TPM2 NULL hierarchy while
using a glib timer event to time the operation. A GSource object is
used to generate an event when the FD underlying the tss2 function
call has data ready. While the application waits for an event indicating
that the CreatePrimary operation is complete, it counts timer events
that occur every 100ms. Once the CreatePrimary operation completes the
number of timer events that occurred is used to make a rough calculation
of the elapsed time. This value is then printed to the console.
This takes ~300 lines of C code and requires no management or
synchronization of threads. The glib GMainContext is "just a poll()
loop" according to the glib documentation here:

https://developer.gnome.org/programming-guidelines/stable/main-contexts.html.en

and so supporting 'poll' is the easiest way to integrate with glib /
gtk+. This is true of any other event system that relies on 'poll'
instead of worker threads."

[1] 
https://trustedcomputinggroup.org/wp-content/uploads/TSS_SAPI_Version-1.1_Revision-22_review_030918.pdf
[2] https://github.com/tpm2-software/tpm2-tools
[3] https://github.com/tstruk/tpm2-tss/tree/async
[4] https://github.com/flihp/glib-tss2-async-example

---
Changes in v6:
- Changed commit message in the first patch to more specific.
- Chenged labels names in tpm-interface.c

Changes in v5:
- Changed the workqueue allocation time back from the first user interface
  open to module init.

Changes in v4:
- Changed the way buffer_mutex is handled in nonblocking mode so that
  it is not held when write() returns to user space.

Changes in v3:
- Fixed problem reported by 0-dey kbuild test robot around __exitcall.
  It complained because there is a module_exit() in another file already.
- Added info on example application from Philip

Changes in v2:
- Split the change into two separate patches. First patch adds a pointer
  to the space to the struct file_priv to have access to it from the async job.
  This is to avoid memory allocations on every write call. Now everything
  what's needed is in the file_priv struct.
- Renamed the 'work' member of the timer to avoid confusion.
  Now there are 'timeout_work' and 'async_work'.
- Removed the global wait queue and moved it to file_priv.
- Only creating the work queue when the first file is opened.

Tadeusz Struk (2):
  tpm: add ptr to the tpm_space struct to file_priv
  tpm: add support for nonblocking operation

 drivers/char/tpm/tpm-dev-common.c |  150 +++--
 drivers/char/tpm/tpm-dev.c|   22 +++--
 drivers/char/tpm/tpm-dev.h|   19 +++--
 drivers/char/tpm/tpm-interface.c  |1 
 drivers/char/tpm/tpm.h|1 
 drivers/char/tpm/tpmrm-dev.c  |   31 
 6 files changed, 152 insertions(+), 72 deletions(-)

--
TS

[PATCH v6 2/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

Currently the TPM driver only supports blocking calls, which doesn't allow
asynchronous IO operations to the TPM hardware.
This patch changes it and adds support for nonblocking write and a new poll
function to enable applications, which want to take advantage of this.

Tested-by: Philip Tricca 
Signed-off-by: Tadeusz Struk 
---
 drivers/char/tpm/tpm-dev-common.c |  141 -
 drivers/char/tpm/tpm-dev.c|1 
 drivers/char/tpm/tpm-dev.h|   13 ++-
 drivers/char/tpm/tpm-interface.c  |   24 +-
 drivers/char/tpm/tpm.h|2 +
 drivers/char/tpm/tpmrm-dev.c  |1 
 6 files changed, 137 insertions(+), 45 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c 
b/drivers/char/tpm/tpm-dev-common.c
index f0c033b69b62..99b5133a9d05 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -17,11 +17,36 @@
  * License.
  *
  */
+#include 
 #include 
 #include 
+#include 
 #include "tpm.h"
 #include "tpm-dev.h"
 
+static struct workqueue_struct *tpm_dev_wq;
+static DEFINE_MUTEX(tpm_dev_wq_lock);
+
+static void tpm_async_work(struct work_struct *work)
+{
+   struct file_priv *priv =
+   container_of(work, struct file_priv, async_work);
+   ssize_t ret;
+
+   mutex_lock(&priv->buffer_mutex);
+   priv->command_enqueued = false;
+   ret = tpm_transmit(priv->chip, priv->space, priv->data_buffer,
+  sizeof(priv->data_buffer), 0);
+
+   tpm_put_ops(priv->chip);
+   if (ret > 0) {
+   priv->data_pending = ret;
+   mod_timer(&priv->user_read_timer, jiffies + (120 * HZ));
+   }
+   mutex_unlock(&priv->buffer_mutex);
+   wake_up_interruptible(&priv->async_wait);
+}
+
 static void user_reader_timeout(struct timer_list *t)
 {
struct file_priv *priv = from_timer(priv, t, user_read_timer);
@@ -29,17 +54,19 @@ static void user_reader_timeout(struct timer_list *t)
pr_warn("TPM user space timeout is deprecated (pid=%d)\n",
task_tgid_nr(current));
 
-   schedule_work(&priv->work);
+   schedule_work(&priv->timeout_work);
 }
 
-static void timeout_work(struct work_struct *work)
+static void tpm_timeout_work(struct work_struct *work)
 {
-   struct file_priv *priv = container_of(work, struct file_priv, work);
+   struct file_priv *priv = container_of(work, struct file_priv,
+ timeout_work);
 
mutex_lock(&priv->buffer_mutex);
priv->data_pending = 0;
memset(priv->data_buffer, 0, sizeof(priv->data_buffer));
mutex_unlock(&priv->buffer_mutex);
+   wake_up_interruptible(&priv->async_wait);
 }
 
 void tpm_common_open(struct file *file, struct tpm_chip *chip,
@@ -50,8 +77,9 @@ void tpm_common_open(struct file *file, struct tpm_chip *chip,
 
mutex_init(&priv->buffer_mutex);
timer_setup(&priv->user_read_timer, user_reader_timeout, 0);
-   INIT_WORK(&priv->work, timeout_work);
-
+   INIT_WORK(&priv->timeout_work, tpm_timeout_work);
+   INIT_WORK(&priv->async_work, tpm_async_work);
+   init_waitqueue_head(&priv->async_wait);
file->private_data = priv;
 }
 
@@ -63,15 +91,17 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
int rc;
 
del_singleshot_timer_sync(&priv->user_read_timer);
-   flush_work(&priv->work);
+   flush_work(&priv->timeout_work);
mutex_lock(&priv->buffer_mutex);
 
if (priv->data_pending) {
ret_size = min_t(ssize_t, size, priv->data_pending);
-   rc = copy_to_user(buf, priv->data_buffer, ret_size);
-   memset(priv->data_buffer, 0, priv->data_pending);
-   if (rc)
-   ret_size = -EFAULT;
+   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;
+   }
 
priv->data_pending = 0;
}
@@ -84,10 +114,9 @@ ssize_t tpm_common_write(struct file *file, const char 
__user *buf,
 size_t size, loff_t *off)
 {
struct file_priv *priv = file->private_data;
-   size_t in_size = size;
-   ssize_t out_size;
+   int ret = 0;
 
-   if (in_size > TPM_BUFSIZE)
+   if (size > TPM_BUFSIZE)
return -E2BIG;
 
mutex_lock(&priv->buffer_mutex);
@@ -96,21 +125,20 @@ ssize_t tpm_common_write(struct file *file, const char 
__user *buf,
 * tpm_read or a user_read_timer timeout. This also preven

[PATCH v5 2/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

Currently the TPM driver only supports blocking calls, which doesn't allow
asynchronous IO operations to the TPM hardware.
This patch changes it and adds support for nonblocking write and a new poll
function to enable applications, which want to take advantage of this.

Tested-by: Philip Tricca 
Signed-off-by: Tadeusz Struk 
---
 drivers/char/tpm/tpm-dev-common.c |  142 -
 drivers/char/tpm/tpm-dev.c|1 
 drivers/char/tpm/tpm-dev.h|   13 ++-
 drivers/char/tpm/tpm-interface.c  |   24 +-
 drivers/char/tpm/tpm.h|2 +
 drivers/char/tpm/tpmrm-dev.c  |1 
 6 files changed, 138 insertions(+), 45 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c 
b/drivers/char/tpm/tpm-dev-common.c
index f0c033b69b62..a7ae0072044b 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -17,11 +17,36 @@
  * License.
  *
  */
+#include 
 #include 
 #include 
+#include 
 #include "tpm.h"
 #include "tpm-dev.h"
 
+static struct workqueue_struct *tpm_dev_wq;
+static DEFINE_MUTEX(tpm_dev_wq_lock);
+
+static void tpm_async_work(struct work_struct *work)
+{
+   struct file_priv *priv =
+   container_of(work, struct file_priv, async_work);
+   ssize_t ret;
+
+   mutex_lock(&priv->buffer_mutex);
+   priv->command_enqueued = false;
+   ret = tpm_transmit(priv->chip, priv->space, priv->data_buffer,
+  sizeof(priv->data_buffer), 0);
+
+   tpm_put_ops(priv->chip);
+   if (ret > 0) {
+   priv->data_pending = ret;
+   mod_timer(&priv->user_read_timer, jiffies + (120 * HZ));
+   }
+   mutex_unlock(&priv->buffer_mutex);
+   wake_up_interruptible(&priv->async_wait);
+}
+
 static void user_reader_timeout(struct timer_list *t)
 {
struct file_priv *priv = from_timer(priv, t, user_read_timer);
@@ -29,17 +54,19 @@ static void user_reader_timeout(struct timer_list *t)
pr_warn("TPM user space timeout is deprecated (pid=%d)\n",
task_tgid_nr(current));
 
-   schedule_work(&priv->work);
+   schedule_work(&priv->timeout_work);
 }
 
-static void timeout_work(struct work_struct *work)
+static void tpm_timeout_work(struct work_struct *work)
 {
-   struct file_priv *priv = container_of(work, struct file_priv, work);
+   struct file_priv *priv = container_of(work, struct file_priv,
+ timeout_work);
 
mutex_lock(&priv->buffer_mutex);
priv->data_pending = 0;
memset(priv->data_buffer, 0, sizeof(priv->data_buffer));
mutex_unlock(&priv->buffer_mutex);
+   wake_up_interruptible(&priv->async_wait);
 }
 
 void tpm_common_open(struct file *file, struct tpm_chip *chip,
@@ -50,8 +77,9 @@ void tpm_common_open(struct file *file, struct tpm_chip *chip,
 
mutex_init(&priv->buffer_mutex);
timer_setup(&priv->user_read_timer, user_reader_timeout, 0);
-   INIT_WORK(&priv->work, timeout_work);
-
+   INIT_WORK(&priv->timeout_work, tpm_timeout_work);
+   INIT_WORK(&priv->async_work, tpm_async_work);
+   init_waitqueue_head(&priv->async_wait);
file->private_data = priv;
 }
 
@@ -63,15 +91,17 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
int rc;
 
del_singleshot_timer_sync(&priv->user_read_timer);
-   flush_work(&priv->work);
+   flush_work(&priv->timeout_work);
mutex_lock(&priv->buffer_mutex);
 
if (priv->data_pending) {
ret_size = min_t(ssize_t, size, priv->data_pending);
-   rc = copy_to_user(buf, priv->data_buffer, ret_size);
-   memset(priv->data_buffer, 0, priv->data_pending);
-   if (rc)
-   ret_size = -EFAULT;
+   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;
+   }
 
priv->data_pending = 0;
}
@@ -84,10 +114,9 @@ ssize_t tpm_common_write(struct file *file, const char 
__user *buf,
 size_t size, loff_t *off)
 {
struct file_priv *priv = file->private_data;
-   size_t in_size = size;
-   ssize_t out_size;
+   int ret = 0;
 
-   if (in_size > TPM_BUFSIZE)
+   if (size > TPM_BUFSIZE)
return -E2BIG;
 
mutex_lock(&priv->buffer_mutex);
@@ -96,21 +125,20 @@ ssize_t tpm_common_write(struct file *file, const char 
__user *buf,
 * tpm_read or a user_read_timer timeout. This also preven

[PATCH v5 0/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

The TCG SAPI specification [1] defines a set of functions, which allow
applications to use the TPM device in either blocking or non-blocking fashion.
Each command defined by the specification has a corresponding
Tss2_Sys__Prepare() and Tss2_Sys__Complete() call, which
together with Tss2_Sys_ExecuteAsync() is designed to allow asynchronous
mode of operation. Currently the TPM driver supports only blocking calls,
which doesn't allow asynchronous IO operations.
This patch changes it and adds support for nonblocking write and a new poll
function to enable applications, which want to take advantage of this feature.
The new functionality can be tested using standard TPM tools implemented
in [2], together with modified TCTI from [3], and an example application
by Philip Tricca [4]. Here is a short description from Philip:

"The example application `glib-tss2-event` uses a glib main event loop
to create an RSA 2048 primary key in the TPM2 NULL hierarchy while
using a glib timer event to time the operation. A GSource object is
used to generate an event when the FD underlying the tss2 function
call has data ready. While the application waits for an event indicating
that the CreatePrimary operation is complete, it counts timer events
that occur every 100ms. Once the CreatePrimary operation completes the
number of timer events that occurred is used to make a rough calculation
of the elapsed time. This value is then printed to the console.
This takes ~300 lines of C code and requires no management or
synchronization of threads. The glib GMainContext is "just a poll()
loop" according to the glib documentation here:

https://developer.gnome.org/programming-guidelines/stable/main-contexts.html.en

and so supporting 'poll' is the easiest way to integrate with glib /
gtk+. This is true of any other event system that relies on 'poll'
instead of worker threads."

[1] 
https://trustedcomputinggroup.org/wp-content/uploads/TSS_SAPI_Version-1.1_Revision-22_review_030918.pdf
[2] https://github.com/tpm2-software/tpm2-tools
[3] https://github.com/tstruk/tpm2-tss/tree/async
[4] https://github.com/flihp/glib-tss2-async-example

---
Changes in v5:
- Changed the workqueue allocation time back from the first user interface
  open to module init.

Changes in v4:
- Changed the way buffer_mutex is handled in nonblocking mode so that
  it is not held when write() returns to user space.

Changes in v3:
- Fixed problem reported by 0-dey kbuild test robot around __exitcall.
  It complained because there is a module_exit() in another file already.
- Added info on example application from Philip

Changes in v2:
- Split the change into two separate patches. First patch adds a pointer
  to the space to the struct file_priv to have access to it from the async job.
  This is to avoid memory allocations on every write call. Now everything
  what's needed is in the file_priv struct.
- Renamed the 'work' member of the timer to avoid confusion.
  Now there are 'timeout_work' and 'async_work'.
- Removed the global wait queue and moved it to file_priv.
- Only creating the work queue when the first file is opened.

Tadeusz Struk (2):
  tpm: add ptr to the tpm_space struct to file_priv
  tpm: add support for nonblocking operation

 drivers/char/tpm/tpm-dev-common.c |  150 +++--
 drivers/char/tpm/tpm-dev.c|   22 +++--
 drivers/char/tpm/tpm-dev.h|   19 +++--
 drivers/char/tpm/tpm-interface.c  |1 
 drivers/char/tpm/tpm.h|1 
 drivers/char/tpm/tpmrm-dev.c  |   31 
 6 files changed, 152 insertions(+), 72 deletions(-)

--
TS

[PATCH v5 1/2] tpm: add ptr to the tpm_space struct to file_priv

[Tadeusz Struk]

Add a ptr to struct tpm_space to the file_priv to have an easy
access to it in the async job without the need to allocate memory.
This also allows to consolidate of the write operations for
the two interfaces.

Tested-by: Philip Tricca 
Signed-off-by: Tadeusz Struk 
---
 drivers/char/tpm/tpm-dev-common.c |8 +---
 drivers/char/tpm/tpm-dev.c|   10 ++
 drivers/char/tpm/tpm-dev.h|5 +++--
 drivers/char/tpm/tpmrm-dev.c  |   14 ++
 4 files changed, 12 insertions(+), 25 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c 
b/drivers/char/tpm/tpm-dev-common.c
index e4a04b2d3c32..f0c033b69b62 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -43,9 +43,11 @@ static void timeout_work(struct work_struct *work)
 }
 
 void tpm_common_open(struct file *file, struct tpm_chip *chip,
-struct file_priv *priv)
+struct file_priv *priv, struct tpm_space *space)
 {
priv->chip = chip;
+   priv->space = space;
+
mutex_init(&priv->buffer_mutex);
timer_setup(&priv->user_read_timer, user_reader_timeout, 0);
INIT_WORK(&priv->work, timeout_work);
@@ -79,7 +81,7 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
 }
 
 ssize_t tpm_common_write(struct file *file, const char __user *buf,
-size_t size, loff_t *off, struct tpm_space *space)
+size_t size, loff_t *off)
 {
struct file_priv *priv = file->private_data;
size_t in_size = size;
@@ -119,7 +121,7 @@ ssize_t tpm_common_write(struct file *file, const char 
__user *buf,
mutex_unlock(&priv->buffer_mutex);
return -EPIPE;
}
-   out_size = tpm_transmit(priv->chip, space, priv->data_buffer,
+   out_size = tpm_transmit(priv->chip, priv->space, priv->data_buffer,
sizeof(priv->data_buffer), 0);
 
tpm_put_ops(priv->chip);
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index ebd74ab5abef..98b9630c3a36 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -39,7 +39,7 @@ static int tpm_open(struct inode *inode, struct file *file)
if (priv == NULL)
goto out;
 
-   tpm_common_open(file, chip, priv);
+   tpm_common_open(file, chip, priv, NULL);
 
return 0;
 
@@ -48,12 +48,6 @@ static int tpm_open(struct inode *inode, struct file *file)
return -ENOMEM;
 }
 
-static ssize_t tpm_write(struct file *file, const char __user *buf,
-size_t size, loff_t *off)
-{
-   return tpm_common_write(file, buf, size, off, NULL);
-}
-
 /*
  * Called on file close
  */
@@ -73,6 +67,6 @@ const struct file_operations tpm_fops = {
.llseek = no_llseek,
.open = tpm_open,
.read = tpm_common_read,
-   .write = tpm_write,
+   .write = tpm_common_write,
.release = tpm_release,
 };
diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
index b24cfb4d3ee1..4048677bbd78 100644
--- a/drivers/char/tpm/tpm-dev.h
+++ b/drivers/char/tpm/tpm-dev.h
@@ -6,6 +6,7 @@
 
 struct file_priv {
struct tpm_chip *chip;
+   struct tpm_space *space;
 
/* Data passed to and from the tpm via the read/write calls */
size_t data_pending;
@@ -18,11 +19,11 @@ struct file_priv {
 };
 
 void tpm_common_open(struct file *file, struct tpm_chip *chip,
-struct file_priv *priv);
+struct file_priv *priv, struct tpm_space *space);
 ssize_t tpm_common_read(struct file *file, char __user *buf,
size_t size, loff_t *off);
 ssize_t tpm_common_write(struct file *file, const char __user *buf,
-size_t size, loff_t *off, struct tpm_space *space);
+size_t size, loff_t *off);
 void tpm_common_release(struct file *file, struct file_priv *priv);
 
 #endif
diff --git a/drivers/char/tpm/tpmrm-dev.c b/drivers/char/tpm/tpmrm-dev.c
index 1a0e97a5da5a..96006c6b9696 100644
--- a/drivers/char/tpm/tpmrm-dev.c
+++ b/drivers/char/tpm/tpmrm-dev.c
@@ -28,7 +28,7 @@ static int tpmrm_open(struct inode *inode, struct file *file)
return -ENOMEM;
}
 
-   tpm_common_open(file, chip, &priv->priv);
+   tpm_common_open(file, chip, &priv->priv, &priv->space);
 
return 0;
 }
@@ -45,21 +45,11 @@ static int tpmrm_release(struct inode *inode, struct file 
*file)
return 0;
 }
 
-static ssize_t tpmrm_write(struct file *file, const char __user *buf,
-  size_t size, loff_t *off)
-{
-   struct file_priv *fpriv = file->private_data;
-   struct tpmrm_priv *priv = container_of(fpriv, struct tpmrm_priv, priv);
-
-   return tpm_common_write(file, buf, size, off, &priv->space);
-}
-
 const struct file_operation

Re: [PATCH v4 2/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

On 08/10/2018 12:00 PM, James Bottomley wrote:
> On Fri, 2018-08-10 at 11:56 -0700, Tadeusz Struk wrote:
>> On 08/10/2018 11:48 AM, James Bottomley wrote:
>>> On Fri, 2018-08-10 at 11:21 -0700, Tadeusz Struk wrote:
>>>> and the feedback I got from Jason was:
>>>>
>>>> "I wonder if it is worth creating this when the first file is
>>>> opened.. Lots of systems have TPMs but few use the userspace.."
>>>>
>>>> so I changed this to allocate the WQ on first open. I think it
>>>> makes sense, but I leave it to you to decide.
>>>
>>> If the reason is to not create a wq unless it's needed, shouldn't
>>> the condition actually be first open with flag O_NONBLOCK?
>>>
>>
>> Not really because one can do:
>>
>> int fd = open("/dev/tpm0", O_RDWR);
>> fcntl(fd, F_SETFL, O_NONBLOCK);
> 
> so move the condition to first need to queue ...
> 

That would work, even though this is not how this is usually done.
The first open looks like the sweet spot between module init
and first need to queue.
Thanks,
-- 
Tadeusz

Re: [PATCH v4 2/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

On 08/10/2018 11:48 AM, James Bottomley wrote:
> On Fri, 2018-08-10 at 11:21 -0700, Tadeusz Struk wrote:
>> and the feedback I got from Jason was:
>>
>> "I wonder if it is worth creating this when the first file is
>> opened.. Lots of systems have TPMs but few use the userspace.."
>>
>> so I changed this to allocate the WQ on first open. I think it makes
>> sense, but I leave it to you to decide.
> 
> If the reason is to not create a wq unless it's needed, shouldn't the
> condition actually be first open with flag O_NONBLOCK?
> 

Not really because one can do:

int fd = open("/dev/tpm0", O_RDWR);
fcntl(fd, F_SETFL, O_NONBLOCK);

-- 
Tadeusz

Re: [PATCH v4 2/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

On 08/10/2018 10:43 AM, Jarkko Sakkinen wrote:
>> +static struct workqueue_struct *tpm_dev_wq;
> A naming contradiction with tpm_common_read() and tpm_common_write(). To
> sort that up I would suggest adding a commit to the patch set that
> renames these functions as tpm_dev_common_read() and
> tpm_dev_common_write() and use the name tpm_common_dev_wq here.
> 

Currently we have: tpm_open(), tpm_write(), tpm_release() in tpm-dev.c
tpmrm_open(), tpmrm_read(), tpmrm_write(), tpmrm_release() in tpmrm-dev.c
tpm_common_open(), tpm_common_read(), tpm_common_write(), tpm_common_release() 
in tpm-dev-common.c

I think that's pretty consistent. Do you want me to rename all of them to 
tpm_dev_*()?
I don't see any value in doing this. What about if I just rename: 
tpm_dev_wq_lock to tpm_common_wq_lock, and tpm_dev_wq to tpm_common_wq?

>> +static DEFINE_MUTEX(tpm_dev_wq_lock);
> This is an unacceptable way to do it, Rather add:
> 
> int __init  tpm_dev_common_init(void)
> {
>   tpm_dev_common_wq = alloc_workqueue("tpm_dev_wq", WQ_MEM_RECLAIM, 0);
>   if (!tpm_dev_common_wq)
>   return -ENOMEM;
> 
>   return 0;
> }
> 
> and call this in the driver initialization.
> 
That was the way it was implemented in v1 
https://patchwork.kernel.org/patch/10442125/

See: static int __init tpm_dev_common_init(void)

and the feedback I got from Jason was:

"I wonder if it is worth creating this when the first file is
opened.. Lots of systems have TPMs but few use the userspace.."

so I changed this to allocate the WQ on first open. I think it makes sense,
but I leave it to you to decide.

Tadeusz,
-- 
Tadeusz

Re: [PATCH v4 1/2] tpm: add ptr to the tpm_space struct to file_priv

[Tadeusz Struk]

On 08/10/2018 10:27 AM, Jarkko Sakkinen wrote:
> On Tue, Aug 07, 2018 at 01:27:44PM -0700, Tadeusz Struk wrote:
>> Add a ptr to struct tpm_space to the file_priv to have an easy
>> access to it in the async job without the need to allocate memory.
>> This also allows to consolidate of the write operations for
>> the two interfaces.
> 
> I think the 2nd premise should be the priority and the first premise should
> be removed as it is not needed in any possible way to justify the
> change.

Jarkko,
The main reason why the pointer to tpm_space struct was added to the
file_priv was to have access to space in the async job when it is enqueued
via the /dev/tpm interface. Currently it is only available in tpmrm_priv.
Otherwise I would need to introduce yet another struct what would consist of
a ptr file_priv and a ptr to tpm_space and allocate it on every enqueue.
Much easier was to to it this way. The consolidation was only a side effect
of this so I think the description is correct.
Thanks,
-- 
Tadeusz

[PATCH v4 1/2] tpm: add ptr to the tpm_space struct to file_priv

[Tadeusz Struk]

Add a ptr to struct tpm_space to the file_priv to have an easy
access to it in the async job without the need to allocate memory.
This also allows to consolidate of the write operations for
the two interfaces.

Tested-by: Philip Tricca 
Signed-off-by: Tadeusz Struk 
---
 drivers/char/tpm/tpm-dev-common.c |8 +---
 drivers/char/tpm/tpm-dev.c|   10 ++
 drivers/char/tpm/tpm-dev.h|5 +++--
 drivers/char/tpm/tpmrm-dev.c  |   14 ++
 4 files changed, 12 insertions(+), 25 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c 
b/drivers/char/tpm/tpm-dev-common.c
index e4a04b2d3c32..f0c033b69b62 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -43,9 +43,11 @@ static void timeout_work(struct work_struct *work)
 }
 
 void tpm_common_open(struct file *file, struct tpm_chip *chip,
-struct file_priv *priv)
+struct file_priv *priv, struct tpm_space *space)
 {
priv->chip = chip;
+   priv->space = space;
+
mutex_init(&priv->buffer_mutex);
timer_setup(&priv->user_read_timer, user_reader_timeout, 0);
INIT_WORK(&priv->work, timeout_work);
@@ -79,7 +81,7 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
 }
 
 ssize_t tpm_common_write(struct file *file, const char __user *buf,
-size_t size, loff_t *off, struct tpm_space *space)
+size_t size, loff_t *off)
 {
struct file_priv *priv = file->private_data;
size_t in_size = size;
@@ -119,7 +121,7 @@ ssize_t tpm_common_write(struct file *file, const char 
__user *buf,
mutex_unlock(&priv->buffer_mutex);
return -EPIPE;
}
-   out_size = tpm_transmit(priv->chip, space, priv->data_buffer,
+   out_size = tpm_transmit(priv->chip, priv->space, priv->data_buffer,
sizeof(priv->data_buffer), 0);
 
tpm_put_ops(priv->chip);
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index ebd74ab5abef..98b9630c3a36 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -39,7 +39,7 @@ static int tpm_open(struct inode *inode, struct file *file)
if (priv == NULL)
goto out;
 
-   tpm_common_open(file, chip, priv);
+   tpm_common_open(file, chip, priv, NULL);
 
return 0;
 
@@ -48,12 +48,6 @@ static int tpm_open(struct inode *inode, struct file *file)
return -ENOMEM;
 }
 
-static ssize_t tpm_write(struct file *file, const char __user *buf,
-size_t size, loff_t *off)
-{
-   return tpm_common_write(file, buf, size, off, NULL);
-}
-
 /*
  * Called on file close
  */
@@ -73,6 +67,6 @@ const struct file_operations tpm_fops = {
.llseek = no_llseek,
.open = tpm_open,
.read = tpm_common_read,
-   .write = tpm_write,
+   .write = tpm_common_write,
.release = tpm_release,
 };
diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
index b24cfb4d3ee1..4048677bbd78 100644
--- a/drivers/char/tpm/tpm-dev.h
+++ b/drivers/char/tpm/tpm-dev.h
@@ -6,6 +6,7 @@
 
 struct file_priv {
struct tpm_chip *chip;
+   struct tpm_space *space;
 
/* Data passed to and from the tpm via the read/write calls */
size_t data_pending;
@@ -18,11 +19,11 @@ struct file_priv {
 };
 
 void tpm_common_open(struct file *file, struct tpm_chip *chip,
-struct file_priv *priv);
+struct file_priv *priv, struct tpm_space *space);
 ssize_t tpm_common_read(struct file *file, char __user *buf,
size_t size, loff_t *off);
 ssize_t tpm_common_write(struct file *file, const char __user *buf,
-size_t size, loff_t *off, struct tpm_space *space);
+size_t size, loff_t *off);
 void tpm_common_release(struct file *file, struct file_priv *priv);
 
 #endif
diff --git a/drivers/char/tpm/tpmrm-dev.c b/drivers/char/tpm/tpmrm-dev.c
index 1a0e97a5da5a..96006c6b9696 100644
--- a/drivers/char/tpm/tpmrm-dev.c
+++ b/drivers/char/tpm/tpmrm-dev.c
@@ -28,7 +28,7 @@ static int tpmrm_open(struct inode *inode, struct file *file)
return -ENOMEM;
}
 
-   tpm_common_open(file, chip, &priv->priv);
+   tpm_common_open(file, chip, &priv->priv, &priv->space);
 
return 0;
 }
@@ -45,21 +45,11 @@ static int tpmrm_release(struct inode *inode, struct file 
*file)
return 0;
 }
 
-static ssize_t tpmrm_write(struct file *file, const char __user *buf,
-  size_t size, loff_t *off)
-{
-   struct file_priv *fpriv = file->private_data;
-   struct tpmrm_priv *priv = container_of(fpriv, struct tpmrm_priv, priv);
-
-   return tpm_common_write(file, buf, size, off, &priv->space);
-}
-
 const struct file_operation

[PATCH v4 2/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

Currently the TPM driver only supports blocking calls, which doesn't allow
asynchronous IO operations to the TPM hardware.
This patch changes it and adds support for nonblocking write and a new poll
function to enable applications, which want to take advantage of this.

Tested-by: Philip Tricca 
Signed-off-by: Tadeusz Struk 
---
 drivers/char/tpm/tpm-dev-common.c |  149 -
 drivers/char/tpm/tpm-dev.c|   16 +++-
 drivers/char/tpm/tpm-dev.h|   17 +++-
 drivers/char/tpm/tpm-interface.c  |1 
 drivers/char/tpm/tpm.h|1 
 drivers/char/tpm/tpmrm-dev.c  |   19 +++--
 6 files changed, 150 insertions(+), 53 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c 
b/drivers/char/tpm/tpm-dev-common.c
index f0c033b69b62..f71d80b94451 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -17,11 +17,36 @@
  * License.
  *
  */
+#include 
 #include 
 #include 
+#include 
 #include "tpm.h"
 #include "tpm-dev.h"
 
+static struct workqueue_struct *tpm_dev_wq;
+static DEFINE_MUTEX(tpm_dev_wq_lock);
+
+static void tpm_async_work(struct work_struct *work)
+{
+   struct file_priv *priv =
+   container_of(work, struct file_priv, async_work);
+   ssize_t ret;
+
+   mutex_lock(&priv->buffer_mutex);
+   priv->command_enqueued = false;
+   ret = tpm_transmit(priv->chip, priv->space, priv->data_buffer,
+  sizeof(priv->data_buffer), 0);
+
+   tpm_put_ops(priv->chip);
+   if (ret > 0) {
+   priv->data_pending = ret;
+   mod_timer(&priv->user_read_timer, jiffies + (120 * HZ));
+   }
+   mutex_unlock(&priv->buffer_mutex);
+   wake_up_interruptible(&priv->async_wait);
+}
+
 static void user_reader_timeout(struct timer_list *t)
 {
struct file_priv *priv = from_timer(priv, t, user_read_timer);
@@ -29,30 +54,44 @@ static void user_reader_timeout(struct timer_list *t)
pr_warn("TPM user space timeout is deprecated (pid=%d)\n",
task_tgid_nr(current));
 
-   schedule_work(&priv->work);
+   schedule_work(&priv->timeout_work);
 }
 
-static void timeout_work(struct work_struct *work)
+static void tpm_timeout_work(struct work_struct *work)
 {
-   struct file_priv *priv = container_of(work, struct file_priv, work);
+   struct file_priv *priv = container_of(work, struct file_priv,
+ timeout_work);
 
mutex_lock(&priv->buffer_mutex);
priv->data_pending = 0;
memset(priv->data_buffer, 0, sizeof(priv->data_buffer));
mutex_unlock(&priv->buffer_mutex);
+   wake_up_interruptible(&priv->async_wait);
 }
 
-void tpm_common_open(struct file *file, struct tpm_chip *chip,
-struct file_priv *priv, struct tpm_space *space)
+int tpm_common_open(struct file *file, struct tpm_chip *chip,
+   struct file_priv *priv, struct tpm_space *space)
 {
priv->chip = chip;
priv->space = space;
 
mutex_init(&priv->buffer_mutex);
timer_setup(&priv->user_read_timer, user_reader_timeout, 0);
-   INIT_WORK(&priv->work, timeout_work);
-
+   INIT_WORK(&priv->timeout_work, tpm_timeout_work);
+   INIT_WORK(&priv->async_work, tpm_async_work);
+   init_waitqueue_head(&priv->async_wait);
file->private_data = priv;
+   mutex_lock(&tpm_dev_wq_lock);
+   if (!tpm_dev_wq) {
+   tpm_dev_wq = alloc_workqueue("tpm_dev_wq", WQ_MEM_RECLAIM, 0);
+   if (!tpm_dev_wq) {
+   mutex_unlock(&tpm_dev_wq_lock);
+   return -ENOMEM;
+   }
+   }
+
+   mutex_unlock(&tpm_dev_wq_lock);
+   return 0;
 }
 
 ssize_t tpm_common_read(struct file *file, char __user *buf,
@@ -63,15 +102,17 @@ ssize_t tpm_common_read(struct file *file, char __user 
*buf,
int rc;
 
del_singleshot_timer_sync(&priv->user_read_timer);
-   flush_work(&priv->work);
+   flush_work(&priv->timeout_work);
mutex_lock(&priv->buffer_mutex);
 
if (priv->data_pending) {
ret_size = min_t(ssize_t, size, priv->data_pending);
-   rc = copy_to_user(buf, priv->data_buffer, ret_size);
-   memset(priv->data_buffer, 0, priv->data_pending);
-   if (rc)
-   ret_size = -EFAULT;
+   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;
+   }
 
 

[PATCH v4 0/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

The TCG SAPI specification [1] defines a set of functions, which allow
applications to use the TPM device in either blocking or non-blocking fashion.
Each command defined by the specification has a corresponding
Tss2_Sys__Prepare() and Tss2_Sys__Complete() call, which
together with Tss2_Sys_ExecuteAsync() is designed to allow asynchronous
mode of operation. Currently the TPM driver supports only blocking calls,
which doesn't allow asynchronous IO operations.
This patch changes it and adds support for nonblocking write and a new poll
function to enable applications, which want to take advantage of this feature.
The new functionality can be tested using standard TPM tools implemented
in [2], together with modified TCTI from [3], and an example application
by Philip Tricca [4]. Here is a short description from Philip:

"The example application `glib-tss2-event` uses a glib main event loop
to create an RSA 2048 primary key in the TPM2 NULL hierarchy while
using a glib timer event to time the operation. A GSource object is
used to generate an event when the FD underlying the tss2 function
call has data ready. While the application waits for an event indicating
that the CreatePrimary operation is complete, it counts timer events
that occur every 100ms. Once the CreatePrimary operation completes the
number of timer events that occurred is used to make a rough calculation
of the elapsed time. This value is then printed to the console.
This takes ~300 lines of C code and requires no management or
synchronization of threads. The glib GMainContext is "just a poll()
loop" according to the glib documentation here:

https://developer.gnome.org/programming-guidelines/stable/main-contexts.html.en

and so supporting 'poll' is the easiest way to integrate with glib /
gtk+. This is true of any other event system that relies on 'poll'
instead of worker threads."

[1] 
https://trustedcomputinggroup.org/wp-content/uploads/TSS_SAPI_Version-1.1_Revision-22_review_030918.pdf
[2] https://github.com/tpm2-software/tpm2-tools
[3] https://github.com/tstruk/tpm2-tss/tree/async
[4] https://github.com/flihp/glib-tss2-async-example

---
Changes in v4:
- Changed the way buffer_mutex is handled in nonblocking mode so that
  it is not held when write() returns to user space.

Changes in v3:
- Fixed problem reported by 0-dey kbuild test robot around __exitcall.
  It complained because there is a module_exit() in another file already.
- Added info on example application from Philip

Changes in v2:
- Split the change into two separate patches. First patch adds a pointer
  to the space to the struct file_priv to have access to it from the async job.
  This is to avoid memory allocations on every write call. Now everything
  what's needed is in the file_priv struct.
- Renamed the 'work' member of the timer to avoid confusion.
  Now there are 'timeout_work' and 'async_work'.
- Removed the global wait queue and moved it to file_priv.
- Only creating the work queue when the first file is opened.

Tadeusz Struk (2):
  tpm: add ptr to the tpm_space struct to file_priv
  tpm: add support for nonblocking operation

 drivers/char/tpm/tpm-dev-common.c |  150 +++--
 drivers/char/tpm/tpm-dev.c|   22 +++--
 drivers/char/tpm/tpm-dev.h|   19 +++--
 drivers/char/tpm/tpm-interface.c  |1 
 drivers/char/tpm/tpm.h|1 
 drivers/char/tpm/tpmrm-dev.c  |   31 
 6 files changed, 152 insertions(+), 72 deletions(-)

--
TS

Re: [PATCH v3 RESEND 2/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

On 08/07/2018 11:20 AM, Jason Gunthorpe wrote:
> Doesn't lockdep complain when locks are left held after returning to
> user space? Even if it doesn't, that is a pretty ugly thing to do.

I didn't notice anything from lockdep during my testing,
but I will change it to release the lock before returning.
Will send v4 soon.
Thanks,
-- 
Tadeusz

Re: [PATCH v3 RESEND 2/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

On 08/06/2018 05:35 PM, James Bottomley wrote:
> On Mon, 2018-08-06 at 17:09 -0700, Tadeusz Struk wrote:
>> On 08/06/2018 04:05 PM, James Bottomley wrote:
>>> For an async interface, shouldn't I be able to queue an
>>> arbitrary number of commands without blocking?
>>
>> That was the approach in the v1 version of this patch, but
>> Jason requested this to be changed so that only one command
>> at a time can be processed.
> 
> He did?  I don't remember that.  I think he told you the TPM itself can
> only process one operation at once so you didn't need an elaborate
> allocation scheme.

Right, but the allocation was needed only if more than one command
would be queued at a given time.

> 
> But anyway, if you're happy to limit the interface to block after one
> command is issued, how is it useful as an asynchronous interface?  I
> thought the whole argument for the patch was to avoid the producer-
> consumer approach which is possible with the current interface and to
> use a fully event driven polling interface which can be implemented
> single threaded.  If you can block in submission, this latter isn't
> really possible because your interface isn't really asynchronous.

Well it is. This change makes the interface non-blocking and adds a poll
interface. Application can submit a command in a non-blocking way, go
do something else and get a notification via poll mechanism when the
response is ready to consume. We could implement it in a way that more
commands can be queued at a time, but in this case there would need to
be limit on how many commands can be en-queued. Allowing to send many
commands without any limit could be harmful. So what would it be? 10? 50?
And what would happen if an application sends 10 commands only to find
out the the first has failed? The drive doesn't know about that as it
only copies buffers back and forth. There will need to be an interface
for the application to rollback all the enqueued commands and stat over.
Also what would be the use case for this? TPM is not a crypto accelerator
where one submits a batch of buffers for encryption. Usually the sequence
of commands requires that subsequent command needs to refer the result
from the previous one. For example first command creates a key and the
second does something with it passing a handle to the key created in step
one. Do you have any particular scenario in mind for multiple commands
in-flight?

Thanks,
-- 
Tadeusz

Re: [PATCH v3 RESEND 2/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

On 08/06/2018 04:05 PM, James Bottomley wrote:
> For an async interface, shouldn't I be able to queue an
> arbitrary number of commands without blocking?

That was the approach in the v1 version of this patch, but
Jason requested this to be changed so that only one command
at a time can be processed.
Thanks,
-- 
Tadeusz

[PATCH v3 RESEND 1/2] tpm: add ptr to the tpm_space struct to file_priv

[Tadeusz Struk]

Add a ptr to struct tpm_space to the file_priv to have an easy
access to it in the async job without the need to allocate memory.
This also allows to consolidate of the write operations for
the two interfaces.

Tested-by: Philip Tricca 
Signed-off-by: Tadeusz Struk 
---
 drivers/char/tpm/tpm-dev-common.c |8 +---
 drivers/char/tpm/tpm-dev.c|   10 ++
 drivers/char/tpm/tpm-dev.h|5 +++--
 drivers/char/tpm/tpmrm-dev.c  |   14 ++
 4 files changed, 12 insertions(+), 25 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c 
b/drivers/char/tpm/tpm-dev-common.c
index e4a04b2d3c32..f0c033b69b62 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -43,9 +43,11 @@ static void timeout_work(struct work_struct *work)
 }
 
 void tpm_common_open(struct file *file, struct tpm_chip *chip,
-struct file_priv *priv)
+struct file_priv *priv, struct tpm_space *space)
 {
priv->chip = chip;
+   priv->space = space;
+
mutex_init(&priv->buffer_mutex);
timer_setup(&priv->user_read_timer, user_reader_timeout, 0);
INIT_WORK(&priv->work, timeout_work);
@@ -79,7 +81,7 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
 }
 
 ssize_t tpm_common_write(struct file *file, const char __user *buf,
-size_t size, loff_t *off, struct tpm_space *space)
+size_t size, loff_t *off)
 {
struct file_priv *priv = file->private_data;
size_t in_size = size;
@@ -119,7 +121,7 @@ ssize_t tpm_common_write(struct file *file, const char 
__user *buf,
mutex_unlock(&priv->buffer_mutex);
return -EPIPE;
}
-   out_size = tpm_transmit(priv->chip, space, priv->data_buffer,
+   out_size = tpm_transmit(priv->chip, priv->space, priv->data_buffer,
sizeof(priv->data_buffer), 0);
 
tpm_put_ops(priv->chip);
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index ebd74ab5abef..98b9630c3a36 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -39,7 +39,7 @@ static int tpm_open(struct inode *inode, struct file *file)
if (priv == NULL)
goto out;
 
-   tpm_common_open(file, chip, priv);
+   tpm_common_open(file, chip, priv, NULL);
 
return 0;
 
@@ -48,12 +48,6 @@ static int tpm_open(struct inode *inode, struct file *file)
return -ENOMEM;
 }
 
-static ssize_t tpm_write(struct file *file, const char __user *buf,
-size_t size, loff_t *off)
-{
-   return tpm_common_write(file, buf, size, off, NULL);
-}
-
 /*
  * Called on file close
  */
@@ -73,6 +67,6 @@ const struct file_operations tpm_fops = {
.llseek = no_llseek,
.open = tpm_open,
.read = tpm_common_read,
-   .write = tpm_write,
+   .write = tpm_common_write,
.release = tpm_release,
 };
diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
index b24cfb4d3ee1..4048677bbd78 100644
--- a/drivers/char/tpm/tpm-dev.h
+++ b/drivers/char/tpm/tpm-dev.h
@@ -6,6 +6,7 @@
 
 struct file_priv {
struct tpm_chip *chip;
+   struct tpm_space *space;
 
/* Data passed to and from the tpm via the read/write calls */
size_t data_pending;
@@ -18,11 +19,11 @@ struct file_priv {
 };
 
 void tpm_common_open(struct file *file, struct tpm_chip *chip,
-struct file_priv *priv);
+struct file_priv *priv, struct tpm_space *space);
 ssize_t tpm_common_read(struct file *file, char __user *buf,
size_t size, loff_t *off);
 ssize_t tpm_common_write(struct file *file, const char __user *buf,
-size_t size, loff_t *off, struct tpm_space *space);
+size_t size, loff_t *off);
 void tpm_common_release(struct file *file, struct file_priv *priv);
 
 #endif
diff --git a/drivers/char/tpm/tpmrm-dev.c b/drivers/char/tpm/tpmrm-dev.c
index 1a0e97a5da5a..96006c6b9696 100644
--- a/drivers/char/tpm/tpmrm-dev.c
+++ b/drivers/char/tpm/tpmrm-dev.c
@@ -28,7 +28,7 @@ static int tpmrm_open(struct inode *inode, struct file *file)
return -ENOMEM;
}
 
-   tpm_common_open(file, chip, &priv->priv);
+   tpm_common_open(file, chip, &priv->priv, &priv->space);
 
return 0;
 }
@@ -45,21 +45,11 @@ static int tpmrm_release(struct inode *inode, struct file 
*file)
return 0;
 }
 
-static ssize_t tpmrm_write(struct file *file, const char __user *buf,
-  size_t size, loff_t *off)
-{
-   struct file_priv *fpriv = file->private_data;
-   struct tpmrm_priv *priv = container_of(fpriv, struct tpmrm_priv, priv);
-
-   return tpm_common_write(file, buf, size, off, &priv->space);
-}
-
 const struct file_operation

[PATCH v3 RESEND 2/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

Currently the TPM driver only supports blocking calls, which doesn't allow
asynchronous IO operations to the TPM hardware.
This patch changes it and adds support for nonblocking write and a new poll
function to enable applications, which want to take advantage of this.

Tested-by: Philip Tricca 
Signed-off-by: Tadeusz Struk 
---
 drivers/char/tpm/tpm-dev-common.c |  145 +++--
 drivers/char/tpm/tpm-dev.c|   16 +++-
 drivers/char/tpm/tpm-dev.h|   16 +++-
 drivers/char/tpm/tpm-interface.c  |1 
 drivers/char/tpm/tpm.h|1 
 drivers/char/tpm/tpmrm-dev.c  |   19 -
 6 files changed, 145 insertions(+), 53 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c 
b/drivers/char/tpm/tpm-dev-common.c
index f0c033b69b62..3f2089f75c30 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -17,11 +17,34 @@
  * License.
  *
  */
+#include 
 #include 
 #include 
+#include 
 #include "tpm.h"
 #include "tpm-dev.h"
 
+static struct workqueue_struct *tpm_dev_wq;
+static DEFINE_MUTEX(tpm_dev_wq_lock);
+
+static void tpm_async_work(struct work_struct *work)
+{
+   struct file_priv *priv =
+   container_of(work, struct file_priv, async_work);
+   ssize_t ret;
+
+   ret = tpm_transmit(priv->chip, priv->space, priv->data_buffer,
+  sizeof(priv->data_buffer), 0);
+
+   tpm_put_ops(priv->chip);
+   if (ret > 0) {
+   priv->data_pending = ret;
+   mod_timer(&priv->user_read_timer, jiffies + (120 * HZ));
+   }
+   mutex_unlock(&priv->buffer_mutex);
+   wake_up_interruptible(&priv->async_wait);
+}
+
 static void user_reader_timeout(struct timer_list *t)
 {
struct file_priv *priv = from_timer(priv, t, user_read_timer);
@@ -29,30 +52,44 @@ static void user_reader_timeout(struct timer_list *t)
pr_warn("TPM user space timeout is deprecated (pid=%d)\n",
task_tgid_nr(current));
 
-   schedule_work(&priv->work);
+   schedule_work(&priv->timeout_work);
 }
 
-static void timeout_work(struct work_struct *work)
+static void tpm_timeout_work(struct work_struct *work)
 {
-   struct file_priv *priv = container_of(work, struct file_priv, work);
+   struct file_priv *priv = container_of(work, struct file_priv,
+ timeout_work);
 
mutex_lock(&priv->buffer_mutex);
priv->data_pending = 0;
memset(priv->data_buffer, 0, sizeof(priv->data_buffer));
mutex_unlock(&priv->buffer_mutex);
+   wake_up_interruptible(&priv->async_wait);
 }
 
-void tpm_common_open(struct file *file, struct tpm_chip *chip,
-struct file_priv *priv, struct tpm_space *space)
+int tpm_common_open(struct file *file, struct tpm_chip *chip,
+   struct file_priv *priv, struct tpm_space *space)
 {
priv->chip = chip;
priv->space = space;
 
mutex_init(&priv->buffer_mutex);
timer_setup(&priv->user_read_timer, user_reader_timeout, 0);
-   INIT_WORK(&priv->work, timeout_work);
-
+   INIT_WORK(&priv->timeout_work, tpm_timeout_work);
+   INIT_WORK(&priv->async_work, tpm_async_work);
+   init_waitqueue_head(&priv->async_wait);
file->private_data = priv;
+   mutex_lock(&tpm_dev_wq_lock);
+   if (!tpm_dev_wq) {
+   tpm_dev_wq = alloc_workqueue("tpm_dev_wq", WQ_MEM_RECLAIM, 0);
+   if (!tpm_dev_wq) {
+   mutex_unlock(&tpm_dev_wq_lock);
+   return -ENOMEM;
+   }
+   }
+
+   mutex_unlock(&tpm_dev_wq_lock);
+   return 0;
 }
 
 ssize_t tpm_common_read(struct file *file, char __user *buf,
@@ -63,15 +100,17 @@ ssize_t tpm_common_read(struct file *file, char __user 
*buf,
int rc;
 
del_singleshot_timer_sync(&priv->user_read_timer);
-   flush_work(&priv->work);
+   flush_work(&priv->timeout_work);
mutex_lock(&priv->buffer_mutex);
 
if (priv->data_pending) {
ret_size = min_t(ssize_t, size, priv->data_pending);
-   rc = copy_to_user(buf, priv->data_buffer, ret_size);
-   memset(priv->data_buffer, 0, priv->data_pending);
-   if (rc)
-   ret_size = -EFAULT;
+   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;
+   }
 
priv->data_pending = 0;
}
@@ -84,10 +123,9 @@ ssize_t tpm_common_writ

[PATCH v3 RESEND 0/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

The TCG SAPI specification [1] defines a set of functions, which allow
applications to use the TPM device in either blocking or non-blocking fashion.
Each command defined by the specification has a corresponding
Tss2_Sys__Prepare() and Tss2_Sys__Complete() call, which
together with Tss2_Sys_ExecuteAsync() is designed to allow asynchronous
mode of operation. Currently the TPM driver supports only blocking calls,
which doesn't allow asynchronous IO operations.
This patch changes it and adds support for nonblocking write and a new poll
function to enable applications, which want to take advantage of this feature.
The new functionality can be tested using standard TPM tools implemented
in [2], together with modified TCTI from [3], and an example application
by Philip Tricca [4]. Here is a short description from Philip:

"The example application `glib-tss2-event` uses a glib main event loop
to create an RSA 2048 primary key in the TPM2 NULL hierarchy while
using a glib timer event to time the operation. A GSource object is
used to generate an event when the FD underlying the tss2 function
call has data ready. While the application waits for an event indicating
that the CreatePrimary operation is complete, it counts timer events
that occur every 100ms. Once the CreatePrimary operation completes the
number of timer events that occurred is used to make a rough calculation
of the elapsed time. This value is then printed to the console.
This takes ~300 lines of C code and requires no management or
synchronization of threads. The glib GMainContext is "just a poll()
loop" according to the glib documentation here:

https://developer.gnome.org/programming-guidelines/stable/main-contexts.html.en

and so supporting 'poll' is the easiest way to integrate with glib /
gtk+. This is true of any other event system that relies on 'poll'
instead of worker threads."

[1] 
https://trustedcomputinggroup.org/wp-content/uploads/TSS_SAPI_Version-1.1_Revision-22_review_030918.pdf
[2] https://github.com/tpm2-software/tpm2-tools
[3] https://github.com/tstruk/tpm2-tss/tree/async
[4] https://github.com/flihp/glib-tss2-async-example

---
Changes in v3:
- Fixed problem reported by 0-dey kbuild test robot around __exitcall.
  It complained because there is a module_exit() in another file already.
- Added info on example application from Philip

Changes in v2:
- Split the change into two separate patches. First patch adds a pointer
  to the space to the struct file_priv to have access to it from the async job.
  This is to avoid memory allocations on every write call. Now everything
  what's needed is in the file_priv struct.
- Renamed the 'work' member of the timer to avoid confusion.
  Now there are 'timeout_work' and 'async_work'.
- Removed the global wait queue and moved it to file_priv.
- Only creating the work queue when the first file is opened.

Tadeusz Struk (2):
  tpm: add ptr to the tpm_space struct to file_priv
  tpm: add support for nonblocking operation

 drivers/char/tpm/tpm-dev-common.c |  150 +++--
 drivers/char/tpm/tpm-dev.c|   22 +++--
 drivers/char/tpm/tpm-dev.h|   19 +++--
 drivers/char/tpm/tpm-interface.c  |1 
 drivers/char/tpm/tpm.h|1 
 drivers/char/tpm/tpmrm-dev.c  |   31 
 6 files changed, 152 insertions(+), 72 deletions(-)

--
TS

Re: [PATCH] tpm: add support for partial reads

[Tadeusz Struk]

On 07/23/2018 03:08 PM, Jason Gunthorpe wrote:
> On Mon, Jul 23, 2018 at 03:00:20PM -0700, Tadeusz Struk wrote:
>> On 07/23/2018 02:56 PM, Jason Gunthorpe wrote:
>>> The proposed patch doesn't clear the data_pending if the entire buffer
>>> is not consumed, so of course it is ABI breaking, that really isn't OK.
>> The data_pending will be cleared by the timeout handler if the user doesn't
>> read the response fully before the timeout expires. The is the same situation
>> if the user would not read the response at all.
> That causes write() to fail with EBUSY
> 
> NAK from me on breaking the ABI like this

What if we introduce this new behavior only for the non-blocking mode
as James suggested? Or do you have some other suggestions?

Thanks,
-- 
Tadeusz

Re: [PATCH] tpm: add support for partial reads

[Tadeusz Struk]

On 07/23/2018 02:56 PM, Jason Gunthorpe wrote:
> The proposed patch doesn't clear the data_pending if the entire buffer
> is not consumed, so of course it is ABI breaking, that really isn't OK.

The data_pending will be cleared by the timeout handler if the user doesn't
read the response fully before the timeout expires. The is the same situation
if the user would not read the response at all.
Thanks,
-- 
Tadeusz

Re: [PATCH] tpm: add support for partial reads

[Tadeusz Struk]

On 07/23/2018 02:13 PM, James Bottomley wrote:
> The current patch does, you even provided a use case in your last email
>  (it's do command to get sizing followed by do command with correctly
> sized buffer). 

The example I provided was: #1 send a command, #2 read the response header
(10 bytes), get the actual response size from the header and then #3 read
the full response (response size - size of the header bytes).

> 
> However, if you tie it to O_NONBLOCK, it won't because no-one currently
> opens the TPM device non blocking so it's an ABI conformant
> discriminator of the uses.  Tying to O_NONBLOCK should be simple
> because it's in file->f_flags.

I think that it might be an option. Especially that I have this on top of
the async patch. Let's discuss this when Jarkko is back.

Thanks,
-- 
Tadeusz

Re: [PATCH] tpm: add support for partial reads

[Tadeusz Struk]

On 07/23/2018 01:19 PM, Jarkko Sakkinen wrote:
> In this case I do not have any major evidence of any major benefit *and*
> the change breaks the ABI.

As I said before - this does not break the ABI.
As for the benefits - it help user space in how they implement the receive
path. Application does not need to provide a 4K buffer for every read even
if the response is, for instance 8 bytes long.
Thanks,
-- 
Tadeusz

Re: [PATCH] tpm: add support for partial reads

[Tadeusz Struk]

On 07/19/2018 01:27 PM, James Bottomley wrote:
> On Thu, 2018-07-19 at 13:12 -0700, Tadeusz Struk wrote:
>> On 07/19/2018 12:52 PM, James Bottomley wrote:
>>> The ABI break is the error case as I outlined above.  We can't
>>> assume everyone uses the current interface without getting an error
>>> and one error and your hosed is a nasty failure case to change the
>>> interface to. 
>>
>> Well, if there is a broken application out there that doesn't work
>> today it will not work after this change neither.
> 
> It doesn't have to be broken ... it could be using EFAULT to probe the
> buffer size for instance.  That's the point of not breaking the ABI:
> you don't second guess what applications are doing.
> 

Looking at the existing implementation again:
https://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git/tree/drivers/char/tpm/tpm-dev-common.c?h=next-tpm#n56

EFAULT is returned only if the copy_to_user() fails.
So today, if an application wants read 1 byte of a response, and provides
1 byte buffer for it, then only 1 byte of the response will be copied,
no error code will be returned, and the rest of the response will be gone.
I don't really see how and why would anyone use EFAULT err to probe for
the buffer size. That would really be a broken application.

Thanks,
-- 
Tadeusz

Re: [PATCH] tpm: add support for partial reads

[Tadeusz Struk]

On 07/19/2018 12:52 PM, James Bottomley wrote:
> The ABI break is the error case as I outlined above.  We can't assume
> everyone uses the current interface without getting an error and one
> error and your hosed is a nasty failure case to change the interface
> to. 

Well, if there is a broken application out there that doesn't work today
it will not work after this change neither.

> Plus, if you assume everyone is passing 4k buffers, why would you
> even need the fragmentation case?

So that people don't need to do this anymore and we can run a
spec compliant TCTI on top of /dev/tpm.

> 
>>> It might be possible to layer the behaviour you want compatibly
>>> into the current device structure (say an ioctl to switch to the
>>> fragment behaviour) but I've got to ask why we'd go to the
>>> complexity without a use case?
>> New IOCTL would add extra complexity, which isn't necessary.
> So what's wrong with fragmenting in the layer above the device driver
> (in userspace) and not actually changing the kernel?

Because it is much easier to implement in the driver, and
we can run a spec compliant TCTI on top of /dev/tpm.

Thanks,
-- 
Tadeusz

Re: [PATCH] tpm: add support for partial reads

[Tadeusz Struk]

On 07/19/2018 11:47 AM, James Bottomley wrote:
> On Thu, 2018-07-19 at 10:54 -0700, Tadeusz Struk wrote:
>> On 07/19/2018 10:19 AM, James Bottomley wrote:
>>> That's just an implementation, though, what's the use case?
>>
>> Hi James,
>> The use case is described in the TCTI spec [1] in the
>> "3.2.5.2 receive" section.
> 
> Well, yes, but I think we've all agreed that the /dev/tpm and
> /dev/tpmrmX aren't TCTI interfaces, although you can layer TCTI on top
> of them, so why not simply do fragmentation on top if you need it?
> 
> The reason for not doing it in the interface is that it alters the ABI.
>  Before this patch we had a hard packet boundary: one packet per read,
> one per write and a -EFAULT if you fail to provide a correctly sized
> buffer.  Now if you provide a buffer too small but don't know about the
> fragmentation you're going to misprocess a packet (because you think
> you got a whole reply but you didn't) and then you get a -EBUSY on your
> next command which you don't know how to handle.  The only way out is
> to update the applications to handle the new behaviour, which is a no-
> no in Linux ABI terms.

Don't all the existing applications that read a response in one go
do a 4K read now? So nothing will change for them. They will work
exactly the same with this change as they do without it.
This doesn't break the ABI.

> 
> It might be possible to layer the behaviour you want compatibly into
> the current device structure (say an ioctl to switch to the fragment
> behaviour) but I've got to ask why we'd go to the complexity without a
> use case?

New IOCTL would add extra complexity, which isn't necessary.

Thanks,
-- 
Tadeusz

Re: [PATCH] tpm: add support for partial reads

[Tadeusz Struk]

On 07/19/2018 10:19 AM, James Bottomley wrote:
> That's just an implementation, though, what's the use case?

Hi James,
The use case is described in the TCTI spec [1] in the
"3.2.5.2 receive" section.

Thanks,
-- 
Tadeusz

[1] 
https://trustedcomputinggroup.org/wp-content/uploads/TSS_TCTI_v1.0_r12_PUBLIC_REVIEW.pdf

Re: [PATCH] tpm: add support for partial reads

[Tadeusz Struk]

On 07/19/2018 08:52 AM, 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 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.
> 
> The usecase is implemented in this TSS commit:
> https://github.com/tpm2-software/tpm2-tss/commit/ce982f67a67dc08e24683d30b05800648d8a264c
> 
> Signed-off-by: Tadeusz Struk 

This needs to go on top of this:
https://patchwork.kernel.org/patch/10460863/

Thanks,
-- 
Tadeusz

[PATCH] 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 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.

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

Signed-off-by: Tadeusz Struk 
---
 drivers/char/tpm/tpm-dev-common.c |   32 +++-
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c 
b/drivers/char/tpm/tpm-dev-common.c
index 3f2089f75c30..f5b614984dbc 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -99,20 +99,34 @@ 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) {
-   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;
+   ret_size = min_t(ssize_t, size + *off, priv->data_pending);
+   if (ret_size <= 0) {
+   ret_size = 0;
+   priv->data_pending = 0;
+   *off = 0;
+   goto out;
}
 
-   priv->data_pending = 0;
+   rc = copy_to_user(buf, priv->data_buffer + *off, ret_size);
+   if (rc) {
+   memset(priv->data_buffer, 0, priv->data_pending);
+   ret_size = -EFAULT;
+   priv->data_pending = 0;
+   *off = 0;
+   } else {
+   memset(priv->data_buffer + *off, 0, ret_size);
+   priv->data_pending -= ret_size;
+   *off += ret_size;
+   }
+   }
+out:
+   if (!priv->data_pending) {
+   del_singleshot_timer_sync(&priv->user_read_timer);
+   flush_work(&priv->timeout_work);
+   *off = 0;
}
 
mutex_unlock(&priv->buffer_mutex);

Re: [PATCH v3 0/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

On 06/21/2018 10:17 AM, Jarkko Sakkinen wrote:
> On Tue, Jun 19, 2018 at 05:45:35PM -0700, Tadeusz Struk wrote:
>> Hi Jarkko,
>> Thanks for reviewing the patch.
>> There are applications/frameworks where a worker thread is not an option.
>> Take for example the IoT use-cases and frameworks like IoT.js, or "Node.js 
>> for IoT".
>> They are all single threaded, event-driven frameworks, using non-blocking 
>> I/O as the base of their processing model.
>> Similarly embedded applications, which are basically just a single threaded 
>> event loop, quite often don't use threads because of resources constrains.
>>
>> If your concern is that user space will not adopt to this, I can say that 
>> TSS library [1] is currently blocked on this feature, and we can not enable 
>> some of the use-cases mentioned above because of this.
>>
>> Thanks,
>> Tadeusz
>>
>> [1] https://github.com/tpm2-software/tpm2-tss
> 
> I put this into "mathematical" terms. TPM is by nature is blocking. It
> does not scale this way so you are essentially just simulating
> non-blocking behaviour.
> 

That is correct, and this is exactly why we need this.
Thanks,
-- 
Tadeusz

Re: [PATCH v3 0/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

On 06/20/2018 10:26 PM, James Bottomley wrote:
>> Yes, it does polling:
>> http://docs.libuv.org/en/v1.x/design.html#the-i-o-loop
> But that's for networking.  You'll be talking to the TPM RM over the
> file descriptor so that follows the thread pool model in
> 
> http://docs.libuv.org/en/v1.x/design.html#file-i-o
> 
> This precisely describes the current file descriptor abstraction we'd
> use for the TPM.

That is for the file IO that doesn't support non-blocking, because there
is no need for it as the operations are "fast".
Operations on the TPM would fall under the io loop model.

> 
>> Regardless of how it actually might be used, I'm happy that we agree
>> on that this *is* the right thing to do.
> I didn't say that.  I think using a single worker thread queue is the
> correct abstraction for the TPM.  If there's a legacy use case for
> poll(), I don't see why not since the code seems to be fairly small and
> self contained, but I don't really see it as correct or necessary to do
> it that way.

This discussion starts to go around the circle. You don't agree, but you
also don't disagree? Is this what you are saying?
Thanks,
-- 
Tadeusz

Re: [PATCH v3 0/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

On 06/20/2018 04:59 PM, James Bottomley wrote:
> I'm slightly surprised by this statement.  I thought IoT Node.js
> runtimes (of which there are far too many, so I haven't looked at all
> of them) use libuv or one of the forks:
> 
> http://libuv.org/
> 
> As the basis for their I/O handling?  While libuv can do polling for
> event driven interfaces it also support the worker thread model just as
> easily:
> 
> http://docs.libuv.org/en/v1.x/threadpool.html

Yes, it does polling:
http://docs.libuv.org/en/v1.x/design.html#the-i-o-loop

> 
>> Similarly embedded applications, which are basically just a single
>> threaded event loop, quite often don't use threads because of
>> resources constrains.
> It's hard for me, as a kernel developer, to imagine any embedded
> scenario using the Linux kernel that would not allow threads unless the
> writers simply didn't bother with synchronization: The kernel schedules
> at the threads level and can't be configured not to use them plus
> threads are inherently more lightweight than processes so they're a
> natural fit for resource constrained scenarios.
> 
> That's still not to say we shouldn't do this, but I've got to say I
> think the only consumers would be old fashioned C code: the code we
> used to write before we had thread libraries that did use signals and
> poll() for a single threaded event driven monolith (think green
> threads), because all the new webby languages use threading either
> explicitly or at the core of their operation.

Regardless of how it actually might be used, I'm happy that we agree on
that this *is* the right thing to do.
Thanks,
Tadeusz

Re: [PATCH v3 0/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

On 06/19/2018 06:10 AM, Jarkko Sakkinen wrote:
> On Tue, Jun 12, 2018 at 10:58:26AM -0700, Tadeusz Struk wrote:
>> The TCG SAPI specification [1] defines a set of functions, which allows
>> applications to use the TPM device in either blocking or non-blocking 
>> fashion.
>> Each command defined by the specification has a corresponding
>> Tss2_Sys__Prepare() and Tss2_Sys__Complete() call, which
>> together with Tss2_Sys_ExecuteAsync() is designed to allow asynchronous
>> mode of operation. Currently the TPM driver supports only blocking calls,
>> which doesn't allow asynchronous IO operations.
>> This patch changes it and adds support for nonblocking write and a new poll
>> function to enable applications, which want to take advantage of this 
>> feature.
>> The new functionality can be tested using standard TPM tools implemented
>> in [2], together with modified TCTI from [3].
>>
>> [1] 
>> https://trustedcomputinggroup.org/wp-content/uploads/TSS_SAPI_Version-1.1_Revision-22_review_030918.pdf
>> [2] https://github.com/tpm2-software/tpm2-tools
>> [3] https://github.com/tstruk/tpm2-tss/tree/async
> 
> For me the value is still a bit questionable. The benchmark looks a bit
> flakky to give much figures how this would work with real world workloads.
> 
> I read James response and I also have to question why not just a worker
> thread in user space? TPM does only one command at a time anyways.
> 
> Cannot take this in before I know that user space will (1) adapt to
> this and (2) gain value compared to a worker thread.
> 
Hi Jarkko,
Thanks for reviewing the patch.
There are applications/frameworks where a worker thread is not an option.
Take for example the IoT use-cases and frameworks like IoT.js, or "Node.js for 
IoT".
They are all single threaded, event-driven frameworks, using non-blocking I/O 
as the base of their processing model.
Similarly embedded applications, which are basically just a single threaded 
event loop, quite often don't use threads because of resources constrains.

If your concern is that user space will not adopt to this, I can say that TSS 
library [1] is currently blocked on this feature, and we can not enable some of 
the use-cases mentioned above because of this.

Thanks,
Tadeusz

[1] https://github.com/tpm2-software/tpm2-tss

Re: [PATCH v3 2/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

On 06/13/2018 10:55 AM, J Freyensee wrote:
>> +    /*
>> + * If in nonblocking mode schedule an async job to send
>> + * the command return the size.
>> + * In case of error the err code will be returned in
>> + * the subsequent read call.
>> + */
>> +    if (file->f_flags & O_NONBLOCK) {
>> +    queue_work(tpm_dev_wq, &priv->async_work);
>> +    return size;
> 
> Apologies for the question, but should there be a mutex_unlock() here?  It's 
> about the only return statement I am seeing where I cannot tell if a 
> mutex_unlock() will be called before return or is needed before return.  The 
> rest of the code is pretty obvious the return statements are being 
> re-factored to an out: block where the mutex_unlock() will always be called 
> before returning.

Hi Jay,
We need to hold the lock until the whole command is sent and the device is 
ready for next one.
In case of the async job the mutex in unlocked in tpm_async_work() just after 
the tpm_transmit() returns.
Thanks for reviewing.
-- 
Tadeusz

[PATCH v3 0/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

The TCG SAPI specification [1] defines a set of functions, which allows
applications to use the TPM device in either blocking or non-blocking fashion.
Each command defined by the specification has a corresponding
Tss2_Sys__Prepare() and Tss2_Sys__Complete() call, which
together with Tss2_Sys_ExecuteAsync() is designed to allow asynchronous
mode of operation. Currently the TPM driver supports only blocking calls,
which doesn't allow asynchronous IO operations.
This patch changes it and adds support for nonblocking write and a new poll
function to enable applications, which want to take advantage of this feature.
The new functionality can be tested using standard TPM tools implemented
in [2], together with modified TCTI from [3].

[1] 
https://trustedcomputinggroup.org/wp-content/uploads/TSS_SAPI_Version-1.1_Revision-22_review_030918.pdf
[2] https://github.com/tpm2-software/tpm2-tools
[3] https://github.com/tstruk/tpm2-tss/tree/async

---
Changes in v3:
- Fixed problem reported by 0-dey kbuild test robot around __exitcall.
  It complained because there is a module_exit() in another file already.

Changes in v2:
- Split the change into two separate patches. First patch adds a pointer
  to the space to the struct file_priv to have access to it from the async job.
  This is to avoid memory allocations on every write call. Now everything
  what's needed is in the file_priv struct.
- Renamed the 'work' member of the timer to avoid confusion.
  Now there are 'timeout_work' and 'async_work'.
- Removed the global wait queue and moved it to file_priv.
- Only creating the work queue when the first file is opened.

Tadeusz Struk (2):
  tpm: add ptr to the tpm_space struct to file_priv
  tpm: add support for nonblocking operation

 drivers/char/tpm/tpm-dev-common.c |  150 +++--
 drivers/char/tpm/tpm-dev.c|   22 +++--
 drivers/char/tpm/tpm-dev.h|   19 +++--
 drivers/char/tpm/tpm-interface.c  |1 
 drivers/char/tpm/tpm.h|1 
 drivers/char/tpm/tpmrm-dev.c  |   31 
 6 files changed, 152 insertions(+), 72 deletions(-)

--
TS

[PATCH v3 1/2] tpm: add ptr to the tpm_space struct to file_priv

[Tadeusz Struk]

Add a ptr to struct tpm_space to the file_priv to have an easy
access to it in the async job without the need to allocate memory.
This also allows to consolidate of the write operations for
the two interfaces.

Signed-off-by: Tadeusz Struk 
---
 drivers/char/tpm/tpm-dev-common.c |8 +---
 drivers/char/tpm/tpm-dev.c|   10 ++
 drivers/char/tpm/tpm-dev.h|5 +++--
 drivers/char/tpm/tpmrm-dev.c  |   14 ++
 4 files changed, 12 insertions(+), 25 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c 
b/drivers/char/tpm/tpm-dev-common.c
index e4a04b2d3c32..f0c033b69b62 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -43,9 +43,11 @@ static void timeout_work(struct work_struct *work)
 }
 
 void tpm_common_open(struct file *file, struct tpm_chip *chip,
-struct file_priv *priv)
+struct file_priv *priv, struct tpm_space *space)
 {
priv->chip = chip;
+   priv->space = space;
+
mutex_init(&priv->buffer_mutex);
timer_setup(&priv->user_read_timer, user_reader_timeout, 0);
INIT_WORK(&priv->work, timeout_work);
@@ -79,7 +81,7 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
 }
 
 ssize_t tpm_common_write(struct file *file, const char __user *buf,
-size_t size, loff_t *off, struct tpm_space *space)
+size_t size, loff_t *off)
 {
struct file_priv *priv = file->private_data;
size_t in_size = size;
@@ -119,7 +121,7 @@ ssize_t tpm_common_write(struct file *file, const char 
__user *buf,
mutex_unlock(&priv->buffer_mutex);
return -EPIPE;
}
-   out_size = tpm_transmit(priv->chip, space, priv->data_buffer,
+   out_size = tpm_transmit(priv->chip, priv->space, priv->data_buffer,
sizeof(priv->data_buffer), 0);
 
tpm_put_ops(priv->chip);
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index ebd74ab5abef..98b9630c3a36 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -39,7 +39,7 @@ static int tpm_open(struct inode *inode, struct file *file)
if (priv == NULL)
goto out;
 
-   tpm_common_open(file, chip, priv);
+   tpm_common_open(file, chip, priv, NULL);
 
return 0;
 
@@ -48,12 +48,6 @@ static int tpm_open(struct inode *inode, struct file *file)
return -ENOMEM;
 }
 
-static ssize_t tpm_write(struct file *file, const char __user *buf,
-size_t size, loff_t *off)
-{
-   return tpm_common_write(file, buf, size, off, NULL);
-}
-
 /*
  * Called on file close
  */
@@ -73,6 +67,6 @@ const struct file_operations tpm_fops = {
.llseek = no_llseek,
.open = tpm_open,
.read = tpm_common_read,
-   .write = tpm_write,
+   .write = tpm_common_write,
.release = tpm_release,
 };
diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
index b24cfb4d3ee1..4048677bbd78 100644
--- a/drivers/char/tpm/tpm-dev.h
+++ b/drivers/char/tpm/tpm-dev.h
@@ -6,6 +6,7 @@
 
 struct file_priv {
struct tpm_chip *chip;
+   struct tpm_space *space;
 
/* Data passed to and from the tpm via the read/write calls */
size_t data_pending;
@@ -18,11 +19,11 @@ struct file_priv {
 };
 
 void tpm_common_open(struct file *file, struct tpm_chip *chip,
-struct file_priv *priv);
+struct file_priv *priv, struct tpm_space *space);
 ssize_t tpm_common_read(struct file *file, char __user *buf,
size_t size, loff_t *off);
 ssize_t tpm_common_write(struct file *file, const char __user *buf,
-size_t size, loff_t *off, struct tpm_space *space);
+size_t size, loff_t *off);
 void tpm_common_release(struct file *file, struct file_priv *priv);
 
 #endif
diff --git a/drivers/char/tpm/tpmrm-dev.c b/drivers/char/tpm/tpmrm-dev.c
index 1a0e97a5da5a..96006c6b9696 100644
--- a/drivers/char/tpm/tpmrm-dev.c
+++ b/drivers/char/tpm/tpmrm-dev.c
@@ -28,7 +28,7 @@ static int tpmrm_open(struct inode *inode, struct file *file)
return -ENOMEM;
}
 
-   tpm_common_open(file, chip, &priv->priv);
+   tpm_common_open(file, chip, &priv->priv, &priv->space);
 
return 0;
 }
@@ -45,21 +45,11 @@ static int tpmrm_release(struct inode *inode, struct file 
*file)
return 0;
 }
 
-static ssize_t tpmrm_write(struct file *file, const char __user *buf,
-  size_t size, loff_t *off)
-{
-   struct file_priv *fpriv = file->private_data;
-   struct tpmrm_priv *priv = container_of(fpriv, struct tpmrm_priv, priv);
-
-   return tpm_common_write(file, buf, size, off, &priv->space);
-}
-
 const struct file_operations tpmrm_fops = {
.ow

[PATCH v3 2/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

Currently the TPM driver only supports blocking calls, which doesn't allow
asynchronous IO operations to the TPM hardware.
This patch changes it and adds support for nonblocking write and a new poll
function to enable applications, which want to take advantage of this.

Signed-off-by: Tadeusz Struk 
---
 drivers/char/tpm/tpm-dev-common.c |  146 +++--
 drivers/char/tpm/tpm-dev.c|   16 +++-
 drivers/char/tpm/tpm-dev.h|   16 +++-
 drivers/char/tpm/tpm-interface.c  |1 
 drivers/char/tpm/tpm.h|1 
 drivers/char/tpm/tpmrm-dev.c  |   19 -
 6 files changed, 146 insertions(+), 53 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c 
b/drivers/char/tpm/tpm-dev-common.c
index f0c033b69b62..f57c000fd84a 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -17,11 +17,35 @@
  * License.
  *
  */
+#include 
+#include 
 #include 
 #include 
+#include 
 #include "tpm.h"
 #include "tpm-dev.h"
 
+static struct workqueue_struct *tpm_dev_wq;
+static DEFINE_MUTEX(tpm_dev_wq_lock);
+
+static void tpm_async_work(struct work_struct *work)
+{
+   struct file_priv *priv =
+   container_of(work, struct file_priv, async_work);
+   ssize_t ret;
+
+   ret = tpm_transmit(priv->chip, priv->space, priv->data_buffer,
+  sizeof(priv->data_buffer), 0);
+
+   tpm_put_ops(priv->chip);
+   if (ret > 0) {
+   priv->data_pending = ret;
+   mod_timer(&priv->user_read_timer, jiffies + (120 * HZ));
+   }
+   mutex_unlock(&priv->buffer_mutex);
+   wake_up_interruptible(&priv->async_wait);
+}
+
 static void user_reader_timeout(struct timer_list *t)
 {
struct file_priv *priv = from_timer(priv, t, user_read_timer);
@@ -29,30 +53,44 @@ static void user_reader_timeout(struct timer_list *t)
pr_warn("TPM user space timeout is deprecated (pid=%d)\n",
task_tgid_nr(current));
 
-   schedule_work(&priv->work);
+   schedule_work(&priv->timeout_work);
 }
 
-static void timeout_work(struct work_struct *work)
+static void tpm_timeout_work(struct work_struct *work)
 {
-   struct file_priv *priv = container_of(work, struct file_priv, work);
+   struct file_priv *priv = container_of(work, struct file_priv,
+ timeout_work);
 
mutex_lock(&priv->buffer_mutex);
priv->data_pending = 0;
memset(priv->data_buffer, 0, sizeof(priv->data_buffer));
mutex_unlock(&priv->buffer_mutex);
+   wake_up_interruptible(&priv->async_wait);
 }
 
-void tpm_common_open(struct file *file, struct tpm_chip *chip,
-struct file_priv *priv, struct tpm_space *space)
+int tpm_common_open(struct file *file, struct tpm_chip *chip,
+   struct file_priv *priv, struct tpm_space *space)
 {
priv->chip = chip;
priv->space = space;
 
mutex_init(&priv->buffer_mutex);
timer_setup(&priv->user_read_timer, user_reader_timeout, 0);
-   INIT_WORK(&priv->work, timeout_work);
-
+   INIT_WORK(&priv->timeout_work, tpm_timeout_work);
+   INIT_WORK(&priv->async_work, tpm_async_work);
+   init_waitqueue_head(&priv->async_wait);
file->private_data = priv;
+   mutex_lock(&tpm_dev_wq_lock);
+   if (!tpm_dev_wq) {
+   tpm_dev_wq = alloc_workqueue("tpm_dev_wq", WQ_MEM_RECLAIM, 0);
+   if (!tpm_dev_wq) {
+   mutex_unlock(&tpm_dev_wq_lock);
+   return -ENOMEM;
+   }
+   }
+
+   mutex_unlock(&tpm_dev_wq_lock);
+   return 0;
 }
 
 ssize_t tpm_common_read(struct file *file, char __user *buf,
@@ -63,15 +101,17 @@ ssize_t tpm_common_read(struct file *file, char __user 
*buf,
int rc;
 
del_singleshot_timer_sync(&priv->user_read_timer);
-   flush_work(&priv->work);
+   flush_work(&priv->timeout_work);
mutex_lock(&priv->buffer_mutex);
 
if (priv->data_pending) {
ret_size = min_t(ssize_t, size, priv->data_pending);
-   rc = copy_to_user(buf, priv->data_buffer, ret_size);
-   memset(priv->data_buffer, 0, priv->data_pending);
-   if (rc)
-   ret_size = -EFAULT;
+   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;
+   }
 
priv->data_pending = 0;
}
@@ -84,10 +124,9 @@ ssize_t tpm_common_write(struct file 

Re: [PATCH v2 2/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

On 06/11/2018 07:53 PM, kbuild test robot wrote:
> Hi Tadeusz,
> # save the attached .config to linux build tree
> make ARCH=i386 
> 
> All error/warnings (new ones prefixed by >>):
> 
>>> drivers/char//tpm/tpm-dev-common.c:223:1: warning: data definition has no 
>>> type or storage class
> __exitcall(tpm_dev_common_exit);
> ^~
>>> drivers/char//tpm/tpm-dev-common.c:223:1: error: type defaults to 'int' in 
>>> declaration of '__exitcall' [-Werror=implicit-int]
>>> drivers/char//tpm/tpm-dev-common.c:223:1: warning: parameter names (without 
>>> types) in function declaration
>drivers/char//tpm/tpm-dev-common.c:215:20: warning: 'tpm_dev_common_exit' 
> defined but not used [-Wunused-function]
> static void __exit tpm_dev_common_exit(void)
>^~~
>cc1: some warnings being treated as errors
> 
> vim +223 drivers/char//tpm/tpm-dev-common.c
> 
>222
>  > 223__exitcall(tpm_dev_common_exit);

It is complaining about __exitcall here, because there is a module_exit() call
in drivers/char/tpm/tpm-interface.c already and the two files are build into 
the same object.
It is strange that the problem only manifests itself when cross compiling with 
ARCH=i386. Native 64bit build works fine.
I will simply call the tpm_dev_common_exit() from the tpm_exit() and re-spin a 
v3.
Thanks again for the report.
-- 
Tadeusz

Re: [PATCH v2 2/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

On 06/11/2018 07:53 PM, kbuild test robot wrote:
> Hi Tadeusz,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on char-misc/char-misc-testing]
> [also build test ERROR on next-20180608]
> [cannot apply to v4.17]

Hi,
Thanks for the report. This patch should go on top of
https://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git/?h=next-tpm
I will check the config tomorrow.
Thanks,
-- 
Tadeusz

[PATCH v2 0/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

The TCG SAPI specification [1] defines a set of functions, which allows
applications to use the TPM device in either blocking or non-blocking fashion.
Each command defined by the specification has a corresponding
Tss2_Sys__Prepare() and Tss2_Sys__Complete() call, which
together with Tss2_Sys_ExecuteAsync() is designed to allow asynchronous
mode of operation. Currently the TPM driver supports only blocking calls,
which doesn't allow asynchronous IO operations.
This patch changes it and adds support for nonblocking write and a new poll
function to enable applications, which want to take advantage of this feature.
The new functionality can be tested using standard TPM tools implemented
in [2], together with modified TCTI from [3].

[1] 
https://trustedcomputinggroup.org/wp-content/uploads/TSS_SAPI_Version-1.1_Revision-22_review_030918.pdf
[2] https://github.com/tpm2-software/tpm2-tools
[3] https://github.com/tstruk/tpm2-tss/tree/async

---
Changes in v2:
- Split the change into two separate patches. First patch adds a pointer
  to the space to the struct file_priv to have access to it from the async job.
  This is to avoid memory allocations on every write call. Now everything
  what's needed is in the file_priv struct.
- Renamed the 'work' member of the timer to avoid confusion.
  Now there are 'timeout_work' and 'async_work'.
- Removed the global wait queue and moved it to file_priv.
- Only creating the work queue when the first file is opened.

Tadeusz Struk (2):
  tpm: add ptr to the tpm_space struct to file_priv
  tpm: add support for nonblocking operation

 drivers/char/tpm/tpm-dev-common.c |  152 -
 drivers/char/tpm/tpm-dev.c|   22 +++--
 drivers/char/tpm/tpm-dev.h|   19 +++--
 drivers/char/tpm/tpmrm-dev.c  |   31 
 4 files changed, 152 insertions(+), 72 deletions(-)

--
TS

[PATCH v2 1/2] tpm: add ptr to the tpm_space struct to file_priv

[Tadeusz Struk]

Add a ptr to struct tpm_space to the file_priv to have an easy
access to it in the async job without the need to allocate memory.
This also allows to consolidate of the write operations for
the two interfaces.

Signed-off-by: Tadeusz Struk 
---
 drivers/char/tpm/tpm-dev-common.c |8 +---
 drivers/char/tpm/tpm-dev.c|   10 ++
 drivers/char/tpm/tpm-dev.h|5 +++--
 drivers/char/tpm/tpmrm-dev.c  |   14 ++
 4 files changed, 12 insertions(+), 25 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c 
b/drivers/char/tpm/tpm-dev-common.c
index e4a04b2d3c32..f0c033b69b62 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -43,9 +43,11 @@ static void timeout_work(struct work_struct *work)
 }
 
 void tpm_common_open(struct file *file, struct tpm_chip *chip,
-struct file_priv *priv)
+struct file_priv *priv, struct tpm_space *space)
 {
priv->chip = chip;
+   priv->space = space;
+
mutex_init(&priv->buffer_mutex);
timer_setup(&priv->user_read_timer, user_reader_timeout, 0);
INIT_WORK(&priv->work, timeout_work);
@@ -79,7 +81,7 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
 }
 
 ssize_t tpm_common_write(struct file *file, const char __user *buf,
-size_t size, loff_t *off, struct tpm_space *space)
+size_t size, loff_t *off)
 {
struct file_priv *priv = file->private_data;
size_t in_size = size;
@@ -119,7 +121,7 @@ ssize_t tpm_common_write(struct file *file, const char 
__user *buf,
mutex_unlock(&priv->buffer_mutex);
return -EPIPE;
}
-   out_size = tpm_transmit(priv->chip, space, priv->data_buffer,
+   out_size = tpm_transmit(priv->chip, priv->space, priv->data_buffer,
sizeof(priv->data_buffer), 0);
 
tpm_put_ops(priv->chip);
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index ebd74ab5abef..98b9630c3a36 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -39,7 +39,7 @@ static int tpm_open(struct inode *inode, struct file *file)
if (priv == NULL)
goto out;
 
-   tpm_common_open(file, chip, priv);
+   tpm_common_open(file, chip, priv, NULL);
 
return 0;
 
@@ -48,12 +48,6 @@ static int tpm_open(struct inode *inode, struct file *file)
return -ENOMEM;
 }
 
-static ssize_t tpm_write(struct file *file, const char __user *buf,
-size_t size, loff_t *off)
-{
-   return tpm_common_write(file, buf, size, off, NULL);
-}
-
 /*
  * Called on file close
  */
@@ -73,6 +67,6 @@ const struct file_operations tpm_fops = {
.llseek = no_llseek,
.open = tpm_open,
.read = tpm_common_read,
-   .write = tpm_write,
+   .write = tpm_common_write,
.release = tpm_release,
 };
diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
index b24cfb4d3ee1..4048677bbd78 100644
--- a/drivers/char/tpm/tpm-dev.h
+++ b/drivers/char/tpm/tpm-dev.h
@@ -6,6 +6,7 @@
 
 struct file_priv {
struct tpm_chip *chip;
+   struct tpm_space *space;
 
/* Data passed to and from the tpm via the read/write calls */
size_t data_pending;
@@ -18,11 +19,11 @@ struct file_priv {
 };
 
 void tpm_common_open(struct file *file, struct tpm_chip *chip,
-struct file_priv *priv);
+struct file_priv *priv, struct tpm_space *space);
 ssize_t tpm_common_read(struct file *file, char __user *buf,
size_t size, loff_t *off);
 ssize_t tpm_common_write(struct file *file, const char __user *buf,
-size_t size, loff_t *off, struct tpm_space *space);
+size_t size, loff_t *off);
 void tpm_common_release(struct file *file, struct file_priv *priv);
 
 #endif
diff --git a/drivers/char/tpm/tpmrm-dev.c b/drivers/char/tpm/tpmrm-dev.c
index 1a0e97a5da5a..96006c6b9696 100644
--- a/drivers/char/tpm/tpmrm-dev.c
+++ b/drivers/char/tpm/tpmrm-dev.c
@@ -28,7 +28,7 @@ static int tpmrm_open(struct inode *inode, struct file *file)
return -ENOMEM;
}
 
-   tpm_common_open(file, chip, &priv->priv);
+   tpm_common_open(file, chip, &priv->priv, &priv->space);
 
return 0;
 }
@@ -45,21 +45,11 @@ static int tpmrm_release(struct inode *inode, struct file 
*file)
return 0;
 }
 
-static ssize_t tpmrm_write(struct file *file, const char __user *buf,
-  size_t size, loff_t *off)
-{
-   struct file_priv *fpriv = file->private_data;
-   struct tpmrm_priv *priv = container_of(fpriv, struct tpmrm_priv, priv);
-
-   return tpm_common_write(file, buf, size, off, &priv->space);
-}
-
 const struct file_operations tpmrm_fops = {
.ow

[PATCH v2 2/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

Currently the TPM driver only supports blocking calls, which doesn't allow
asynchronous IO operations to the TPM hardware.
This patch changes it and adds support for nonblocking write and a new poll
function to enable applications, which want to take advantage of this.

Signed-off-by: Tadeusz Struk 
---
 drivers/char/tpm/tpm-dev-common.c |  148 -
 drivers/char/tpm/tpm-dev.c|   16 +++-
 drivers/char/tpm/tpm-dev.h|   16 +++-
 drivers/char/tpm/tpmrm-dev.c  |   19 -
 4 files changed, 146 insertions(+), 53 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c 
b/drivers/char/tpm/tpm-dev-common.c
index f0c033b69b62..97625639e20f 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -17,11 +17,35 @@
  * License.
  *
  */
+#include 
+#include 
 #include 
 #include 
+#include 
 #include "tpm.h"
 #include "tpm-dev.h"
 
+static struct workqueue_struct *tpm_dev_wq;
+static DEFINE_MUTEX(tpm_dev_wq_lock);
+
+static void tpm_async_work(struct work_struct *work)
+{
+   struct file_priv *priv =
+   container_of(work, struct file_priv, async_work);
+   ssize_t ret;
+
+   ret = tpm_transmit(priv->chip, priv->space, priv->data_buffer,
+  sizeof(priv->data_buffer), 0);
+
+   tpm_put_ops(priv->chip);
+   if (ret > 0) {
+   priv->data_pending = ret;
+   mod_timer(&priv->user_read_timer, jiffies + (120 * HZ));
+   }
+   mutex_unlock(&priv->buffer_mutex);
+   wake_up_interruptible(&priv->async_wait);
+}
+
 static void user_reader_timeout(struct timer_list *t)
 {
struct file_priv *priv = from_timer(priv, t, user_read_timer);
@@ -29,30 +53,44 @@ static void user_reader_timeout(struct timer_list *t)
pr_warn("TPM user space timeout is deprecated (pid=%d)\n",
task_tgid_nr(current));
 
-   schedule_work(&priv->work);
+   schedule_work(&priv->timeout_work);
 }
 
-static void timeout_work(struct work_struct *work)
+static void tpm_timeout_work(struct work_struct *work)
 {
-   struct file_priv *priv = container_of(work, struct file_priv, work);
+   struct file_priv *priv = container_of(work, struct file_priv,
+ timeout_work);
 
mutex_lock(&priv->buffer_mutex);
priv->data_pending = 0;
memset(priv->data_buffer, 0, sizeof(priv->data_buffer));
mutex_unlock(&priv->buffer_mutex);
+   wake_up_interruptible(&priv->async_wait);
 }
 
-void tpm_common_open(struct file *file, struct tpm_chip *chip,
-struct file_priv *priv, struct tpm_space *space)
+int tpm_common_open(struct file *file, struct tpm_chip *chip,
+   struct file_priv *priv, struct tpm_space *space)
 {
priv->chip = chip;
priv->space = space;
 
mutex_init(&priv->buffer_mutex);
timer_setup(&priv->user_read_timer, user_reader_timeout, 0);
-   INIT_WORK(&priv->work, timeout_work);
-
+   INIT_WORK(&priv->timeout_work, tpm_timeout_work);
+   INIT_WORK(&priv->async_work, tpm_async_work);
+   init_waitqueue_head(&priv->async_wait);
file->private_data = priv;
+   mutex_lock(&tpm_dev_wq_lock);
+   if (!tpm_dev_wq) {
+   tpm_dev_wq = alloc_workqueue("tpm_dev_wq", WQ_MEM_RECLAIM, 0);
+   if (!tpm_dev_wq) {
+   mutex_unlock(&tpm_dev_wq_lock);
+   return -ENOMEM;
+   }
+   }
+
+   mutex_unlock(&tpm_dev_wq_lock);
+   return 0;
 }
 
 ssize_t tpm_common_read(struct file *file, char __user *buf,
@@ -63,15 +101,17 @@ ssize_t tpm_common_read(struct file *file, char __user 
*buf,
int rc;
 
del_singleshot_timer_sync(&priv->user_read_timer);
-   flush_work(&priv->work);
+   flush_work(&priv->timeout_work);
mutex_lock(&priv->buffer_mutex);
 
if (priv->data_pending) {
ret_size = min_t(ssize_t, size, priv->data_pending);
-   rc = copy_to_user(buf, priv->data_buffer, ret_size);
-   memset(priv->data_buffer, 0, priv->data_pending);
-   if (rc)
-   ret_size = -EFAULT;
+   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;
+   }
 
priv->data_pending = 0;
}
@@ -84,10 +124,9 @@ ssize_t tpm_common_write(struct file *file, const char 
__user *buf,
 size_t size, loff_t *off)
 {
 

Re: [PATCH] tpm: fix race condition in tpm_common_write()

[Tadeusz Struk]

On 05/23/2018 06:23 AM, Jarkko Sakkinen wrote:
> Ouch o_O Do you have a fixes tag for this one?
> 

This one is quite tricky.
The original bug was introduced by abce9ac292e13 (tpm: Propagate error from 
tpm_transmit to fix a timeout hang)
and the code back then was in drivers/char/tpm/tpm-interface.c file

Then there were two other commits that moved the code around:
afdba32e2a9ea (tpm: Pull everything related to /dev/tpmX into tpm-dev.c)
which moved it from drivers/char/tpm/tpm-interface.c into 
drivers/char/tpm/tpm-dev.c

and last one, ecb38e2f521b0 (tpm: split out tpm-dev.c into tpm-dev.c and 
tpm-common-dev.c)
which moved it from drivers/char/tpm/tpm-dev.c into tpm-common-dev.c

I have no idea how to tag it. Maybe we can use:
Fixes: ecb38e2f521b0 ("tpm: split out tpm-dev.c into tpm-dev.c and 
tpm-common-dev.c")

And then it probably needs to be back ported manually all the way back to 
abce9ac292e13.

Thanks,
-- 
Tadeusz

Re: Problem with RSA test from testmgr

[Tadeusz Struk]

On 03/01/2017 10:21 PM, Corentin Labbe wrote:
> I am finishing a patch that made testmgr test both (padded and unpadded).

Even if you patch the test vectors there is no guarantee that a user
of the API will always have the plain text padded.
It can be anything between 1 and the key size.
This needs to be the driver who adds padding if needed.
See how other implementations handle it.
Thanks,
-- 
Tadeusz

Re: Problem with RSA test from testmgr

[Tadeusz Struk]

Hi Stephan,
On 03/01/2017 10:08 PM, Stephan Müller wrote:
>>  memset(ptextp, 0, 256);
>>  memcpy(ptextp + 64 - 8, ptext_ex, plen);
> I actually have tested that and it did not return the data the kernel 
> implementation would return

It did for me:
Result 64 plen=8
63 1c cd 7b e1 7e e4 de c9 a8 89 a1 74 cb 3c 63 7d 24 ec 83 c3 15 e4 7f 73 05 
34 d1 ec 22 bb 8a 5e 32 39 6d c1 1d 7d 50 3b 9f 7a ad f0 2e 25 53 9f 6e bd 4c 
55 84 0c 9b cf 1a 4b 51 1e 9e 0c 06

Are you sure you are compering this with the fist test vector?
http://lxr.free-electrons.com/source/crypto/testmgr.h#L183

Thanks,
-- 
Tadeusz

Re: Problem with RSA test from testmgr

[Tadeusz Struk]

Hi Corentin,
On 03/01/2017 04:04 AM, Corentin Labbe wrote:
>> I would think the issue is that the OpenSSL BIGNUM lib has some issues: when 
>> calculating m^e mod n, m has to be equal to the key size. The kernel's MPI 
>> code handles the case where m is smaller than the key size.
>>
>> Note, in your code below, ptext is the 8 bytes from ptext_ex plus trailing 
>> zeroes whereas the kernel uses just the 8 bytes.
>>
>> It seems that your implementation has the same issue.
>>
>> What about the following test: change vector->m to be 64 bytes (i.e. 
>> RSA_size(key) in size in testmgr.h and check the output of crypto/rsa.c, 
>> openssl's output with the app below and your RSA hardware.
> I got the following:
> 
> [1.086228] alg: akcipher: encrypt test failed. Invalid output
> [1.092196] : 6e 7c 8a 75 e7 30 80 d1 5e ab 9b db a2 cf ed db
> [1.098882] 0010: c9 b2 db 43 bd 9a b9 75 27 f3 73 d9 73 b7 81 8c
> [1.105524] 0020: 49 e8 45 fc 43 44 f5 6d f0 f7 b8 f2 ae 6b ae 49
> [1.112090] 0030: 1b 8e 50 c6 88 4e 99 09 78 14 f2 5d 99 c3 7f f9
> [1.118747] alg: akcipher: test 1 failed for rsa-generic, err=-22
> (Exactly the output of my hardare and openssl test)
> 
> So the problem is just that my hardware does not handle non-padded data.

The difference between openssl's RSA_private_decrypt() and the akcipher api
is that openssl only takes only one size, flen, for both src and dst buffers,
so in your test app you need to do something like this:

memset(ptextp, 0, 256);
memcpy(ptextp + 64 - 8, ptext_ex, plen);

key = RSA_new();

key->n = BN_bin2bn(n, sizeof(n)-1, key->n);
key->e = BN_bin2bn(e, sizeof(e)-1, key->e);

num = RSA_public_encrypt(RSA_size(key), ptextp, ctext, key, 
RSA_NO_PADDING);

The akcipher API has separate sizes for both the src and dst. It is the length 
of the
scatterlist in the akcipher_request. If a HW can't handle different buffers 
lengths
then its driver needs to add the padding internally.

Thanks,
-- 
Tadeusz

[PATCH RFC 2/2] IB/hfi1: Fix port ordering issue in a multiport device

[Tadeusz Struk]

Some hardware has multiple HFIs within the same ASIC, each one on a
sepatate bus number. In some devices the numbers labeled on the
faceplate of the device don't match the PCI bus order, and the result
is that the devices (ports) are probed in the opposite order of their
port numbers. The result is IB device unit numbers are in reverse order
from the faceplate numbering. This leads to confusion, and errors.
Use EPROBE_DEFER error code to enforce correct port order.

Reviewed-by: Ira Weiny 
Signed-off-by: Tadeusz Struk 
---
 drivers/infiniband/hw/hfi1/chip.c |   95 -
 drivers/infiniband/hw/hfi1/chip.h |2 -
 drivers/infiniband/hw/hfi1/init.c |1 
 3 files changed, 94 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/chip.c 
b/drivers/infiniband/hw/hfi1/chip.c
index ef72bc2..45beeae 100644
--- a/drivers/infiniband/hw/hfi1/chip.c
+++ b/drivers/infiniband/hw/hfi1/chip.c
@@ -14365,6 +14365,79 @@ static int check_int_registers(struct hfi1_devdata *dd)
return -EINVAL;
 }
 
+struct deferred_dev {
+   struct list_head list;
+   u64 guid;
+   u32 bdf;
+   bool deferred;
+};
+
+static LIST_HEAD(device_list);
+static DEFINE_MUTEX(device_list_lock); /* protects device_list */
+#define HFI_ASIC_GUID(guid) ((guid) & ~(1ULL << GUID_HFI_INDEX_SHIFT))
+
+static int hfi1_need_defer(struct hfi1_devdata *dd)
+{
+   struct deferred_dev *def_dev = NULL;
+   struct pci_dev *pdev = dd->pcidev;
+   static bool deferred;
+   static u64 deferred_guid;
+   bool found_deferred = false;
+
+   mutex_lock(&device_list_lock);
+   /* Try to find an re-probed device first based on the BDF address */
+   list_for_each_entry(def_dev, &device_list, list) {
+   if (def_dev->bdf == (pdev->bus->number << 8 | pdev->devfn)) {
+   found_deferred = true;
+   break;
+   }
+   }
+
+   /* If not found then allocate one */
+   if (!found_deferred) {
+   def_dev = kzalloc(sizeof(*def_dev), GFP_KERNEL);
+
+   if (!def_dev) {
+   mutex_unlock(&device_list_lock);
+   return -ENOMEM;
+   }
+
+   def_dev->guid = dd->base_guid;
+   def_dev->bdf = pdev->bus->number << 8 | pdev->devfn;
+   def_dev->deferred = false;
+   list_add_tail(&def_dev->list, &device_list);
+   }
+
+   /*
+* If device has been already deferred we need to wait for the other
+* port and defer other devices until we get to the other port or
+* back the same device
+*/
+   if (deferred && (HFI_ASIC_GUID(deferred_guid) != dd->base_guid) &&
+   !def_dev->deferred) {
+   def_dev->deferred = true;
+   mutex_unlock(&device_list_lock);
+   return -EPROBE_DEFER;
+   } else if (!deferred && ((dd->base_guid >> GUID_HFI_INDEX_SHIFT) & 1)) {
+   /*
+* If no device is currently deferred check by guid if we need
+* to defer this one and if so store the deferred guid to find
+* the sister port by the guid
+*/
+   deferred = true;
+   def_dev->deferred = true;
+   deferred_guid = dd->base_guid;
+   mutex_unlock(&device_list_lock);
+   return -EPROBE_DEFER;
+   }
+
+   deferred = false;
+   def_dev->deferred = false;
+   mutex_unlock(&device_list_lock);
+
+   return 0;
+}
+
 /**
  * Allocate and initialize the device structure for the hfi.
  * @dev: the pci_dev for hfi1_ib device
@@ -14456,6 +14529,13 @@ struct hfi1_devdata *hfi1_init_dd(struct pci_dev *pdev,
if (ret < 0)
goto bail_free;
 
+   /* needs to be done before we look for the peer device */
+   read_guid(dd);
+
+   ret = hfi1_need_defer(dd);
+   if (ret)
+   goto bail_cleanup;
+
/* verify that reads actually work, save revision for reset check */
dd->revision = read_csr(dd, CCE_REVISION);
if (dd->revision == ~(u64)0) {
@@ -14539,9 +14619,6 @@ struct hfi1_devdata *hfi1_init_dd(struct pci_dev *pdev,
else if (dd->rcv_intr_timeout_csr == 0 && rcv_intr_timeout)
dd->rcv_intr_timeout_csr = 1;
 
-   /* needs to be done before we look for the peer device */
-   read_guid(dd);
-
/* set up shared ASIC data with peer device */
ret = init_asic_data(dd);
if (ret)
@@ -14702,6 +14779,18 @@ struct hfi1_devdata *hfi1_init_dd(struct pci_dev *pdev,
return dd;
 }
 
+void hfi1_device_list_cleanup(void)
+{
+   struct deferred_dev *dev, *tmp;
+
+   mutex_lock(&device_list_lock);
+   list_f

[PATCH RFC 1/2] driver core: allow EPROBE_DEFER after boot

[Tadeusz Struk]

Currently EPROBE_DEFER error code is only honored by the core during
boot time, where the deferred probes are triggered by the late_initcall.
After boot, if a driver returns EPROBE_DEFER for whatever reason, during
manual insmod, it is not handled, as there is nothing to trigger the
driver_deferred_probe_trigger() function.
This change allow drivers to be re-probed after boot.
The deferred_trigger_count counter is not needed as the
driver_deferred_probe_trigger() is safe to call multiple times.
There is also a warning added, which warns if drivers would try to abuse
EPROBE_DEFER causing the core to busy loop.

Reviewed-by: Ira Weiny 
Signed-off-by: Tadeusz Struk 
---
 drivers/base/dd.c  |   26 +-
 drivers/base/driver.c  |2 +-
 include/linux/device.h |2 ++
 3 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index a1fbf55..dfee412 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -51,7 +51,6 @@
 static DEFINE_MUTEX(deferred_probe_mutex);
 static LIST_HEAD(deferred_probe_pending_list);
 static LIST_HEAD(deferred_probe_active_list);
-static atomic_t deferred_trigger_count = ATOMIC_INIT(0);
 
 /*
  * In some cases, like suspend to RAM or hibernation, It might be reasonable
@@ -142,17 +141,6 @@ static bool driver_deferred_probe_enable = false;
  * This functions moves all devices from the pending list to the active
  * list and schedules the deferred probe workqueue to process them.  It
  * should be called anytime a driver is successfully bound to a device.
- *
- * Note, there is a race condition in multi-threaded probe. In the case where
- * more than one device is probing at the same time, it is possible for one
- * probe to complete successfully while another is about to defer. If the 
second
- * depends on the first, then it will get put on the pending list after the
- * trigger event has already occurred and will be stuck there.
- *
- * The atomic 'deferred_trigger_count' is used to determine if a successful
- * trigger has occurred in the midst of probing a driver. If the trigger count
- * changes in the midst of a probe, then deferred processing should be 
triggered
- * again.
  */
 static void driver_deferred_probe_trigger(void)
 {
@@ -165,7 +153,6 @@ static void driver_deferred_probe_trigger(void)
 * into the active list so they can be retried by the workqueue
 */
mutex_lock(&deferred_probe_mutex);
-   atomic_inc(&deferred_trigger_count);
list_splice_tail_init(&deferred_probe_pending_list,
  &deferred_probe_active_list);
mutex_unlock(&deferred_probe_mutex);
@@ -324,7 +311,6 @@ static DECLARE_WAIT_QUEUE_HEAD(probe_waitqueue);
 static int really_probe(struct device *dev, struct device_driver *drv)
 {
int ret = -EPROBE_DEFER;
-   int local_trigger_count = atomic_read(&deferred_trigger_count);
bool test_remove = IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE) &&
   !drv->suppress_bind_attrs;
 
@@ -384,6 +370,8 @@ static int really_probe(struct device *dev, struct 
device_driver *drv)
ret = drv->probe(dev);
if (ret)
goto probe_failed;
+   else
+   atomic_set(&drv->deferred_probe_ctr, 0);
}
 
if (test_remove) {
@@ -435,9 +423,13 @@ static int really_probe(struct device *dev, struct 
device_driver *drv)
/* Driver requested deferred probing */
dev_dbg(dev, "Driver %s requests probe deferral\n", drv->name);
driver_deferred_probe_add(dev);
-   /* Did a trigger occur while probing? Need to re-trigger if yes 
*/
-   if (local_trigger_count != atomic_read(&deferred_trigger_count))
-   driver_deferred_probe_trigger();
+   /* Need to re-trigger, but prevent busy looping */
+   if (atomic_add_return(1, &drv->deferred_probe_ctr) % 10 == 0) {
+   dev_warn(dev, "Driver %s loops on probe deferred\n",
+drv->name);
+   msleep(1000);
+   }
+   driver_deferred_probe_trigger();
break;
case -ENODEV:
case -ENXIO:
diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index 4eabfe2..afbd84de 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -174,7 +174,7 @@ int driver_register(struct device_driver *drv)
return ret;
}
kobject_uevent(&drv->p->kobj, KOBJ_ADD);
-
+   atomic_set(&drv->deferred_probe_ctr, 0);
return ret;
 }
 EXPORT_SYMBOL_GPL(driver_register);
diff --git a/include/linux/device.h b/include/linux/device.h
index 491b4c0c..2992fde 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -2

Re: [PATCH v8 6/6] crypto: AF_ALG - add support for key_id

[Tadeusz Struk]

On 07/08/2016 09:38 AM, Mat Martineau wrote:
> Are the inputs and outputs defined for ALG_OP_VERIFY in SET_KEY mode
> going to work for hardware keys (like TPM) in SET_KEY_ID mode? That's
> needed if the verify SET_KEY_ID mode is to be added later.

Yes, we will just need to change the verify_signature() in public_key.c
to be consistent with the rest of handlers. What we need really is the
src (encrypted input), key (or key id), and an output buffer where we
can copy the result to.
Thanks,
-- 
TS

Re: [PATCH v8 6/6] crypto: AF_ALG - add support for key_id

[Tadeusz Struk]

Hi Mat,
On 07/06/2016 12:38 PM, Mat Martineau wrote:
>> So it looks like the only thing that we need to return to the user in
>> this case is the return code. Do you agree?
> 
> The way verify_signature is implemented today, the only output is the
> return code. For verify, maybe no read is required (just sendmsg() and
> check the return code).
> 
> But this isn't the extent of the problem: verify_signature needs both
> the signature to be verified and the expected hash as inputs. How is the
> expected hash provided? Would you include it as a cmsg header?
> ALG_OP_VERIFY should have consistent inputs and outputs whether the key
> was set with ALG_SET_KEY_ID or ALG_SET_KEY.

The signature of verify_signature() is quite different from the other
new public key handlers, i.e. create_signature(), encrypt_blob(), and
decrypt_blob(). For verify_signature() we need the following parameters:
encrypted src, hash function to use, expected digest.
The expected digest could be optional if we would modify the
verify_signature() to return the decrypted buffer.
I think the best solution for now would be to just return -ENOPROTOOPT
for verify_signature in SET_KEY_ID mode.
All the four operations will be supported in the SET_KEY mode and
all but verify_signature() will be supported in the SET_KEY_ID mode.
This can added later if we will find a way to pass all parameters in a
consistent way. What do you think? If you are ok with that I will send a
new version soon.
Thanks,
-- 
TS

Re: [PATCH v8 6/6] crypto: AF_ALG - add support for key_id

[Tadeusz Struk]

Hi Mat,
On 06/29/2016 11:43 AM, Mat Martineau wrote:
>> +ret = verify_signature(key, &sig);
>> +if (!ret) {
>> +req->dst_len = sizeof(digest);
> 
> I think you fixed the BUG_ON() problem but there's still an issue with
> the handling of the digest. Check the use of sig->digest in
> public_key_verify_signature(), it's an input not an output. Right now it
> looks like 20 uninitialized bytes are compared with the computed digest
> within verify_signature, and then the unintialized bytes are copied to
> req->dst here.
> 
> With some modifications to public_key_verify_signature you could get the
> digest you need, but I'm not sure if verification with a hardware key
> (like a key in a TPM) can or can not provide the digest needed. Maybe
> this is why the verify_signature hook in struct asymmetric_key_subtype
> is optional.
> 
>> +scatterwalk_map_and_copy(digest, req->dst, 0, req->dst_len, 1);
>> +} 

So it looks like the only thing that we need to return to the user in
this case is the return code. Do you agree?
Thanks,
-- 
TS

[PATCH v8 2/6] crypto: AF_ALG -- add setpubkey setsockopt call

[Tadeusz Struk]

From: Stephan Mueller 

For supporting asymmetric ciphers, user space must be able to set the
public key. The patch adds a new setsockopt call for setting the public
key.

Signed-off-by: Stephan Mueller 
Signed-off-by: Tadeusz Struk 
---
 crypto/af_alg.c |   18 +-
 include/crypto/if_alg.h |1 +
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index f5e18c2..24dc082 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -202,13 +202,17 @@ unlock:
 }
 
 static int alg_setkey(struct sock *sk, char __user *ukey,
- unsigned int keylen)
+ unsigned int keylen,
+ int (*setkey)(void *private, const u8 *key,
+   unsigned int keylen))
 {
struct alg_sock *ask = alg_sk(sk);
-   const struct af_alg_type *type = ask->type;
u8 *key;
int err;
 
+   if (!setkey)
+   return -ENOPROTOOPT;
+
key = sock_kmalloc(sk, keylen, GFP_KERNEL);
if (!key)
return -ENOMEM;
@@ -217,7 +221,7 @@ static int alg_setkey(struct sock *sk, char __user *ukey,
if (copy_from_user(key, ukey, keylen))
goto out;
 
-   err = type->setkey(ask->private, key, keylen);
+   err = setkey(ask->private, key, keylen);
 
 out:
sock_kzfree_s(sk, key, keylen);
@@ -247,10 +251,14 @@ static int alg_setsockopt(struct socket *sock, int level, 
int optname,
case ALG_SET_KEY:
if (sock->state == SS_CONNECTED)
goto unlock;
-   if (!type->setkey)
+
+   err = alg_setkey(sk, optval, optlen, type->setkey);
+   break;
+   case ALG_SET_PUBKEY:
+   if (sock->state == SS_CONNECTED)
goto unlock;
 
-   err = alg_setkey(sk, optval, optlen);
+   err = alg_setkey(sk, optval, optlen, type->setpubkey);
break;
case ALG_SET_AEAD_AUTHSIZE:
if (sock->state == SS_CONNECTED)
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index a2bfd78..6c3e6e7 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -52,6 +52,7 @@ struct af_alg_type {
void *(*bind)(const char *name, u32 type, u32 mask);
void (*release)(void *private);
int (*setkey)(void *private, const u8 *key, unsigned int keylen);
+   int (*setpubkey)(void *private, const u8 *key, unsigned int keylen);
int (*accept)(void *private, struct sock *sk);
int (*accept_nokey)(void *private, struct sock *sk);
int (*setauthsize)(void *private, unsigned int authsize);

[PATCH v8 4/6] crypto: algif_akcipher - enable compilation

[Tadeusz Struk]

From: Stephan Mueller 

Add the Makefile and Kconfig updates to allow algif_akcipher to be
compiled.

Signed-off-by: Stephan Mueller 
Signed-off-by: Tadeusz Struk 
---
 crypto/Kconfig  |9 +
 crypto/Makefile |1 +
 2 files changed, 10 insertions(+)

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 1d33beb..3c6113e 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -1627,6 +1627,15 @@ config CRYPTO_USER_API_AEAD
  This option enables the user-spaces interface for AEAD
  cipher algorithms.
 
+config CRYPTO_USER_API_AKCIPHER
+   tristate "User-space interface for asymmetric key cipher algorithms"
+   depends on NET
+   select CRYPTO_AKCIPHER2
+   select CRYPTO_USER_API
+   help
+ This option enables the user-spaces interface for asymmetric
+ key cipher algorithms.
+
 config CRYPTO_HASH_INFO
bool
 
diff --git a/crypto/Makefile b/crypto/Makefile
index 4f4ef7e..c51ac16 100644
--- a/crypto/Makefile
+++ b/crypto/Makefile
@@ -121,6 +121,7 @@ obj-$(CONFIG_CRYPTO_USER_API_HASH) += algif_hash.o
 obj-$(CONFIG_CRYPTO_USER_API_SKCIPHER) += algif_skcipher.o
 obj-$(CONFIG_CRYPTO_USER_API_RNG) += algif_rng.o
 obj-$(CONFIG_CRYPTO_USER_API_AEAD) += algif_aead.o
+obj-$(CONFIG_CRYPTO_USER_API_AKCIPHER) += algif_akcipher.o
 
 #
 # generic algorithms and the async_tx api

[PATCH v8 6/6] crypto: AF_ALG - add support for key_id

[Tadeusz Struk]

This patch adds support for asymmetric key type to AF_ALG.
It will work as follows: A new PF_ALG socket options are
added on top of existing ALG_SET_KEY and ALG_SET_PUBKEY, namely
ALG_SET_KEY_ID and ALG_SET_PUBKEY_ID for setting public and
private keys respectively. When these new options will be used
the user, instead of providing the key material, will provide a
key id and the key itself will be obtained from kernel keyring
subsystem. The user will use the standard tools (keyctl tool
or the keyctl syscall) for key instantiation and to obtain the
key id. The key id can also be obtained by reading the
/proc/keys file.

When a key corresponding to the given keyid is found, it is stored
in the socket context and subsequent crypto operation invoked by the
user will use the new asymmetric accessor functions instead of akcipher
api. The asymmetric subtype can internally use akcipher api or
invoke operations defined by a given subtype, depending on the
key type.

Signed-off-by: Tadeusz Struk 
---
 crypto/af_alg.c |   10 ++
 crypto/algif_akcipher.c |  212 ++-
 include/crypto/if_alg.h |1 
 include/uapi/linux/if_alg.h |2 
 4 files changed, 220 insertions(+), 5 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 24dc082..59c8244 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -260,6 +260,16 @@ static int alg_setsockopt(struct socket *sock, int level, 
int optname,
 
err = alg_setkey(sk, optval, optlen, type->setpubkey);
break;
+
+   case ALG_SET_KEY_ID:
+   case ALG_SET_PUBKEY_ID:
+   /* ALG_SET_KEY_ID is only for akcipher */
+   if (!strcmp(type->name, "akcipher") ||
+   sock->state == SS_CONNECTED)
+   goto unlock;
+
+   err = alg_setkey(sk, optval, optlen, type->setkeyid);
+   break;
case ALG_SET_AEAD_AUTHSIZE:
if (sock->state == SS_CONNECTED)
goto unlock;
diff --git a/crypto/algif_akcipher.c b/crypto/algif_akcipher.c
index 2b8d37e..106f715 100644
--- a/crypto/algif_akcipher.c
+++ b/crypto/algif_akcipher.c
@@ -14,6 +14,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 #include 
 #include 
 #include 
@@ -29,6 +31,7 @@ struct akcipher_sg_list {
 
 struct akcipher_tfm {
struct crypto_akcipher *akcipher;
+   char keyid[12];
bool has_key;
 };
 
@@ -37,6 +40,7 @@ struct akcipher_ctx {
struct af_alg_sgl rsgl[ALG_MAX_PAGES];
 
struct af_alg_completion completion;
+   struct key *key;
 
unsigned long used;
 
@@ -317,6 +321,158 @@ unlock:
return err ? err : size;
 }
 
+static int asym_key_encrypt(const struct key *key, struct akcipher_request 
*req)
+{
+   struct kernel_pkey_params params = {0};
+   char *src = NULL, *dst = NULL, *in, *out;
+   int ret;
+
+   if (!sg_is_last(req->src)) {
+   src = kmalloc(req->src_len, GFP_KERNEL);
+   if (!src)
+   return -ENOMEM;
+   scatterwalk_map_and_copy(src, req->src, 0, req->src_len, 0);
+   in = src;
+   } else {
+   in = sg_virt(req->src);
+   }
+   if (!sg_is_last(req->dst)) {
+   dst = kmalloc(req->dst_len, GFP_KERNEL);
+   if (!dst) {
+   kfree(src);
+   return -ENOMEM;
+   }
+   out = dst;
+   } else {
+   out = sg_virt(req->dst);
+   }
+   params.key = (struct key *)key;
+   params.in_len = req->src_len;
+   params.out_len = req->dst_len;
+   ret = encrypt_blob(¶ms, in, out);
+   if (ret)
+   goto free;
+
+   if (dst)
+   scatterwalk_map_and_copy(dst, req->dst, 0, req->dst_len, 1);
+free:
+   kfree(src);
+   kfree(dst);
+   return ret;
+}
+
+static int asym_key_decrypt(const struct key *key, struct akcipher_request 
*req)
+{
+   struct kernel_pkey_params params = {0};
+   char *src = NULL, *dst = NULL, *in, *out;
+   int ret;
+
+   if (!sg_is_last(req->src)) {
+   src = kmalloc(req->src_len, GFP_KERNEL);
+   if (!src)
+   return -ENOMEM;
+   scatterwalk_map_and_copy(src, req->src, 0, req->src_len, 0);
+   in = src;
+   } else {
+   in = sg_virt(req->src);
+   }
+   if (!sg_is_last(req->dst)) {
+   dst = kmalloc(req->dst_len, GFP_KERNEL);
+   if (!dst) {
+   kfree(src);
+   return -ENOMEM;
+   }
+   out = dst;
+   } else {
+   out = sg_virt(req->dst);
+   }
+   params.key = (struct key *)key;
+   params.in_len = req->src_len;
+   params.out_len = req->dst_len;
+  

[PATCH v8 3/6] crypto: AF_ALG -- add asymmetric cipher interface

[Tadeusz Struk]

From: Stephan Mueller 

This patch adds the user space interface for asymmetric ciphers. The
interface allows the use of sendmsg as well as vmsplice to provide data.

This version has been rebased on top of 4.7 and a few chackpatch issues
have been fixed. This version also removes the constrain on the output
buffer size.

Signed-off-by: Stephan Mueller 
Signed-off-by: Tadeusz Struk 
---
 crypto/algif_akcipher.c |  531 +++
 1 file changed, 531 insertions(+)
 create mode 100644 crypto/algif_akcipher.c

diff --git a/crypto/algif_akcipher.c b/crypto/algif_akcipher.c
new file mode 100644
index 000..8dd6354
--- /dev/null
+++ b/crypto/algif_akcipher.c
@@ -0,0 +1,531 @@
+/*
+ * algif_akcipher: User-space interface for asymmetric cipher algorithms
+ *
+ * Copyright (C) 2015, Stephan Mueller 
+ *
+ * This file provides the user-space API for asymmetric ciphers.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+struct akcipher_sg_list {
+   unsigned int cur;
+   struct scatterlist sg[ALG_MAX_PAGES];
+};
+
+struct akcipher_ctx {
+   struct akcipher_sg_list tsgl;
+   struct af_alg_sgl rsgl[ALG_MAX_PAGES];
+
+   struct af_alg_completion completion;
+
+   unsigned long used;
+
+   unsigned int len;
+   bool more;
+   bool merge;
+   int op;
+
+   struct akcipher_request req;
+};
+
+static inline int akcipher_sndbuf(struct sock *sk)
+{
+   struct alg_sock *ask = alg_sk(sk);
+   struct akcipher_ctx *ctx = ask->private;
+
+   return max_t(int, max_t(int, sk->sk_sndbuf & PAGE_MASK, PAGE_SIZE) -
+ ctx->used, 0);
+}
+
+static inline bool akcipher_writable(struct sock *sk)
+{
+   return akcipher_sndbuf(sk) >= PAGE_SIZE;
+}
+
+static void akcipher_put_sgl(struct sock *sk)
+{
+   struct alg_sock *ask = alg_sk(sk);
+   struct akcipher_ctx *ctx = ask->private;
+   struct akcipher_sg_list *sgl = &ctx->tsgl;
+   struct scatterlist *sg = sgl->sg;
+   unsigned int i;
+
+   for (i = 0; i < sgl->cur; i++) {
+   if (!sg_page(sg + i))
+   continue;
+
+   put_page(sg_page(sg + i));
+   sg_assign_page(sg + i, NULL);
+   }
+   sg_init_table(sg, ALG_MAX_PAGES);
+   sgl->cur = 0;
+   ctx->used = 0;
+   ctx->more = 0;
+   ctx->merge = 0;
+}
+
+static void akcipher_wmem_wakeup(struct sock *sk)
+{
+   struct socket_wq *wq;
+
+   if (!akcipher_writable(sk))
+   return;
+
+   rcu_read_lock();
+   wq = rcu_dereference(sk->sk_wq);
+   if (wq_has_sleeper(&wq->wait))
+   wake_up_interruptible_sync_poll(&wq->wait, POLLIN |
+  POLLRDNORM |
+  POLLRDBAND);
+   sk_wake_async(sk, SOCK_WAKE_WAITD, POLL_IN);
+   rcu_read_unlock();
+}
+
+static int akcipher_wait_for_data(struct sock *sk, unsigned int flags)
+{
+   struct alg_sock *ask = alg_sk(sk);
+   struct akcipher_ctx *ctx = ask->private;
+   long timeout;
+   DEFINE_WAIT(wait);
+   int err = -ERESTARTSYS;
+
+   if (flags & MSG_DONTWAIT)
+   return -EAGAIN;
+
+   set_bit(SOCKWQ_ASYNC_WAITDATA, &sk->sk_socket->flags);
+
+   for (;;) {
+   if (signal_pending(current))
+   break;
+   prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);
+   timeout = MAX_SCHEDULE_TIMEOUT;
+   if (sk_wait_event(sk, &timeout, !ctx->more)) {
+   err = 0;
+   break;
+   }
+   }
+   finish_wait(sk_sleep(sk), &wait);
+
+   clear_bit(SOCKWQ_ASYNC_WAITDATA, &sk->sk_socket->flags);
+
+   return err;
+}
+
+static void akcipher_data_wakeup(struct sock *sk)
+{
+   struct alg_sock *ask = alg_sk(sk);
+   struct akcipher_ctx *ctx = ask->private;
+   struct socket_wq *wq;
+
+   if (ctx->more)
+   return;
+   if (!ctx->used)
+   return;
+
+   rcu_read_lock();
+   wq = rcu_dereference(sk->sk_wq);
+   if (wq_has_sleeper(&wq->wait))
+   wake_up_interruptible_sync_poll(&wq->wait, POLLOUT |
+  POLLRDNORM |
+  POLLRDBAND);
+   sk_wake_async(sk, SOCK_WAKE_SPACE, POLL_OUT);
+   rcu_read_unlock();
+}
+
+static int akcipher_sendmsg(str

[PATCH v8 1/6] crypto: AF_ALG -- add sign/verify API

[Tadeusz Struk]

From: Stephan Mueller 

Add the flags for handling signature generation and signature
verification.

Also, the patch adds the interface for setting a public key.

Signed-off-by: Stephan Mueller 
Signed-off-by: Tadeusz Struk 
---
 include/uapi/linux/if_alg.h |3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
index f2acd2f..02e6162 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -34,9 +34,12 @@ struct af_alg_iv {
 #define ALG_SET_OP 3
 #define ALG_SET_AEAD_ASSOCLEN  4
 #define ALG_SET_AEAD_AUTHSIZE  5
+#define ALG_SET_PUBKEY 6
 
 /* Operations */
 #define ALG_OP_DECRYPT 0
 #define ALG_OP_ENCRYPT 1
+#define ALG_OP_SIGN2
+#define ALG_OP_VERIFY  3
 
 #endif /* _LINUX_IF_ALG_H */

[PATCH v8 0/6] crypto: algif - add akcipher

[Tadeusz Struk]

First four patches are a resend algif_akcipher from
Stephan Mueller, with minor changes after rebase on top of 4.7-rc1.

The next three patches add support for keys stored in system
keyring subsystem.

First patch adds algif_akcipher nokey hadlers.

Second patch adds generic sign, verify, encrypt, decrypt accessors
functions to the asymmetric key type. These will be defined by
asymmetric subtypes, similarly to how public_key currently defines
the verify_signature function.

Third patch adds support for ALG_SET_KEY_ID and ALG_SET_PUBKEY_ID
commands to AF_ALG and setkeyid operation to the af_alg_type struct.
If the keyid is used then the afalg layer acquires the key for the
keyring subsystem and uses the new asymmetric accessor functions
instead of akcipher api. The asymmetric subtypes can use akcipher
api internally.

Patches are generate against:
http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-next

v8 hanges:
- copy the result to user for verify after the signature_verify
  operation. Before only the return code was checked, but not the
  actual data. Reported by Mat Martineau
- remove the constrain on the output buffer size as requested by
  Mat Martineau
- ifx uninitialize variable issue, reported by Mat Martineau

v7 changes:
- update to reflect changes in kernel_pkey_params struct

v6 changes:
- rabased on top of
http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-asym-keyctl

v5 changes:
- drop public key changes and use new version provided by David

v4 changes:
- don't use internal public_key struct in af_alg.
- add generic accessor functions to asymmetric key type, which take
  the generic struct key type and resolve the specific subtype internally

v3 changes:
- include Stephan's patches (rebased on 4.6-rc1)
- add algif_akcipher nokey hadlers
- add public_key info struct to public_key and helper query functions
- add a check if a key is a software accessible key on af_alg, and
  return -ENOKEY if it isn't

v2 changes:
- pass the original skcipher request in ablkcipher.base.data instead of
  casting it back from the ablkcipher request.
- rename _req to base_req
- dropped 3/3

---

Stephan Mueller (4):
  crypto: AF_ALG -- add sign/verify API
  crypto: AF_ALG -- add setpubkey setsockopt call
  crypto: AF_ALG -- add asymmetric cipher interface
  crypto: algif_akcipher - enable compilation

Tadeusz Struk (2):
  crypto: algif_akcipher - add ops_nokey
  crypto: AF_ALG - add support for key_id


 crypto/Kconfig  |9 
 crypto/Makefile |1 
 crypto/af_alg.c |   28 +
 crypto/algif_akcipher.c |  878 +++
 include/crypto/if_alg.h |2 
 include/uapi/linux/if_alg.h |5 
 6 files changed, 918 insertions(+), 5 deletions(-)
 create mode 100644 crypto/algif_akcipher.c

--
TS

[PATCH v8 5/6] crypto: algif_akcipher - add ops_nokey

[Tadeusz Struk]

Similar to algif_skcipher and algif_hash, algif_akcipher needs
to prevent user space from using the interface in an improper way.
This patch adds nokey ops handlers, which do just that.

Signed-off-by: Tadeusz Struk 
---
 crypto/algif_akcipher.c |  159 +--
 1 file changed, 152 insertions(+), 7 deletions(-)

diff --git a/crypto/algif_akcipher.c b/crypto/algif_akcipher.c
index 8dd6354..2b8d37e 100644
--- a/crypto/algif_akcipher.c
+++ b/crypto/algif_akcipher.c
@@ -27,6 +27,11 @@ struct akcipher_sg_list {
struct scatterlist sg[ALG_MAX_PAGES];
 };
 
+struct akcipher_tfm {
+   struct crypto_akcipher *akcipher;
+   bool has_key;
+};
+
 struct akcipher_ctx {
struct akcipher_sg_list tsgl;
struct af_alg_sgl rsgl[ALG_MAX_PAGES];
@@ -439,25 +444,151 @@ static struct proto_ops algif_akcipher_ops = {
.poll   =   akcipher_poll,
 };
 
+static int akcipher_check_key(struct socket *sock)
+{
+   int err = 0;
+   struct sock *psk;
+   struct alg_sock *pask;
+   struct akcipher_tfm *tfm;
+   struct sock *sk = sock->sk;
+   struct alg_sock *ask = alg_sk(sk);
+
+   lock_sock(sk);
+   if (ask->refcnt)
+   goto unlock_child;
+
+   psk = ask->parent;
+   pask = alg_sk(ask->parent);
+   tfm = pask->private;
+
+   err = -ENOKEY;
+   lock_sock_nested(psk, SINGLE_DEPTH_NESTING);
+   if (!tfm->has_key)
+   goto unlock;
+
+   if (!pask->refcnt++)
+   sock_hold(psk);
+
+   ask->refcnt = 1;
+   sock_put(psk);
+
+   err = 0;
+
+unlock:
+   release_sock(psk);
+unlock_child:
+   release_sock(sk);
+
+   return err;
+}
+
+static int akcipher_sendmsg_nokey(struct socket *sock, struct msghdr *msg,
+ size_t size)
+{
+   int err;
+
+   err = akcipher_check_key(sock);
+   if (err)
+   return err;
+
+   return akcipher_sendmsg(sock, msg, size);
+}
+
+static ssize_t akcipher_sendpage_nokey(struct socket *sock, struct page *page,
+  int offset, size_t size, int flags)
+{
+   int err;
+
+   err = akcipher_check_key(sock);
+   if (err)
+   return err;
+
+   return akcipher_sendpage(sock, page, offset, size, flags);
+}
+
+static int akcipher_recvmsg_nokey(struct socket *sock, struct msghdr *msg,
+ size_t ignored, int flags)
+{
+   int err;
+
+   err = akcipher_check_key(sock);
+   if (err)
+   return err;
+
+   return akcipher_recvmsg(sock, msg, ignored, flags);
+}
+
+static struct proto_ops algif_akcipher_ops_nokey = {
+   .family =   PF_ALG,
+
+   .connect=   sock_no_connect,
+   .socketpair =   sock_no_socketpair,
+   .getname=   sock_no_getname,
+   .ioctl  =   sock_no_ioctl,
+   .listen =   sock_no_listen,
+   .shutdown   =   sock_no_shutdown,
+   .getsockopt =   sock_no_getsockopt,
+   .mmap   =   sock_no_mmap,
+   .bind   =   sock_no_bind,
+   .accept =   sock_no_accept,
+   .setsockopt =   sock_no_setsockopt,
+
+   .release=   af_alg_release,
+   .sendmsg=   akcipher_sendmsg_nokey,
+   .sendpage   =   akcipher_sendpage_nokey,
+   .recvmsg=   akcipher_recvmsg_nokey,
+   .poll   =   akcipher_poll,
+};
+
 static void *akcipher_bind(const char *name, u32 type, u32 mask)
 {
-   return crypto_alloc_akcipher(name, type, mask);
+   struct akcipher_tfm *tfm;
+   struct crypto_akcipher *akcipher;
+
+   tfm = kzalloc(sizeof(*tfm), GFP_KERNEL);
+   if (!tfm)
+   return ERR_PTR(-ENOMEM);
+
+   akcipher = crypto_alloc_akcipher(name, type, mask);
+   if (IS_ERR(akcipher)) {
+   kfree(tfm);
+   return ERR_CAST(akcipher);
+   }
+
+   tfm->akcipher = akcipher;
+   return tfm;
 }
 
 static void akcipher_release(void *private)
 {
-   crypto_free_akcipher(private);
+   struct akcipher_tfm *tfm = private;
+   struct crypto_akcipher *akcipher = tfm->akcipher;
+
+   crypto_free_akcipher(akcipher);
+   kfree(tfm);
 }
 
 static int akcipher_setprivkey(void *private, const u8 *key,
   unsigned int keylen)
 {
-   return crypto_akcipher_set_priv_key(private, key, keylen);
+   struct akcipher_tfm *tfm = private;
+   struct crypto_akcipher *akcipher = tfm->akcipher;
+   int err;
+
+   err = crypto_akcipher_set_priv_key(akcipher, key, keylen);
+   tfm->has_key = !err;
+   return err;
 }
 
 static int akcipher_setpubkey(void *private, const u8 *key, unsigned int 
keylen)
 {
-   return crypto_akcipher_set_pub_key(private, key, keylen);
+ 

Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

[Tadeusz Struk]

On 06/09/2016 11:36 AM, Stephan Mueller wrote:
> Am Donnerstag, 9. Juni 2016, 11:27:13 schrieb Mat Martineau:
> 
> Hi Mat, Tadeusz,
> 
>> On Thu, 9 Jun 2016, Stephan Mueller wrote:
>>> Am Donnerstag, 9. Juni 2016, 11:18:04 schrieb Mat Martineau:
>>>
>>> Hi Mat,
>>>
> Or is your concern that the user space interface restricts things too
> much
> and thus prevents a valid use case?

 The latter - my primary concern is the constraint this places on
 userspace
 by forcing larger buffer sizes than might be necessary for the operation.
 struct akcipher_request has separate members for src_len and dst_len, and
 dst_len is documented as needing "to be at least as big as the expected
 result depending on the operation". Not the maximum result, the expected
 result. It's also documented that the cipher will generate an error if
 dst_len is insufficient and update the value with the required size.

 I'm updating some userspace TLS code that worked with an earlier,
 unmerged
 patch set for AF_ALG akcipher (from last year). The read calls with
 shorter buffers were the main porting problem.
>>>
>>> I see -- are you proposing to drop that check entirely?
>>
>> Yes.
> 
> Ok, after checking the code again, I think that dropping that sanity check 
> should be ok given that this length is part of the akcipher API.
> 
> Tadeusz, as you are currently managing that patch set, would you re-spin it 
> with the following check removed?
> 
> + if (usedpages < akcipher_calcsize(ctx)) {
> + err = -EMSGSIZE;
> + goto unlock;
> + }
> 

Ok, I'll update the patch.
Thanks,
-- 
TS