[PATCH v2 1/2] tpm: Fix error handling in async work

[PATCH v2 1/2] tpm: Fix error handling in async work

[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
returns an -EINVAL error to the caller. The 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.
Make sure that error conditions also contribute to the poll mask
so that a correct error code could passed back to the caller.

Cc: Jarkko Sakkinen <jarkko@kernel.org>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: <linux-integrity@vger.kernel.org>
Cc: <stable@vger.kernel.org>
Cc: <linux-kernel@vger.kernel.org>
Fixes: 9e1b74a63f77 ("tpm: add support for nonblocking operation")
Signed-off-by: Tadeusz Struk <tstruk@gmail.com>
---
Changes in v2:
- Updated commit message with better problem description.
- Fixed typeos.
---
 drivers/char/tpm/tpm-dev-common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index c08cbb306636..fe2679f84cb6 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -69,7 +69,7 @@ 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 != 0) {
 		priv->response_length = ret;
 		mod_timer(&priv->user_read_timer, jiffies + (120 * HZ));
 	}
-- 
2.30.2

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

[Tadeusz Struk]

Add a test for tpm2 spaces in async mode that checks if
the code handles invalid handles correctly.

Cc: Jarkko Sakkinen <jarkko@kernel.org>
Cc: Shuah Khan <shuah@kernel.org>
Cc: <linux-integrity@vger.kernel.org>
Cc: <linux-kselftest@vger.kernel.org>
Cc: <linux-kernel@vger.kernel.org>
Signed-off-by: Tadeusz Struk <tstruk@gmail.com>
---
 tools/testing/selftests/tpm2/tpm2_tests.py | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/tools/testing/selftests/tpm2/tpm2_tests.py b/tools/testing/selftests/tpm2/tpm2_tests.py
index 9d764306887b..b373b0936e40 100644
--- a/tools/testing/selftests/tpm2/tpm2_tests.py
+++ b/tools/testing/selftests/tpm2/tpm2_tests.py
@@ -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_invlid_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()
-- 
2.30.2

Re: [PATCH v2 1/2] tpm: Fix error handling in async work

[Jarkko Sakkinen]

On Tue, 2021-12-28 at 21:06 -0800, Tadeusz Struk wrote:
> When an invalid (non existing) handle is used in a tpm command,
                                                     ~~~
                                                     TPM

> that uses the resource manager interface (/dev/tpmrm0) the resource
> manager tries to load it from its internal cache, but fails and
> returns an -EINVAL error to the caller. The 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.
> Make sure that error conditions also contribute to the poll mask
> so that a correct error code could passed back to the caller.

I'm not sure what "making sure" means.

> 
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: <linux-integrity@vger.kernel.org>
> Cc: <stable@vger.kernel.org>
> Cc: <linux-kernel@vger.kernel.org>
> Fixes: 9e1b74a63f77 ("tpm: add support for nonblocking operation")
> Signed-off-by: Tadeusz Struk <tstruk@gmail.com>
> ---
> Changes in v2:
> - Updated commit message with better problem description.
> - Fixed typeos.
> ---
>  drivers/char/tpm/tpm-dev-common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/tpm-dev-common.c
> b/drivers/char/tpm/tpm-dev-common.c
> index c08cbb306636..fe2679f84cb6 100644
> --- a/drivers/char/tpm/tpm-dev-common.c
> +++ b/drivers/char/tpm/tpm-dev-common.c
> @@ -69,7 +69,7 @@ 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 != 0) {

What if ret < 0?

You should explain this change in the commit message. Also, consider
adding an inline comment.

>                 priv->response_length = ret;
>                 mod_timer(&priv->user_read_timer, jiffies + (120 *
> HZ));
>         }

BR,
Jarkko

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

[Jarkko Sakkinen]

On Tue, 2021-12-28 at 21:06 -0800, Tadeusz Struk wrote:
> Add a test for tpm2 spaces in async mode that checks if
                 ~~~~
I would rather speak about adding a test case for /dev/tpmrm0.

> the code handles invalid handles correctly.
> 
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: <linux-integrity@vger.kernel.org>
> Cc: <linux-kselftest@vger.kernel.org>
> Cc: <linux-kernel@vger.kernel.org>
> Signed-off-by: Tadeusz Struk <tstruk@gmail.com>
> ---
>  tools/testing/selftests/tpm2/tpm2_tests.py | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/tools/testing/selftests/tpm2/tpm2_tests.py
> b/tools/testing/selftests/tpm2/tpm2_tests.py
> index 9d764306887b..b373b0936e40 100644
> --- a/tools/testing/selftests/tpm2/tpm2_tests.py
> +++ b/tools/testing/selftests/tpm2/tpm2_tests.py
> @@ -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_invlid_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()

BR,
Jarkko

Re: [PATCH v2 1/2] tpm: Fix error handling in async work

[Tadeusz Struk]

On 1/5/22 12:05, Jarkko Sakkinen wrote:
>> until the user_read_timer wakes it up after 120 sec.
>> Make sure that error conditions also contribute to the poll mask
>> so that a correct error code could passed back to the caller.
> I'm not sure what "making sure" means.



> 
>>          tpm_put_ops(priv->chip);
>> -       if (ret > 0) {
>> +       if (ret != 0) {
> What if ret < 0?
> 
> You should explain this change in the commit message. Also, consider
> adding an inline comment.

As the commit message says tpm_dev_transmit() returns -EINVAL when
the given handle doesn't exist, and the tmprm can not find it.
This is not handled properly and the application gets stuck.
I will add a comment here send a new version.

-- 
Thanks,
Tadeusz