tpm: Fix error handling in async work

PATCH In review Tadeusz Struk 2 participants 9 messages Jan 16, 2022 – Jan 17, 2022

Subsystems (4)

Stable branch Kselftest Keys-trusted TPM device driver

lore

Share & tools

lore

Tadeusz Struk

author

2022-01-16 01:27 UTC #0 lore raw

PATCH v4 1/2 tpm: Fix error handling in async work

When an invalid (non existing) handle is used in a TPM command, that uses the resource manager interface (/dev/tpmrm0) the resource manager tries to load it from its internal cache, but fails and the tpm_dev_transmit returns an -EINVAL error to the caller. The existing async handler doesn't handle these error cases currently and the condition in the poll handler never returns mask with EPOLLIN set.
The result is that the poll call blocks and the application gets stuck until the user_read_timer wakes it up after 120 sec.
Change the tpm_dev_async_work function to handle error conditions returned from tpm_dev_transmit they are also reflected in the poll mask and a correct error code could passed back to the caller.

Cc Jarkko Sakkinen <[email protected]>

Cc Jason Gunthorpe <[email protected]>

Cc <[email protected]>

Fixes 9e1b74a63f77 ("tpm: add support for nonblocking operation")

Tested-by Jarkko Sakkinen<[email protected]>

Signed-off-by Tadeusz Struk <[email protected]>

---
Changed in v2:
- Updated commit message with better problem description
- Fixed typeos.
Changed in v3:
- Added a comment to tpm_dev_async_work.
- Updated commit message.
Changed in v4:
- Fixed a typo in the comment.

---
 drivers/char/tpm/tpm-dev-common.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/char/tpm/tpm-dev-common.c +7 -1

index c08cbb306636..dc4c0a0a5129 100644
@@ -69,7 +69,13 @@ static void tpm_dev_async_work(struct work_struct *work)
 	ret = tpm_dev_transmit(priv->chip, priv->space, priv->data_buffer,
 			       sizeof(priv->data_buffer));
 	tpm_put_ops(priv->chip);
-	if (ret > 0) {
+
+	/*
+	 * If ret is > 0 then tpm_dev_transmit returned the size of the
+	 * response. If ret is < 0 then tpm_dev_transmit failed and
+	 * returned an error code.
+	 */
+	if (ret != 0) {
 		priv->response_length = ret;
 		mod_timer(&priv->user_read_timer, jiffies + (120 * HZ));
 	}

Signature · 1 line

2.30.2

Tadeusz Struk

author

2022-01-16 01:27 UTC #1 lore raw

PATCH v3 2/2 selftests: tpm: add async space test with noneexisting handle

Add a test for /dev/tpmrm0 in async mode that checks if the code handles invalid handles correctly.

Cc Jarkko Sakkinen <[email protected]>

Cc Shuah Khan <[email protected]>

Cc <[email protected]>

Tested-by Jarkko Sakkinen<[email protected]>

Signed-off-by Tadeusz Struk <[email protected]>

---
Changed in v2:
- Updated commit message
Changed in v3:
- Fixed typo in the function name

---
 tools/testing/selftests/tpm2/tpm2_tests.py | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

tools/testing/selftests/tpm2/tpm2_tests.py +16 -0

index 9d764306887b..340ffef97fb6 100644
@@ -302,3 +302,19 @@ class AsyncTest(unittest.TestCase):
         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()
+
+    def test_flush_invalid_context(self):
+        log = logging.getLogger(__name__)
+        log.debug(sys._getframe().f_code.co_name)
+
+        async_client = tpm2.Client(tpm2.Client.FLAG_SPACE | tpm2.Client.FLAG_NONBLOCK)
+        log.debug("Calling flush_context passing in an invalid handle ")
+        handle = 0x80123456
+        rc = 0
+        try:
+            async_client.flush_context(handle)
+        except OSError as e:
+            rc = e.errno
+
+        self.assertEqual(rc, 22)
+        async_client.close()

Signature · 1 line

2.30.2

Jarkko Sakkinen

maintainer Reviewed-by on v3 2/2

2022-01-16 14:34 UTC #2 lore raw

Re: [PATCH v3 2/2] selftests: tpm: add async space test with noneexisting handle

Quoted 47 lines · Tadeusz Struk

Add a test for /dev/tpmrm0 in async mode that checks if the code handles invalid handles correctly.
Cc Jarkko Sakkinen <[email protected]>
Cc Shuah Khan <[email protected]>
Cc <[email protected]>
Cc <[email protected]>
Cc <[email protected]>
Tested-by Jarkko Sakkinen<[email protected]>
Signed-off-by Tadeusz Struk <[email protected]>
---
Changed in v2:
- Updated commit message
Changed in v3:
- Fixed typo in the function name
---
 tools/testing/selftests/tpm2/tpm2_tests.py | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
tools/testing/selftests/tpm2/tpm2_tests.py +16 -0
index 9d764306887b..340ffef97fb6 100644
@@ -302,3 +302,19 @@ class AsyncTest(unittest.TestCase):
         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()
+
+    def test_flush_invalid_context(self):
+        log = logging.getLogger(__name__)
+        log.debug(sys._getframe().f_code.co_name)
+
+        async_client = tpm2.Client(tpm2.Client.FLAG_SPACE | tpm2.Client.FLAG_NONBLOCK)
+        log.debug("Calling flush_context passing in an invalid handle ")
+        handle = 0x80123456
+        rc = 0
+        try:
+            async_client.flush_context(handle)
+        except OSError as e:
+            rc = e.errno
+
+        self.assertEqual(rc, 22)
+        async_client.close()
Signature · 1 line
2.30.2

Thank you.

Reviewed-by Jarkko Sakkinen <[email protected]>

BR, Jarkko

Jarkko Sakkinen

maintainer

2022-01-16 20:05 UTC #3 lore raw

Re: [PATCH v3 2/2] selftests: tpm: add async space test with noneexisting handle

Quoted 51 lines · Jarkko Sakkinen

Quoted 47 lines · Tadeusz Struk
Add a test for /dev/tpmrm0 in async mode that checks if the code handles invalid handles correctly.
Cc Jarkko Sakkinen <[email protected]>
Cc Shuah Khan <[email protected]>
Cc <[email protected]>
Cc <[email protected]>
Cc <[email protected]>
Tested-by Jarkko Sakkinen<[email protected]>
Signed-off-by Tadeusz Struk <[email protected]>
---
Changed in v2:
- Updated commit message
Changed in v3:
- Fixed typo in the function name
---
 tools/testing/selftests/tpm2/tpm2_tests.py | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
tools/testing/selftests/tpm2/tpm2_tests.py +16 -0
index 9d764306887b..340ffef97fb6 100644
@@ -302,3 +302,19 @@ class AsyncTest(unittest.TestCase):
         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()
+
+    def test_flush_invalid_context(self):
+        log = logging.getLogger(__name__)
+        log.debug(sys._getframe().f_code.co_name)
+
+        async_client = tpm2.Client(tpm2.Client.FLAG_SPACE | tpm2.Client.FLAG_NONBLOCK)
+        log.debug("Calling flush_context passing in an invalid handle ")
+        handle = 0x80123456
+        rc = 0
+        try:
+            async_client.flush_context(handle)
+        except OSError as e:
+            rc = e.errno
+
+        self.assertEqual(rc, 22)
+        async_client.close()
Signature · 1 line
2.30.2
Thank you.

Quoted 3 lines

Reviewed-by Jarkko Sakkinen <[email protected]>
BR, Jarkko

$ b4 am [email protected]
Looking up https://lore.kernel.org/r/20220116012627.2031-2-tstruk%40gmail.com
Grabbing thread from lore.kernel.org/all/20220116012627.2031-2-tstruk%40gmail.com/t.mbox.gz
Analyzing 4 messages in the thread
Will use the latest revision: v4
You can pick other revisions using the -vN flag
Checking attestation on all messages, may take a moment...
---
  ✓ [PATCH v4 1/2] tpm: Fix error handling in async work
    ✓ Signed: DKIM/gmail.com
    + Reviewed-by: Jarkko Sakkinen <[email protected]> (✓ DKIM/kernel.org)
  ERROR: missing [2/2]!
---
Total patches: 1
---
WARNING: Thread incomplete!
 Link: https://lore.kernel.org/r/[email protected]
 Base: applies clean to current tree
       git am ./v4_20220115_tstruk_tpm_fix_error_handling_in_async_work.mbx

The patch set is broken. It has patches from two different patch sets.

I saved the 2nd patch from my email client.

BR, Jarkko

Tadeusz Struk

author

2022-01-16 22:43 UTC #4 lore raw

Re: [PATCH v3 2/2] selftests: tpm: add async space test with noneexisting handle

Quoted 20 lines · Jarkko Sakkinen

$ b4 [email protected]
Looking uphttps://lore.kernel.org/r/20220116012627.2031-2-tstruk%40gmail.com
Grabbing thread from lore.kernel.org/all/20220116012627.2031-2-tstruk%40gmail.com/t.mbox.gz
Analyzing 4 messages in the thread
Will use the latest revision: v4
You can pick other revisions using the -vN flag
Checking attestation on all messages, may take a moment...
---
   ✓ [PATCH v4 1/2] tpm: Fix error handling in async work
     ✓ Signed: DKIM/gmail.com
     + Reviewed-by: Jarkko Sakkinen<[email protected]>  (✓ DKIM/kernel.org)
   ERROR: missing [2/2]!
---
Total patches: 1
---
WARNING: Thread incomplete!
  Link:https://lore.kernel.org/r/[email protected]
  Base: applies clean to current tree
        git am ./v4_20220115_tstruk_tpm_fix_error_handling_in_async_work.mbx

Quoted 3 lines

The patch set is broken. It has patches from two different patch sets.
I saved the 2nd patch from my email client.

I'm not sure why it doesn't like it. They both are generated with the same git format-patch command. If you look at the source, the first one is:

Message-Id: <[email protected]>

and the second is:

Message-Id: <[email protected]>

and contains:

In-Reply-To: <[email protected]>
References: <[email protected]>

Maybe it doesn't like the fact that the first in v4 and the second in v3? Do you need me to do anything else to get that applied?

--
Thanks,
Tadeusz

Jarkko Sakkinen

maintainer

2022-01-17 01:45 UTC #5 lore raw

Re: [PATCH v3 2/2] selftests: tpm: add async space test with noneexisting handle

Quoted 39 lines · Tadeusz Struk

Quoted 20 lines · Jarkko Sakkinen
$ b4 [email protected]
Looking uphttps://lore.kernel.org/r/20220116012627.2031-2-tstruk%40gmail.com
Grabbing thread from lore.kernel.org/all/20220116012627.2031-2-tstruk%40gmail.com/t.mbox.gz
Analyzing 4 messages in the thread
Will use the latest revision: v4
You can pick other revisions using the -vN flag
Checking attestation on all messages, may take a moment...
---
   ✓ [PATCH v4 1/2] tpm: Fix error handling in async work
     ✓ Signed: DKIM/gmail.com
     + Reviewed-by: Jarkko Sakkinen<[email protected]>  (✓ DKIM/kernel.org)
   ERROR: missing [2/2]!
---
Total patches: 1
---
WARNING: Thread incomplete!
  Link:https://lore.kernel.org/r/[email protected]
  Base: applies clean to current tree
        git am ./v4_20220115_tstruk_tpm_fix_error_handling_in_async_work.mbx
Quoted 3 lines
The patch set is broken. It has patches from two different patch sets.
I saved the 2nd patch from my email client.
I'm not sure why it doesn't like it. They both are generated with the same git format-patch command. If you look at the source, the first one is:
Message-Id: <[email protected]>
and the second is:
Message-Id: <[email protected]>
and contains:
In-Reply-To: <[email protected]>
References: <[email protected]>

Quoted 6 lines

Maybe it doesn't like the fact that the first in v4 and the second in v3? Do you need me to do anything else to get that applied?
--
Thanks,
Tadeusz

NP, Both are applied now.

BR, Jarkko

Tadeusz Struk

author

2022-01-17 02:55 UTC #6 lore raw

Re: [PATCH v3 2/2] selftests: tpm: add async space test with noneexisting handle

Quoted 1 line · Jarkko Sakkinen

NP, Both are applied now.

Looking at
https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/
I can see only the 2/2 selftest applied.

Jarkko Sakkinen

maintainer

2022-01-17 07:03 UTC #7 lore raw

Re: [PATCH v3 2/2] selftests: tpm: add async space test with noneexisting handle

Quoted 6 lines · Tadeusz Struk

Quoted 1 line · Jarkko Sakkinen
NP, Both are applied now.
Looking at
https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/
I can see only the 2/2 selftest applied.

It was my bad, forgot the update the remote master. Now it should be good.

BR, Jarkko

Jarkko Sakkinen

maintainer Reviewed-by on v4 1/2

2022-01-16 14:34 UTC #8 lore raw

In reply to Tadeusz Struk

Quoted 19 lines · Tadeusz Struk

When an invalid (non existing) handle is used in a TPM command, that uses the resource manager interface (/dev/tpmrm0) the resource manager tries to load it from its internal cache, but fails and the tpm_dev_transmit returns an -EINVAL error to the caller. The existing async handler doesn't handle these error cases currently and the condition in the poll handler never returns mask with EPOLLIN set.
The result is that the poll call blocks and the application gets stuck until the user_read_timer wakes it up after 120 sec.
Change the tpm_dev_async_work function to handle error conditions returned from tpm_dev_transmit they are also reflected in the poll mask and a correct error code could passed back to the caller.
Cc Jarkko Sakkinen <[email protected]>
Cc Jason Gunthorpe <[email protected]>
Cc <[email protected]>
Cc <[email protected]>
Cc <[email protected]>

Quoted 3 lines

Fixes 9e1b74a63f77 ("tpm: add support for nonblocking operation")
Tested-by Jarkko Sakkinen<[email protected]>
Signed-off-by Tadeusz Struk <[email protected]>

Thank you.

Reviewed-by Jarkko Sakkinen <[email protected]>

BR, Jarkko