[PATCH =v2 1/3] tpm: fix invalid locking in NONBLOCKING mode

[PATCH =v2 1/3] tpm: fix invalid locking in NONBLOCKING mode

[Tadeusz Struk]

When an application sends TPM commands in NONBLOCKING mode
the driver holds chip->tpm_mutex returning from write(),
which triggers: "WARNING: lock held when returning to user space".
To fix this issue the driver needs to release the mutex before
returning and acquire it again in tpm_dev_async_work() before
sending the command.

Cc: stable@vger.kernel.org
Fixes: 9e1b74a63f776 (tpm: add support for nonblocking operation)
Reported-by: Jeffrin Jose T <jeffrin@rajagiritech.edu.in>
Tested-by: Jeffrin Jose T <jeffrin@rajagiritech.edu.in>
Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
---
Changes in v2:
- Updated commit message as requested
- Add the fix and test updates to the same series
---
 drivers/char/tpm/tpm-dev-common.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index 2ec47a69a2a6..b23b0b999232 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -61,6 +61,12 @@ static void tpm_dev_async_work(struct work_struct *work)
 
 	mutex_lock(&priv->buffer_mutex);
 	priv->command_enqueued = false;
+	ret = tpm_try_get_ops(priv->chip);
+	if (ret) {
+		priv->response_length = ret;
+		goto out;
+	}
+
 	ret = tpm_dev_transmit(priv->chip, priv->space, priv->data_buffer,
 			       sizeof(priv->data_buffer));
 	tpm_put_ops(priv->chip);
@@ -68,6 +74,7 @@ static void tpm_dev_async_work(struct work_struct *work)
 		priv->response_length = ret;
 		mod_timer(&priv->user_read_timer, jiffies + (120 * HZ));
 	}
+out:
 	mutex_unlock(&priv->buffer_mutex);
 	wake_up_interruptible(&priv->async_wait);
 }
@@ -204,6 +211,7 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
 	if (file->f_flags & O_NONBLOCK) {
 		priv->command_enqueued = true;
 		queue_work(tpm_dev_wq, &priv->async_work);
+		tpm_put_ops(priv->chip);
 		mutex_unlock(&priv->buffer_mutex);
 		return size;
 	}

[PATCH =v2 2/3] tpm: selftest: add test covering async mode

[Tadeusz Struk]

Add a test that sends a tpm cmd in an async mode.
Currently there is a gap in test coverage with regards
to this functionality.

Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
---
 tools/testing/selftests/tpm2/test_smoke.sh |    1 +
 tools/testing/selftests/tpm2/tpm2.py       |   19 +++++++++++++++++--
 tools/testing/selftests/tpm2/tpm2_tests.py |   13 +++++++++++++
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/tpm2/test_smoke.sh b/tools/testing/selftests/tpm2/test_smoke.sh
index 80521d46220c..cb54ab637ea6 100755
--- a/tools/testing/selftests/tpm2/test_smoke.sh
+++ b/tools/testing/selftests/tpm2/test_smoke.sh
@@ -2,3 +2,4 @@
 # SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
 
 python -m unittest -v tpm2_tests.SmokeTest
+python -m unittest -v tpm2_tests.AsyncTest
diff --git a/tools/testing/selftests/tpm2/tpm2.py b/tools/testing/selftests/tpm2/tpm2.py
index 828c18584624..d0fcb66a88a6 100644
--- a/tools/testing/selftests/tpm2/tpm2.py
+++ b/tools/testing/selftests/tpm2/tpm2.py
@@ -6,8 +6,8 @@ import socket
 import struct
 import sys
 import unittest
-from fcntl import ioctl
-
+import fcntl
+import select
 
 TPM2_ST_NO_SESSIONS = 0x8001
 TPM2_ST_SESSIONS = 0x8002
@@ -352,6 +352,7 @@ def hex_dump(d):
 class Client:
     FLAG_DEBUG = 0x01
     FLAG_SPACE = 0x02
+    FLAG_NONBLOCK = 0x04
     TPM_IOC_NEW_SPACE = 0xa200
 
     def __init__(self, flags = 0):
@@ -362,13 +363,27 @@ class Client:
         else:
             self.tpm = open('/dev/tpmrm0', 'r+b', buffering=0)
 
+        if (self.flags & Client.FLAG_NONBLOCK):
+            flags = fcntl.fcntl(self.tpm, fcntl.F_GETFL)
+            flags |= os.O_NONBLOCK
+            fcntl.fcntl(self.tpm, fcntl.F_SETFL, flags)
+            self.tpm_poll = select.poll()
+
     def close(self):
         self.tpm.close()
 
     def send_cmd(self, cmd):
         self.tpm.write(cmd)
+
+        if (self.flags & Client.FLAG_NONBLOCK):
+            self.tpm_poll.register(self.tpm, select.POLLIN)
+            self.tpm_poll.poll(10000)
+
         rsp = self.tpm.read()
 
+        if (self.flags & Client.FLAG_NONBLOCK):
+            self.tpm_poll.unregister(self.tpm)
+
         if (self.flags & Client.FLAG_DEBUG) != 0:
             sys.stderr.write('cmd' + os.linesep)
             sys.stderr.write(hex_dump(cmd) + os.linesep)
diff --git a/tools/testing/selftests/tpm2/tpm2_tests.py b/tools/testing/selftests/tpm2/tpm2_tests.py
index d4973be53493..728be7c69b76 100644
--- a/tools/testing/selftests/tpm2/tpm2_tests.py
+++ b/tools/testing/selftests/tpm2/tpm2_tests.py
@@ -288,3 +288,16 @@ class SpaceTest(unittest.TestCase):
 
         self.assertEqual(rc, tpm2.TPM2_RC_COMMAND_CODE |
                          tpm2.TSS2_RESMGR_TPM_RC_LAYER)
+
+class AsyncTest(unittest.TestCase):
+    def setUp(self):
+        logging.basicConfig(filename='AsyncTest.log', level=logging.DEBUG)
+
+    def test_async(self):
+        log = logging.getLogger(__name__)
+        log.debug(sys._getframe().f_code.co_name)
+
+        async_client = tpm2.Client(tpm2.Client.FLAG_NONBLOCK)
+        log.debug("Calling get_cap in a NON_BLOCKING mode")
+        async_client.get_cap(tpm2.TPM2_CAP_HANDLES, tpm2.HR_LOADED_SESSION)
+        async_client.close()

Re: [PATCH =v2 2/3] tpm: selftest: add test covering async mode

[Jarkko Sakkinen]

On Thu, 2019-12-12 at 09:48 -0800, Tadeusz Struk wrote:
> Add a test that sends a tpm cmd in an async mode.
> Currently there is a gap in test coverage with regards
> to this functionality.
> 
> Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>

LGTM, thank you.

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

/Jarkko

[PATCH =v2 3/3] tpm: selftest: cleanup after unseal with wrong auth/policy test

[Tadeusz Struk]

Unseal with wrong auth or wrong policy test affects DA lockout
and eventually causes the tests to fail with:
"ProtocolError: TPM_RC_LOCKOUT: rc=0x00000921"
when the tests run multiple times.
Send tpm clear command after the test to reset the DA counters.

Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
---
 tools/testing/selftests/tpm2/test_smoke.sh |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/testing/selftests/tpm2/test_smoke.sh b/tools/testing/selftests/tpm2/test_smoke.sh
index cb54ab637ea6..8155c2ea7ccb 100755
--- a/tools/testing/selftests/tpm2/test_smoke.sh
+++ b/tools/testing/selftests/tpm2/test_smoke.sh
@@ -3,3 +3,8 @@
 
 python -m unittest -v tpm2_tests.SmokeTest
 python -m unittest -v tpm2_tests.AsyncTest
+
+CLEAR_CMD=$(which tpm2_clear)
+if [ -n $CLEAR_CMD ]; then
+	tpm2_clear -T device
+fi

Re: [PATCH =v2 3/3] tpm: selftest: cleanup after unseal with wrong auth/policy test

[James Bottomley]

On Thu, 2019-12-12 at 09:48 -0800, Tadeusz Struk wrote:
> Unseal with wrong auth or wrong policy test affects DA lockout
> and eventually causes the tests to fail with:
> "ProtocolError: TPM_RC_LOCKOUT: rc=0x00000921"
> when the tests run multiple times.
> Send tpm clear command after the test to reset the DA counters.
> 
> Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
> ---
>  tools/testing/selftests/tpm2/test_smoke.sh |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/tools/testing/selftests/tpm2/test_smoke.sh
> b/tools/testing/selftests/tpm2/test_smoke.sh
> index cb54ab637ea6..8155c2ea7ccb 100755
> --- a/tools/testing/selftests/tpm2/test_smoke.sh
> +++ b/tools/testing/selftests/tpm2/test_smoke.sh
> @@ -3,3 +3,8 @@
>  
>  python -m unittest -v tpm2_tests.SmokeTest
>  python -m unittest -v tpm2_tests.AsyncTest
> +
> +CLEAR_CMD=$(which tpm2_clear)
> +if [ -n $CLEAR_CMD ]; then
> +	tpm2_clear -T device
> +fi

TPM2_Clear reprovisions the SPS ... that would make all currently
exported TPM keys go invalid.  I know these tests should be connected
to a vTPM, so doing this should be safe, but if this accidentally got
executed on your laptop all TPM relying functions would be disrupted,
which doesn't seem to be the best thing to hard wire into a test.

What about doing a TPM2_DictionaryAttackLockReset instead, which is the
least invasive route to fixing the problem ... provided you know what
the lockout authorization is.

James

Re: [PATCH =v2 3/3] tpm: selftest: cleanup after unseal with wrong auth/policy test

[Tadeusz Struk]

On 12/12/19 11:51 AM, James Bottomley wrote:
> TPM2_Clear reprovisions the SPS ... that would make all currently
> exported TPM keys go invalid.  I know these tests should be connected
> to a vTPM, so doing this should be safe, but if this accidentally got
> executed on your laptop all TPM relying functions would be disrupted,
> which doesn't seem to be the best thing to hard wire into a test.

That is true, but it will need to be executed as root, and root
should know what she/he is doing ;)

> 
> What about doing a TPM2_DictionaryAttackLockReset instead, which is the
> least invasive route to fixing the problem ... provided you know what
> the lockout authorization is.

I can change tpm2_clear to tpm2_dictionarylockout -c if we want to make
it foolproof. In this case we can assume that the lockout auth is empty.

-- 
Tadeusz

Re: [PATCH =v2 3/3] tpm: selftest: cleanup after unseal with wrong auth/policy test

[James Bottomley]

On Thu, 2019-12-12 at 12:49 -0800, Tadeusz Struk wrote:
> On 12/12/19 11:51 AM, James Bottomley wrote:
> > TPM2_Clear reprovisions the SPS ... that would make all currently
> > exported TPM keys go invalid.  I know these tests should be
> > connected to a vTPM, so doing this should be safe, but if this
> > accidentally got executed on your laptop all TPM relying functions
> > would be disrupted, which doesn't seem to be the best thing to hard
> > wire into a test.
> 
> That is true, but it will need to be executed as root, and root
> should know what she/he is doing ;)

Not in the modern kernel resource manager world: anyone who is in the
tpm group can access the tpmrm device and we haven't added a dangerous
command filter like we promised we would, so unless they have actually
set lockout or platform authorization, they'll find they can execute it


> > What about doing a TPM2_DictionaryAttackLockReset instead, which is
> > the least invasive route to fixing the problem ... provided you
> > know what the lockout authorization is.
> 
> I can change tpm2_clear to tpm2_dictionarylockout -c if we want to
> make it foolproof. In this case we can assume that the lockout auth
> is empty.

Well, if it isn't TPM2_Clear would refuse to execute as well since that
requires either lockout auth or platform + physical presence.

James

Re: [PATCH =v2 3/3] tpm: selftest: cleanup after unseal with wrong auth/policy test

[Tadeusz Struk]

On 12/12/19 12:54 PM, James Bottomley wrote:
> Not in the modern kernel resource manager world: anyone who is in the
> tpm group can access the tpmrm device and we haven't added a dangerous
> command filter like we promised we would, so unless they have actually
> set lockout or platform authorization, they'll find they can execute it

The default for the tpm2_* tools with '-T device' switch is to talk to
/dev/tpm0.

If one would try to run it, by mistake, it would fail with:

$ tpm2_clear -T device
ERROR:tcti:src/tss2-tcti/tcti-device.c:439:Tss2_Tcti_Device_Init()
Failed to open device file /dev/tpm0: Permission denied

To point it to /dev/tpmrm0 it would need to be:
$ tpm2_clear -T device:/dev/tpmrm0

-- 
Tadeusz

Re: [PATCH =v2 3/3] tpm: selftest: cleanup after unseal with wrong auth/policy test

[James Bottomley]

On December 12, 2019 4:07:26 PM EST, Tadeusz Struk <tadeusz.struk@intel.com> wrote:
>On 12/12/19 12:54 PM, James Bottomley wrote:
>> Not in the modern kernel resource manager world: anyone who is in the
>> tpm group can access the tpmrm device and we haven't added a
>dangerous
>> command filter like we promised we would, so unless they have
>actually
>> set lockout or platform authorization, they'll find they can execute
>it
>
>The default for the tpm2_* tools with '-T device' switch is to talk to
>/dev/tpm0.
>
>If one would try to run it, by mistake, it would fail with:
>
>$ tpm2_clear -T device
>ERROR:tcti:src/tss2-tcti/tcti-device.c:439:Tss2_Tcti_Device_Init()
>Failed to open device file /dev/tpm0: Permission denied
>
>To point it to /dev/tpmrm0 it would need to be:
>$ tpm2_clear -T device:/dev/tpmrm0

And most other toolkits talk to the tpmrm device because the tpm 1.2 daemon based architecture didn't work so well.  The point is that if tpm2_clear works on your emulator, it likely works on your real tpm, so making the tests safer to run is not unreasonable.

James

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH =v2 3/3] tpm: selftest: cleanup after unseal with wrong auth/policy test

[Jarkko Sakkinen]

On Thu, 2019-12-12 at 12:49 -0800, Tadeusz Struk wrote:
> I can change tpm2_clear to tpm2_dictionarylockout -c if we want to make
> it foolproof. In this case we can assume that the lockout auth is empty.

Check that your fix applies cleanly to for-linus-v5.5-rc3 before you
send it [*]. I'll amend it then to the appropriate commit.

[*] git://git.infradead.org/users/jjs/linux-tpmdd.git

/Jarkko

Re: [PATCH =v2 1/3] tpm: fix invalid locking in NONBLOCKING mode

[Jarkko Sakkinen]

On Thu, 2019-12-12 at 09:48 -0800, Tadeusz Struk wrote:
> When an application sends TPM commands in NONBLOCKING mode
> the driver holds chip->tpm_mutex returning from write(),
> which triggers: "WARNING: lock held when returning to user space".
> To fix this issue the driver needs to release the mutex before
> returning and acquire it again in tpm_dev_async_work() before
> sending the command.

This is way better, thank you. I'll put this to my rc3 PR.

> 
> Cc: stable@vger.kernel.org
> Fixes: 9e1b74a63f776 (tpm: add support for nonblocking operation)
> Reported-by: Jeffrin Jose T <jeffrin@rajagiritech.edu.in>
> Tested-by: Jeffrin Jose T <jeffrin@rajagiritech.edu.in>
> Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>

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

/Jarkko