Re: [PATCH v4 2/7] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling

[Jarkko Sakkinen]

On Wed, 24 Oct 2018, James Bottomley wrote:

+static void KDFa(u8 *key, int keylen, const char *label, u8 *u,
+u8 *v, int bytes, u8 *out)


Should this be in lower case? I would rename it as tpm_kdfa().


This one is defined as KDFa() in the standards and it's not TPM
specific (although some standards refer to it as KDFA).  I'm not averse
to making them tpm_kdfe() and tpm_kdfa() but I was hoping that one day
the crypto subsystem would need them and we could move them in there
because KDFs are the new shiny in crypto primitives (TLS 1.2 started
using them, for instance).


I care more about tracing and debugging than naming and having 'tpm_' in
front of every TPM function makes tracing a lean process. AFAIK using
upper case letters is against kernel coding conventions. I'm not sure
why this would make an exception on that.


Why doesn't it matter here?


Because, as the comment says, it eventually gets overwritten by running
ecdh to derive the two co-ordinates.  (pointers to these two
uninitialized areas are passed into the ecdh destination sg list).


Oh, I just misunderstood the comment. Wouldn't it be easier to say that
the data is initialized later?


+   buf_len = crypto_ecdh_key_len();
+   if (sizeof(encoded_key) < buf_len) {
+   dev_err(>dev, "salt buffer too small needs
%d\n",
+   buf_len);
+   goto out;
+   }


In what situation this can happen? Can sizeof(encoded_key) >=
buf_len?


Yes, but only if someone is trying to crack your ecdh.  One of the
security issues in ecdh is if someone makes a very specific point
choice (usually in the cofactor space) that has a very short period,
the attacker can guess the input to KDFe.  In this case if TPM genie
provided a specially crafted attack EC point, we'd detect it here
because the resulting buffer would be too short.


Right. Thank you for the explanation. Here some kind of comment might
not be a bad idea...


In general this function should have a clear explanation what it does
and maybe less these one character variables but instead variables
with more documenting names. Explain in high level what algorithms
are used and how the salt is calculated.


I'll try, but this is a rather complex function.


Understood. I do not expect perfection here and we can improve
documetation later on.

For anyone wanting to review James' patches and w/o much experience on
EC, I recommend reading this article:

https://arstechnica.com/information-technology/2013/10/a-relatively-easy-to-understand-primer-on-elliptic-curve-cryptography/

I read it few years ago and refreshed my memory few days ago by
re-reading it.




+
+/**
+ * tpm_buf_append_hmac_session() append a TPM session element
+ * @buf: The buffer to be appended
+ * @auth: the auth structure allocated by
tpm2_start_auth_session()
+ * @attributes: The session attributes
+ * @passphrase: The session authority (NULL if none)
+ * @passphraselen: The length of the session authority (0 if none)


The alignment.


the alignment of what?


We generally have parameter descriptions tab-aligned.


Why there would be trailing zeros?


Because TPM 1.2 mandated zero padded fixed size passphrases so the TPM
2.0 standard specifies a way of converting these to variable size
strings by eliminating the zero padding.


Ok.


James


I'm also looking forward for the CONTEXT_GAP patch based on the
yesterdays discussion. We do want it and I was stupid not to take it
couple years ago :-) Thanks.

/Jarkko

Re: [PATCH v4 0/7] add integrity and security to TPM2 transactions

[Jarkko Sakkinen]

On Wed, 24 Oct 2018, James Bottomley wrote:

On Wed, 2018-10-24 at 02:51 +0300, Jarkko Sakkinen wrote:

I would consider sending first a patch set that would iterate the
existing session stuff to be ready for this i.e. merge in two
iterations (emphasis on the word "consider"). We can probably merge
the groundwork quite fast.


I realise we're going to have merge conflicts on the later ones, so why
don't we do this: I'll still send as one series, but you apply the ones
you think are precursors and I'll rebase and resend the rest?

James


Works for me and now I think after yesterdays dicussions etc. that this
should be merged as one series.

/Jarkko

Re: [PATCH v4 2/7] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling

[Jarkko Sakkinen]

On Tue, 23 Oct 2018, Ard Biesheuvel wrote:

On 23 October 2018 at 04:01, James Bottomley
 wrote:

On Mon, 2018-10-22 at 19:19 -0300, Ard Biesheuvel wrote:
[...]

+static void hmac_init(struct shash_desc *desc, u8 *key, int
keylen)
+{
+   u8 pad[SHA256_BLOCK_SIZE];
+   int i;
+
+   desc->tfm = sha256_hash;
+   desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;


I don't think this actually does anything in the shash API
implementation, so you can drop this.


OK, I find crypto somewhat hard to follow.  There were bits I had to
understand, like when I wrote the CFB implementation or when I fixed
the ECDH scatterlist handling, but I've got to confess, in time
honoured tradition I simply copied this from EVM crypto without
actually digging into the code to understand why.



Yeah, it is notoriously hard to use, and we should try to improve that.


James,

I would hope (already said in my review) to use longer than one
character variable names for most of the stuff. I did not quite
understand why you decided to use 'counter' for obvious counter
variable and one character names for non-obvious stuff :-)

I'm not sure where the 'encoded' exactly comes in the variable
name 'encoded_key' especially in the context of these cryptic
names.

/Jarkko

Re: [PATCH v4 5/7] trusted keys: Add session encryption protection to the seal/unseal path

[Jarkko Sakkinen]

The tag in the short description does not look at all. Should be either
"tpm:" or "keys, trusted:".

On Mon, 22 Oct 2018, James Bottomley wrote:

If some entity is snooping the TPM bus, the can see the data going in
to be sealed and the data coming out as it is unsealed.  Add parameter
and response encryption to these cases to ensure that no secrets are
leaked even if the bus is snooped.

As part of doing this conversion it was discovered that policy
sessions can't work with HMAC protected authority because of missing
pieces (the tpm Nonce).  I've added code to work the same way as
before, which will result in potential authority exposure (while still
adding security for the command and the returned blob), and a fixme to
redo the API to get rid of this security hole.

Signed-off-by: James Bottomley 
---
drivers/char/tpm/tpm2-cmd.c | 155 
1 file changed, 98 insertions(+), 57 deletions(-)

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 22f1c7bee173..a8655cd535d1 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -425,7 +425,9 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
{
unsigned int blob_len;
struct tpm_buf buf;
+   struct tpm_buf t2b;
u32 hash;
+   struct tpm2_auth *auth;
int i;
int rc;

@@ -439,45 +441,56 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
if (i == ARRAY_SIZE(tpm2_hash_map))
return -EINVAL;

-   rc = tpm_buf_init(, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
+   rc = tpm2_start_auth_session(chip, );
if (rc)
return rc;

-   tpm_buf_append_u32(, options->keyhandle);
-   tpm2_buf_append_auth(, TPM2_RS_PW,
-NULL /* nonce */, 0,
-0 /* session_attributes */,
-options->keyauth /* hmac */,
-TPM_DIGEST_SIZE);
+   rc = tpm_buf_init(, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
+   if (rc) {
+   tpm2_end_auth_session(auth);
+   return rc;
+   }
+
+   rc = tpm_buf_init_2b();
+   if (rc) {
+   tpm_buf_destroy();
+   tpm2_end_auth_session(auth);
+   return rc;
+   }

+   tpm_buf_append_name(, auth, options->keyhandle, NULL);
+   tpm_buf_append_hmac_session(, auth, TPM2_SA_DECRYPT,
+   options->keyauth, TPM_DIGEST_SIZE);
/* sensitive */
-   tpm_buf_append_u16(, 4 + TPM_DIGEST_SIZE + payload->key_len + 1);
+   tpm_buf_append_u16(, TPM_DIGEST_SIZE);
+   tpm_buf_append(, options->blobauth, TPM_DIGEST_SIZE);
+   tpm_buf_append_u16(, payload->key_len + 1);
+   tpm_buf_append(, payload->key, payload->key_len);
+   tpm_buf_append_u8(, payload->migratable);

-   tpm_buf_append_u16(, TPM_DIGEST_SIZE);
-   tpm_buf_append(, options->blobauth, TPM_DIGEST_SIZE);
-   tpm_buf_append_u16(, payload->key_len + 1);
-   tpm_buf_append(, payload->key, payload->key_len);
-   tpm_buf_append_u8(, payload->migratable);
+   tpm_buf_append_2b(, );

/* public */
-   tpm_buf_append_u16(, 14 + options->policydigest_len);
-   tpm_buf_append_u16(, TPM2_ALG_KEYEDHASH);
-   tpm_buf_append_u16(, hash);
+   tpm_buf_append_u16(, TPM2_ALG_KEYEDHASH);
+   tpm_buf_append_u16(, hash);

/* policy */
if (options->policydigest_len) {
-   tpm_buf_append_u32(, 0);
-   tpm_buf_append_u16(, options->policydigest_len);
-   tpm_buf_append(, options->policydigest,
+   tpm_buf_append_u32(, 0);
+   tpm_buf_append_u16(, options->policydigest_len);
+   tpm_buf_append(, options->policydigest,
   options->policydigest_len);
} else {
-   tpm_buf_append_u32(, TPM2_OA_USER_WITH_AUTH);
-   tpm_buf_append_u16(, 0);
+   tpm_buf_append_u32(, TPM2_OA_USER_WITH_AUTH);
+   tpm_buf_append_u16(, 0);
}

/* public parameters */
-   tpm_buf_append_u16(, TPM2_ALG_NULL);
-   tpm_buf_append_u16(, 0);
+   tpm_buf_append_u16(, TPM2_ALG_NULL);
+   /* unique (zero) */
+   tpm_buf_append_u16(, 0);
+
+   tpm_buf_append_2b(, );

/* outside info */
tpm_buf_append_u16(, 0);
@@ -490,8 +503,11 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
goto out;
}

-   rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 4, 0,
- "sealing data");
+   tpm_buf_fill_hmac_session(, auth);
+
+   rc = tpm_transmit_cmd(chip, >kernel_space, buf.data,
+ PAGE_SIZE, 4, 0, "sealing data");
+   rc = tpm_buf_check_hmac_response(, auth, rc);
if (rc)
goto out;

@@ -509,6 +525,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
payload->blob_len = blob_len;

Re: [PATCH v4 0/7] add integrity and security to TPM2 transactions

[Jarkko Sakkinen]

I would consider sending first a patch set that would iterate the existing
session stuff to be ready for this i.e. merge in two iterations 
(emphasis on the word "consider"). We can probably merge the groundwork

quite fast.

/Jarkko

On Mon, 22 Oct 2018, James Bottomley wrote:

By now, everybody knows we have a problem with the TPM2_RS_PW easy
button on TPM2 in that transactions on the TPM bus can be intercepted
and altered.  The way to fix this is to use real sessions for HMAC
capabilities to ensure integrity and to use parameter and response
encryption to ensure confidentiality of the data flowing over the TPM
bus.

This patch series is about adding a simple API which can ensure the
above properties as a layered addition to the existing TPM handling
code.  This series now includes protections for PCR extend, getting
random numbers from the TPM and data sealing and unsealing.  It
therefore eliminates all uses of TPM2_RS_PW in the kernel and adds
encryption protection to sensitive data flowing into and out of the
TPM.

In the third version I added data sealing and unsealing protection,
apart from one API based problem which means that the way trusted keys
were protected it's not currently possible to HMAC protect an authority
that comes with a policy, so the API will have to be extended to fix
that case

In this fourth version, I tidy up some of the code and add more
security features, the most notable is that we now calculate the NULL
seed name and compare our calculation to the value returned in
TPM2_ReadPublic, which means we now can't be spoofed.  This version
also gives a sysfs variable for the null seed which userspace can use
to run a key certification operation to prove that the TPM was always
secure when communicating with the kernel.

I've verified this using the test suite in the last patch on a VM
connected to a tpm2 emulator.  I also instrumented the emulator to make
sure the sensitive data was properly encrypted.

James

---


James Bottomley (7):
 tpm-buf: create new functions for handling TPM buffers
 tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
 tpm2: add hmac checks to tpm2_pcr_extend()
 tpm2: add session encryption protection to tpm2_get_random()
 trusted keys: Add session encryption protection to the seal/unseal
   path
 tpm: add the null key name as a tpm2 sysfs variable
 tpm2-sessions: NOT FOR COMMITTING add sessions testing

drivers/char/tpm/Kconfig  |3 +
drivers/char/tpm/Makefile |3 +-
drivers/char/tpm/tpm-buf.c|  191 ++
drivers/char/tpm/tpm-chip.c   |1 +
drivers/char/tpm/tpm-sysfs.c  |   27 +-
drivers/char/tpm/tpm.h|  129 ++--
drivers/char/tpm/tpm2-cmd.c   |  248 ---
drivers/char/tpm/tpm2-sessions-test.c |  360 ++
drivers/char/tpm/tpm2-sessions.c  | 1188 +
drivers/char/tpm/tpm2-sessions.h  |   57 ++
10 files changed, 2027 insertions(+), 180 deletions(-)
create mode 100644 drivers/char/tpm/tpm-buf.c
create mode 100644 drivers/char/tpm/tpm2-sessions-test.c
create mode 100644 drivers/char/tpm/tpm2-sessions.c
create mode 100644 drivers/char/tpm/tpm2-sessions.h

--
2.16.4

Re: [PATCH v4 2/7] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling

[Jarkko Sakkinen]

On Mon, 22 Oct 2018, James Bottomley wrote:

This code adds true session based HMAC authentication plus parameter
decryption and response encryption using AES.


In order to reduce complexity it would make sense to split into two
commits: authentication and parameter encryption.



The basic design of this code is to segregate all the nasty crypto,
hash and hmac code into tpm2-sessions.c and export a usable API.

The API first of all starts off by gaining a session with

tpm2_start_auth_session()

Which initiates a session with the TPM and allocates an opaque
tpm2_auth structure to handle the session parameters.  Then the use is
simply:

* tpm_buf_append_name() in place of the tpm_buf_append_u32 for the
 handles


Do not understand the description of this function.



* tpm_buf_append_hmac_session() where tpm2_append_auth() would go


Another fuzzy description.



* tpm_buf_fill_hmac_session() called after the entire command buffer
 is finished but before tpm_transmit_cmd() is called which computes
 the correct HMAC and places it in the command at the correct
 location.


I would call this simply tpm_buf_insert_hmac(). It is clear and precise
name what it does.

These all sould be combined with the existing session stuff inside
tpm2-cmd.c and not have duplicate infrastructures. The file name should
be tpm2-session.c (we neither have tpm2-cmds.c).



Finally, after tpm_transmit_cmd() is called,
tpm_buf_check_hmac_response() is called to check that the returned
HMAC matched and collect the new state for the next use of the
session, if any.

The features of the session is controlled by the session attributes
set in tpm_buf_append_hmac_session().  If TPM2_SA_CONTINUE_SESSION is
not specified, the session will be flushed and the tpm2_auth structure
freed in tpm_buf_check_hmac_response(); otherwise the session may be
used again.  Parameter encryption is specified by or'ing the flag
TPM2_SA_DECRYPT and response encryption by or'ing the flag
TPM2_SA_ENCRYPT.  the various encryptions will be taken care of by
tpm_buf_fill_hmac_session() and tpm_buf_check_hmac_response()
respectively.

To get all of this to work securely, the Kernel now needs a primary
key to encrypt the session salt to, so we derive an EC key from the
NULL seed and store it in the tpm_chip structure.  We also make sure
that this seed remains for the kernel by using a kernel space to take
it out of the TPM when userspace wants to use it.

Signed-off-by: James Bottomley 

---

v2: Added docbook and improved response check API
v3: Add readpublic, fix hmac length, add API for close on error
   allow for the hmac session not being first in the sessions
v4: Make NULL seed template exactly match the SRK ECC template.
   Also check the NULL primary key name is what getpublic returns
   to prevent spoofing.  Also parametrise the name size for reuse
---
drivers/char/tpm/Kconfig |3 +
drivers/char/tpm/Makefile|2 +-
drivers/char/tpm/tpm.h   |   32 +
drivers/char/tpm/tpm2-cmd.c  |   34 +-
drivers/char/tpm/tpm2-sessions.c | 1188 ++
drivers/char/tpm/tpm2-sessions.h |   57 ++
6 files changed, 1300 insertions(+), 16 deletions(-)
create mode 100644 drivers/char/tpm/tpm2-sessions.c
create mode 100644 drivers/char/tpm/tpm2-sessions.h



tpm2-cmd.c changes should be in their own commit e.g.:

"tpm: add own space for in-kernel TPM communication"


diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 0aee88df98d1..8c714d8550c4 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -8,6 +8,9 @@ menuconfig TCG_TPM
select SECURITYFS
select CRYPTO
select CRYPTO_HASH_INFO
+   select CRYPTO_ECDH
+   select CRYPTO_AES
+   select CRYPTO_CFB
---help---
  If you have a TPM security chip in your system, which
  implements the Trusted Computing Group's specification,
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 65d165cce530..1b5f6ccbb86d 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -5,7 +5,7 @@
obj-$(CONFIG_TCG_TPM) += tpm.o
tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o \
 tpm-dev-common.o tpmrm-dev.o eventlog/common.o eventlog/tpm1.o \
-eventlog/tpm2.o tpm2-space.o tpm-buf.o
+eventlog/tpm2.o tpm2-space.o tpm-buf.o tpm2-sessions.o
tpm-$(CONFIG_ACPI) += tpm_ppi.o eventlog/acpi.o
tpm-$(CONFIG_EFI) += eventlog/efi.o
tpm-$(CONFIG_OF) += eventlog/of.o
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index d73701e36eba..d39065d9995d 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -42,6 +42,15 @@
#include 
#endif

+/* fixed define for the curve we use which is NIST_P256 */
+#define EC_PT_SZ   32
+
+/*
+ * fixed define for the size of a name.  This is actually HASHALG size
+ * plus 2, so 32 for SHA256
+ */
+#define TPM2_NAME_SIZE 34


Please, remove this definition completely. It only 

Re: [PATCH v4 1/7] tpm-buf: create new functions for handling TPM buffers

[Jarkko Sakkinen]

On Mon, 22 Oct 2018, James Bottomley wrote:

This separates out the old tpm_buf_... handling functions from static
inlines into tpm.h and makes them their own tpm-buf.c file.  It also
adds handling for tpm2b structures and also incremental pointer
advancing parsers.

Signed-off-by: James Bottomley 


Would also prefer simply "tpm:" tag e.g. (in all of the commits actually
in this series but I remark it just here).

"tpm: create new tpm_buf_functions for parsing and TPM2B"

/Jarkko

Re: [PATCH v4 1/7] tpm-buf: create new functions for handling TPM buffers

[Jarkko Sakkinen]

On Mon, 22 Oct 2018, James Bottomley wrote:

This separates out the old tpm_buf_... handling functions from static
inlines into tpm.h and makes them their own tpm-buf.c file.  It also
adds handling for tpm2b structures and also incremental pointer
advancing parsers.


Nitpicking: when my SGX patch set has been reviewed one of the comments
has been that commit messages should be in imperative form e.g.

"Separate out the old tpm_buf handling functions from static inlines
into tpm.h and make them their..."


Signed-off-by: James Bottomley 

---

v2: added this patch to separate out the API changes
v3: added tpm_buf_reset_cmd()
v4: renamed tpm_buf_reset_cmd() to tpm_buf_reset()
---
drivers/char/tpm/Makefile  |   2 +-
drivers/char/tpm/tpm-buf.c | 191 +
drivers/char/tpm/tpm.h |  96 ---
3 files changed, 208 insertions(+), 81 deletions(-)
create mode 100644 drivers/char/tpm/tpm-buf.c

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 3655258bee73..65d165cce530 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -5,7 +5,7 @@
obj-$(CONFIG_TCG_TPM) += tpm.o
tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o \
 tpm-dev-common.o tpmrm-dev.o eventlog/common.o eventlog/tpm1.o \
-eventlog/tpm2.o tpm2-space.o
+eventlog/tpm2.o tpm2-space.o tpm-buf.o
tpm-$(CONFIG_ACPI) += tpm_ppi.o eventlog/acpi.o
tpm-$(CONFIG_EFI) += eventlog/efi.o
tpm-$(CONFIG_OF) += eventlog/of.o
diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
new file mode 100644
index ..faa22bb09d99
--- /dev/null
+++ b/drivers/char/tpm/tpm-buf.c
@@ -0,0 +1,191 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Handing for tpm2b structures to facilitate the building of commands
+ */
+
+#include "tpm.h"
+
+#include 
+
+#include 
+
+static int __tpm_buf_init(struct tpm_buf *buf)
+{
+   buf->data_page = alloc_page(GFP_HIGHUSER);
+   if (!buf->data_page)
+   return -ENOMEM;
+
+   buf->flags = 0;
+   buf->data = kmap(buf->data_page);
+
+   return 0;
+}
+
+void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
+{
+   struct tpm_input_header *head;
+
+   head = (struct tpm_input_header *) buf->data;
+
+   head->tag = cpu_to_be16(tag);
+   head->length = cpu_to_be32(sizeof(*head));
+   head->ordinal = cpu_to_be32(ordinal);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_reset);
+
+int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
+{
+   int rc;
+
+   rc = __tpm_buf_init(buf);
+   if (rc)
+   return rc;
+
+   tpm_buf_reset(buf, tag, ordinal);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(tpm_buf_init);
+
+int tpm_buf_init_2b(struct tpm_buf *buf)
+{
+   struct tpm_input_header *head;
+   int rc;
+
+   rc = __tpm_buf_init(buf);
+   if (rc)
+   return rc;
+
+   head = (struct tpm_input_header *) buf->data;
+
+   head->length = cpu_to_be32(sizeof(*head));
+
+   buf->flags = TPM_BUF_2B;
+   return 0;
+}
+EXPORT_SYMBOL_GPL(tpm_buf_init_2b);
+
+void tpm_buf_destroy(struct tpm_buf *buf)
+{
+   kunmap(buf->data_page);
+   __free_page(buf->data_page);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_destroy);
+
+static void *tpm_buf_data(struct tpm_buf *buf)
+{
+   if (buf->flags & TPM_BUF_2B)
+   return buf->data + TPM_HEADER_SIZE;
+   return buf->data;
+}
+
+u32 tpm_buf_length(struct tpm_buf *buf)
+{
+   struct tpm_input_header *head = (struct tpm_input_header *)buf->data;
+   u32 len;
+
+   len = be32_to_cpu(head->length);
+   if (buf->flags & TPM_BUF_2B)
+   len -= sizeof(*head);
+   return len;
+}
+EXPORT_SYMBOL_GPL(tpm_buf_length);
+
+u16 tpm_buf_tag(struct tpm_buf *buf)
+{
+   struct tpm_input_header *head = (struct tpm_input_header *)buf->data;
+
+   return be16_to_cpu(head->tag);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_tag);
+
+void tpm_buf_append(struct tpm_buf *buf,
+   const unsigned char *new_data,
+   unsigned int new_len)
+{
+   struct tpm_input_header *head = (struct tpm_input_header *) buf->data;
+   u32 len = be32_to_cpu(head->length);
+
+   /* Return silently if overflow has already happened. */
+   if (buf->flags & TPM_BUF_OVERFLOW)
+   return;
+
+   if ((len + new_len) > PAGE_SIZE) {
+   WARN(1, "tpm_buf: overflow\n");
+   buf->flags |= TPM_BUF_OVERFLOW;
+   return;
+   }
+
+   memcpy(>data[len], new_data, new_len);
+   head->length = cpu_to_be32(len + new_len);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append);
+
+void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value)
+{
+   tpm_buf_append(buf, , 1);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append_u8);
+
+void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value)
+{
+   __be16 value2 = cpu_to_be16(value);
+
+   tpm_buf_append(buf, (u8 *) , 2);
+}

Re: [PATCH v3 0/6] add integrity and security to TPM2 transactions

[Jarkko Sakkinen]

On Mon, Mar 12, 2018 at 08:57:13AM -0700, James Bottomley wrote:
> I think the way I'm going to fix the trusted key policy problem is to
> move it back into the kernel for the simple PCR lock policy (which will
> make changing from 1.2 to 2.0 seamless because the external Key API
> will then become the same) so the kernel gets the missing TPM nonce and
> can then do TPM2_PolicyAuthValue.

Sounds reasonable.

> User generated policy sessions for trusted keys are very flexible but
> also a hugely bad idea for consumers because it's so different from the
> way 1.2 works and it means now the user has to exercise a TPM API to
> produce the policy sessions.
> 
> Longer term, I think having a particular trusted key represent a policy
> session which can then be attached to a different trusted key
> representing the blob is the best idea because we can expose the policy
> build up via the trusted key API and keep all the TPM nastiness inside
> the kernel.

/Jarkko

Re: [PATCH v3 1/6] tpm-buf: create new functions for handling TPM buffers

[Jarkko Sakkinen]

On Fri, 2018-03-16 at 13:58 +0200, Jarkko Sakkinen wrote:
> On Sat, 2018-03-10 at 14:14 -0800, James Bottomley wrote:
> > TPM_BUF_OVERFLOW= BIT(0),
> > +   TPM_BUF_2B  = BIT(1),
> 
> Instead of re-using this I would prefer to have another enum for
> buffer type. tpm_buf_init() could have the signature:
> 
> int tpm_buf_init(unsigned int type);
> 
> For commands there should be a function:
> 
> void tpm_buf_set_command_header(struct tpm_buf *buf, u16 tag, u32 ordinal);
> 
> And tpm_buf_append_2b() should not exist at all. It should be
> maintained automatically by other append commands.

Can you send the next version this patch as a separate entity? Once
I can land this we have kind of stable ground for the following
patches. After that it is easier test and review them.

/Jarkko

Re: [PATCH v3 1/6] tpm-buf: create new functions for handling TPM buffers

[Jarkko Sakkinen]

On Sat, 2018-03-10 at 14:14 -0800, James Bottomley wrote:
>   TPM_BUF_OVERFLOW= BIT(0),
> + TPM_BUF_2B  = BIT(1),

Instead of re-using this I would prefer to have another enum for
buffer type. tpm_buf_init() could have the signature:

int tpm_buf_init(unsigned int type);

For commands there should be a function:

void tpm_buf_set_command_header(struct tpm_buf *buf, u16 tag, u32 ordinal);

And tpm_buf_append_2b() should not exist at all. It should be
maintained automatically by other append commands.

/Jarkko

Re: [RFC 0/5] add integrity and security to TPM2 transactions

[Jarkko Sakkinen]

On Sat, 2018-03-10 at 10:29 -0800, James Bottomley wrote:
> OK, you might want to wait for v3 then.  I've got it working with
> sealed (trusted) keys, well except for a problem with the trusted keys
> API that means we can't protect the password for policy based keys.  I
> think the API is finally complete, so I'll send v3 as a PATCH not an
> RFC.
> 
> The point of the last patch is to show the test rig for this I'm
> running in a VM using an instrumented tpm2 emulator to prove we're
> getting all the correct data in and out (and that the encryption and
> hmac are working); more physical TPM testing would be useful ..

Sorry, I did not notice this email in my inbox before I responded
to you v3 cover letter :-) Thank you.

/Jarkko

Re: [PATCH v3 0/6] add integrity and security to TPM2 transactions

[Jarkko Sakkinen]

On Sat, 2018-03-10 at 14:13 -0800, James Bottomley wrote:
> By now, everybody knows we have a problem with the TPM2_RS_PW easy
> button on TPM2 in that transactions on the TPM bus can be intercepted
> and altered.  The way to fix this is to use real sessions for HMAC
> capabilities to ensure integrity and to use parameter and response
> encryption to ensure confidentiality of the data flowing over the TPM
> bus.
> 
> This patch series is about adding a simple API which can ensure the
> above properties as a layered addition to the existing TPM handling
> code.  This series now includes protections for PCR extend, getting
> random numbers from the TPM and data sealing and unsealing.  It
> therefore eliminates all uses of TPM2_RS_PW in the kernel and adds
> encryption protection to sensitive data flowing into and out of the
> TPM.
> 
> This series is also dependent on additions to the crypto subsystem to
> fix problems in the elliptic curve key handling and add the Cipher
> FeedBack encryption scheme:
> 
> https://marc.info/?l=linux-crypto-vger=151994371015475
> 
> In the third version I've added data sealing and unsealing protection,
> apart from one API based problem which means that the way trusted keys
> were protected it's not currently possible to HMAC protect an authority
> that comes with a policy, so the API will have to be extended to fix
> that case
> 
> I've verified this using the test suite in the last patch on a VM
> connected to a tpm2 emulator.  I also instrumented the emulator to make
> sure the sensitive data was properly encrypted.
> 
> James

1. Can I ignore v2 and just review/test this version? I haven't even
   peeked into v2 yet.
2. Do you know in which kernel version will the crypto additions land?

/Jarkko

Re: [RFC 0/5] add integrity and security to TPM2 transactions

[Jarkko Sakkinen]

On Wed, 2018-03-07 at 15:29 -0800, James Bottomley wrote:
> By now, everybody knows we have a problem with the TPM2_RS_PW easy
> button on TPM2 in that transactions on the TPM bus can be intercepted
> and altered.  The way to fix this is to use real sessions for HMAC
> capabilities to ensure integrity and to use parameter and response
> encryption to ensure confidentiality of the data flowing over the TPM
> bus.
> 
> This RFC is about adding a simple API which can ensure the above
> properties as a layered addition to the existing TPM handling code.
>  Eventually we can add this to the random number generator, the PCR
> extensions and the trusted key handling, but this all depends on the
> conversion to tpm_buf which is not yet upstream, so I've constructed a
> second patch which demonstrates the new API in a test module for those
> who wish to play with it.
> 
> This series is also dependent on additions to the crypto subsystem to
> fix problems in the elliptic curve key handling and add the Cipher
> FeedBack encryption scheme:
> 
> https://marc.info/?l=linux-crypto-vger=151994371015475
> 
> In the second version, I added security HMAC to our PCR extend and
> encryption to the returned random number generators and also extracted
> the parsing and tpm2b construction API into a new file.
> 
> James

Might take up until end of next week before I have time to try this out.
Anyway, I'll see if I get this running on my systems before at the code
that much.

/Jarkko

Re: [PATCH 1/2] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling

[Jarkko Sakkinen]

On Mon, Mar 05, 2018 at 06:58:32AM -0800, James Bottomley wrote:
> On Mon, 2018-03-05 at 13:35 +0200, Jarkko Sakkinen wrote:
> > On Fri, Mar 02, 2018 at 10:06:15PM -0800, James Bottomley wrote:
> > > 
> > > diff --git a/drivers/char/tpm/tpm2b.h b/drivers/char/tpm/tpm2b.h
> > > new file mode 100644
> > > index ..c7726f2895aa
> > > --- /dev/null
> > > +++ b/drivers/char/tpm/tpm2b.h
> > > @@ -0,0 +1,82 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#ifndef _TPM2_TPM2B_H
> > > +#define _TPM2_TPM2B_H
> > > +/*
> > > + * Handing for tpm2b structures to facilitate the building of
> > > commands
> > > + */
> > > +
> > > +#include "tpm.h"
> > > +
> > > +#include 
> > > +
> > > +struct tpm2b {
> > > + struct tpm_buf buf;
> > > +};
> > > +
> > > +/* opaque structure, holds auth session parameters like the
> > > session key */
> > > +struct tpm2_auth;
> > > +
> > > +static inline int tpm2b_init(struct tpm2b *buf)
> > > +{
> > > + return tpm_buf_init(>buf, 0, 0);
> > > +}
> > > +
> > > +static inline void tpm2b_reset(struct tpm2b *buf)
> > > +{
> > > + struct tpm_input_header *head;
> > > +
> > > + head = (struct tpm_input_header *)buf->buf.data;
> > > + head->length = cpu_to_be32(sizeof(*head));
> > > +}
> > > +
> > > +static inline void tpm2b_append(struct tpm2b *buf, const unsigned
> > > char *data,
> > > + unsigned int len)
> > > +{
> > > + tpm_buf_append(>buf, data, len);
> > > +}
> > > +
> > > +#define TPM2B_APPEND(type) \
> > > + static inline void tpm2b_append_##type(struct tpm2b *buf,
> > > const type value) { tpm_buf_append_##type(>buf, value); }
> > > +
> > > +TPM2B_APPEND(u8)
> > > +TPM2B_APPEND(u16)
> > > +TPM2B_APPEND(u32)
> > > +
> > > +static inline void *tpm2b_buffer(const struct tpm2b *buf)
> > > +{
> > > + return buf->buf.data + sizeof(struct tpm_input_header);
> > > +}
> > > +
> > > +static inline u16 tpm2b_len(struct tpm2b *buf)
> > > +{
> > > + return tpm_buf_length(>buf) - sizeof(struct
> > > tpm_input_header);
> > > +}
> > > +
> > > +static inline void tpm2b_destroy(struct tpm2b *buf)
> > > +{
> > > + tpm_buf_destroy(>buf);
> > > +}
> > > +
> > > +static inline void tpm_buf_append_2b(struct tpm_buf *buf, struct
> > > tpm2b *tpm2b)
> > > +{
> > > + u16 len = tpm2b_len(tpm2b);
> > > +
> > > + tpm_buf_append_u16(buf, len);
> > > + tpm_buf_append(buf, tpm2b_buffer(tpm2b), len);
> > > + /* clear the buf for reuse */
> > > + tpm2b_reset(tpm2b);
> > > +}
> > > +
> > > +/* Macros for unmarshalling known size BE data */
> > > +#define GET_INC(type)\
> > > +static inline u##type get_inc_##type(const u8 **ptr) {   \
> > > + u##type val;\
> > > + val = get_unaligned_be##type(*ptr); \
> > > + *ptr += sizeof(val);\
> > > + return val; \
> > > +}
> > > +
> > > +GET_INC(16)
> > > +GET_INC(32)
> > > +
> > > +#endif
> > > -- 
> > > 2.12.3
> > > 
> > 
> > Some meta stuff:
> > 
> > * Add me to TO-field because I should probably review and eventually
> >   test these, right?
> 
> Eventually; they're an RFC because we need to get the API right first
> (I've already got a couple of fixes to it).

For me the big picture looks good. You saw my comments about details.
Refine those and I think this would already be digestable change.

> > * CC to linux-security-module
> 
> There's no change to anything in security module, so why?  All security
> module people who care about the TPM should be on linux-integrity and
> those who don't likely don't want to see the changes.  The reason
> linux-crypto is on the cc is because I want them to make sure I'm using
> their crypto system correctly.

See:

https://kernsec.org/wiki/index.php/Linux_Kernel_Integrity

The big changes that affect the whole security tree in direct or
indirect ways should go through that list. This was a wish from
James Morris.

> 
> > * Why there is no RFC tag given that these are so quite large
> > changes?
> 
> There is an RFC tag on 0/2

Ah, sorry, so it is.

> > * Why in hell tpm2b.h?
> 
> Because all sized TPM structures are in 2B form and manipulating them
> can be made a lot easier with helper routines.

I see it now that I looked the code in more detail.

Suggestions to move forward:

* Add enum tpm_buf_type { TPM_BUF_COMMAND, TPM_BUF_2B } and use
  struct tpm_buf for both.
* Move tpm_buf_* stuff from tpm.h and tpm2-cmd.c to tpm_buf_*.[ch].

I would even suggest to move current inline functions to tpm_buf.c
so that they can be traced. Performance impact is neglible but
tracing is an useful asset for testing.

For get_inc* I would just roll out two separate functions as the
redudancy is quite neglibe. I just want to keep things as simple
and trivial as possible.

> James

/Jarkko

Re: [PATCH 1/2] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling

[Jarkko Sakkinen]

On Mon, Mar 05, 2018 at 01:35:33PM +0200, Jarkko Sakkinen wrote:
> On Fri, Mar 02, 2018 at 10:06:15PM -0800, James Bottomley wrote:
> > diff --git a/drivers/char/tpm/tpm2b.h b/drivers/char/tpm/tpm2b.h
> > new file mode 100644
> > index ..c7726f2895aa
> > --- /dev/null
> > +++ b/drivers/char/tpm/tpm2b.h
> > @@ -0,0 +1,82 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _TPM2_TPM2B_H
> > +#define _TPM2_TPM2B_H
> > +/*
> > + * Handing for tpm2b structures to facilitate the building of commands
> > + */
> > +
> > +#include "tpm.h"
> > +
> > +#include 
> > +
> > +struct tpm2b {
> > +   struct tpm_buf buf;
> > +};
> > +
> > +/* opaque structure, holds auth session parameters like the session key */
> > +struct tpm2_auth;
> > +
> > +static inline int tpm2b_init(struct tpm2b *buf)
> > +{
> > +   return tpm_buf_init(>buf, 0, 0);
> > +}
> > +
> > +static inline void tpm2b_reset(struct tpm2b *buf)
> > +{
> > +   struct tpm_input_header *head;
> > +
> > +   head = (struct tpm_input_header *)buf->buf.data;
> > +   head->length = cpu_to_be32(sizeof(*head));
> > +}
> > +
> > +static inline void tpm2b_append(struct tpm2b *buf, const unsigned char 
> > *data,
> > +   unsigned int len)
> > +{
> > +   tpm_buf_append(>buf, data, len);
> > +}
> > +
> > +#define TPM2B_APPEND(type) \
> > +   static inline void tpm2b_append_##type(struct tpm2b *buf, const type 
> > value) { tpm_buf_append_##type(>buf, value); }
> > +
> > +TPM2B_APPEND(u8)
> > +TPM2B_APPEND(u16)
> > +TPM2B_APPEND(u32)
> > +
> > +static inline void *tpm2b_buffer(const struct tpm2b *buf)
> > +{
> > +   return buf->buf.data + sizeof(struct tpm_input_header);
> > +}
> > +
> > +static inline u16 tpm2b_len(struct tpm2b *buf)
> > +{
> > +   return tpm_buf_length(>buf) - sizeof(struct tpm_input_header);
> > +}
> > +
> > +static inline void tpm2b_destroy(struct tpm2b *buf)
> > +{
> > +   tpm_buf_destroy(>buf);
> > +}
> > +
> > +static inline void tpm_buf_append_2b(struct tpm_buf *buf, struct tpm2b 
> > *tpm2b)
> > +{
> > +   u16 len = tpm2b_len(tpm2b);
> > +
> > +   tpm_buf_append_u16(buf, len);
> > +   tpm_buf_append(buf, tpm2b_buffer(tpm2b), len);
> > +   /* clear the buf for reuse */
> > +   tpm2b_reset(tpm2b);
> > +}
> > +
> > +/* Macros for unmarshalling known size BE data */
> > +#define GET_INC(type)  \
> > +static inline u##type get_inc_##type(const u8 **ptr) { \
> > +   u##type val;\
> > +   val = get_unaligned_be##type(*ptr); \
> > +   *ptr += sizeof(val);\
> > +   return val; \
> > +}
> > +
> > +GET_INC(16)
> > +GET_INC(32)
> > +
> > +#endif
> > -- 
> > 2.12.3
> > 
> 
> Some meta stuff:
> 
> * Add me to TO-field because I should probably review and eventually
>   test these, right?
> * CC to linux-security-module
> * Why there is no RFC tag given that these are so quite large changes?
> * Why in hell tpm2b.h?
> 
> /Jarkko

Some other large scale level stuff that I saw:

* Do not document functions in the file header.
* Make the stuff in the header (expect functio descriptions) as
  documentation block:
  https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
  If you don't want to do this, then just move the documentation to
  the commit message. I rather do not have the documentation at all
  in the files than have it in unmaintanable form.
* Create structs for complex things that you add inside TPM commands
  and declare these helpers right before the function and add them
  with tpm_buf_append(). A good metric for this is when you see your
  self adding field names as a comment. This should make these patches
  a factor cleaner. We use this approach in some places such as
  tpm2_get_pcr_allocation().
* Please, oh please get rid of this tpm2b.h. It is a definitive NAK (OK
  I said it already above but cannot really over emphasize it).
* Short summary should be "tpm: add encrypted and integrity protected
  sessions" or something along the lines.

I guess this is enough for first review round?

/Jarkko

Re: [PATCH 1/2] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling

[Jarkko Sakkinen]

On Fri, Mar 02, 2018 at 10:06:15PM -0800, James Bottomley wrote:
> diff --git a/drivers/char/tpm/tpm2b.h b/drivers/char/tpm/tpm2b.h
> new file mode 100644
> index ..c7726f2895aa
> --- /dev/null
> +++ b/drivers/char/tpm/tpm2b.h
> @@ -0,0 +1,82 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _TPM2_TPM2B_H
> +#define _TPM2_TPM2B_H
> +/*
> + * Handing for tpm2b structures to facilitate the building of commands
> + */
> +
> +#include "tpm.h"
> +
> +#include 
> +
> +struct tpm2b {
> + struct tpm_buf buf;
> +};
> +
> +/* opaque structure, holds auth session parameters like the session key */
> +struct tpm2_auth;
> +
> +static inline int tpm2b_init(struct tpm2b *buf)
> +{
> + return tpm_buf_init(>buf, 0, 0);
> +}
> +
> +static inline void tpm2b_reset(struct tpm2b *buf)
> +{
> + struct tpm_input_header *head;
> +
> + head = (struct tpm_input_header *)buf->buf.data;
> + head->length = cpu_to_be32(sizeof(*head));
> +}
> +
> +static inline void tpm2b_append(struct tpm2b *buf, const unsigned char *data,
> + unsigned int len)
> +{
> + tpm_buf_append(>buf, data, len);
> +}
> +
> +#define TPM2B_APPEND(type) \
> + static inline void tpm2b_append_##type(struct tpm2b *buf, const type 
> value) { tpm_buf_append_##type(>buf, value); }
> +
> +TPM2B_APPEND(u8)
> +TPM2B_APPEND(u16)
> +TPM2B_APPEND(u32)
> +
> +static inline void *tpm2b_buffer(const struct tpm2b *buf)
> +{
> + return buf->buf.data + sizeof(struct tpm_input_header);
> +}
> +
> +static inline u16 tpm2b_len(struct tpm2b *buf)
> +{
> + return tpm_buf_length(>buf) - sizeof(struct tpm_input_header);
> +}
> +
> +static inline void tpm2b_destroy(struct tpm2b *buf)
> +{
> + tpm_buf_destroy(>buf);
> +}
> +
> +static inline void tpm_buf_append_2b(struct tpm_buf *buf, struct tpm2b 
> *tpm2b)
> +{
> + u16 len = tpm2b_len(tpm2b);
> +
> + tpm_buf_append_u16(buf, len);
> + tpm_buf_append(buf, tpm2b_buffer(tpm2b), len);
> + /* clear the buf for reuse */
> + tpm2b_reset(tpm2b);
> +}
> +
> +/* Macros for unmarshalling known size BE data */
> +#define GET_INC(type)\
> +static inline u##type get_inc_##type(const u8 **ptr) {   \
> + u##type val;\
> + val = get_unaligned_be##type(*ptr); \
> + *ptr += sizeof(val);\
> + return val; \
> +}
> +
> +GET_INC(16)
> +GET_INC(32)
> +
> +#endif
> -- 
> 2.12.3
> 

Some meta stuff:

* Add me to TO-field because I should probably review and eventually
  test these, right?
* CC to linux-security-module
* Why there is no RFC tag given that these are so quite large changes?
* Why in hell tpm2b.h?

/Jarkko

Re: [PATCH v2] tpm: Move Linux RNG connection to hwrng

[Jarkko Sakkinen]

On Tue, Jan 30, 2018 at 10:52:13PM +1100, James Morris wrote:
> On Tue, 30 Jan 2018, Jarkko Sakkinen wrote:
> 
> > On Sat, Jan 27, 2018 at 12:20:18PM +0530, PrasannaKumar Muralidharan wrote:
> > > Hi Jarkko,
> > > 
> > > On 17 November 2017 at 19:27, Jarkko Sakkinen
> > > <jarkko.sakki...@linux.intel.com> wrote:
> > > > On Fri, Nov 17, 2017 at 03:28:53PM +0200, Jarkko Sakkinen wrote:
> > > >
> > > > At least signed-off-by from PrassanaKumar is missing from the 2nd
> > > > commit. I'll add it.
> > > 
> > > I had the impression that my signed-off-by will be present in this
> > > change. But it is missing in [1]. Is it supposed to be that way?
> > > 
> > > 1. 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=6e592a065d51d26f9d62b8b7501a5114076af8b4
> > > 
> > > Thanks,
> > > PrasannaKumar
> > 
> > Yes, it would be senseful.
> > 
> > James, would it still be possible to amend this tag to security tree?
> 
> Nope, it's been pushed to Linus.

Damn. Well, good that Tested-by is there. I'm sorry about this.

/Jarkko

Re: [PATCH v2] tpm: Move Linux RNG connection to hwrng

[Jarkko Sakkinen]

On Sat, Jan 27, 2018 at 12:20:18PM +0530, PrasannaKumar Muralidharan wrote:
> Hi Jarkko,
> 
> On 17 November 2017 at 19:27, Jarkko Sakkinen
> <jarkko.sakki...@linux.intel.com> wrote:
> > On Fri, Nov 17, 2017 at 03:28:53PM +0200, Jarkko Sakkinen wrote:
> >
> > At least signed-off-by from PrassanaKumar is missing from the 2nd
> > commit. I'll add it.
> 
> I had the impression that my signed-off-by will be present in this
> change. But it is missing in [1]. Is it supposed to be that way?
> 
> 1. 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=6e592a065d51d26f9d62b8b7501a5114076af8b4
> 
> Thanks,
> PrasannaKumar

Yes, it would be senseful.

James, would it still be possible to amend this tag to security tree?

/Jarkko

Re: [PATCH v3] tpm: use struct tpm_chip for tpm_chip_find_get()

[Jarkko Sakkinen]

On Wed, Jan 17, 2018 at 07:43:51PM +0530, PrasannaKumar Muralidharan wrote:
> Hi Jarkko,
> 
> On 14 November 2017 at 20:02, Jarkko Sakkinen
> <jarkko.sakki...@linux.intel.com> wrote:
> > On Sun, Nov 12, 2017 at 10:53:35AM +0530, PrasannaKumar Muralidharan wrote:
> >> Did basic check on tpm rng patch, it works fine. As it depends on this
> >> patch this should be working fine too.
> >>
> >> Tested-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
> >>
> >> Regards,
> >> PrasannaKumar
> >
> > Thank you.
> >
> > /Jarkko
> 
> Wondering what happened to this and tpm rng patch. Is there something
> more to do for this to get merged?
> 
> Thanks,
> PrasannaKumar

Was part of 4.6 PR.

/Jarkko

Re: [PATCH v2] tpm: Move Linux RNG connection to hwrng

[Jarkko Sakkinen]

On Fri, Nov 17, 2017 at 03:28:53PM +0200, Jarkko Sakkinen wrote:
> On Tue, Nov 14, 2017 at 04:34:21PM +0200, Jarkko Sakkinen wrote:
> > On Tue, Nov 07, 2017 at 09:04:04AM -0700, Jason Gunthorpe wrote:
> > > On Tue, Nov 07, 2017 at 08:50:44AM +0530, PrasannaKumar Muralidharan 
> > > wrote:
> > > 
> > > > I am assuming you are talking about the following patches - using
> > > > struct tpm_chip instead of chip number and this patch.
> > > 
> > > yes
> > > 
> > > > I won't be able to test if struct tpm_chip usage as I don't have
> > > > multiple tpm hw in one machine. In case of tpm rng changes I can test
> > > > only the lifecycle of tpm rng device. Is that enough? I feel my test
> > > > will be limited. Please provide your thoughts on this.
> > > 
> > > That is certainly better than no testing.
> > > 
> > > Jason
> > 
> > WFM too.
> > 
> > Tested-by: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>
> > 
> > /Jarkko
> 
> I applied these commits. Please check that everything is correct as
> I had to do manual work with the 2nd commit.
> 
> /Jarkko

At least signed-off-by from PrassanaKumar is missing from the 2nd
commit. I'll add it.

/Jarkko

Re: [PATCH v2] tpm: Move Linux RNG connection to hwrng

[Jarkko Sakkinen]

On Tue, Nov 14, 2017 at 04:34:21PM +0200, Jarkko Sakkinen wrote:
> On Tue, Nov 07, 2017 at 09:04:04AM -0700, Jason Gunthorpe wrote:
> > On Tue, Nov 07, 2017 at 08:50:44AM +0530, PrasannaKumar Muralidharan wrote:
> > 
> > > I am assuming you are talking about the following patches - using
> > > struct tpm_chip instead of chip number and this patch.
> > 
> > yes
> > 
> > > I won't be able to test if struct tpm_chip usage as I don't have
> > > multiple tpm hw in one machine. In case of tpm rng changes I can test
> > > only the lifecycle of tpm rng device. Is that enough? I feel my test
> > > will be limited. Please provide your thoughts on this.
> > 
> > That is certainly better than no testing.
> > 
> > Jason
> 
> WFM too.
> 
> Tested-by: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>
> 
> /Jarkko

I applied these commits. Please check that everything is correct as
I had to do manual work with the 2nd commit.

/Jarkko

Re: [PATCH v2] tpm: Move Linux RNG connection to hwrng

[Jarkko Sakkinen]

On Tue, Nov 07, 2017 at 09:04:04AM -0700, Jason Gunthorpe wrote:
> On Tue, Nov 07, 2017 at 08:50:44AM +0530, PrasannaKumar Muralidharan wrote:
> 
> > I am assuming you are talking about the following patches - using
> > struct tpm_chip instead of chip number and this patch.
> 
> yes
> 
> > I won't be able to test if struct tpm_chip usage as I don't have
> > multiple tpm hw in one machine. In case of tpm rng changes I can test
> > only the lifecycle of tpm rng device. Is that enough? I feel my test
> > will be limited. Please provide your thoughts on this.
> 
> That is certainly better than no testing.
> 
> Jason

WFM too.

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

/Jarkko

Re: [PATCH v3] tpm: use struct tpm_chip for tpm_chip_find_get()

[Jarkko Sakkinen]

On Sun, Nov 12, 2017 at 10:53:35AM +0530, PrasannaKumar Muralidharan wrote:
> Did basic check on tpm rng patch, it works fine. As it depends on this
> patch this should be working fine too.
> 
> Tested-by: PrasannaKumar Muralidharan 
> 
> Regards,
> PrasannaKumar

Thank you.

/Jarkko

Re: [PATCH v2] tpm: Move Linux RNG connection to hwrng

[Jarkko Sakkinen]

On Sun, Nov 05, 2017 at 07:27:04PM -0700, Jason Gunthorpe wrote:
> On Sun, Nov 05, 2017 at 01:05:06PM +0200, Jarkko Sakkinen wrote:
> 
> > I asked to create a series for a reason. Now this doesn't apply because I
> > don't have an ancestor in my git history.
> 
> It would be unusual for me to put your patch into a series unless I am
> also adopting it. eg what happens if there are more comments on it?
> 
> Also, I wasn't sure what branch your patch was against since my tree
> didn't have history for it either..
> 
> Sometimes the maintainer has to sort stuff like this out... :)
> 
> > Please resend as series together with my patch. I can apply neither yet
> > because they have zero tested-by's.
> 
> Hopefully PrasannaKumar can test both patches.
> 
> Jason

Fair enough. I'll look at it.

/Jarkko

Re: [PATCH v2] tpm: Move Linux RNG connection to hwrng

[Jarkko Sakkinen]

On Tue, Oct 31, 2017 at 02:05:03PM -0600, Jason Gunthorpe wrote:
> The tpm-rng.c approach is completely inconsistent with how the kernel
> handles hotplug. Instead manage a hwrng device for each TPM. This will
> cause the kernel to read entropy from the TPM when it is plugged in,
> and allow access to the TPM rng via /dev/hwrng.
> 
> Signed-off-by: PrasannaKumar Muralidharan 
> Signed-off-by: Jason Gunthorpe 
> ---
>  drivers/char/hw_random/Kconfig   | 13 ---
>  drivers/char/hw_random/Makefile  |  1 -
>  drivers/char/hw_random/tpm-rng.c | 50 
> 
>  drivers/char/tpm/Kconfig | 11 +
>  drivers/char/tpm/tpm-chip.c  | 41 
>  drivers/char/tpm/tpm.h   |  4 
>  6 files changed, 52 insertions(+), 68 deletions(-)
>  delete mode 100644 drivers/char/hw_random/tpm-rng.c
> 
> v2 applies against Jarkko's patch
> "tpm: use struct tpm_chip for tpm_chip_find_get()"
> 
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 95a031e9eced07..a20fed182cbcce 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -306,19 +306,6 @@ config HW_RANDOM_POWERNV
>  
> If unsure, say Y.
>  
> -config HW_RANDOM_TPM
> - tristate "TPM HW Random Number Generator support"
> - depends on TCG_TPM
> - default HW_RANDOM
> - ---help---
> -   This driver provides kernel-side support for the Random Number
> -   Generator in the Trusted Platform Module
> -
> -   To compile this driver as a module, choose M here: the
> -   module will be called tpm-rng.
> -
> -   If unsure, say Y.
> -
>  config HW_RANDOM_HISI
>   tristate "Hisilicon Random Number Generator support"
>   depends on HW_RANDOM && ARCH_HISI
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index 39a67defac67cb..91cb8e8213e7c1 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -26,7 +26,6 @@ obj-$(CONFIG_HW_RANDOM_NOMADIK) += nomadik-rng.o
>  obj-$(CONFIG_HW_RANDOM_PSERIES) += pseries-rng.o
>  obj-$(CONFIG_HW_RANDOM_POWERNV) += powernv-rng.o
>  obj-$(CONFIG_HW_RANDOM_HISI) += hisi-rng.o
> -obj-$(CONFIG_HW_RANDOM_TPM) += tpm-rng.o
>  obj-$(CONFIG_HW_RANDOM_BCM2835) += bcm2835-rng.o
>  obj-$(CONFIG_HW_RANDOM_IPROC_RNG200) += iproc-rng200.o
>  obj-$(CONFIG_HW_RANDOM_MSM) += msm-rng.o
> diff --git a/drivers/char/hw_random/tpm-rng.c 
> b/drivers/char/hw_random/tpm-rng.c
> deleted file mode 100644
> index c5e363825af008..00
> --- a/drivers/char/hw_random/tpm-rng.c
> +++ /dev/null
> @@ -1,50 +0,0 @@
> -/*
> - * Copyright (C) 2012 Kent Yoder IBM Corporation
> - *
> - * HWRNG interfaces to pull RNG data from a TPM
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - * GNU General Public License for more details.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, write to the Free Software
> - * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
> - */
> -
> -#include 
> -#include 
> -#include 
> -
> -#define MODULE_NAME "tpm-rng"
> -
> -static int tpm_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> -{
> - return tpm_get_random(NULL, data, max);
> -}
> -
> -static struct hwrng tpm_rng = {
> - .name = MODULE_NAME,
> - .read = tpm_rng_read,
> -};
> -
> -static int __init rng_init(void)
> -{
> - return hwrng_register(_rng);
> -}
> -module_init(rng_init);
> -
> -static void __exit rng_exit(void)
> -{
> - hwrng_unregister(_rng);
> -}
> -module_exit(rng_exit);
> -
> -MODULE_LICENSE("GPL v2");
> -MODULE_AUTHOR("Kent Yoder ");
> -MODULE_DESCRIPTION("RNG driver for TPM devices");
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index a30352202f1fdc..18c81cbe4704ca 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -26,6 +26,17 @@ menuconfig TCG_TPM
>  
>  if TCG_TPM
>  
> +config HW_RANDOM_TPM
> + bool "TPM HW Random Number Generator support"
> + depends on TCG_TPM && HW_RANDOM && !(TCG_TPM=y && HW_RANDOM=m)
> + default y
> + ---help---
> +   This setting exposes the TPM's Random Number Generator as a hwrng
> +   device. This allows the kernel to collect randomness from the TPM at
> +   boot, and provides the TPM randomines in /dev/hwrng.
> +
> +   If unsure, say Y.
> +
>  config TCG_TIS_CORE
>   tristate
>   ---help---
> diff --git 

Re: [PATCH v3] tpm: use struct tpm_chip for tpm_chip_find_get()

[Jarkko Sakkinen]

On Thu, Oct 26, 2017 at 07:40:49PM +0530, PrasannaKumar Muralidharan wrote:
> Hi Jarkko,
> 
> On 26 October 2017 at 19:24, Jarkko Sakkinen
> <jarkko.sakki...@linux.intel.com> wrote:
> > Device number (the character device index) is not a stable identifier
> > for a TPM chip. That is the reason why every call site passes
> > TPM_ANY_NUM to tpm_chip_find_get().
> >
> > This commit changes the API in a way that instead a struct tpm_chip
> > instance is given and NULL means the default chip. In addition, this
> > commit refines the documentation to be up to date with the
> > implementation.
> >
> > Suggested-by: Jason Gunthorpe <jguntho...@obsidianresearch.com> (@chip_num 
> > -> @chip)
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>
> > ---
> > v3:
> > * If chip != NULL was given to tpm_chip_find_get(), it tried to reserve chip
> >   associated with the device number 0 instead of trying to reserve the given
> >   chip instance.
> > v2:
> > * Further defined function documentation.
> > * Changed @chip_num to @chip instead of removing the parameter as suggested 
> > by
> >   Jason Gunthorpe.
> >  drivers/char/hw_random/tpm-rng.c|   2 +-
> >  drivers/char/tpm/tpm-chip.c |  24 ---
> >  drivers/char/tpm/tpm-interface.c| 135 
> > +++-
> >  drivers/char/tpm/tpm.h  |   2 +-
> >  include/linux/tpm.h |  38 +-
> >  security/integrity/ima/ima_crypto.c |   2 +-
> >  security/integrity/ima/ima_init.c   |   2 +-
> >  security/integrity/ima/ima_queue.c  |   2 +-
> >  security/keys/trusted.c |  35 +-
> >  9 files changed, 126 insertions(+), 116 deletions(-)
> >
> > diff --git a/drivers/char/hw_random/tpm-rng.c 
> > b/drivers/char/hw_random/tpm-rng.c
> > index d6d448266f07..c5e363825af0 100644
> > --- a/drivers/char/hw_random/tpm-rng.c
> > +++ b/drivers/char/hw_random/tpm-rng.c
> > @@ -25,7 +25,7 @@
> >
> >  static int tpm_rng_read(struct hwrng *rng, void *data, size_t max, bool 
> > wait)
> >  {
> > -   return tpm_get_random(TPM_ANY_NUM, data, max);
> > +   return tpm_get_random(NULL, data, max);
> >  }
> >
> >  static struct hwrng tpm_rng = {
> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > index a114e8f7fb90..bab9c14e040c 100644
> > --- a/drivers/char/tpm/tpm-chip.c
> > +++ b/drivers/char/tpm/tpm-chip.c
> > @@ -81,21 +81,26 @@ void tpm_put_ops(struct tpm_chip *chip)
> >  EXPORT_SYMBOL_GPL(tpm_put_ops);
> >
> >  /**
> > - * tpm_chip_find_get() - return tpm_chip for a given chip number
> > - * @chip_num: id to find
> > + * tpm_chip_find_get() - find and reserve a TPM chip
> > + * @chip:  a  tpm_chip instance, %NULL for the default chip
> >   *
> > - * The return'd chip has been tpm_try_get_ops'd and must be released via
> > - * tpm_put_ops
> > + * Finds a TPM chip and reserves its class device and operations. The chip 
> > must
> > + * be released with tpm_chip_put_ops() after use.
> > + *
> > + * Return:
> > + * A reserved  tpm_chip instance.
> > + * %NULL if a chip is not found.
> > + * %NULL if the chip is not available.
> >   */
> > -struct tpm_chip *tpm_chip_find_get(int chip_num)
> > +struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip)
> >  {
> > -   struct tpm_chip *chip, *res = NULL;
> > +   struct tpm_chip *res = NULL;
> > +   int chip_num = 0;
> > int chip_prev;
> >
> > mutex_lock(_lock);
> >
> > -   if (chip_num == TPM_ANY_NUM) {
> > -   chip_num = 0;
> > +   if (!chip) {
> > do {
> > chip_prev = chip_num;
> > chip = idr_get_next(_nums_idr, _num);
> > @@ -105,8 +110,7 @@ struct tpm_chip *tpm_chip_find_get(int chip_num)
> > }
> > } while (chip_prev != chip_num);
> > } else {
> > -   chip = idr_find(_nums_idr, chip_num);
> > -   if (chip && !tpm_try_get_ops(chip))
> > +   if (!tpm_try_get_ops(chip))
> > res = chip;
> > }
> >
> > diff --git a/drivers/char/tpm/tpm-interface.c 
> > b/drivers/char/tpm/tpm-interface.c
> > index ebe0a1d36d8c..19f820f775b5 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -809,19 +80

Re: [PATCH v3] tpm: use struct tpm_chip for tpm_chip_find_get()

[Jarkko Sakkinen]

On Thu, Oct 26, 2017 at 03:54:50PM +0200, Jarkko Sakkinen wrote:
> Device number (the character device index) is not a stable identifier
> for a TPM chip. That is the reason why every call site passes
> TPM_ANY_NUM to tpm_chip_find_get().
> 
> This commit changes the API in a way that instead a struct tpm_chip
> instance is given and NULL means the default chip. In addition, this
> commit refines the documentation to be up to date with the
> implementation.
> 
> Suggested-by: Jason Gunthorpe <jguntho...@obsidianresearch.com> (@chip_num -> 
> @chip)
> Signed-off-by: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>

Jason, once you have given this reviewed-by can you do a patch set that
includes this and your and PrasannaKumar's change? It could be a while
before keyring and IMA maintainers have reviewed my change.

/Jarkko

[PATCH v3] tpm: use struct tpm_chip for tpm_chip_find_get()

[Jarkko Sakkinen]

Device number (the character device index) is not a stable identifier
for a TPM chip. That is the reason why every call site passes
TPM_ANY_NUM to tpm_chip_find_get().

This commit changes the API in a way that instead a struct tpm_chip
instance is given and NULL means the default chip. In addition, this
commit refines the documentation to be up to date with the
implementation.

Suggested-by: Jason Gunthorpe <jguntho...@obsidianresearch.com> (@chip_num -> 
@chip)
Signed-off-by: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>
---
v3:
* If chip != NULL was given to tpm_chip_find_get(), it tried to reserve chip
  associated with the device number 0 instead of trying to reserve the given
  chip instance.
v2:
* Further defined function documentation.
* Changed @chip_num to @chip instead of removing the parameter as suggested by
  Jason Gunthorpe.
 drivers/char/hw_random/tpm-rng.c|   2 +-
 drivers/char/tpm/tpm-chip.c |  24 ---
 drivers/char/tpm/tpm-interface.c| 135 +++-
 drivers/char/tpm/tpm.h  |   2 +-
 include/linux/tpm.h |  38 +-
 security/integrity/ima/ima_crypto.c |   2 +-
 security/integrity/ima/ima_init.c   |   2 +-
 security/integrity/ima/ima_queue.c  |   2 +-
 security/keys/trusted.c |  35 +-
 9 files changed, 126 insertions(+), 116 deletions(-)

diff --git a/drivers/char/hw_random/tpm-rng.c b/drivers/char/hw_random/tpm-rng.c
index d6d448266f07..c5e363825af0 100644
--- a/drivers/char/hw_random/tpm-rng.c
+++ b/drivers/char/hw_random/tpm-rng.c
@@ -25,7 +25,7 @@
 
 static int tpm_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 {
-   return tpm_get_random(TPM_ANY_NUM, data, max);
+   return tpm_get_random(NULL, data, max);
 }
 
 static struct hwrng tpm_rng = {
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index a114e8f7fb90..bab9c14e040c 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -81,21 +81,26 @@ void tpm_put_ops(struct tpm_chip *chip)
 EXPORT_SYMBOL_GPL(tpm_put_ops);
 
 /**
- * tpm_chip_find_get() - return tpm_chip for a given chip number
- * @chip_num: id to find
+ * tpm_chip_find_get() - find and reserve a TPM chip
+ * @chip:  a  tpm_chip instance, %NULL for the default chip
  *
- * The return'd chip has been tpm_try_get_ops'd and must be released via
- * tpm_put_ops
+ * Finds a TPM chip and reserves its class device and operations. The chip must
+ * be released with tpm_chip_put_ops() after use.
+ *
+ * Return:
+ * A reserved  tpm_chip instance.
+ * %NULL if a chip is not found.
+ * %NULL if the chip is not available.
  */
-struct tpm_chip *tpm_chip_find_get(int chip_num)
+struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip)
 {
-   struct tpm_chip *chip, *res = NULL;
+   struct tpm_chip *res = NULL;
+   int chip_num = 0;
int chip_prev;
 
mutex_lock(_lock);
 
-   if (chip_num == TPM_ANY_NUM) {
-   chip_num = 0;
+   if (!chip) {
do {
chip_prev = chip_num;
chip = idr_get_next(_nums_idr, _num);
@@ -105,8 +110,7 @@ struct tpm_chip *tpm_chip_find_get(int chip_num)
}
} while (chip_prev != chip_num);
} else {
-   chip = idr_find(_nums_idr, chip_num);
-   if (chip && !tpm_try_get_ops(chip))
+   if (!tpm_try_get_ops(chip))
res = chip;
}
 
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index ebe0a1d36d8c..19f820f775b5 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -809,19 +809,20 @@ int tpm_pcr_read_dev(struct tpm_chip *chip, int pcr_idx, 
u8 *res_buf)
 }
 
 /**
- * tpm_is_tpm2 - is the chip a TPM2 chip?
- * @chip_num:  tpm idx # or ANY
+ * tpm_is_tpm2 - do we a have a TPM2 chip?
+ * @chip:  a  tpm_chip instance, %NULL for the default chip
  *
- * Returns < 0 on error, and 1 or 0 on success depending whether the chip
- * is a TPM2 chip.
+ * Return:
+ * 1 if we have a TPM2 chip.
+ * 0 if we don't have a TPM2 chip.
+ * A negative number for system errors (errno).
  */
-int tpm_is_tpm2(u32 chip_num)
+int tpm_is_tpm2(struct tpm_chip *chip)
 {
-   struct tpm_chip *chip;
int rc;
 
-   chip = tpm_chip_find_get(chip_num);
-   if (chip == NULL)
+   chip = tpm_chip_find_get(chip);
+   if (!chip)
return -ENODEV;
 
rc = (chip->flags & TPM_CHIP_FLAG_TPM2) != 0;
@@ -833,23 +834,19 @@ int tpm_is_tpm2(u32 chip_num)
 EXPORT_SYMBOL_GPL(tpm_is_tpm2);
 
 /**
- * tpm_pcr_read - read a pcr value
- * @chip_num:  tpm idx # or ANY
- * @pcr_idx:   pcr idx to retrieve
- * @res_buf:   TPM_PCR value
- * size of res_buf is 20 bytes (or NULL if you don't care)
+ * tpm_pcr_read - read a PCR value from SHA1 bank
+ * @chip:  a  tpm_chip ins

Re: [PATCH v2] tpm: use struct tpm_chip for tpm_chip_find_get()

[Jarkko Sakkinen]

On Wed, Oct 25, 2017 at 02:17:44PM -0600, Jason Gunthorpe wrote:
> On Wed, Oct 25, 2017 at 10:07:46PM +0200, Jarkko Sakkinen wrote:
> 
> > The id has a nice feature that it is unique for one boot cycle you can
> > even try to get a chip that has been deleted. It has the most stable
> > properties in the long run.
> 
> It isn't unique, we can re-use ids them via idr_alloc(). We should
> never use index inside the kernel.
> 
> > Address is a reusable identifier in one boot cycle.
> 
> It is invalid to pass in a chip for which the caller does not hold a
> kref, so address is the safest argument.
> 
> Jason

I'll buy this too (sending update today).

/Jarkko

Re: [PATCH v2] tpm: use struct tpm_chip for tpm_chip_find_get()

[Jarkko Sakkinen]

On Wed, Oct 25, 2017 at 01:46:33PM -0600, Jason Gunthorpe wrote:
> > struct tpm_chip *tpm_chip_find_get(u64 id)
> > {
> > struct tpm_chup *chip;
> > struct tpm_chip *res = NULL;
> > int chip_num = 0;
> > int chip_prev;
> > 
> > mutex_lock(_lock);
> > 
> > do {
> > chip_prev = chip_num;
> > 
> > chip = idr_get_next(_nums_idr, _num);
> > 
> > if (chip && (!id || id == chip->id) && !tpm_try_get_ops(chip)) {
> > res = chip;
> > break;
> > }
> > } while (chip_prev != chip_num);
> > 
> > mutex_unlock(_lock);
> > 
> > return res;
> > }
> 
> ?? The old version was correct, idr_find_slowpath is better than an
> idr_get_next serach if you already know id.
> 
> PrasannaKumar's solution seems right, if we already have chip, then we
> just need to lock it again:
> 
> struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip)
> {
>   struct tpm_chip *res = NULL;
> 
>   mutex_lock(_lock);
> 
>   if (!chip) {
>   int chip_num = 0;
>   int chip_prev;
> 
>   do {
>   chip_prev = chip_num;
>   chip = idr_get_next(_nums_idr, _num);
>   if (chip && !tpm_try_get_ops(chip)) {
>   res = chip;
>   break;
>   }
>   } while (chip_prev != chip_num);
>   } else {
>   if (!tpm_try_get_ops(chip))
>   res = chip;
>   }
> 
>   mutex_unlock(_lock);
> 
>   return res;
> }
> 
> Jason

The id has a nice feature that it is unique for one boot cycle you can
even try to get a chip that has been deleted. It has the most stable
properties in the long run.

Address is a reusable identifier in one boot cycle.

/Jarkko

Re: [PATCH] tpm: Move Linux RNG connection to hwrng

[Jarkko Sakkinen]

On Wed, Oct 25, 2017 at 01:37:07PM -0600, Jason Gunthorpe wrote:
> On Wed, Oct 25, 2017 at 08:58:17PM +0200, Jarkko Sakkinen wrote:
> > On Wed, Oct 25, 2017 at 08:15:09PM +0530, PrasannaKumar Muralidharan wrote:
> > > > +   if (!IS_ENABLED(CONFIG_HW_RANDOM_TPM))
> > > > +   return 0;
> > > 
> > > Can #ifndef CONFIG_HW_RANDOM_TPM be used instead? That way an if
> > > condition can be avoided.
> > 
> > Nope. There is no reason to avoid the if-condition. Compiler will take
> > care of it. IS_ENABLED() macro is available just for the purpose Jason
> > is using it.
> > 
> > > > +   char tpm_hwrng_name[64];
> > > > +   struct hwrng tpm_hwrng;
> > > > +
> > > 
> > > Can this also be put inside the #ifdef?
> > 
> > Yes. It should be inside #ifdef.
> 
> Then we need #idefs in the .c code, IS_ENABLED is not enough :\ I
> don't think the few bytes matters enough to bother.
> 
> Jason

I'll buy that!

A minor tidbit: could the tpm_ prefix removed from those fields?

/Jarkko

Re: [PATCH v2] tpm: use struct tpm_chip for tpm_chip_find_get()

[Jarkko Sakkinen]

On Wed, Oct 25, 2017 at 08:40:26PM +0530, PrasannaKumar Muralidharan wrote:
> > -struct tpm_chip *tpm_chip_find_get(int chip_num)
> > +struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip)
> >  {
> > -   struct tpm_chip *chip, *res = NULL;
> > +   struct tpm_chip *res = NULL;
> > +   int chip_num = 0;
> > int chip_prev;
> >
> > mutex_lock(_lock);
> >
> > -   if (chip_num == TPM_ANY_NUM) {
> > -   chip_num = 0;
> > +   if (!chip) {
> > do {
> > chip_prev = chip_num;
> > chip = idr_get_next(_nums_idr, _num);
> 
> When chip is not NULL just do tpm_try_get_ops(chip). Current code does
> more things which are not required.

Your observation is right that there is something wrong but conclusions
are incorrect.

It's actually a regression.

If @chip has a value, the code does one iteration of what it is doing in
the first branch of the condition. That is completely bogus semantics to
say the least.

To sort that out I'll introduce a new field to struct tpm_chip:

  u64 id;

This gets a value from a global count every time a chip is created.

The function will become then:

struct tpm_chip *tpm_chip_find_get(u64 id)
{
struct tpm_chup *chip;
struct tpm_chip *res = NULL;
int chip_num = 0;
int chip_prev;

mutex_lock(_lock);

do {
chip_prev = chip_num;

chip = idr_get_next(_nums_idr, _num);

if (chip && (!id || id == chip->id) && !tpm_try_get_ops(chip)) {
res = chip;
break;
}
} while (chip_prev != chip_num);

mutex_unlock(_lock);

return res;
}

Thanks for spotting this out. I'll refine the patch.

/Jarkko

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

[Jarkko Sakkinen]

On Wed, Oct 25, 2017 at 08:21:16PM +0530, PrasannaKumar Muralidharan wrote:
> >> > 2. Moving struct tpm_rng to the TPM client is architecturally
> >> >uacceptable.
> >>
> >> As there was no response to the patch there is no way to know whether
> >> it is acceptable or not.
> >
> > I like the idea of removing the tpm rng driver as discussed in other
> > emails in this thread.
> 
> Thank you.

No, thank you.

I didn't first understand the big idea and only looked at the code
change per se. I apologize for that.

The problem that you went to solve was real and it led to a properly
implemented solution. You were not late from the party. Jason's code
change is derivative work of your code change. That's why his code
change has also your signed-off-by.

Thanks for doing awesome work :-)

/Jarkko

Re: [PATCH] tpm: Move Linux RNG connection to hwrng

[Jarkko Sakkinen]

On Wed, Oct 25, 2017 at 08:15:09PM +0530, PrasannaKumar Muralidharan wrote:
> > +   if (!IS_ENABLED(CONFIG_HW_RANDOM_TPM))
> > +   return 0;
> 
> Can #ifndef CONFIG_HW_RANDOM_TPM be used instead? That way an if
> condition can be avoided.

Nope. There is no reason to avoid the if-condition. Compiler will take
care of it. IS_ENABLED() macro is available just for the purpose Jason
is using it.

> > +   char tpm_hwrng_name[64];
> > +   struct hwrng tpm_hwrng;
> > +
> 
> Can this also be put inside the #ifdef?

Yes. It should be inside #ifdef.

/Jarkko

[PATCH v2] tpm: use struct tpm_chip for tpm_chip_find_get()

[Jarkko Sakkinen]

Device number (the character device index) is not a stable identifier
for a TPM chip. That is the reason why every call site passes
TPM_ANY_NUM to tpm_chip_find_get().

This commit changes the API in a way that instead a struct tpm_chip
instance is given and NULL means the default chip. In addition, this
commit refines the documentation to be up to date with the
implementation.

Suggested-by: Jason Gunthorpe <jguntho...@obsidianresearch.com> (@chip_num -> 
@chip)
Signed-off-by: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>
---
v2:
* Further defined function documentation.
* Changed @chip_num to @chip instead of removing the parameter as suggested by
  Jason Gunthorpe.
 drivers/char/hw_random/tpm-rng.c|   2 +-
 drivers/char/tpm/tpm-chip.c |  21 +++---
 drivers/char/tpm/tpm-interface.c| 135 +++-
 drivers/char/tpm/tpm.h  |   2 +-
 include/linux/tpm.h |  38 +-
 security/integrity/ima/ima_crypto.c |   2 +-
 security/integrity/ima/ima_init.c   |   2 +-
 security/integrity/ima/ima_queue.c  |   2 +-
 security/keys/trusted.c |  35 +-
 9 files changed, 125 insertions(+), 114 deletions(-)

diff --git a/drivers/char/hw_random/tpm-rng.c b/drivers/char/hw_random/tpm-rng.c
index d6d448266f07..c5e363825af0 100644
--- a/drivers/char/hw_random/tpm-rng.c
+++ b/drivers/char/hw_random/tpm-rng.c
@@ -25,7 +25,7 @@
 
 static int tpm_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 {
-   return tpm_get_random(TPM_ANY_NUM, data, max);
+   return tpm_get_random(NULL, data, max);
 }
 
 static struct hwrng tpm_rng = {
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index a114e8f7fb90..c7a4e7fb424d 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -81,21 +81,26 @@ void tpm_put_ops(struct tpm_chip *chip)
 EXPORT_SYMBOL_GPL(tpm_put_ops);
 
 /**
- * tpm_chip_find_get() - return tpm_chip for a given chip number
- * @chip_num: id to find
+ * tpm_chip_find_get() - find and reserve a TPM chip
+ * @chip:  a  tpm_chip instance, %NULL for the default chip
  *
- * The return'd chip has been tpm_try_get_ops'd and must be released via
- * tpm_put_ops
+ * Finds a TPM chip and reserves its class device and operations. The chip must
+ * be released with tpm_chip_put_ops() after use.
+ *
+ * Return:
+ * A reserved  tpm_chip instance.
+ * %NULL if a chip is not found.
+ * %NULL if the chip is not available.
  */
-struct tpm_chip *tpm_chip_find_get(int chip_num)
+struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip)
 {
-   struct tpm_chip *chip, *res = NULL;
+   struct tpm_chip *res = NULL;
+   int chip_num = 0;
int chip_prev;
 
mutex_lock(_lock);
 
-   if (chip_num == TPM_ANY_NUM) {
-   chip_num = 0;
+   if (!chip) {
do {
chip_prev = chip_num;
chip = idr_get_next(_nums_idr, _num);
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index ebe0a1d36d8c..19f820f775b5 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -809,19 +809,20 @@ int tpm_pcr_read_dev(struct tpm_chip *chip, int pcr_idx, 
u8 *res_buf)
 }
 
 /**
- * tpm_is_tpm2 - is the chip a TPM2 chip?
- * @chip_num:  tpm idx # or ANY
+ * tpm_is_tpm2 - do we a have a TPM2 chip?
+ * @chip:  a  tpm_chip instance, %NULL for the default chip
  *
- * Returns < 0 on error, and 1 or 0 on success depending whether the chip
- * is a TPM2 chip.
+ * Return:
+ * 1 if we have a TPM2 chip.
+ * 0 if we don't have a TPM2 chip.
+ * A negative number for system errors (errno).
  */
-int tpm_is_tpm2(u32 chip_num)
+int tpm_is_tpm2(struct tpm_chip *chip)
 {
-   struct tpm_chip *chip;
int rc;
 
-   chip = tpm_chip_find_get(chip_num);
-   if (chip == NULL)
+   chip = tpm_chip_find_get(chip);
+   if (!chip)
return -ENODEV;
 
rc = (chip->flags & TPM_CHIP_FLAG_TPM2) != 0;
@@ -833,23 +834,19 @@ int tpm_is_tpm2(u32 chip_num)
 EXPORT_SYMBOL_GPL(tpm_is_tpm2);
 
 /**
- * tpm_pcr_read - read a pcr value
- * @chip_num:  tpm idx # or ANY
- * @pcr_idx:   pcr idx to retrieve
- * @res_buf:   TPM_PCR value
- * size of res_buf is 20 bytes (or NULL if you don't care)
+ * tpm_pcr_read - read a PCR value from SHA1 bank
+ * @chip:  a  tpm_chip instance, %NULL for the default chip
+ * @pcr_idx:   the PCR to be retrieved
+ * @res_buf:   the value of the PCR
  *
- * The TPM driver should be built-in, but for whatever reason it
- * isn't, protect against the chip disappearing, by incrementing
- * the module usage count.
+ * Return: same as with tpm_transmit_cmd()
  */
-int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf)
+int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
 {
-   struct tpm_chip *chip;
int rc;
 
-   chip = tpm_chip_find_get(chip_num);

Re: [PATCH] tpm: Move Linux RNG connection to hwrng

[Jarkko Sakkinen]

On Tue, Oct 24, 2017 at 03:34:49PM -0600, Jason Gunthorpe wrote:
> On Tue, Oct 24, 2017 at 12:42:35PM -0600, Jason Gunthorpe wrote:
> 
> > This is compile tested only.
> 
> 0day says the kconfig has a problem when randomized, here is the fix I
> will roll into a v2 in a few days:

I will probably have to postpone the review to next week anyway so take
your time :-)

/Jarkko

> 
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index a95725fa77789e..ca89da3e4b2ae9 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -27,16 +27,13 @@ menuconfig TCG_TPM
>  if TCG_TPM
>  
>  config HW_RANDOM_TPM
> -   tristate "TPM HW Random Number Generator support"
> -   depends on TCG_TPM && HW_RANDOM
> -   default HW_RANDOM
> +   bool "TPM HW Random Number Generator support"
> +   depends on TCG_TPM && HW_RANDOM && !(TCG_TPM=y && HW_RANDOM=m)
> +   default y
> ---help---
>   This driver provides kernel-side support for the Random Number
>   Generator in the Trusted Platform Module
>  
> - To compile this driver as a module, choose M here: the
> - module will be called tpm-rng.
> -
>   If unsure, say Y.
>  
>  config TCG_TIS_CORE

Re: [PATCH] tpm: remove chip_num parameter from in-kernel API

[Jarkko Sakkinen]

On Tue, Oct 24, 2017 at 12:52:08PM -0600, Jason Gunthorpe wrote:
> On Mon, Oct 23, 2017 at 02:38:14PM +0200, Jarkko Sakkinen wrote:
> > The reasoning is simple and obvious. Since every call site passes the
> > value TPM_ANY_NUM (0x) the parameter does not have right to exist.
> > Refined the documentation of the corresponding functions.
> 
> I like this patch, but how about a slightly different take, make this
> change instead:
> 
> -struct tpm_chip *tpm_chip_find_get(int chip_num)
> +struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip);
> 
> Where chip == NULL means the default TPM.
> 
> And then at all call sites swap TPM_ANY_NUM to NULL and instead of
> flowing an 'int chip_num' to tpm_chip_find_get, flow the 'struct
> tpm_chip *' directly.
> 
> This gets us much closer to the desired API with about the same amount
> of churn as this patch has.
> 
> Jason

I can do that. I will rework this as a patch set that includes this
proposal. Gives us more clean position to continue. Thank you.

/Jarkko

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

[Jarkko Sakkinen]

On Tue, Oct 24, 2017 at 10:05:20PM +0530, PrasannaKumar Muralidharan wrote:
> > 1. Every user in the kernel is using TPM_ANY_NUM, which means there are
> >no other users.
> 
> Completely agree that there is no in kernel users yet.

And should never be. It's a bogus parameter that makes no sense.

> > 2. Moving struct tpm_rng to the TPM client is architecturally
> >uacceptable.
> 
> As there was no response to the patch there is no way to know whether
> it is acceptable or not.

I like the idea of removing the tpm rng driver as discussed in other
emails in this thread.

> > 3. Using zero deos not give you any better guarantees on anything than
> >just using TPM_ANY_NUM.
> 
> Chip id is used, not zero.

Sorry I misread the patch first time. Anyway it's not any kind of ID to
be trusted.

> > Why this patch is not CC'd to linux-integrity? It modifies the TPM
> > driver. And in the worst way.
> 
> TPM list is moderated and the moderator has not approved it yet.
> get_maintainer script did not say about linux-integrity mailing list.
> 
> It could be doing things in worst way but it is not known until some
> one says. If no one tells it is the case I don't think it is possible
> to fix. Which is what happened.

Understood. We've moved to linux-integr...@vger.kernel.org. MAINTAINERS
update is in the queue for the next kernel release.

> > Implementing the ideas that Jason explained is the senseful way to
> > get stable access. modules.dep makes sure that the modules are loaded
> > in the correct order.
> 
> If that is sensible then it is the way to go.
> 
> There must be a reason to believe what is sensible and what is not.
> Looks like this RFC has helped in judging that.
> 
> Regards,
> PrasannaKumar

Would you be interested to work on patch set that would remove the
existing tpm rng driver and make the TPM driver the customer? It's not
that far away from the work you've been doing already.

/Jarkko

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

[Jarkko Sakkinen]

On Tue, Oct 24, 2017 at 10:02:00AM -0700, Dmitry Torokhov wrote:
> On Tue, Oct 24, 2017 at 9:11 AM, Jason Gunthorpe
>  wrote:
> > On Tue, Oct 24, 2017 at 09:37:33PM +0530, PrasannaKumar Muralidharan wrote:
> >> Hi Jason,
> >>
> >> On 24 October 2017 at 21:25, Jason Gunthorpe
> >>  wrote:
> >> > On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan 
> >> > wrote:
> >> >
> >> >> Please check the RFC [1]. It does use chip id. The rfc has issues and
> >> >> has to be fixed but still there could be users of the API.
> >> >>
> >> >> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
> >> >
> >> > That patch isn't safe at all. You need to store a kref to th chip in
> >> > the hwrng, not parse a string.
> >>
> >> The drivers/char/hw_random/tpm-rng.c module does not store the chip
> >> reference so I guess the usage is safe.
> >
> > It is using the default TPM, it is always safe to use the default tpm.
> 
> tpm-rng is abomination that should be kicked out as soon as possible.
> It wrecks havoc with the power management (TPM chip drivers may go
> into suspend state, but tpm_rng does not do any power management and
> happily forwards requests to suspended hardware) and may be available
> when there is no TPM at all yet (the drivers have not been probed yet,
> or have gotten a deferral, etc).
> 
> TPM core should register HWRNGs when chips are ready.
> 
> Thanks.
> 
> -- 
> Dmitry

I'm fine to review a two patch set where:

1. Patch 1 removes the existing TPM rng driver
2. Patch 2 makes the TPM driver as rng producer

Unrelate to patch that I'm proposing now but this sounds sensible.

/Jarkko

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

[Jarkko Sakkinen]

On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote:
> On 24 October 2017 at 21:14, Jarkko Sakkinen
> <jarkko.sakki...@linux.intel.com> wrote:
> > On Mon, Oct 23, 2017 at 10:31:39AM -0600, Jason Gunthorpe wrote:
> >> On Mon, Oct 23, 2017 at 10:07:31AM -0400, Stefan Berger wrote:
> >>
> >> > >-int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
> >> > >+int tpm_pcr_extend(int pcr_idx, const u8 *hash)
> >> > >  {
> >> >
> >> >
> >> > I think every kernel internal TPM driver API should be called with the
> >> > tpm_chip as a parameter. This is in foresight of namespacing of IMA 
> >> > where we
> >> > want to provide the flexibility of passing a dedicated vTPM to each
> >> > namespace and IMA would use the chip as a parameter to all of these
> >> > functions to talk to the right tpm_vtpm_proxy instance. From that
> >> > perspective this patch goes into the wrong direction.
> >>
> >> Yes, we should ultimately try and get to there.. Someday the
> >> tpm_chip_find_get() will need to become namespace aware.
> >>
> >> But this patch is along the right path, eliminating the chip_num is
> >> the right thing to do..
> >>
> >> > >-  tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
> >> > >+  tpm2 = tpm_is_tpm2();
> >> > >   if (tpm2 < 0)
> >> > >   return tpm2;
> >> > >
> >> > >@@ -1008,7 +1007,7 @@ static int trusted_instantiate(struct key *key,
> >> > >   switch (key_cmd) {
> >> > >   case Opt_load:
> >> > >   if (tpm2)
> >> > >-  ret = tpm_unseal_trusted(TPM_ANY_NUM, payload, 
> >> > >options);
> >> > >+  ret = tpm_unseal_trusted(payload, options);
> >>
> >> Sequences like this are sketchy.
> >>
> >> It should be
> >>
> >> struct tpm_chip *tpm;
> >>
> >> tpm = tpm_chip_find_get()
> >> tpm2 = tpm_is_tpm2(tpm);
> >>
> >> [..]
> >>
> >> if (tpm2)
> >>  ret = tpm_unseal_trusted(tpm, payload, options);
> >>
> >> [..]
> >>
> >> tpm_put_chip(tpm);
> >>
> >> As hot plug could alter the 'tpm' between the two tpm calls.
> >>
> >> Jason
> >
> > This patch just removes bunch of dead code. It does not change existing
> > semantics. What you are saying should be done after the dead code has
> > been removed. This commit is first step to that direction.
> >
> > /Jarkko
> 
> Please check the RFC [1]. It does use chip id. The rfc has issues and
> has to be fixed but still there could be users of the API.
> 
> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
> 
> Regards,
> PrasannaKumar

1. Every user in the kernel is using TPM_ANY_NUM, which means there are
   no other users.
2. Moving struct tpm_rng to the TPM client is architecturally
   uacceptable.
3. Using zero deos not give you any better guarantees on anything than
   just using TPM_ANY_NUM.

Why this patch is not CC'd to linux-integrity? It modifies the TPM
driver. And in the worst way.

Implementing the ideas that Jason explained is the senseful way to
get stable access. modules.dep makes sure that the modules are loaded
in the correct order.

/Jarkko

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

[Jarkko Sakkinen]

On Mon, Oct 23, 2017 at 10:31:39AM -0600, Jason Gunthorpe wrote:
> On Mon, Oct 23, 2017 at 10:07:31AM -0400, Stefan Berger wrote:
> 
> > >-int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
> > >+int tpm_pcr_extend(int pcr_idx, const u8 *hash)
> > >  {
> > 
> > 
> > I think every kernel internal TPM driver API should be called with the
> > tpm_chip as a parameter. This is in foresight of namespacing of IMA where we
> > want to provide the flexibility of passing a dedicated vTPM to each
> > namespace and IMA would use the chip as a parameter to all of these
> > functions to talk to the right tpm_vtpm_proxy instance. From that
> > perspective this patch goes into the wrong direction.
> 
> Yes, we should ultimately try and get to there.. Someday the
> tpm_chip_find_get() will need to become namespace aware.
> 
> But this patch is along the right path, eliminating the chip_num is
> the right thing to do..
> 
> > >-  tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
> > >+  tpm2 = tpm_is_tpm2();
> > >   if (tpm2 < 0)
> > >   return tpm2;
> > >
> > >@@ -1008,7 +1007,7 @@ static int trusted_instantiate(struct key *key,
> > >   switch (key_cmd) {
> > >   case Opt_load:
> > >   if (tpm2)
> > >-  ret = tpm_unseal_trusted(TPM_ANY_NUM, payload, options);
> > >+  ret = tpm_unseal_trusted(payload, options);
> 
> Sequences like this are sketchy.
> 
> It should be
> 
> struct tpm_chip *tpm;
> 
> tpm = tpm_chip_find_get()
> tpm2 = tpm_is_tpm2(tpm);
> 
> [..]
> 
> if (tpm2)
>  ret = tpm_unseal_trusted(tpm, payload, options);
> 
> [..]
> 
> tpm_put_chip(tpm);
> 
> As hot plug could alter the 'tpm' between the two tpm calls.
> 
> Jason

This patch just removes bunch of dead code. It does not change existing
semantics. What you are saying should be done after the dead code has
been removed. This commit is first step to that direction.

/Jarkko

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

[Jarkko Sakkinen]

On Mon, Oct 23, 2017 at 10:07:31AM -0400, Stefan Berger wrote:
> I think every kernel internal TPM driver API should be called with the
> tpm_chip as a parameter. This is in foresight of namespacing of IMA where we
> want to provide the flexibility of passing a dedicated vTPM to each
> namespace and IMA would use the chip as a parameter to all of these
> functions to talk to the right tpm_vtpm_proxy instance. From that
> perspective this patch goes into the wrong direction.
> 
>Stefan

The goal of this patch is to kernel code that never gets executed. It
removes a load of completely dead code. It is the only thing that this
commit does. Why do you think this is "going into wrong direction" if it
only removes dead code and refines the documentation up to date?

After the dead code has been removed it makes sense to propose a better
mechanism. Maybe the one that you are speaking about. But you need to
remove the cruft first.

/Jarkko

[PATCH] tpm: remove chip_num parameter from in-kernel API

[Jarkko Sakkinen]

The reasoning is simple and obvious. Since every call site passes the
value TPM_ANY_NUM (0x) the parameter does not have right to exist.
Refined the documentation of the corresponding functions.

Signed-off-by: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>
---
 drivers/char/hw_random/tpm-rng.c|  2 +-
 drivers/char/tpm/tpm-chip.c | 38 
 drivers/char/tpm/tpm-interface.c| 87 ++---
 drivers/char/tpm/tpm.h  |  2 +-
 include/linux/tpm.h | 43 --
 security/integrity/ima/ima_crypto.c |  2 +-
 security/integrity/ima/ima_init.c   |  2 +-
 security/integrity/ima/ima_queue.c  |  2 +-
 security/keys/trusted.c | 35 ---
 9 files changed, 101 insertions(+), 112 deletions(-)

diff --git a/drivers/char/hw_random/tpm-rng.c b/drivers/char/hw_random/tpm-rng.c
index d6d448266f07..8823efcddab8 100644
--- a/drivers/char/hw_random/tpm-rng.c
+++ b/drivers/char/hw_random/tpm-rng.c
@@ -25,7 +25,7 @@
 
 static int tpm_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 {
-   return tpm_get_random(TPM_ANY_NUM, data, max);
+   return tpm_get_random(data, max);
 }
 
 static struct hwrng tpm_rng = {
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index a114e8f7fb90..ec35643b 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -81,34 +81,32 @@ void tpm_put_ops(struct tpm_chip *chip)
 EXPORT_SYMBOL_GPL(tpm_put_ops);
 
 /**
- * tpm_chip_find_get() - return tpm_chip for a given chip number
- * @chip_num: id to find
+ * tpm_chip_find_get() - reserved the first available TPM chip
  *
- * The return'd chip has been tpm_try_get_ops'd and must be released via
- * tpm_put_ops
+ * Finds the first available TPM chip and reserves its class device and
+ * operations.
+ *
+ * Return: a reserved  tpm_chip instance
  */
-struct tpm_chip *tpm_chip_find_get(int chip_num)
+struct tpm_chip *tpm_chip_find_get(void)
 {
-   struct tpm_chip *chip, *res = NULL;
+   struct tpm_chip *chip;
+   struct tpm_chip *res = NULL;
int chip_prev;
+   int chip_num;
 
mutex_lock(_lock);
 
-   if (chip_num == TPM_ANY_NUM) {
-   chip_num = 0;
-   do {
-   chip_prev = chip_num;
-   chip = idr_get_next(_nums_idr, _num);
-   if (chip && !tpm_try_get_ops(chip)) {
-   res = chip;
-   break;
-   }
-   } while (chip_prev != chip_num);
-   } else {
-   chip = idr_find(_nums_idr, chip_num);
-   if (chip && !tpm_try_get_ops(chip))
+   chip_num = 0;
+
+   do {
+   chip_prev = chip_num;
+   chip = idr_get_next(_nums_idr, _num);
+   if (chip && !tpm_try_get_ops(chip)) {
res = chip;
-   }
+   break;
+   }
+   } while (chip_prev != chip_num);
 
mutex_unlock(_lock);
 
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index d8e2e5bca903..b3907d3556ce 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -802,18 +802,19 @@ int tpm_pcr_read_dev(struct tpm_chip *chip, int pcr_idx, 
u8 *res_buf)
 }
 
 /**
- * tpm_is_tpm2 - is the chip a TPM2 chip?
- * @chip_num:  tpm idx # or ANY
+ * tpm_is_tpm2 - do we a have a TPM2 chip?
  *
- * Returns < 0 on error, and 1 or 0 on success depending whether the chip
- * is a TPM2 chip.
+ * Return:
+ * 1 if we have a TPM2 chip.
+ * 0 if we don't have a TPM2 chip.
+ * A negative number for system errors (errno).
  */
-int tpm_is_tpm2(u32 chip_num)
+int tpm_is_tpm2(void)
 {
struct tpm_chip *chip;
int rc;
 
-   chip = tpm_chip_find_get(chip_num);
+   chip = tpm_chip_find_get();
if (chip == NULL)
return -ENODEV;
 
@@ -826,22 +827,18 @@ int tpm_is_tpm2(u32 chip_num)
 EXPORT_SYMBOL_GPL(tpm_is_tpm2);
 
 /**
- * tpm_pcr_read - read a pcr value
- * @chip_num:  tpm idx # or ANY
- * @pcr_idx:   pcr idx to retrieve
- * @res_buf:   TPM_PCR value
- * size of res_buf is 20 bytes (or NULL if you don't care)
+ * tpm_pcr_read - read a PCR value from SHA1 bank
+ * @pcr_idx:   the PCR to be retrieved
+ * @res_buf:   the value of the PCR
  *
- * The TPM driver should be built-in, but for whatever reason it
- * isn't, protect against the chip disappearing, by incrementing
- * the module usage count.
+ * Return: same as with tpm_transmit_cmd()
  */
-int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf)
+int tpm_pcr_read(int pcr_idx, u8 *res_buf)
 {
struct tpm_chip *chip;
int rc;
 
-   chip = tpm_chip_find_get(chip_num);
+   chip = tpm_chip_find_get();
if (chip == NULL)
return -ENODEV;
if (chip->flags &

[PATCH v2 2/3] keys, trusted: select hash algorithm for TPM2 chips

[Jarkko Sakkinen]

Added 'hash=' option for selecting the hash algorithm for add_key()
syscall and documentation for it.

Added entry for sm3-256 to the following tables in order to support
TPM_ALG_SM3_256:

* hash_algo_name
* hash_digest_size

Includes support for the following hash algorithms:

* sha1
* sha256
* sha384
* sha512
* sm3-256

Signed-off-by: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>
Tested-by: Colin Ian King <colin.k...@canonical.com>
Reviewed-by: James Morris <james.l.mor...@oracle.com>
---
 Documentation/security/keys-trusted-encrypted.txt |  3 ++
 crypto/hash_info.c|  2 ++
 drivers/char/tpm/tpm.h| 10 +--
 drivers/char/tpm/tpm2-cmd.c   | 36 +--
 include/crypto/hash_info.h|  3 ++
 include/keys/trusted-type.h   |  1 +
 include/uapi/linux/hash_info.h|  1 +
 security/keys/Kconfig |  1 +
 security/keys/trusted.c   | 27 -
 9 files changed, 77 insertions(+), 7 deletions(-)

diff --git a/Documentation/security/keys-trusted-encrypted.txt 
b/Documentation/security/keys-trusted-encrypted.txt
index e105ae9..fd2565b 100644
--- a/Documentation/security/keys-trusted-encrypted.txt
+++ b/Documentation/security/keys-trusted-encrypted.txt
@@ -38,6 +38,9 @@ Usage:
pcrlock=  pcr number to be extended to "lock" blob
migratable= 0|1 indicating permission to reseal to new PCR values,
default 1 (resealing allowed)
+   hash=  hash algorithm name as a string. For TPM 1.x the only
+  allowed value is sha1. For TPM 2.x the allowed values
+ are sha1, sha256, sha384, sha512 and sm3-256.
 
 "keyctl print" returns an ascii hex copy of the sealed key, which is in 
standard
 TPM_STORED_DATA format.  The key length for new keys are always in bytes.
diff --git a/crypto/hash_info.c b/crypto/hash_info.c
index 3e7ff46..7b1e0b1 100644
--- a/crypto/hash_info.c
+++ b/crypto/hash_info.c
@@ -31,6 +31,7 @@ const char *const hash_algo_name[HASH_ALGO__LAST] = {
[HASH_ALGO_TGR_128] = "tgr128",
[HASH_ALGO_TGR_160] = "tgr160",
[HASH_ALGO_TGR_192] = "tgr192",
+   [HASH_ALGO_SM3_256] = "sm3-256",
 };
 EXPORT_SYMBOL_GPL(hash_algo_name);
 
@@ -52,5 +53,6 @@ const int hash_digest_size[HASH_ALGO__LAST] = {
[HASH_ALGO_TGR_128] = TGR128_DIGEST_SIZE,
[HASH_ALGO_TGR_160] = TGR160_DIGEST_SIZE,
[HASH_ALGO_TGR_192] = TGR192_DIGEST_SIZE,
+   [HASH_ALGO_SM3_256] = SM3256_DIGEST_SIZE,
 };
 EXPORT_SYMBOL_GPL(hash_digest_size);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 347fc61..542a80c 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -83,16 +83,20 @@ enum tpm2_structures {
 };
 
 enum tpm2_return_codes {
-   TPM2_RC_INITIALIZE  = 0x0100,
-   TPM2_RC_TESTING = 0x090A,
+   TPM2_RC_HASH= 0x0083, /* RC_FMT1 */
+   TPM2_RC_INITIALIZE  = 0x0100, /* RC_VER1 */
TPM2_RC_DISABLED= 0x0120,
+   TPM2_RC_TESTING = 0x090A, /* RC_WARN */
 };
 
 enum tpm2_algorithms {
TPM2_ALG_SHA1   = 0x0004,
TPM2_ALG_KEYEDHASH  = 0x0008,
TPM2_ALG_SHA256 = 0x000B,
-   TPM2_ALG_NULL   = 0x0010
+   TPM2_ALG_SHA384 = 0x000C,
+   TPM2_ALG_SHA512 = 0x000D,
+   TPM2_ALG_NULL   = 0x0010,
+   TPM2_ALG_SM3_256= 0x0012,
 };
 
 enum tpm2_command_codes {
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index c121304..d9d0822 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -16,6 +16,7 @@
  */
 
 #include "tpm.h"
+#include 
 #include 
 
 enum tpm2_object_attributes {
@@ -104,6 +105,19 @@ struct tpm2_cmd {
union tpm2_cmd_params   params;
 } __packed;
 
+struct tpm2_hash {
+   unsigned int crypto_id;
+   unsigned int tpm_id;
+};
+
+static struct tpm2_hash tpm2_hash_map[] = {
+   {HASH_ALGO_SHA1, TPM2_ALG_SHA1},
+   {HASH_ALGO_SHA256, TPM2_ALG_SHA256},
+   {HASH_ALGO_SHA384, TPM2_ALG_SHA384},
+   {HASH_ALGO_SHA512, TPM2_ALG_SHA512},
+   {HASH_ALGO_SM3_256, TPM2_ALG_SM3_256},
+};
+
 /*
  * Array with one entry per ordinal defining the maximum amount
  * of time the chip could take to return the result. The values
@@ -429,8 +443,20 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
 {
unsigned int blob_len;
struct tpm_buf buf;
+   u32 hash;
+   int i;
int rc;
 
+   for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) {
+   if (options->hash == tpm2_hash_map[i].crypto_id) {
+   hash = tpm2_hash_map[i].tpm_id;
+   break;
+   }
+ 

[PATCH v2 0/3] TPM 2.0 trusted key features for v4.5

[Jarkko Sakkinen]

These are the remaining features to enable trusted keys for TPM 2.0 that were
not finished by the v4.4 merge window. These patches enable authorization
policy based sealing (like using PCRs together with a password for example or
something more complicated) with a user selected hash algorithm.

Jarkko Sakkinen (3):
  keys, trusted: fix: *do not* allow duplicate key options
  keys, trusted: select hash algorithm for TPM2 chips
  keys, trusted: seal with a TPM2 authorization policy

 Documentation/security/keys-trusted-encrypted.txt | 31 +++-
 crypto/hash_info.c|  2 +
 drivers/char/tpm/tpm.h| 10 ++--
 drivers/char/tpm/tpm2-cmd.c   | 60 ---
 include/crypto/hash_info.h|  3 ++
 include/keys/trusted-type.h   |  5 ++
 include/uapi/linux/hash_info.h|  1 +
 security/keys/Kconfig |  1 +
 security/keys/trusted.c   | 56 -
 9 files changed, 147 insertions(+), 22 deletions(-)

-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/2] TPM 2.0 trusted key features for v4.5

[Jarkko Sakkinen]

These are the remaining features to enable trusted keys for TPM 2.0 that very
not finished by the v4.4 merge window. These patches enable authorization
policy based sealing (like using PCRs together with a password for example or
something more complicated) with a user selected hash algorithm.

Jarkko Sakkinen (2):
  keys, trusted: select hash algorithm for TPM2 chips
  keys, trusted: seal with a policy

 Documentation/security/keys-trusted-encrypted.txt | 31 ++
 crypto/hash_info.c|  2 +
 drivers/char/tpm/tpm.h| 10 +++-
 drivers/char/tpm/tpm2-cmd.c   | 60 ---
 include/crypto/hash_info.h|  3 +
 include/keys/trusted-type.h   |  4 ++
 include/uapi/linux/hash_info.h|  1 +
 security/keys/Kconfig |  1 +
 security/keys/trusted.c   | 73 ++-
 9 files changed, 161 insertions(+), 24 deletions(-)

-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] keys, trusted: select hash algorithm for TPM2 chips

[Jarkko Sakkinen]

Added 'hash=' option for selecting the hash algorithm for add_key()
syscall and documentation for it.

Added entry for sm3-256 to the following tables in order to support
TPM_ALG_SM3_256:

* hash_algo_name
* hash_digest_size

Includes support for the following hash algorithms:

* sha1
* sha256
* sha384
* sha512
* sm3-256

v2:

* Added missing select CRYPTO_HASH_INFO to security/keys/Kconfig

v3:

* Squashed patches into a single patch as the commits did not make
  alone any sense.
* Added a klog message when TPM 1.x is used for sealing and other than
  SHA-1 is used as the hash algorithm.
* Got rid of TPM2_HASH_COUNT and moved into ARRAY_SIZE(tpm2_hash_map).

v4:

* Added missing select CRYPTO_HASH_INFO to drivers/char/tpm/Kconfig

v5:

* Minor clean ups.
* Removed dev_dbg() from tpm2-cmd.c in order to get rid of
  CRYPTO_HASH_INFO dep.

Signed-off-by: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>
Reviewed-by: James Morris <james.l.mor...@oracle.com>
---
 Documentation/security/keys-trusted-encrypted.txt |  3 ++
 crypto/hash_info.c|  2 ++
 drivers/char/tpm/tpm.h| 10 +--
 drivers/char/tpm/tpm2-cmd.c   | 36 +--
 include/crypto/hash_info.h|  3 ++
 include/keys/trusted-type.h   |  1 +
 include/uapi/linux/hash_info.h|  1 +
 security/keys/Kconfig |  1 +
 security/keys/trusted.c   | 27 -
 9 files changed, 77 insertions(+), 7 deletions(-)

diff --git a/Documentation/security/keys-trusted-encrypted.txt 
b/Documentation/security/keys-trusted-encrypted.txt
index e105ae9..fd2565b 100644
--- a/Documentation/security/keys-trusted-encrypted.txt
+++ b/Documentation/security/keys-trusted-encrypted.txt
@@ -38,6 +38,9 @@ Usage:
pcrlock=  pcr number to be extended to "lock" blob
migratable= 0|1 indicating permission to reseal to new PCR values,
default 1 (resealing allowed)
+   hash=  hash algorithm name as a string. For TPM 1.x the only
+  allowed value is sha1. For TPM 2.x the allowed values
+ are sha1, sha256, sha384, sha512 and sm3-256.
 
 "keyctl print" returns an ascii hex copy of the sealed key, which is in 
standard
 TPM_STORED_DATA format.  The key length for new keys are always in bytes.
diff --git a/crypto/hash_info.c b/crypto/hash_info.c
index 3e7ff46..7b1e0b1 100644
--- a/crypto/hash_info.c
+++ b/crypto/hash_info.c
@@ -31,6 +31,7 @@ const char *const hash_algo_name[HASH_ALGO__LAST] = {
[HASH_ALGO_TGR_128] = "tgr128",
[HASH_ALGO_TGR_160] = "tgr160",
[HASH_ALGO_TGR_192] = "tgr192",
+   [HASH_ALGO_SM3_256] = "sm3-256",
 };
 EXPORT_SYMBOL_GPL(hash_algo_name);
 
@@ -52,5 +53,6 @@ const int hash_digest_size[HASH_ALGO__LAST] = {
[HASH_ALGO_TGR_128] = TGR128_DIGEST_SIZE,
[HASH_ALGO_TGR_160] = TGR160_DIGEST_SIZE,
[HASH_ALGO_TGR_192] = TGR192_DIGEST_SIZE,
+   [HASH_ALGO_SM3_256] = SM3256_DIGEST_SIZE,
 };
 EXPORT_SYMBOL_GPL(hash_digest_size);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a4257a3..cdd49cd 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -83,16 +83,20 @@ enum tpm2_structures {
 };
 
 enum tpm2_return_codes {
-   TPM2_RC_INITIALIZE  = 0x0100,
-   TPM2_RC_TESTING = 0x090A,
+   TPM2_RC_HASH= 0x0083, /* RC_FMT1 */
+   TPM2_RC_INITIALIZE  = 0x0100, /* RC_VER1 */
TPM2_RC_DISABLED= 0x0120,
+   TPM2_RC_TESTING = 0x090A, /* RC_WARN */
 };
 
 enum tpm2_algorithms {
TPM2_ALG_SHA1   = 0x0004,
TPM2_ALG_KEYEDHASH  = 0x0008,
TPM2_ALG_SHA256 = 0x000B,
-   TPM2_ALG_NULL   = 0x0010
+   TPM2_ALG_SHA384 = 0x000C,
+   TPM2_ALG_SHA512 = 0x000D,
+   TPM2_ALG_NULL   = 0x0010,
+   TPM2_ALG_SM3_256= 0x0012,
 };
 
 enum tpm2_command_codes {
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index c121304..d9d0822 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -16,6 +16,7 @@
  */
 
 #include "tpm.h"
+#include 
 #include 
 
 enum tpm2_object_attributes {
@@ -104,6 +105,19 @@ struct tpm2_cmd {
union tpm2_cmd_params   params;
 } __packed;
 
+struct tpm2_hash {
+   unsigned int crypto_id;
+   unsigned int tpm_id;
+};
+
+static struct tpm2_hash tpm2_hash_map[] = {
+   {HASH_ALGO_SHA1, TPM2_ALG_SHA1},
+   {HASH_ALGO_SHA256, TPM2_ALG_SHA256},
+   {HASH_ALGO_SHA384, TPM2_ALG_SHA384},
+   {HASH_ALGO_SHA512, TPM2_ALG_SHA512},
+   {HASH_ALGO_SM3_256, TPM2_ALG_SM3_256},
+};
+
 /*
  * Array with one entry per ordinal defining the maximum amount
  * of 

[PATCH v3] keys, trusted: select hash algorithm for TPM2 chips

[Jarkko Sakkinen]

Added 'hash=' option for selecting the hash algorithm for add_key()
syscall and documentation for it.

Added entry for sm3-256 to the following tables in order to support
TPM_ALG_SM3_256:

* hash_algo_name
* hash_digest_size

Includes support for the following hash algorithms:

* sha1
* sha256
* sha384
* sha512
* sm3-256

v2:

* Added the missing dependency to CRYPTO_HASH_INFO

v3:

* Squashed patches into a single patch as the commits did not make
  alone any sense.
* Added a klog message when TPM 1.x is used for sealing and other than
  SHA-1 is used as the hash algorithm.
* Got rid of TPM2_HASH_COUNT and moved into ARRAY_SIZE(tpm2_hash_map).

Signed-off-by: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>
---
 Documentation/security/keys-trusted-encrypted.txt |  3 ++
 crypto/hash_info.c|  2 ++
 drivers/char/tpm/tpm.h| 10 --
 drivers/char/tpm/tpm2-cmd.c   | 40 +--
 include/crypto/hash_info.h|  3 ++
 include/keys/trusted-type.h   |  1 +
 include/uapi/linux/hash_info.h|  1 +
 security/keys/Kconfig |  1 +
 security/keys/trusted.c   | 23 -
 9 files changed, 77 insertions(+), 7 deletions(-)

diff --git a/Documentation/security/keys-trusted-encrypted.txt 
b/Documentation/security/keys-trusted-encrypted.txt
index e105ae9..fd2565b 100644
--- a/Documentation/security/keys-trusted-encrypted.txt
+++ b/Documentation/security/keys-trusted-encrypted.txt
@@ -38,6 +38,9 @@ Usage:
pcrlock=  pcr number to be extended to "lock" blob
migratable= 0|1 indicating permission to reseal to new PCR values,
default 1 (resealing allowed)
+   hash=  hash algorithm name as a string. For TPM 1.x the only
+  allowed value is sha1. For TPM 2.x the allowed values
+ are sha1, sha256, sha384, sha512 and sm3-256.
 
 "keyctl print" returns an ascii hex copy of the sealed key, which is in 
standard
 TPM_STORED_DATA format.  The key length for new keys are always in bytes.
diff --git a/crypto/hash_info.c b/crypto/hash_info.c
index 3e7ff46..7b1e0b1 100644
--- a/crypto/hash_info.c
+++ b/crypto/hash_info.c
@@ -31,6 +31,7 @@ const char *const hash_algo_name[HASH_ALGO__LAST] = {
[HASH_ALGO_TGR_128] = "tgr128",
[HASH_ALGO_TGR_160] = "tgr160",
[HASH_ALGO_TGR_192] = "tgr192",
+   [HASH_ALGO_SM3_256] = "sm3-256",
 };
 EXPORT_SYMBOL_GPL(hash_algo_name);
 
@@ -52,5 +53,6 @@ const int hash_digest_size[HASH_ALGO__LAST] = {
[HASH_ALGO_TGR_128] = TGR128_DIGEST_SIZE,
[HASH_ALGO_TGR_160] = TGR160_DIGEST_SIZE,
[HASH_ALGO_TGR_192] = TGR192_DIGEST_SIZE,
+   [HASH_ALGO_SM3_256] = SM3256_DIGEST_SIZE,
 };
 EXPORT_SYMBOL_GPL(hash_digest_size);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a4257a3..cdd49cd 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -83,16 +83,20 @@ enum tpm2_structures {
 };
 
 enum tpm2_return_codes {
-   TPM2_RC_INITIALIZE  = 0x0100,
-   TPM2_RC_TESTING = 0x090A,
+   TPM2_RC_HASH= 0x0083, /* RC_FMT1 */
+   TPM2_RC_INITIALIZE  = 0x0100, /* RC_VER1 */
TPM2_RC_DISABLED= 0x0120,
+   TPM2_RC_TESTING = 0x090A, /* RC_WARN */
 };
 
 enum tpm2_algorithms {
TPM2_ALG_SHA1   = 0x0004,
TPM2_ALG_KEYEDHASH  = 0x0008,
TPM2_ALG_SHA256 = 0x000B,
-   TPM2_ALG_NULL   = 0x0010
+   TPM2_ALG_SHA384 = 0x000C,
+   TPM2_ALG_SHA512 = 0x000D,
+   TPM2_ALG_NULL   = 0x0010,
+   TPM2_ALG_SM3_256= 0x0012,
 };
 
 enum tpm2_command_codes {
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index bd7039f..3acc7b5 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -16,6 +16,7 @@
  */
 
 #include "tpm.h"
+#include 
 #include 
 
 enum tpm2_object_attributes {
@@ -104,6 +105,19 @@ struct tpm2_cmd {
union tpm2_cmd_params   params;
 } __packed;
 
+struct tpm2_hash {
+   unsigned int crypto_id;
+   unsigned int tpm_id;
+};
+
+static struct tpm2_hash tpm2_hash_map[] = {
+   {HASH_ALGO_SHA1, TPM2_ALG_SHA1},
+   {HASH_ALGO_SHA256, TPM2_ALG_SHA256},
+   {HASH_ALGO_SHA384, TPM2_ALG_SHA384},
+   {HASH_ALGO_SHA512, TPM2_ALG_SHA512},
+   {HASH_ALGO_SM3_256, TPM2_ALG_SM3_256},
+};
+
 /*
  * Array with one entry per ordinal defining the maximum amount
  * of time the chip could take to return the result. The values
@@ -429,8 +443,24 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
 {
unsigned int blob_len;
struct tpm_buf buf;
+   u32 hash = TPM2_ALG_SHA256;
+   int i;
int rc;
 
+   if (o

[PATCH v4] keys, trusted: select hash algorithm for TPM2 chips

[Jarkko Sakkinen]

Added 'hash=' option for selecting the hash algorithm for add_key()
syscall and documentation for it.

Added entry for sm3-256 to the following tables in order to support
TPM_ALG_SM3_256:

* hash_algo_name
* hash_digest_size

Includes support for the following hash algorithms:

* sha1
* sha256
* sha384
* sha512
* sm3-256

v2:

* Added missing select CRYPTO_HASH_INFO in security/keys/Kconfig

v3:

* Squashed patches into a single patch as the commits did not make
  alone any sense.
* Added a klog message when TPM 1.x is used for sealing and other than
  SHA-1 is used as the hash algorithm.
* Got rid of TPM2_HASH_COUNT and moved into ARRAY_SIZE(tpm2_hash_map).

v4:

* Added missing select CRYPTO_HASH_INFO in drivers/char/tpm/Kconfig

Signed-off-by: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>
---
 Documentation/security/keys-trusted-encrypted.txt |  3 ++
 crypto/hash_info.c|  2 ++
 drivers/char/tpm/Kconfig  |  1 +
 drivers/char/tpm/tpm.h| 10 --
 drivers/char/tpm/tpm2-cmd.c   | 40 +--
 include/crypto/hash_info.h|  3 ++
 include/keys/trusted-type.h   |  1 +
 include/uapi/linux/hash_info.h|  1 +
 security/keys/Kconfig |  1 +
 security/keys/trusted.c   | 23 -
 10 files changed, 78 insertions(+), 7 deletions(-)

diff --git a/Documentation/security/keys-trusted-encrypted.txt 
b/Documentation/security/keys-trusted-encrypted.txt
index e105ae9..fd2565b 100644
--- a/Documentation/security/keys-trusted-encrypted.txt
+++ b/Documentation/security/keys-trusted-encrypted.txt
@@ -38,6 +38,9 @@ Usage:
pcrlock=  pcr number to be extended to "lock" blob
migratable= 0|1 indicating permission to reseal to new PCR values,
default 1 (resealing allowed)
+   hash=  hash algorithm name as a string. For TPM 1.x the only
+  allowed value is sha1. For TPM 2.x the allowed values
+ are sha1, sha256, sha384, sha512 and sm3-256.
 
 "keyctl print" returns an ascii hex copy of the sealed key, which is in 
standard
 TPM_STORED_DATA format.  The key length for new keys are always in bytes.
diff --git a/crypto/hash_info.c b/crypto/hash_info.c
index 3e7ff46..7b1e0b1 100644
--- a/crypto/hash_info.c
+++ b/crypto/hash_info.c
@@ -31,6 +31,7 @@ const char *const hash_algo_name[HASH_ALGO__LAST] = {
[HASH_ALGO_TGR_128] = "tgr128",
[HASH_ALGO_TGR_160] = "tgr160",
[HASH_ALGO_TGR_192] = "tgr192",
+   [HASH_ALGO_SM3_256] = "sm3-256",
 };
 EXPORT_SYMBOL_GPL(hash_algo_name);
 
@@ -52,5 +53,6 @@ const int hash_digest_size[HASH_ALGO__LAST] = {
[HASH_ALGO_TGR_128] = TGR128_DIGEST_SIZE,
[HASH_ALGO_TGR_160] = TGR160_DIGEST_SIZE,
[HASH_ALGO_TGR_192] = TGR192_DIGEST_SIZE,
+   [HASH_ALGO_SM3_256] = SM3256_DIGEST_SIZE,
 };
 EXPORT_SYMBOL_GPL(hash_digest_size);
diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 3b84a8b..bd86261 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -6,6 +6,7 @@ menuconfig TCG_TPM
tristate "TPM Hardware Support"
depends on HAS_IOMEM
select SECURITYFS
+   select CRYPTO_HASH_INFO
---help---
  If you have a TPM security chip in your system, which
  implements the Trusted Computing Group's specification,
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a4257a3..cdd49cd 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -83,16 +83,20 @@ enum tpm2_structures {
 };
 
 enum tpm2_return_codes {
-   TPM2_RC_INITIALIZE  = 0x0100,
-   TPM2_RC_TESTING = 0x090A,
+   TPM2_RC_HASH= 0x0083, /* RC_FMT1 */
+   TPM2_RC_INITIALIZE  = 0x0100, /* RC_VER1 */
TPM2_RC_DISABLED= 0x0120,
+   TPM2_RC_TESTING = 0x090A, /* RC_WARN */
 };
 
 enum tpm2_algorithms {
TPM2_ALG_SHA1   = 0x0004,
TPM2_ALG_KEYEDHASH  = 0x0008,
TPM2_ALG_SHA256 = 0x000B,
-   TPM2_ALG_NULL   = 0x0010
+   TPM2_ALG_SHA384 = 0x000C,
+   TPM2_ALG_SHA512 = 0x000D,
+   TPM2_ALG_NULL   = 0x0010,
+   TPM2_ALG_SM3_256= 0x0012,
 };
 
 enum tpm2_command_codes {
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index bd7039f..3acc7b5 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -16,6 +16,7 @@
  */
 
 #include "tpm.h"
+#include 
 #include 
 
 enum tpm2_object_attributes {
@@ -104,6 +105,19 @@ struct tpm2_cmd {
union tpm2_cmd_params   params;
 } __packed;
 
+struct tpm2_hash {
+   unsigned int crypto_id;
+   unsigned int tpm_id;
+};

[PATCH v2 2/3] crypto: add entry for sm3-256

[Jarkko Sakkinen]

Added entry for sm3-256 to the following tables:

* hash_algo_name
* hash_digest_size

Needed for TPM 2.0 trusted key sealing.

Signed-off-by: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>
---
 crypto/hash_info.c | 2 ++
 include/crypto/hash_info.h | 3 +++
 include/uapi/linux/hash_info.h | 1 +
 3 files changed, 6 insertions(+)

diff --git a/crypto/hash_info.c b/crypto/hash_info.c
index 3e7ff46..7b1e0b1 100644
--- a/crypto/hash_info.c
+++ b/crypto/hash_info.c
@@ -31,6 +31,7 @@ const char *const hash_algo_name[HASH_ALGO__LAST] = {
[HASH_ALGO_TGR_128] = "tgr128",
[HASH_ALGO_TGR_160] = "tgr160",
[HASH_ALGO_TGR_192] = "tgr192",
+   [HASH_ALGO_SM3_256] = "sm3-256",
 };
 EXPORT_SYMBOL_GPL(hash_algo_name);
 
@@ -52,5 +53,6 @@ const int hash_digest_size[HASH_ALGO__LAST] = {
[HASH_ALGO_TGR_128] = TGR128_DIGEST_SIZE,
[HASH_ALGO_TGR_160] = TGR160_DIGEST_SIZE,
[HASH_ALGO_TGR_192] = TGR192_DIGEST_SIZE,
+   [HASH_ALGO_SM3_256] = SM3256_DIGEST_SIZE,
 };
 EXPORT_SYMBOL_GPL(hash_digest_size);
diff --git a/include/crypto/hash_info.h b/include/crypto/hash_info.h
index e1e5a3e..56f217d 100644
--- a/include/crypto/hash_info.h
+++ b/include/crypto/hash_info.h
@@ -34,6 +34,9 @@
 #define TGR160_DIGEST_SIZE 20
 #define TGR192_DIGEST_SIZE 24
 
+/* not defined in include/crypto/ */
+#define SM3256_DIGEST_SIZE 32
+
 extern const char *const hash_algo_name[HASH_ALGO__LAST];
 extern const int hash_digest_size[HASH_ALGO__LAST];
 
diff --git a/include/uapi/linux/hash_info.h b/include/uapi/linux/hash_info.h
index ca18c45..ebf8fd8 100644
--- a/include/uapi/linux/hash_info.h
+++ b/include/uapi/linux/hash_info.h
@@ -31,6 +31,7 @@ enum hash_algo {
HASH_ALGO_TGR_128,
HASH_ALGO_TGR_160,
HASH_ALGO_TGR_192,
+   HASH_ALGO_SM3_256,
HASH_ALGO__LAST
 };
 
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 0/4] TPM2: select hash algorithm for a trusted key

[Jarkko Sakkinen]

Jarkko Sakkinen (4):
  crypto: add entry for sm3-256
  tpm: choose hash algorithm for sealing when using TPM 2.0
  keys, trusted: select the hash algorithm
  keys, trusted: update documentation for 'hash=' option

 Documentation/security/keys-trusted-encrypted.txt |  3 ++
 crypto/hash_info.c|  2 ++
 drivers/char/tpm/tpm.h| 10 --
 drivers/char/tpm/tpm2-cmd.c   | 42 +--
 include/crypto/hash_info.h|  3 ++
 include/keys/trusted-type.h   |  1 +
 include/uapi/linux/hash_info.h|  1 +
 security/keys/trusted.c   | 20 ++-
 8 files changed, 75 insertions(+), 7 deletions(-)

-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 1/4] crypto: add entry for sm3-256

[Jarkko Sakkinen]

Added entry for sm3-256 to the following tables:

* hash_algo_name
* hash_digest_size

Needed for TPM 2.0 trusted key sealing.

Signed-off-by: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>
---
 crypto/hash_info.c | 2 ++
 include/crypto/hash_info.h | 3 +++
 include/uapi/linux/hash_info.h | 1 +
 3 files changed, 6 insertions(+)

diff --git a/crypto/hash_info.c b/crypto/hash_info.c
index 3e7ff46..6f3a113 100644
--- a/crypto/hash_info.c
+++ b/crypto/hash_info.c
@@ -31,6 +31,7 @@ const char *const hash_algo_name[HASH_ALGO__LAST] = {
[HASH_ALGO_TGR_128] = "tgr128",
[HASH_ALGO_TGR_160] = "tgr160",
[HASH_ALGO_TGR_192] = "tgr192",
+   [HASH_ALGO_SM3_256] = "sm3-256",
 };
 EXPORT_SYMBOL_GPL(hash_algo_name);
 
@@ -52,5 +53,6 @@ const int hash_digest_size[HASH_ALGO__LAST] = {
[HASH_ALGO_TGR_128] = TGR128_DIGEST_SIZE,
[HASH_ALGO_TGR_160] = TGR160_DIGEST_SIZE,
[HASH_ALGO_TGR_192] = TGR192_DIGEST_SIZE,
+   [HASH_ALGO_SM3_256] = SM3_256_DIGEST_SIZE,
 };
 EXPORT_SYMBOL_GPL(hash_digest_size);
diff --git a/include/crypto/hash_info.h b/include/crypto/hash_info.h
index e1e5a3e..d86e050 100644
--- a/include/crypto/hash_info.h
+++ b/include/crypto/hash_info.h
@@ -34,6 +34,9 @@
 #define TGR160_DIGEST_SIZE 20
 #define TGR192_DIGEST_SIZE 24
 
+/* not defined in include/crypto/ */
+#define SM3_256_DIGEST_SIZE 32
+
 extern const char *const hash_algo_name[HASH_ALGO__LAST];
 extern const int hash_digest_size[HASH_ALGO__LAST];
 
diff --git a/include/uapi/linux/hash_info.h b/include/uapi/linux/hash_info.h
index ca18c45..ebf8fd8 100644
--- a/include/uapi/linux/hash_info.h
+++ b/include/uapi/linux/hash_info.h
@@ -31,6 +31,7 @@ enum hash_algo {
HASH_ALGO_TGR_128,
HASH_ALGO_TGR_160,
HASH_ALGO_TGR_192,
+   HASH_ALGO_SM3_256,
HASH_ALGO__LAST
 };
 
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html