All of lore.kernel.org
 help / color / mirror / Atom feed
From: Javier Martinez Canillas <javierm@redhat.com>
To: linux-kernel@vger.kernel.org
Cc: Javier Martinez Canillas <javierm@redhat.com>,
	Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	Peter Huewe <peterhuewe@gmx.de>,
	Philip Tricca <philip.b.tricca@intel.com>,
	Jason Gunthorpe <jgunthorpe@obsidianresearch.com>,
	linux-integrity@vger.kernel.org,
	William Roberts <william.c.roberts@intel.com>
Subject: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails
Date: Fri, 17 Nov 2017 11:07:24 +0100	[thread overview]
Message-ID: <20171117100724.19257-1-javierm@redhat.com> (raw)

According to the TPM Library Specification, a TPM device must do a command
header validation before processing and return a TPM_RC_COMMAND_CODE code
if the command is not implemented and the TPM_RC_COMMAND_SIZE code if the
command buffer size is not correct.

So user-space will expect to handle these response codes as errors, but if
the in-kernel resource manager is used (/dev/tpmrm?) then an -EINVAL errno
code is returned instead if the command isn't implemented or the buffer
size isn't correct. This confuses user-space since doesn't expect that.

This is also not consistent with the behavior when not using TPM spaces
and accessing the TPM directly (/dev/tpm?), in this case the command is
is sent to the TPM anyways and user-space can get an error from the TPM.

Instead of returning an -EINVAL errno code when the tpm_validate_command()
function fails, allow the command to be sent to the TPM but just don't do
any TPM space management. That way the TPM can report back a proper error
and the behavior be consistent when using either /dev/tpm? or /dev/tpmrm?.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

---

Hello,

This patch is an RFC because I'm not sure if this is the correct way to fix this
issue. I'm not that familiar with the TPM driver so may had missed some details.

And example of user-space getting confused by the TPM chardev returning -EINVAL
when sending a not supported TPM command can be seen in this tpm2-tools issue:

https://github.com/intel/tpm2-tools/issues/621

Best regards,
Javier

 drivers/char/tpm/tpm-interface.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 1d6729be4cd6..86527e9a27cc 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -390,9 +390,14 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	u32 count, ordinal;
 	unsigned long stop;
 	bool need_locality;
+	bool cmd_validated;
 
-	if (!tpm_validate_command(chip, space, buf, bufsiz))
-		return -EINVAL;
+	/*
+	 * If command validation fails, sent it to the TPM anyways so it can
+	 * report a proper error to user-space. Just don't do any TPM space
+	 * management in this case.
+	 */
+	cmd_validated = tpm_validate_command(chip, space, buf, bufsiz);
 
 	if (bufsiz > TPM_BUFSIZE)
 		bufsiz = TPM_BUFSIZE;
@@ -424,9 +429,11 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 		chip->locality = rc;
 	}
 
-	rc = tpm2_prepare_space(chip, space, ordinal, buf);
-	if (rc)
-		goto out;
+	if (cmd_validated) {
+		rc = tpm2_prepare_space(chip, space, ordinal, buf);
+		if (rc)
+			goto out;
+	}
 
 	rc = chip->ops->send(chip, (u8 *) buf, count);
 	if (rc < 0) {
@@ -481,7 +488,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 		goto out;
 	}
 
-	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
+	if (cmd_validated)
+		rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
 
 out:
 	if (need_locality && chip->ops->relinquish_locality) {
-- 
2.14.3

             reply	other threads:[~2017-11-17 10:07 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-17 10:07 Javier Martinez Canillas [this message]
2017-11-17 16:57 ` [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails Jason Gunthorpe
2017-11-17 17:56   ` Javier Martinez Canillas
2017-11-17 17:58     ` Jason Gunthorpe
2017-11-17 18:10       ` Javier Martinez Canillas
2017-11-17 18:17         ` Jason Gunthorpe
2017-11-17 18:34           ` Javier Martinez Canillas
2017-11-17 19:14             ` Roberts, William C
2017-11-17 19:14               ` Roberts, William C
2017-11-17 23:55               ` Jason Gunthorpe
2017-11-17 23:55                 ` Jason Gunthorpe
2017-11-18  0:53                 ` Javier Martinez Canillas
2017-11-18  0:53                   ` Javier Martinez Canillas
2017-11-19 15:27                   ` Jason Gunthorpe
2017-11-20  9:26                     ` Javier Martinez Canillas
2017-11-20 16:14                       ` Roberts, William C
2017-11-20 18:02                         ` Jason Gunthorpe
2017-11-20 18:04                       ` Jason Gunthorpe
2017-12-08 20:03           ` Ken Goldman
2017-12-08 20:18             ` Jason Gunthorpe
2017-12-08 19:58   ` Ken Goldman
2017-11-20 23:15 ` Jarkko Sakkinen
2017-11-21  9:07   ` Javier Martinez Canillas
2017-11-21  9:27     ` Javier Martinez Canillas
2017-11-21 12:30     ` Jarkko Sakkinen
2017-11-21 12:49       ` Javier Martinez Canillas
     [not found]         ` <DB638850A6A2434A93ECADDA0BC838905F09D5D9@ORSMSX103.amr.corp.intel.com>
2017-11-22 17:16           ` FW: " flihp
2017-11-22 19:25             ` Javier Martinez Canillas
2017-11-26 14:21               ` Jarkko Sakkinen
2017-11-29 11:26                 ` Javier Martinez Canillas
2017-11-22 20:13             ` Jason Gunthorpe
2017-12-08 20:16               ` Ken Goldman
2017-12-08 20:20                 ` Jason Gunthorpe
2017-11-26 14:18             ` Jarkko Sakkinen
2017-11-26 23:23               ` Javier Martinez Canillas
2017-11-26 14:14         ` Jarkko Sakkinen
2017-11-21 20:29       ` Roberts, William C
2017-11-22  9:26         ` Javier Martinez Canillas
2017-11-22  9:26           ` Javier Martinez Canillas
2017-11-26 14:12           ` Jarkko Sakkinen
2017-11-26 23:19             ` Javier Martinez Canillas
2017-12-08 20:11           ` Ken Goldman
2017-11-26 14:06         ` Jarkko Sakkinen
2017-12-08 20:20           ` Ken Goldman
2017-12-08 21:34             ` Javier Martinez Canillas
2017-12-17 16:47               ` Jarkko Sakkinen
2017-12-17 18:18                 ` Javier Martinez Canillas
2017-12-22 17:38                 ` Ken Goldman
2017-12-14 13:11             ` Jarkko Sakkinen
2017-12-08 19:51 ` Ken Goldman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20171117100724.19257-1-javierm@redhat.com \
    --to=javierm@redhat.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=jgunthorpe@obsidianresearch.com \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peterhuewe@gmx.de \
    --cc=philip.b.tricca@intel.com \
    --cc=william.c.roberts@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.