Re: [PATCH RESEND] KEYS: trusted: Use ASN.1 encoded OID
On Thu, 2024-05-23 at 11:30 -0400, James Bottomley wrote:
> On Thu, 2024-05-23 at 16:54 +0300, Jarkko Sakkinen wrote:
> > On Thu May 23, 2024 at 4:38 PM EEST, James Bottomley wrote:
> > > On Thu, 2024-05-23 at 16:19 +0300, Jarkko Sakkinen wrote:
> > > > There's no reason to encode OID_TPMSealedData at run-time, as
> > > > it
> > > > never changes.
> > > >
> > > > Replace it with the encoded version, which has exactly the same
> > > > size:
> > > >
> > > > 67 81 05 0A 01 05
> > > >
> > > > Include OBJECT IDENTIFIER (0x06) tag and length as the epilogue
> > > > so
> > > > that the OID can be simply copied to the blob.
> > >
> > > This is true, but if we're going to do this, we should expand the
> > > OID
> > > registry functions (in lib/oid_registry.c) to do something like
> > > encode_OID. The registry already contains the hex above minus
> > > the
> > > two
> > > prefixes (which are easy to add).
> >
> > Yes, I do agree with this idea, and I named variable the I named
> > it to make it obvious that generation is possible.
> >
> > It would be best to have a single source, which could be just
> > a CSV file with entries like:
> >
> > ,
> >
> > And then in scripts/ there should be a script that takes this
> > source and generates oid_registry.gen.{h,c}. The existing
> > oid_registry.h should really just include oid_registry.gen.h
> > then to make this transparent change.
> >
> > And then in the series where OID's are encoded per-subsystem
> > patch that takes pre-encoded OID into use.
> >
> > Happy to review such patch set if it is pushed forward.
>
> Heh, OK, since I'm the one who thinks it's quite easy, I'll give it a
> go.
Turns out it's actually really simple. This would go as three patches:
adding the feature to lib/oid_registry.c using it in trusted keys and
removing the now unused OID encode from lib/asn1_encode.c but I'm
attaching here (minus the removal) to give an idea.
James
---
diff --git a/include/linux/oid_registry.h b/include/linux/oid_registry.h
index 51421fdbb0ba..87a6bcb2f5c0 100644
--- a/include/linux/oid_registry.h
+++ b/include/linux/oid_registry.h
@@ -151,5 +151,6 @@ extern enum OID look_up_OID(const void *data, size_t
datasize);
extern int parse_OID(const void *data, size_t datasize, enum OID *oid);
extern int sprint_oid(const void *, size_t, char *, size_t);
extern int sprint_OID(enum OID, char *, size_t);
+extern ssize_t encode_OID(enum OID, u8 *, size_t);
#endif /* _LINUX_OID_REGISTRY_H */
diff --git a/lib/oid_registry.c b/lib/oid_registry.c
index fe6705cfd780..45f97e1e0f91 100644
--- a/lib/oid_registry.c
+++ b/lib/oid_registry.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include "oid_registry_data.c"
MODULE_DESCRIPTION("OID Registry");
@@ -196,3 +197,31 @@ int sprint_OID(enum OID oid, char *buffer, size_t bufsize)
return ret;
}
EXPORT_SYMBOL_GPL(sprint_OID);
+
+/**
+ * encode_OID - embed an ASN.1 encoded OID in the provide buffer
+ * @oid: The OID to encode
+ * @buffer: The buffer to encode to
+ * @bufsize: the maximum size of the buffer
+ *
+ * Returns: negative error or encoded size in the buffer.
+ */
+ssize_t encode_OID(enum OID oid, u8 *buffer, size_t bufsize)
+{
+ int oid_size;
+
+ BUG_ON(oid >= OID__NR);
+
+ oid_size = oid_index[oid + 1] - oid_index[oid];
+
+ if (bufsize < oid_size + 2)
+ return -EINVAL;
+
+ buffer[0] = _tag(UNIV, PRIM, OID);
+ buffer[1] = oid_size;
+
+ memcpy(&buffer[2], &oid_data[oid_index[oid]], oid_size);
+
+ return oid_size;
+}
+EXPORT_SYMBOL_GPL(encode_OID);
diff --git a/security/keys/trusted-keys/trusted_tpm2.c
b/security/keys/trusted-keys/trusted_tpm2.c
index 9c7ac2e423d3..b6f34ff0ca5c 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -19,8 +19,6 @@
#include "tpm2key.asn1.h"
#include "tpm2-policy.h"
-static u32 tpm2key_oid[] = { 2, 23, 133, 10, 1, 5 };
-
static int tpm2_key_encode(struct trusted_key_payload *payload,
struct trusted_key_options *options,
u8 *src, u32 len)
@@ -31,6 +29,7 @@ static int tpm2_key_encode(struct trusted_key_payload
*payload,
u8 *end_work = scratch + SCRATCH_SIZE;
u8 *priv, *pub;
u16 priv_len, pub_len;
+ int ret;
priv_len = get_unaligned_be16(src) + 2;
priv = src;
@@ -43,8 +42,10 @@ static int tpm2_key_encode(struct trusted_key_payload
*payload,
if (!scratch)
return -ENOMEM;
- work = asn1_encode_oid(work, end_work, tpm2key_oid,
- asn1_oid_len(tpm2key_oid));
+ ret = encode_OID(OID_TPMSealedData, work, end_work - work);
+ if (ret < 0)
+ return ret;
+ work += ret;
if (options->blobauth_len == 0) {
unsigned char bool[3], *w = bool;
Re: [PATCH RESEND] KEYS: trusted: Use ASN.1 encoded OID
On Thu, 2024-05-23 at 18:37 +0300, Jarkko Sakkinen wrote:
> On Thu May 23, 2024 at 6:30 PM EEST, James Bottomley wrote:
> > On Thu, 2024-05-23 at 16:54 +0300, Jarkko Sakkinen wrote:
> > > On Thu May 23, 2024 at 4:38 PM EEST, James Bottomley wrote:
> > > > On Thu, 2024-05-23 at 16:19 +0300, Jarkko Sakkinen wrote:
> > > > > There's no reason to encode OID_TPMSealedData at run-time, as
> > > > > it never changes.
> > > > >
> > > > > Replace it with the encoded version, which has exactly the
> > > > > same size:
> > > > >
> > > > > 67 81 05 0A 01 05
> > > > >
> > > > > Include OBJECT IDENTIFIER (0x06) tag and length as the
> > > > > epilogue so that the OID can be simply copied to the blob.
> > > >
> > > > This is true, but if we're going to do this, we should expand
> > > > the OID registry functions (in lib/oid_registry.c) to do
> > > > something like encode_OID. The registry already contains the
> > > > hex above minus the two prefixes (which are easy to add).
> > >
> > > Yes, I do agree with this idea, and I named variable the I named
> > > it to make it obvious that generation is possible.
> > >
> > > It would be best to have a single source, which could be just
> > > a CSV file with entries like:
> > >
> > > ,
> > >
> > > And then in scripts/ there should be a script that takes this
> > > source and generates oid_registry.gen.{h,c}. The existing
> > > oid_registry.h should really just include oid_registry.gen.h
> > > then to make this transparent change.
> > >
> > > And then in the series where OID's are encoded per-subsystem
> > > patch that takes pre-encoded OID into use.
> > >
> > > Happy to review such patch set if it is pushed forward.
> >
> > Heh, OK, since I'm the one who thinks it's quite easy, I'll give it
> > a go.
>
> I guess if it cleaned up multiple sites in kernel then it could
> be considered useful. I'd guess that there is at least a few
> locations that also encode OID.
This should be the only one currently. The ASN.1 encoding was added to
the kernel to support the trusted keys pipe use case. However, if you
want the kernel to construct and pipe out asymmetric keys, that would
be the second use case.
James
Re: [PATCH RESEND] KEYS: trusted: Use ASN.1 encoded OID
On Thu, 2024-05-23 at 16:54 +0300, Jarkko Sakkinen wrote:
> On Thu May 23, 2024 at 4:38 PM EEST, James Bottomley wrote:
> > On Thu, 2024-05-23 at 16:19 +0300, Jarkko Sakkinen wrote:
> > > There's no reason to encode OID_TPMSealedData at run-time, as it
> > > never changes.
> > >
> > > Replace it with the encoded version, which has exactly the same
> > > size:
> > >
> > > 67 81 05 0A 01 05
> > >
> > > Include OBJECT IDENTIFIER (0x06) tag and length as the epilogue
> > > so
> > > that the OID can be simply copied to the blob.
> >
> > This is true, but if we're going to do this, we should expand the
> > OID
> > registry functions (in lib/oid_registry.c) to do something like
> > encode_OID. The registry already contains the hex above minus the
> > two
> > prefixes (which are easy to add).
>
> Yes, I do agree with this idea, and I named variable the I named
> it to make it obvious that generation is possible.
>
> It would be best to have a single source, which could be just
> a CSV file with entries like:
>
> ,
>
> And then in scripts/ there should be a script that takes this
> source and generates oid_registry.gen.{h,c}. The existing
> oid_registry.h should really just include oid_registry.gen.h
> then to make this transparent change.
>
> And then in the series where OID's are encoded per-subsystem
> patch that takes pre-encoded OID into use.
>
> Happy to review such patch set if it is pushed forward.
Heh, OK, since I'm the one who thinks it's quite easy, I'll give it a
go.
James
Re: [PATCH RESEND] KEYS: trusted: Use ASN.1 encoded OID
On Thu, 2024-05-23 at 16:19 +0300, Jarkko Sakkinen wrote: > There's no reason to encode OID_TPMSealedData at run-time, as it > never changes. > > Replace it with the encoded version, which has exactly the same size: > > 67 81 05 0A 01 05 > > Include OBJECT IDENTIFIER (0x06) tag and length as the epilogue so > that the OID can be simply copied to the blob. This is true, but if we're going to do this, we should expand the OID registry functions (in lib/oid_registry.c) to do something like encode_OID. The registry already contains the hex above minus the two prefixes (which are easy to add). I also note: > @ -51,8 +52,8 @@ static int tpm2_key_encode(struct > trusted_key_payload *payload, > if (!scratch) > return -ENOMEM; > > - work = asn1_encode_oid(work, end_work, tpm2key_oid, > - asn1_oid_len(tpm2key_oid)); > + work = memcpy(work, OID_TPMSealedData_ASN1, > sizeof(OID_TPMSealedData_ASN1)); > + work += sizeof(OID_TPMSealedData_ASN1); You lost the actually fits check. This is somewhat irrelevant for TPM keys because the OID is first in the structure and thus will never overflow, but it might matter for other uses. James
Re: [PATCH v2 4/6] KEYS: trusted: Move tpm2_key_decode() to the TPM driver
On Tue, 2024-05-21 at 22:44 +0100, David Howells wrote: > Jarkko Sakkinen wrote: > > > On Tue May 21, 2024 at 9:18 PM EEST, James Bottomley wrote: > > ... > > You don't save a single byte of memory with any constant that > > dictates the size requirements for multiple modules in two disjoint > > subsystems. > > I think James is just suggesting you replace your limit argument with > a constant not that you always allocate that amount of memory. Exactly. All we use it for is the -E2BIG check to ensure user space isn't allowed to run away with loads of kernel memory. > What the limit should be, OTOH, is up for discussion, but PAGE_SIZE > seems not unreasonable. A page is fine currently (MAX_BLOB_SIZE is 512). However, it may be too small for some of the complex policies when they're introduced. I'm not bothered about what it currently is, I just want it to be able to be increased easily when the time comes. James
Re: [PATCH v2 4/6] KEYS: trusted: Move tpm2_key_decode() to the TPM driver
On Tue, 2024-05-21 at 06:16 +0300, Jarkko Sakkinen wrote:
[...]
> diff --git a/include/crypto/tpm2_key.h b/include/crypto/tpm2_key.h
> new file mode 100644
> index ..acf41b2e0c92
> --- /dev/null
> +++ b/include/crypto/tpm2_key.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __LINUX_TPM2_KEY_H__
> +#define __LINUX_TPM2_KEY_H__
> +
> +#include
> +
> +/*
> + * TPM2 ASN.1 key
> + */
> +struct tpm2_key {
> + u32 parent;
> + const u8 *blob;
> + u32 blob_len;
> + const u8 *pub;
> + u32 pub_len;
> + const u8 *priv;
> + u32 priv_len;
> +};
> +
> +int tpm2_key_decode(const u8 *src, u32 src_len, struct tpm2_key
> *key,
> + u32 max_key_len);
I don't think this is a good idea. Trusted keys already have a pre-
defined max payload size (MAX_BLOB_SIZE in include/keys/trusted-type.h)
and I've already had to increase this several times because once you
get policy attached to a key, it can get pretty big (over a page).
Exactly the same thing will happen to asymmetric keys as well, so it
does make sense that they share the same maximum (probably in a more
generic header, though).
Since the code already right sizes the allocation and all we check with
this is whether it's over a pre-defined maximum, it's way easier if
that maximum is defined in a header rather than passed in in several
places making increasing the maximum really hard because you have to
chase all the threading.
James
Re: [PATCH] crypto: api - Do not load modules until algapi is ready
On May 18, 2024 5:32:56 AM PDT, Herbert Xu wrote: >On Sat, May 18, 2024 at 02:04:18PM +0300, Jarkko Sakkinen wrote: >> >> Right does this mean for TPM driver that a crypto API invocation not >> having everthing needed loaded will block until this is not the case? > >All this does is disable module loading by the Crypto API (because >there is no point and it may deadlock) until such a point where >all/most drivers have finished loading. > >So if the algorithm is missing (which shouldn't happen because of >Kconfig selects), then it will simply fail. I have a curiosity question: if Eric is right and it's looking for an optional hmac accelerator module, why don't I see this? The only real config difference between what I tested and what Nicholas did is he's arm and I'm x86. James -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [PATCH v8 18/22] tpm: add session encryption protection to tpm2_get_random()
On Fri, 2024-05-17 at 15:43 +0200, Ard Biesheuvel wrote:
> On Fri, 17 May 2024 at 15:35, James Bottomley
> wrote:
[...]
> > Thanks for the analysis. If I look at how CRYPTO_ECC does it, that
> > selects CRYPTO_RNG_DEFAULT which pulls in CRYPTO_DRBG, so the fix
> > would be the attached. Does that look right to you Ard?
>
> No it doesn't - it's CRYPTO_RNG_DEFAULT not CRYTPO_RNG_DEFAULT :-)
>
> With that fixed,
>
> Acked-by: Ard Biesheuvel
Erm, oops, sorry about that; so attached is the update.
James
---8>8>8><8<8<8---
>From 2ac337a33e6416ef806e2c692b9239d193e8468f Mon Sep 17 00:00:00 2001
From: James Bottomley
Date: Fri, 17 May 2024 06:29:31 -0700
Subject: [PATCH] tpm: Fix sessions cryptography requirement for Random Numbers
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The ECDH code in tpm2-sessions.c requires an initial random number
generator to generate the key pair. If the configuration doesn't have
CONFIG_RNG_DEFAULT, it will try to pull this in as a module (which is
impossible for the early kernel boot where the TPM starts). Fix this
by selecting the required RNG.
Reported-by: Nícolas F. R. A. Prado
Fixes: 1b6d7f9eb150 ("tpm: add session encryption protection to
tpm2_get_random()")
Acked-by: Ard Biesheuvel
Signed-off-by: James Bottomley
---
drivers/char/tpm/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 4f83ee7021d0..ecdd3db4be2b 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -31,6 +31,7 @@ config TCG_TPM2_HMAC
bool "Use HMAC and encrypted transactions on the TPM bus"
default y
select CRYPTO_ECDH
+ select CRYPTO_RNG_DEFAULT
select CRYPTO_LIB_AESCFB
select CRYPTO_LIB_SHA256
help
--
2.35.3
Re: [PATCH v8 18/22] tpm: add session encryption protection to tpm2_get_random()
On Fri, 2024-05-17 at 09:20 +0200, Ard Biesheuvel wrote:
> On Fri, 17 May 2024 at 03:59, James Bottomley
> wrote:
> >
> > On Thu, 2024-05-16 at 20:25 -0400, Nícolas F. R. A. Prado wrote:
> ...
> > > KernelCI has identified a new warning and I tracked it down to
> > > this
> > > commit. It
> > > was observed on the following platforms:
> > > * mt8183-kukui-jacuzzi-juniper-sku16
> > > * sc7180-trogdor-kingoftown
> > > (but probably affects all platforms that have a tpm driver with
> > > async
> > > probe)
> > >
> > > [ 2.175146] Call trace:
> > > [ 2.177587] __request_module+0x188/0x1f4
> > > [ 2.181596] crypto_alg_mod_lookup+0x178/0x21c
> > > [ 2.186042] crypto_alloc_tfm_node+0x58/0x114
> > > [ 2.190396] crypto_alloc_shash+0x24/0x30
> > > [ 2.194404] drbg_init_hash_kernel+0x28/0xdc
> > > [ 2.198673] drbg_kcapi_seed+0x21c/0x420
> > > [ 2.202593] crypto_rng_reset+0x84/0xb4
> > > [ 2.206425] crypto_get_default_rng+0xa4/0xd8
> > > [ 2.210779] ecc_gen_privkey+0x58/0xd0
> > > [ 2.214526] ecdh_set_secret+0x90/0x198
> > > [ 2.218360] tpm_buf_append_salt+0x164/0x2dc
> >
> > This looks like a misconfiguration. The kernel is trying to load
> > the
> > ecdh module, but it should have been selected as built in by this
> > in
> > drivers/char/tpm/Kconfig:
> >
> > config TCG_TPM2_HMAC
> > bool "Use HMAC and encrypted transactions on the TPM bus"
> > default y
> > select CRYPTO_ECDH
> > select CRYPTO_LIB_AESCFB
> > select CRYPTO_LIB_SHA256
> >
>
> The module request is not for ECDH itself but for the DRBG it
> attempts
> to use to generate the secret.
>
> Given that CRYPTO_ECDH does not strictly require a DRBG in principle,
> but does in this particular case, I think it makes sense to select
> CRYPTO_DRBG here (or depend on it being builtin), rather than
> updating the Kconfig rules for CRYPTO_ECDH itself.
Thanks for the analysis. If I look at how CRYPTO_ECC does it, that
selects CRYPTO_RNG_DEFAULT which pulls in CRYPTO_DRBG, so the fix would
be the attached. Does that look right to you Ard? And does it work
Nicolas?
James
---8>8>8><8<8<8---
>From 8c60ffd959eaa65627aca6596c35bb9cbfd9bee6 Mon Sep 17 00:00:00 2001
From: James Bottomley
Date: Fri, 17 May 2024 06:29:31 -0700
Subject: [PATCH] tpm: Fix sessions cryptography requirement for Random Numbers
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The ECDH code in tpm2-sessions.c requires an initial random number
generator to generate the key pair. If the configuration doesn't have
CONFIG_RNG_DEFAULT, it will try to pull this in as a module (which is
impossible for the early kernel boot where the TPM starts). Fix this
by selecting the required RNG.
Reported-by: Nícolas F. R. A. Prado
Fixes: 1b6d7f9eb150 ("tpm: add session encryption protection to
tpm2_get_random()")
Signed-off-by: James Bottomley
---
drivers/char/tpm/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 4f83ee7021d0..12065eddb922 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -31,6 +31,7 @@ config TCG_TPM2_HMAC
bool "Use HMAC and encrypted transactions on the TPM bus"
default y
select CRYPTO_ECDH
+ select CRYTPO_RNG_DEFAULT
select CRYPTO_LIB_AESCFB
select CRYPTO_LIB_SHA256
help
--
2.35.3
Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support
On Thu, 2024-03-14 at 04:52 -0700, James Prestwood wrote: > I'm also not entirely sure why this stuff continues to be removed > from the kernel. First MD4, then it got reverted, then this (now > reverted, thanks). Both cases there was not clear justification of > why it was being removed. I think this is some misunderstanding of the NIST and FIPS requirements with regards to hashes, ciphers and bits of security. The bottom line is that neither NIST nor FIPS requires the removal of the sha1 algorithm at all. Both of them still support it for HMAC (for now). In addition, the FIPS requirement is only that you not *issue* sha1 hashed signatures. FIPS still allows you to verify legacy signatures with sha1 as the signing hash (for backwards compatibility reasons). Enterprises with no legacy and no HMAC requirements *may* remove the hash, but it's not mandated. So *removing* sha1 from the certificate code was the wrong thing to do. We should have configurably prevented using sha1 as the algorithm for new signatures but kept it for signature verification. Can we please get this sorted out before 2025, because next up is the FIPS requirement to move to at least 128 bits of security which will see RSA2048 deprecated in a similar way: We should refuse to issue RSA2048 signatures, but will still be allowed to verify them for legacy reasons. James
Re: [PATCH v1 0/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
On Thu, 2021-04-01 at 18:50 +0530, Sumit Garg wrote:
> On Thu, 1 Apr 2021 at 15:36, Ahmad Fatoum
> wrote:
> > Hello Richard,
> >
> > On 31.03.21 21:36, Richard Weinberger wrote:
> > > James,
> > >
> > > - Ursprüngliche Mail -
> > > > Von: "James Bottomley"
> > > > Well, yes. For the TPM, there's a defined ASN.1 format for the
> > > > keys:
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/jejb/openssl_tpm2_engine.git/tree/tpm2-asn.h
> > > >
> > > > and part of the design of the file is that it's distinguishable
> > > > either
> > > > in DER or PEM (by the guards) format so any crypto application
> > > > can know
> > > > it's dealing with a TPM key simply by inspecting the file. I
> > > > think you
> > > > need the same thing for CAAM and any other format.
> > > >
> > > > We're encouraging new ASN.1 formats to be of the form
> > > >
> > > > SEQUENCE {
> > > >type OBJECT IDENTIFIER
> > > >... key specific fields ...
> > > > }
> > > >
> > > > Where you choose a defined OID to represent the key and that
> > > > means
> > > > every key even in DER form begins with a unique binary
> > > > signature.
> > >
> > > I like this idea.
> > > Ahmad, what do you think?
> > >
> > > That way we could also get rid off the kernel parameter and all
> > > the fall back logic,
> > > given that we find a way to reliable detect TEE blobs too...
> >
> > Sounds good to me. Sumit, your thoughts on doing this for TEE as
> > well?
> >
>
> AFAIU, ASN.1 formating should be independent of trusted keys backends
> which could be abstracted to trusted keys core layer so that every
> backend could be plugged in seamlessly.
>
> James,
>
> Would it be possible to achieve this?
I'm not quite sure what you're asking. The saved format of the keys is
particular to the underlying hardware. The ASN.1 wraps this so we have
an identifier, some useful information to help load the key (like the
policy systemements) and then the blobs the hardware expects.
This makes the ASN.1 specific to the backend but having a
distinguishing OID that allows anyone to tell which backend it needs
from the file.
So you can call the ASN.1 format that begins with the type OID, the
"universal" format, but if you mean there should be a standard ASN.1
format for all keys that defines all the fields then that's not
possible because the fields after type are very specific to the
underlying hardware.
James
Re: [PATCH v1 0/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
On Wed, 2021-03-31 at 20:36 +0200, Richard Weinberger wrote:
> James,
>
> - Ursprüngliche Mail -
> > Von: "James Bottomley"
> > > On Wed, Mar 17, 2021 at 3:08 PM Ahmad Fatoum <
> > > a.fat...@pengutronix.de wrote:
> > > > keyctl add trusted $KEYNAME "load $(cat ~/kmk.blob)" @s
> > >
> > > Is there a reason why we can't pass the desired backend name in
> > > the
> > > trusted key parameters?
> > > e.g.
> > > keyctl add trusted $KEYNAME "backendtype caam load $(cat
> > > ~/kmk.blob)"
> > > @s
> >
> > Why would you want to in the load? The blob should be type
> > specific, so a TPM key shouldn't load as a CAAM key and vice versa
> > ... and if they're not they need to be made so before the patches
> > go upstream.
>
> I fear right now there is no good way to detect whether a blob is
> desired for CAAM or TPM.
At least for the TPM the old format is two TPM2B structures, and the
new one is ASN.1 so either should be completely distinguishable over
what CAAM does.
> > I could possibly see that you might want to be type specific in the
> > create, but once you're simply loading an already created key, the
> > trusted key subsystem should be able to figure what to do on its
> > own.
>
> So you have some kind of container format in mind which denotes the
> type of the blob?
Well, yes. For the TPM, there's a defined ASN.1 format for the keys:
https://git.kernel.org/pub/scm/linux/kernel/git/jejb/openssl_tpm2_engine.git/tree/tpm2-asn.h
and part of the design of the file is that it's distinguishable either
in DER or PEM (by the guards) format so any crypto application can know
it's dealing with a TPM key simply by inspecting the file. I think you
need the same thing for CAAM and any other format.
We're encouraging new ASN.1 formats to be of the form
SEQUENCE {
type OBJECT IDENTIFIER
... key specific fields ...
}
Where you choose a defined OID to represent the key and that means
every key even in DER form begins with a unique binary signature.
James
Re: [PATCH v1 0/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
On Wed, 2021-03-31 at 00:04 +0200, Richard Weinberger wrote: > Ahmad, > > On Wed, Mar 17, 2021 at 3:08 PM Ahmad Fatoum > wrote: > > keyctl add trusted $KEYNAME "load $(cat ~/kmk.blob)" @s > > Is there a reason why we can't pass the desired backend name in the > trusted key parameters? > e.g. > keyctl add trusted $KEYNAME "backendtype caam load $(cat ~/kmk.blob)" > @s Why would you want to in the load? The blob should be type specific, so a TPM key shouldn't load as a CAAM key and vice versa ... and if they're not they need to be made so before the patches go upstream. I could possibly see that you might want to be type specific in the create, but once you're simply loading an already created key, the trusted key subsystem should be able to figure what to do on its own. James
Re: [PATCH v1 3/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
On Wed, 2021-03-24 at 16:49 -0400, Mimi Zohar wrote:
> On Wed, 2021-03-24 at 09:14 -0700, James Bottomley wrote:
> > On Tue, 2021-03-23 at 14:07 -0400, Mimi Zohar wrote:
> > > On Tue, 2021-03-23 at 17:35 +0100, Ahmad Fatoum wrote:
> > > > Hello Horia,
> > > >
> > > > On 21.03.21 21:48, Horia Geantă wrote:
> > > > > On 3/16/2021 7:02 PM, Ahmad Fatoum wrote:
> > > > > [...]
> > > > > > +struct trusted_key_ops caam_trusted_key_ops = {
> > > > > > + .migratable = 0, /* non-migratable */
> > > > > > + .init = trusted_caam_init,
> > > > > > + .seal = trusted_caam_seal,
> > > > > > + .unseal = trusted_caam_unseal,
> > > > > > + .exit = trusted_caam_exit,
> > > > > > +};
> > > > > caam has random number generation capabilities, so it's worth
> > > > > using that
> > > > > by implementing .get_random.
> > > >
> > > > If the CAAM HWRNG is already seeding the kernel RNG, why not
> > > > use
> > > > the kernel's?
> > > >
> > > > Makes for less code duplication IMO.
> > >
> > > Using kernel RNG, in general, for trusted keys has been discussed
> > > before. Please refer to Dave Safford's detailed explanation for
> > > not
> > > using it [1].
> > >
> > > [1]
> > > https://lore.kernel.org/linux-integrity/bca04d5d9a3b764c9b7405bba4d4a3c035f2a...@alpmbapa12.e2k.ad.ge.com/
> >
> > I still don't think relying on one source of randomness to be
> > cryptographically secure is a good idea. The fear of bugs in the
> > kernel entropy pool is reasonable, but since it's widely used
> > they're
> > unlikely to persist very long. Studies have shown that some TPMs
> > (notably the chinese manufactured ones) have suspicious failures in
> > their RNGs:
> >
> > https://www.researchgate.net/publication/45934562_Benchmarking_the_True_Random_Number_Generator_of_TPM_Chips
> >
> > And most cryptograhpers recommend using a TPM for entropy mixing
> > rather
> > than directly:
> >
> > https://blog.cryptographyengineering.com/category/rngs/
> >
> > The TPMFail paper also shows that in spite of NIST certification
> > things can go wrong with a TPM:
> >
> > https://tpm.fail/
>
> We already had a lengthy discussion on replacing the TPM RNG with the
> kernel RNG for trusted keys, when TEE was being introduced
> [2,3]. I'm not interested in re-hashing that discussion here. The
> only difference now is that CAAM is a new trust source. I suspect
> the same concerns/issues persist, but at least in this case using the
> kernel RNG would not be a regression.
Upstreaming the ASN.1 parser gives us a way to create trusted keys
outside the kernel and so choose any RNG that suits the user, so I
don't think there's any need to rehash for TPM based keys either.
However CaaM doesn't have the ability to create keys outside the kernel
yet, so they do need to consider the problem.
James
> [2] Pascal Van Leeuwen on mixing different sources of entropy and
> certification -
>
> https://lore.kernel.org/linux-integrity/mn2pr20mb29732a856a40131a671f949fca...@mn2pr20mb2973.namprd20.prod.outlook.com/
> [3] Jarrko on "regression" and tpm_asym.c -
> https://lore.kernel.org/linux-integrity/20191014190033.ga15...@linux.intel.com/
>
>
> Mimi
>
Re: [PATCH v1 3/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
On Tue, 2021-03-23 at 14:07 -0400, Mimi Zohar wrote:
> On Tue, 2021-03-23 at 17:35 +0100, Ahmad Fatoum wrote:
> > Hello Horia,
> >
> > On 21.03.21 21:48, Horia Geantă wrote:
> > > On 3/16/2021 7:02 PM, Ahmad Fatoum wrote:
> > > [...]
> > > > +struct trusted_key_ops caam_trusted_key_ops = {
> > > > + .migratable = 0, /* non-migratable */
> > > > + .init = trusted_caam_init,
> > > > + .seal = trusted_caam_seal,
> > > > + .unseal = trusted_caam_unseal,
> > > > + .exit = trusted_caam_exit,
> > > > +};
> > > caam has random number generation capabilities, so it's worth
> > > using that
> > > by implementing .get_random.
> >
> > If the CAAM HWRNG is already seeding the kernel RNG, why not use
> > the kernel's?
> >
> > Makes for less code duplication IMO.
>
> Using kernel RNG, in general, for trusted keys has been discussed
> before. Please refer to Dave Safford's detailed explanation for not
> using it [1].
>
> thanks,
>
> Mimi
>
> [1]
> https://lore.kernel.org/linux-integrity/bca04d5d9a3b764c9b7405bba4d4a3c035f2a...@alpmbapa12.e2k.ad.ge.com/
I still don't think relying on one source of randomness to be
cryptographically secure is a good idea. The fear of bugs in the
kernel entropy pool is reasonable, but since it's widely used they're
unlikely to persist very long. Studies have shown that some TPMs
(notably the chinese manufactured ones) have suspicious failures in
their RNGs:
https://www.researchgate.net/publication/45934562_Benchmarking_the_True_Random_Number_Generator_of_TPM_Chips
And most cryptograhpers recommend using a TPM for entropy mixing rather
than directly:
https://blog.cryptographyengineering.com/category/rngs/
The TPMFail paper also shows that in spite of NIST certification
things can go wrong with a TPM:
https://tpm.fail/
James
Re: [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
On Tue, 2020-09-15 at 20:49 -0400, Eric Snowberg wrote: > The Secure Boot Forbidden Signature Database, dbx, contains a list of > now revoked signatures and keys previously approved to boot with UEFI > Secure Boot enabled. The dbx is capable of containing any number of > EFI_CERT_X509_SHA256_GUID, EFI_CERT_SHA256_GUID, and > EFI_CERT_X509_GUID entries. > > Currently when EFI_CERT_X509_GUID are contained in the dbx, the > entries are skipped. > > Add support for EFI_CERT_X509_GUID dbx entries. When a > EFI_CERT_X509_GUID is found, it is added as an asymmetrical key to > the .blacklist keyring. Anytime the .platform keyring is used, the > keys in the .blacklist keyring are referenced, if a matching key is > found, the key will be rejected. > > Signed-off-by: Eric Snowberg If you're using shim, as most of our users are, you have no access to dbx to blacklist certificates. Plus our security envelope includes the Mok variables, so you should also be paying attestion to MokListX (or it's RT equivalent: MokListXRT). If you add this to the patch, we get something that is mechanistically complete and which also allows users to add certs to their Mok blacklist. James
Re: [PATCH] certs: Add EFI_CERT_X509_GUID support for dbx entries
On Wed, 2021-01-13 at 13:40 +, David Howells wrote: > Hi Linus, > > Are you willing to take this between merge windows - or does it need > to wait for the next merge window? It's not technically a bug fix to > the kernel, but it does have a CVE attached to it. > > Note that I've also updated Jarkko's address in his Reviewed-by since > his Intel address no longer works. Sorry, late to the party. I suppose I lost the argument that we shouldn't really be trusting any certs from db when shim is in operation because they're all EFI binary signing ones and will usually simply be the microsoft certificate and possibly an OEM platform one and we're usually pivoting the root of trust to the certificates in the MokList. However, if we are going to do this, we should also be blacklisting the certificates in MokListX which the OS sees through MokListXRT. Since MokListX is an essential piece of our revocation infrastructure it should have been mentioned in the CVE but wasn't for some reason. James
Re: [PATCH] KVM/SVM: add support for SEV attestation command
On Wed, 2020-12-09 at 21:25 -0600, Brijesh Singh wrote:
> Noted, I will send v2 with these fixed.
I ran a test on this. It turns out for rome systems you need firmware
md_sev_fam17h_model3xh_0.24b0A (or later) installed to get this and the
QEMU patch with the base64 decoding fixed, but with that
Tested-by: James Bottomley
Attached is the test programme I used.
James
---
#!/usr/bin/python3
##
# Python script get an attestation and verify it with the PEK
#
# This assumes you've already exported the pek.cert with sev-tool
# from https://github.com/AMDESE/sev-tool.git
#
# sev-tool --export_cert_chain
#
# creates several files, the only one this script needs is pek.cert
#
# Tables and chapters refer to the amd 55766.pdf document
#
# https://www.amd.com/system/files/TechDocs/55766_SEV-KM_API_Specification.pdf
##
import sys
import os
import base64
import hashlib
from argparse import ArgumentParser
from Crypto.PublicKey import ECC
from Crypto.Math.Numbers import Integer
from git.qemu.python.qemu import qmp
if __name__ == "__main__":
parser = ArgumentParser(description='Inject secret into SEV')
parser.add_argument('--pek-cert',
help='The Platform DH certificate in binary form',
default='pek.cert')
parser.add_argument('--socket',
help='Socket to connect to QMP on, defaults to
localhost:6550',
default='localhost:6550')
args = parser.parse_args()
if (args.socket[0] == '/'):
socket = args.socket
elif (':' in args.socket):
s = args.socket.split(':')
socket = (s[0], int(s[1]))
else:
parse.error('--socket must be : or /path/to/unix')
fh = open(args.pek_cert, 'rb')
pek = bytearray(fh.read())
curve = int.from_bytes(pek[16:20], byteorder='little')
curves = {
1: 'p256',
2: 'p384'
}
Qx = int.from_bytes(bytes(pek[20:92]), byteorder='little')
Qy = int.from_bytes(bytes(pek[92:164]), byteorder='little')
pubkey = ECC.construct(point_x=Qx, point_y=Qy, curve=curves[curve])
Qmp = qmp.QEMUMonitorProtocol(address=socket);
Qmp.connect()
caps = Qmp.command('query-sev')
print('SEV query found API={api-major}.{api-minor} build={build-id}
policy={policy}\n'.format(**caps))
nonce=os.urandom(16)
report = Qmp.command('query-sev-attestation-report',
mnonce=base64.b64encode(nonce).decode())
a = base64.b64decode(report['data'])
##
# returned data is formulated as Table 60. Attestation Report Buffer
##
rnonce = a[0:16]
rmeas = a[16:48]
if (nonce != rnonce):
sys.exit('returned nonce doesn\'t match input nonce')
policy = int.from_bytes(a[48:52], byteorder='little')
usage = int.from_bytes(a[52:56], byteorder='little')
algo = int.from_bytes(a[56:60], byteorder='little')
if (policy != caps['policy']):
sys.exit('Policy mismatch:', policy, '!=', caps['policy'])
if (usage != 0x1002):
sys.exit('error PEK is not specified in usage: ', usage)
if (algo == 0x2):
h = hashlib.sha256()
elif (algo == 0x102):
##
# The spec (6.8) says the signature must be ECDSA-SHA256 so this
# should be impossible, but it turns out to be the way our
# current test hardware produces its signature
##
h = hashlib.sha384()
else:
sys.exit('unrecognized signing algorithm: ', algo)
h.update(a[0:52])
sig = a[64:208]
r = int.from_bytes(sig[0:72],byteorder='little')
s = int.from_bytes(sig[72:144],byteorder='little')
##
# subtlety: r and s are little (AMD defined) z is big (crypto requirement)
##
z = int.from_bytes(h.digest(), byteorder='big')
##
# python crypto doesn't have a way of passing in r and s as
# integers and I'm not inclined to wrap them up as a big endian
# binary signature to have Signature.DSS unwrap them again, so
# call the _verify() private interface that does take integers
##
if (not pubkey._verify(Integer(z), (Integer(r), Integer(s:
sys.exit('returned signature did not verify')
print('usage={usage}, algorithm={algo}'.format(usage=hex(usage),
algo=hex(algo)))
print('ovmf-hash: ', rmeas.hex())
Re: [RFC PATCH v1 00/12] Replace strstarts() by str_has_prefix()
On Fri, 2020-12-04 at 18:03 +0100, laniel_fran...@privacyrequired.com
wrote:
> In this patch set, I replaced all calls to strstarts() by calls to
> str_has_prefix(). Indeed, the kernel has two functions to test if a
> string begins with an other:
> 1. strstarts() which returns a bool, so 1 if the string begins with
> the prefix,0 otherwise.
> 2. str_has_prefix() which returns the length of the prefix or 0.
>
> str_has_prefix() was introduced later than strstarts(), in commit
> 495d714ad140 which also stated that str_has_prefix() should replace
> strstarts(). This is what this patch set does.
What's the reason why? If you look at the use cases for the
replacement of strstart() they're all cases where we need to know the
length we're skipping and this is hard coded, leading to potential
errors later. This is a classic example: 3d739c1f6156 ("tracing: Use
the return of str_has_prefix() to remove open coded numbers"). However
you're not doing this transformation in the conversion, so the
conversion is pretty useless. I also see no case for replacing
strstart() where we're using it simply as a boolean without needing to
know the length of the prefix.
James
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Tue, 2020-11-24 at 13:32 -0800, Kees Cook wrote: > On Mon, Nov 23, 2020 at 08:31:30AM -0800, James Bottomley wrote: > > Really, no ... something which produces no improvement has no value > > at all ... we really shouldn't be wasting maintainer time with it > > because it has a cost to merge. I'm not sure we understand where > > the balance lies in value vs cost to merge but I am confident in > > the zero value case. > > What? We can't measure how many future bugs aren't introduced because > the kernel requires explicit case flow-control statements for all new > code. No but we can measure how vulnerable our current coding habits are to the mistake this warning would potentially prevent. I don't think it's wrong to extrapolate that if we had no instances at all of prior coding problems we likely wouldn't have any in future either making adopting the changes needed to enable the warning valueless ... that's the zero value case I was referring to above. Now, what we have seems to be about 6 cases (at least what's been shown in this thread) where a missing break would cause potentially user visible issues. That means the value of this isn't zero, but it's not a no-brainer massive win either. That's why I think asking what we've invested vs the return isn't a useless exercise. > We already enable -Wimplicit-fallthrough globally, so that's not the > discussion. The issue is that Clang is (correctly) even more strict > than GCC for this, so these are the remaining ones to fix for full > Clang coverage too. > > People have spent more time debating this already than it would have > taken to apply the patches. :) You mean we've already spent 90% of the effort to come this far so we might as well go the remaining 10% because then at least we get some return? It's certainly a clinching argument in defence procurement ... > This is about robustness and language wrangling. It's a big code- > base, and this is the price of our managing technical debt for > permanent robustness improvements. (The numbers I ran from Gustavo's > earlier patches were that about 10% of the places adjusted were > identified as legitimate bugs being fixed. This final series may be > lower, but there are still bugs being found from it -- we need to > finish this and shut the door on it for good.) I got my six patches by analyzing the lwn.net report of the fixes that was cited which had 21 of which 50% didn't actually change the emitted code, and 25% didn't have a user visible effect. But the broader point I'm making is just because the compiler people come up with a shiny new warning doesn't necessarily mean the problem it's detecting is one that causes us actual problems in the code base. I'd really be happier if we had a theory about what classes of CVE or bug we could eliminate before we embrace the next new warning. James
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, 2020-11-23 at 19:56 +0100, Miguel Ojeda wrote: > On Mon, Nov 23, 2020 at 4:58 PM James Bottomley > wrote: > > Well, I used git. It says that as of today in Linus' tree we have > > 889 patches related to fall throughs and the first series went in > > in october 2017 ... ignoring a couple of outliers back to February. > > I can see ~10k insertions over ~1k commits and 15 years that mention > a fallthrough in the entire repo. That is including some commits > (like the biggest one, 960 insertions) that have nothing to do with C > fallthrough. A single kernel release has an order of magnitude more > changes than this... > > But if we do the math, for an author, at even 1 minute per line > change and assuming nothing can be automated at all, it would take 1 > month of work. For maintainers, a couple of trivial lines is noise > compared to many other patches. So you think a one line patch should take one minute to produce ... I really don't think that's grounded in reality. I suppose a one line patch only takes a minute to merge with b4 if no-one reviews or tests it, but that's not really desirable. > In fact, this discussion probably took more time than the time it > would take to review the 200 lines. :-) I'm framing the discussion in terms of the whole series of changes we have done for fall through, both what's in the tree currently (889 patches) both in terms of the produce and the consumer. That's what I used for my figures for cost. > > We're also complaining about the inability to recruit maintainers: > > > > https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/ > > > > And burn out: > > > > http://antirez.com/news/129 > > Accepting trivial and useful 1-line patches Part of what I'm trying to measure is the "and useful" bit because that's not a given. > is not what makes a voluntary maintainer quit... so the proverb "straw which broke the camel's back" uniquely doesn't apply to maintainers > Thankless work with demanding deadlines is. That's another potential reason, but it doesn't may other reasons less valid. > > The whole crux of your argument seems to be maintainers' time isn't > > important so we should accept all trivial patches > > I have not said that, at all. In fact, I am a voluntary one and I > welcome patches like this. It takes very little effort on my side to > review and it helps the kernel overall. Well, you know, subsystems are very different in terms of the amount of patches a maintainer has to process per release cycle of the kernel. If a maintainer is close to capacity, additional patches, however trivial, become a problem. If a maintainer has spare cycles, trivial patches may look easy. > Paid maintainers are the ones that can take care of big > features/reviews. > > > What I'm actually trying to articulate is a way of measuring value > > of the patch vs cost ... it has nothing really to do with who foots > > the actual bill. > > I understand your point, but you were the one putting it in terms of > a junior FTE. No, I evaluated the producer side in terms of an FTE. What we're mostly arguing about here is the consumer side: the maintainers and people who have to rework their patch sets. I estimated that at 100h. > In my view, 1 month-work (worst case) is very much worth > removing a class of errors from a critical codebase. > > > One thesis I'm actually starting to formulate is that this > > continual devaluing of maintainers is why we have so much > > difficulty keeping and recruiting them. > > That may very well be true, but I don't feel anybody has devalued > maintainers in this discussion. You seem to be saying that because you find it easy to merge trivial patches, everyone should. I'm reminded of a friend long ago who thought being a Tees River Pilot was a sinecure because he could navigate the Tees blindfold. What he forgot, of course, is that just because it's easy with a trawler doesn't mean it's easy with an oil tanker. In fact it takes longer to qualify as a Tees River Pilot than it does to get a PhD. James
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, 2020-11-23 at 07:03 -0600, Gustavo A. R. Silva wrote: > On Sun, Nov 22, 2020 at 11:53:55AM -0800, James Bottomley wrote: > > On Sun, 2020-11-22 at 11:22 -0800, Joe Perches wrote: > > > On Sun, 2020-11-22 at 11:12 -0800, James Bottomley wrote: > > > > On Sun, 2020-11-22 at 10:25 -0800, Joe Perches wrote: > > > > > On Sun, 2020-11-22 at 10:21 -0800, James Bottomley wrote: > > > > > > Please tell me our reward for all this effort isn't a > > > > > > single missing error print. > > > > > > > > > > There were quite literally dozens of logical defects found > > > > > by the fallthrough additions. Very few were logging only. > > > > > > > > So can you give us the best examples (or indeed all of them if > > > > someone is keeping score)? hopefully this isn't a US election > > > > situation ... > > > > > > Gustavo? Are you running for congress now? > > > > > > https://lwn.net/Articles/794944/ > > > > That's 21 reported fixes of which about 50% seem to produce no > > change in code behaviour at all, a quarter seem to have no user > > visible effect with the remaining quarter producing unexpected > > errors on obscure configuration parameters, which is why no-one > > really noticed them before. > > The really important point here is the number of bugs this has > prevented and will prevent in the future. See an example of this, > below: > > https://lore.kernel.org/linux-iio/20190813135802.gb27...@kroah.com/ I think this falls into the same category as the other six bugs: it changes the output/input for parameters but no-one has really noticed, usually because the command is obscure or the bias effect is minor. > This work is still relevant, even if the total number of issues/bugs > we find in the process is zero (which is not the case). Really, no ... something which produces no improvement has no value at all ... we really shouldn't be wasting maintainer time with it because it has a cost to merge. I'm not sure we understand where the balance lies in value vs cost to merge but I am confident in the zero value case. > "The sucky thing about doing hard work to deploy hardening is that > the result is totally invisible by definition (things not happening) > [..]" > - Dmitry Vyukov Really, no. Something that can't be measured at all doesn't exist. And actually hardening is one of those things you can measure (which I do have to admit isn't true for everything in the security space) ... it's number of exploitable bugs found before you did it vs number of exploitable bugs found after you did it. Usually hardening eliminates a class of bug, so the way I've measured hardening before is to go through the CVE list for the last couple of years for product X, find all the bugs that are of the class we're looking to eliminate and say if we had hardened X against this class of bug we'd have eliminated Y% of the exploits. It can be quite impressive if Y is a suitably big number. James
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, 2020-11-23 at 15:19 +0100, Miguel Ojeda wrote: > On Sun, Nov 22, 2020 at 11:36 PM James Bottomley > wrote: > > Well, it seems to be three years of someone's time plus the > > maintainer review time and series disruption of nearly a thousand > > patches. Let's be conservative and assume the producer worked > > about 30% on the series and it takes about 5-10 minutes per patch > > to review, merge and for others to rework existing series. So > > let's say it's cost a person year of a relatively junior engineer > > producing the patches and say 100h of review and application > > time. The latter is likely the big ticket item because it's what > > we have in least supply in the kernel (even though it's 20x vs the > > producer time). > > How are you arriving at such numbers? It is a total of ~200 trivial > lines. Well, I used git. It says that as of today in Linus' tree we have 889 patches related to fall throughs and the first series went in in october 2017 ... ignoring a couple of outliers back to February. > > It's not about the risk of the changes it's about the cost of > > implementing them. Even if you discount the producer time (which > > someone gets to pay for, and if I were the engineering manager, I'd > > be unhappy about), the review/merge/rework time is pretty > > significant in exchange for six minor bug fixes. Fine, when a new > > compiler warning comes along it's certainly reasonable to see if we > > can benefit from it and the fact that the compiler people think > > it's worthwhile is enough evidence to assume this initially. But > > at some point you have to ask whether that assumption is supported > > by the evidence we've accumulated over the time we've been using > > it. And if the evidence doesn't support it perhaps it is time to > > stop the experiment. > > Maintainers routinely review 1-line trivial patches, not to mention > internal API changes, etc. We're also complaining about the inability to recruit maintainers: https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/ And burn out: http://antirez.com/news/129 The whole crux of your argument seems to be maintainers' time isn't important so we should accept all trivial patches ... I'm pushing back on that assumption in two places, firstly the valulessness of the time and secondly that all trivial patches are valuable. > If some company does not want to pay for that, that's fine, but they > don't get to be maintainers and claim `Supported`. What I'm actually trying to articulate is a way of measuring value of the patch vs cost ... it has nothing really to do with who foots the actual bill. One thesis I'm actually starting to formulate is that this continual devaluing of maintainers is why we have so much difficulty keeping and recruiting them. James
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, 2020-11-23 at 09:54 +1100, Finn Thain wrote: > But is anyone keeping score of the regressions? If unreported bugs > count, what about unreported regressions? Well, I was curious about the former (obviously no tool will tell me about the latter), so I asked git what patches had a fall-through series named in a fixes tag and these three popped out: 9cf51446e686 bpf, powerpc: Fix misuse of fallthrough in bpf_jit_comp() 6a9dc5fd6170 lib: Revert use of fallthrough pseudo-keyword in lib/ 91dbd73a1739 mips/oprofile: Fix fallthrough placement I don't think any of these is fixing a significant problem, but they did cause someone time and trouble to investigate. James
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, 2020-11-22 at 21:35 +0100, Miguel Ojeda wrote: > On Sun, Nov 22, 2020 at 7:22 PM James Bottomley > wrote: > > Well, it's a problem in an error leg, sure, but it's not a really > > compelling reason for a 141 patch series, is it? All that fixing > > this error will do is get the driver to print "oh dear there's a > > problem" under four more conditions than it previously did. > > > > We've been at this for three years now with nearly a thousand > > patches, firstly marking all the fall throughs with /* fall through > > */ and later changing it to fallthrough. At some point we do have > > to ask if the effort is commensurate with the protection > > afforded. Please tell me our reward for all this effort isn't a > > single missing error print. > > It isn't that much effort, isn't it? Well, it seems to be three years of someone's time plus the maintainer review time and series disruption of nearly a thousand patches. Let's be conservative and assume the producer worked about 30% on the series and it takes about 5-10 minutes per patch to review, merge and for others to rework existing series. So let's say it's cost a person year of a relatively junior engineer producing the patches and say 100h of review and application time. The latter is likely the big ticket item because it's what we have in least supply in the kernel (even though it's 20x vs the producer time). > Plus we need to take into account the future mistakes that it might > prevent, too. So even if there were zero problems found so far, it is > still a positive change. Well, the question I was asking is if it's worth the cost which I've tried to outline above. > I would agree if these changes were high risk, though; but they are > almost trivial. It's not about the risk of the changes it's about the cost of implementing them. Even if you discount the producer time (which someone gets to pay for, and if I were the engineering manager, I'd be unhappy about), the review/merge/rework time is pretty significant in exchange for six minor bug fixes. Fine, when a new compiler warning comes along it's certainly reasonable to see if we can benefit from it and the fact that the compiler people think it's worthwhile is enough evidence to assume this initially. But at some point you have to ask whether that assumption is supported by the evidence we've accumulated over the time we've been using it. And if the evidence doesn't support it perhaps it is time to stop the experiment. James
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, 2020-11-22 at 11:22 -0800, Joe Perches wrote: > On Sun, 2020-11-22 at 11:12 -0800, James Bottomley wrote: > > On Sun, 2020-11-22 at 10:25 -0800, Joe Perches wrote: > > > On Sun, 2020-11-22 at 10:21 -0800, James Bottomley wrote: > > > > Please tell me our reward for all this effort isn't a single > > > > missing error print. > > > > > > There were quite literally dozens of logical defects found > > > by the fallthrough additions. Very few were logging only. > > > > So can you give us the best examples (or indeed all of them if > > someone is keeping score)? hopefully this isn't a US election > > situation ... > > Gustavo? Are you running for congress now? > > https://lwn.net/Articles/794944/ That's 21 reported fixes of which about 50% seem to produce no change in code behaviour at all, a quarter seem to have no user visible effect with the remaining quarter producing unexpected errors on obscure configuration parameters, which is why no-one really noticed them before. James
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, 2020-11-22 at 10:25 -0800, Joe Perches wrote: > On Sun, 2020-11-22 at 10:21 -0800, James Bottomley wrote: > > Please tell me our reward for all this effort isn't a single > > missing error print. > > There were quite literally dozens of logical defects found > by the fallthrough additions. Very few were logging only. So can you give us the best examples (or indeed all of them if someone is keeping score)? hopefully this isn't a US election situation ... James
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, 2020-11-22 at 08:17 -0800, Kees Cook wrote: > On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote: > > On Fri, 20 Nov 2020 11:30:40 -0800 Kees Cook wrote: > > > On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote: > > > > On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote: > > > > > This series aims to fix almost all remaining fall-through > > > > > warnings in order to enable -Wimplicit-fallthrough for Clang. > > > > > > > > > > In preparation to enable -Wimplicit-fallthrough for Clang, > > > > > explicitly add multiple break/goto/return/fallthrough > > > > > statements instead of just letting the code fall through to > > > > > the next case. > > > > > > > > > > Notice that in order to enable -Wimplicit-fallthrough for > > > > > Clang, this change[1] is meant to be reverted at some point. > > > > > So, this patch helps to move in that direction. > > > > > > > > > > Something important to mention is that there is currently a > > > > > discrepancy between GCC and Clang when dealing with switch > > > > > fall-through to empty case statements or to cases that only > > > > > contain a break/continue/return statement[2][3][4]. > > > > > > > > Are we sure we want to make this change? Was it discussed > > > > before? > > > > > > > > Are there any bugs Clangs puritanical definition of fallthrough > > > > helped find? > > > > > > > > IMVHO compiler warnings are supposed to warn about issues that > > > > could be bugs. Falling through to default: break; can hardly be > > > > a bug?! > > > > > > It's certainly a place where the intent is not always clear. I > > > think this makes all the cases unambiguous, and doesn't impact > > > the machine code, since the compiler will happily optimize away > > > any behavioral redundancy. > > > > If none of the 140 patches here fix a real bug, and there is no > > change to machine code then it sounds to me like a W=2 kind of a > > warning. > > FWIW, this series has found at least one bug so far: > https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=kr...@mail.gmail.com/ Well, it's a problem in an error leg, sure, but it's not a really compelling reason for a 141 patch series, is it? All that fixing this error will do is get the driver to print "oh dear there's a problem" under four more conditions than it previously did. We've been at this for three years now with nearly a thousand patches, firstly marking all the fall throughs with /* fall through */ and later changing it to fallthrough. At some point we do have to ask if the effort is commensurate with the protection afforded. Please tell me our reward for all this effort isn't a single missing error print. James
Re: [RFC] MAINTAINERS tag for cleanup robot
On Sun, 2020-11-22 at 08:10 -0800, Tom Rix wrote: > On 11/22/20 6:56 AM, Matthew Wilcox wrote: > > On Sun, Nov 22, 2020 at 06:46:46AM -0800, Tom Rix wrote: > > > On 11/21/20 7:23 PM, Matthew Wilcox wrote: > > > > On Sat, Nov 21, 2020 at 08:50:58AM -0800, t...@redhat.com > > > > wrote: > > > > > The fixer review is > > > > > https://reviews.llvm.org/D91789 > > > > > > > > > > A run over allyesconfig for x86_64 finds 62 issues, 5 are > > > > > false positives. The false positives are caused by macros > > > > > passed to other macros and by some macro expansions that did > > > > > not have an extra semicolon. > > > > > > > > > > This cleans up about 1,000 of the current 10,000 -Wextra- > > > > > semi-stmt warnings in linux-next. > > > > Are any of them not false-positives? It's all very well to > > > > enable stricter warnings, but if they don't fix any bugs, > > > > they're just churn. > > > > > > > While enabling additional warnings may be a side effect of this > > > effort > > > > > > the primary goal is to set up a cleaning robot. After that a > > > refactoring robot. > > Why do we need such a thing? Again, it sounds like more churn. > > It's really annoying when I'm working on something important that > > gets derailed by pointless churn. Churn also makes it harder to > > backport patches to earlier kernels. > > > A refactoring example on moving to treewide, consistent use of a new > api may help. > > Consider > > 2efc459d06f1630001e3984854848a5647086232 > > sysfs: Add sysfs_emit and sysfs_emit_at to format sysfs output > > A new api for printing in the sysfs. How do we use it treewide ? > > Done manually, it would be a heroic effort requiring high level > maintainers pushing and likely only get partially done. > > If a refactoring programatic fixit is done and validated on a one > subsystem, it can run on all the subsystems. > > The effort is a couple of weeks to write and validate the fixer, > hours to run over the tree. > > It won't be perfect but will be better than doing it manually. Here's a thought: perhaps we don't. sysfs_emit isn't a "new api" its a minor rewrap of existing best practice. The damage caused by the churn of forcing its use everywhere would far outweigh any actual benefit because pretty much every bug in this area has already been caught and killed by existing tools. We can enforce sysfs_emit going forwards using tools like checkpatch but there's no benefit and a lot of harm to be done by trying to churn the entire tree retrofitting it (both in terms of review time wasted as well as patch series derailed). James
Re: [RFC] MAINTAINERS tag for cleanup robot
On Sat, 2020-11-21 at 08:50 -0800, t...@redhat.com wrote: > A difficult part of automating commits is composing the subsystem > preamble in the commit log. For the ongoing effort of a fixer > producing > one or two fixes a release the use of 'treewide:' does not seem > appropriate. > > It would be better if the normal prefix was used. Unfortunately > normal is > not consistent across the tree. > > > D: Commit subsystem prefix > > ex/ for FPGA DFL DRIVERS > > D: fpga: dfl: > I've got to bet this is going to cause more issues than it solves. SCSI uses scsi: : for drivers but not every driver has a MAINTAINERS entry. We use either scsi: or scsi: core: for mid layer things, but we're not consistent. Block uses blk-: for all of it's stuff but almost no s have a MAINTAINERS entry. So the next thing you're going to cause is an explosion of suggested MAINTAINERs entries. Has anyone actually complained about treewide:? James
Re: [Ocfs2-devel] [RFC] treewide: cleanup unreachable breaks
On Sun, 2020-10-18 at 19:59 +0100, Matthew Wilcox wrote: > On Sat, Oct 17, 2020 at 09:09:28AM -0700, t...@redhat.com wrote: > > clang has a number of useful, new warnings see > > https://urldefense.com/v3/__https://clang.llvm.org/docs/DiagnosticsReference.html__;!!GqivPVa7Brio!Krxz78O3RKcB9JBMVo_F98FupVhj_jxX60ddN6tKGEbv_cnooXc1nnBmchm-e_O9ieGnyQ$ > > > > Please get your IT department to remove that stupidity. If you > can't, please send email from a non-Red Hat email address. Actually, the problem is at Oracle's end somewhere in the ocfs2 list ... if you could fix it, that would be great. The usual real mailing lists didn't get this transformation https://lore.kernel.org/bpf/20201017160928.12698-1-t...@redhat.com/ but the ocfs2 list archive did: https://oss.oracle.com/pipermail/ocfs2-devel/2020-October/015330.html I bet Oracle IT has put some spam filter on the list that mangles URLs this way. James
Re: [Ocfs2-devel] [RFC] treewide: cleanup unreachable breaks
On Sun, 2020-10-18 at 20:16 +0100, Matthew Wilcox wrote: > On Sun, Oct 18, 2020 at 12:13:35PM -0700, James Bottomley wrote: > > On Sun, 2020-10-18 at 19:59 +0100, Matthew Wilcox wrote: > > > On Sat, Oct 17, 2020 at 09:09:28AM -0700, t...@redhat.com wrote: > > > > clang has a number of useful, new warnings see > > > > https://urldefense.com/v3/__https://clang.llvm.org/docs/DiagnosticsReference.html__;!!GqivPVa7Brio!Krxz78O3RKcB9JBMVo_F98FupVhj_jxX60ddN6tKGEbv_cnooXc1nnBmchm-e_O9ieGnyQ$ > > > > > > > > > > Please get your IT department to remove that stupidity. If you > > > can't, please send email from a non-Red Hat email address. > > > > Actually, the problem is at Oracle's end somewhere in the ocfs2 > > list ... if you could fix it, that would be great. The usual real > > mailing lists didn't get this transformation > > > > https://lore.kernel.org/bpf/20201017160928.12698-1-t...@redhat.com/ > > > > but the ocfs2 list archive did: > > > > https://oss.oracle.com/pipermail/ocfs2-devel/2020-October/015330.html > > > > I bet Oracle IT has put some spam filter on the list that mangles > > URLs this way. > > *sigh*. I'm sure there's a way. I've raised it with someone who > should be able to fix it. As someone who works for IBM I can only say I feel your pain ... James
Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()
On Thu, 2019-10-17 at 18:22 +0530, Sumit Garg wrote: > On Thu, 17 Oct 2019 at 00:40, James Bottomley > wrote: > > > > On Wed, 2019-10-16 at 19:25 +0300, Jarkko Sakkinen wrote: > > > On Wed, Oct 16, 2019 at 08:34:12AM -0400, James Bottomley wrote: > > > > reversible ciphers are generally frowned upon in random number > > > > generation, that's why the krng uses chacha20. In general I > > > > think we shouldn't try to code our own mixing and instead > > > > should get the krng to do it for us using whatever the > > > > algorithm du jour that the crypto guys have blessed is. That's > > > > why I proposed adding the TPM output to the krng as entropy > > > > input and then taking the output of the krng. > > > > > > It is already registered as hwrng. What else? > > > > It only contributes entropy once at start of OS. > > > > Why not just configure quality parameter of TPM hwrng as follows? It > would automatically initiate a kthread during hwrng_init() to feed > entropy from TPM to kernel random numbers pool (see: > drivers/char/hw_random/core.c +142). The question was asked before by Jerry. The main reason is we still can't guarantee that at 1024 the hwrng will choose the TPM as the best source (the problem being it only chooses one) and the mixing is done periodically in a timer thread so it's still vulnerable to an entropy exhaustion attack. I think it's better to source the random number in the TPM when asked but mix it with whatever entropy we have in the pool using the crypto people's mixing algorithm. This definitely avoids exhaustion and provides some protection against single source rng compromises. James > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm- > chip.c > index 3d6d394..fcc3817 100644 > --- a/drivers/char/tpm/tpm-chip.c > +++ b/drivers/char/tpm/tpm-chip.c > @@ -548,6 +548,7 @@ static int tpm_add_hwrng(struct tpm_chip *chip) > "tpm-rng-%d", chip->dev_num); > chip->hwrng.name = chip->hwrng_name; > chip->hwrng.read = tpm_hwrng_read; > + chip->hwrng.quality = 1024; /* Here we assume TPM provides > full entropy */ > return hwrng_register(&chip->hwrng); > > } > > > > Was the issue that it is only used as seed when the rng is > > > init'd > > > first? I haven't at this point gone to the internals of krng. > > > > Basically it was similar to your xor patch except I got the kernel > > rng > > to do the mixing, so it would use the chacha20 cipher at the moment > > until they decide that's unsafe and change it to something else: > > > > https://lore.kernel.org/linux-crypto/1570227068.17537.4.camel@Hanse > > nPartnership.com/ > > > > It uses add_hwgenerator_randomness() to do the mixing. It also has > > an > > unmixed source so that read of the TPM hwrng device works as > > expected. > > Above suggestion is something similar to yours but utilizing the > framework already provided via hwrng core. > > -Sumit > > > > > James > > > > > > > > > > > >
Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()
On Wed, 2019-10-16 at 19:25 +0300, Jarkko Sakkinen wrote: > On Wed, Oct 16, 2019 at 08:34:12AM -0400, James Bottomley wrote: > > reversible ciphers are generally frowned upon in random number > > generation, that's why the krng uses chacha20. In general I think > > we shouldn't try to code our own mixing and instead should get the > > krng to do it for us using whatever the algorithm du jour that the > > crypto guys have blessed is. That's why I proposed adding the TPM > > output to the krng as entropy input and then taking the output of > > the krng. > > It is already registered as hwrng. What else? It only contributes entropy once at start of OS. > Was the issue that it is only used as seed when the rng is init'd > first? I haven't at this point gone to the internals of krng. Basically it was similar to your xor patch except I got the kernel rng to do the mixing, so it would use the chacha20 cipher at the moment until they decide that's unsafe and change it to something else: https://lore.kernel.org/linux-crypto/1570227068.17537.4.ca...@hansenpartnership.com/ It uses add_hwgenerator_randomness() to do the mixing. It also has an unmixed source so that read of the TPM hwrng device works as expected. James
Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()
On Wed, 2019-10-16 at 14:00 +0300, Jarkko Sakkinen wrote: > On Mon, Oct 14, 2019 at 12:29:57PM -0700, James Bottomley wrote: > > The job of the in-kernel rng is simply to produce a mixed entropy > > pool from which we can draw random numbers. The idea is that quite > > a few attackers have identified the rng as being a weak point in > > the security architecture of the kernel, so if we mix entropy from > > all the sources we have, you have to compromise most of them to > > gain some predictive power over the rng sequence. > > The documentation says that krng is suitable for key generation. > Should the documentation changed to state that it is unsuitable? How do you get that from the argument above? The krng is about the best we have in terms of unpredictable key generation, so of course it is suitable ... provided you give the entropy enough time to have sufficient entropy. It's also not foolproof ... Bernstein did a speculation about how you could compromise all our input sources for entropy. However the more sources we have the more difficult the compromise becomes. > > The point is not how certified the TPM RNG is, the point is that > > it's a single source and if we rely on it solely for some > > applications, like trusted keys, then it gives the attackers a > > single known point to go after. This may be impossible for script > > kiddies, but it won't be for nation states ... are you going to > > exclusively trust the random number you got from your chinese > > certified TPM? > > I'd suggest approach where TPM RNG result is xored with krng result. reversible ciphers are generally frowned upon in random number generation, that's why the krng uses chacha20. In general I think we shouldn't try to code our own mixing and instead should get the krng to do it for us using whatever the algorithm du jour that the crypto guys have blessed is. That's why I proposed adding the TPM output to the krng as entropy input and then taking the output of the krng. James > > Remember also that the attack doesn't have to be to the TPM only, > > it could be the pathway by which we get the random number, which > > involves components outside of the TPM certification. > > Yeah, I do get this. > > /Jarkko >
Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()
On Mon, 2019-10-14 at 22:00 +0300, Jarkko Sakkinen wrote: > On Wed, Oct 09, 2019 at 12:11:06PM +, Safford, David (GE Global > Research, US) wrote: > > > > > From: Jarkko Sakkinen > > > Sent: Tuesday, October 8, 2019 7:54 PM > > > To: Ken Goldman > > > Cc: Safford, David (GE Global Research, US) > > >; Mimi > > > Zohar ; linux-integr...@vger.kernel.org; > > > sta...@vger.kernel.org; open list:ASYMMETRIC KEYS > > > ; open list:CRYPTO API > > cry...@vger.kernel.org>; open list > > > Subject: EXT: Re: [PATCH] KEYS: asym_tpm: Switch to > > > get_random_bytes() > > > > > > On Wed, Oct 09, 2019 at 02:49:35AM +0300, Jarkko Sakkinen wrote: > > > > On Mon, Oct 07, 2019 at 06:13:01PM -0400, Ken Goldman wrote: > > > > > The TPM library specification states that the TPM must comply > > > > > with NIST SP800-90 A. > > > > > > > > > > https://trustedcomputinggroup.org/membership/certification/tp > > > > > m-certified-products/ > > > > > > > > > > shows that the TPMs get third party certification, Common > > > > > Criteria EAL 4+. > > > > > > > > > > While it's theoretically possible that an attacker could > > > > > compromise both the TPM vendors and the evaluation agencies, > > > > > we do have EAL 4+ assurance against both 1 and 2. > > > > > > > > Certifications do not equal to trust. > > > > > > And for trusted keys the least trust solution is to do generation > > > with the kernel assets and sealing with TPM. With TEE the least > > > trust solution is equivalent. > > > > > > Are you proposing that the kernel random number generation should > > > be removed? That would be my conclusion of this discussion if I > > > would agree any of this (I don't). > > > > > > /Jarkko > > > > No one is suggesting that. > > > > You are suggesting changing the documented behavior of trusted > > keys, and that would cause problems for some of our use cases. > > While certification may not in your mind be equal to trust, it is > > equal to compliance with mandatory regulations. > > > > Perhaps rather than arguing past each other, we should look into > > providing users the ability to choose, as an argument to keyctl? > > > > dave > > I'm taking my words back in the regression part as regression would > need really a failing system. Definitely the fixes tag should be > removed from my patch. > > What is anyway the role of the kernel rng? Why does it exist and when > exactly it should be used? This exactly where the whole review > process throughout the "chain of command" failed misserably with > tpm_asym.c. > > The commit message for tpm_asym.c does not document the design choice > in any possible way and still was merged to the mainline. > > Before knowning the answer to the "existential" question we are > somewhat paralyzed on moving forward with trusted keys (e.g. > paralyzed to merge TEE backend). > > Your proposal might make sense but I don't really want to say > anything since I'm completely cluesless of the role of the kernel > rng. Looks like everyone who participated to the review process of > tpm_asym.c, is too. The job of the in-kernel rng is simply to produce a mixed entropy pool from which we can draw random numbers. The idea is that quite a few attackers have identified the rng as being a weak point in the security architecture of the kernel, so if we mix entropy from all the sources we have, you have to compromise most of them to gain some predictive power over the rng sequence. The point is not how certified the TPM RNG is, the point is that it's a single source and if we rely on it solely for some applications, like trusted keys, then it gives the attackers a single known point to go after. This may be impossible for script kiddies, but it won't be for nation states ... are you going to exclusively trust the random number you got from your chinese certified TPM? Remember also that the attack doesn't have to be to the TPM only, it could be the pathway by which we get the random number, which involves components outside of the TPM certification. James
Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()
On Fri, 2019-10-04 at 13:11 -0700, Jerry Snitselaar wrote:
> On Fri Oct 04 19, Jerry Snitselaar wrote:
> > On Fri Oct 04 19, James Bottomley wrote:
> > > On Fri, 2019-10-04 at 11:33 -0700, Jerry Snitselaar wrote:
> > > > On Fri Oct 04 19, James Bottomley wrote:
> > > > > On Fri, 2019-10-04 at 21:22 +0300, Jarkko Sakkinen wrote:
> > > > > > On Thu, Oct 03, 2019 at 04:59:37PM -0700, James Bottomley
> > > > > > wrote:
> > > > > > > I think the principle of using multiple RNG sources for
> > > > > > > strong keys is a sound one, so could I propose a
> > > > > > > compromise: We have a tpm subsystem random number
> > > > > > > generator that, when asked for random bytes first
> > > > > > > extracts bytes from the TPM RNG and places it into
> > > > > > > the kernel entropy pool and then asks for random
> > > > > > > bytes from the kernel RNG? That way, it will always have
> > > > > > > the entropy to satisfy the request and in the worst case,
> > > > > > > where the kernel has picked up no other entropy sources
> > > > > > > at all it will be equivalent to what we have now (single
> > > > > > > entropy source) but usually it will be a much better
> > > > > > > mixed entropy source.
> > > > > >
> > > > > > I think we should rely the existing architecture where TPM
> > > > > > is contributing to the entropy pool as hwrng.
> > > > >
> > > > > That doesn't seem to work: when I trace what happens I see us
> > > > > inject 32 bytes of entropy at boot time, but never again. I
> > > > > think the problem is the kernel entropy pool is push not pull
> > > > > and we have no triggering event in the TPM to get us to
> > > > > push. I suppose we could set a timer to do this or perhaps
> > > > > there is a pull hook and we haven't wired it up correctly?
> > > > >
> > > > > James
> > > > >
> > > >
> > > > Shouldn't hwrng_fillfn be pulling from it?
> > >
> > > It should, but the problem seems to be it only polls the
> > > "current" hw rng ... it doesn't seem to have a concept that there
> > > may be more than one. What happens, according to a brief reading
> > > of the code, is when multiple are registered, it determines what
> > > the "best" one is and then only pulls from that. What I think it
> > > should be doing is filling from all of them using the entropy
> > > quality to adjust how many bits we get.
> > >
> > > James
> > >
> >
> > Most of them don't even set quality, including the tpm, so they end
> > up at the end of the list. For the ones that do I'm not sure how
> > they determined the value. For example virtio-rng sets quality to
> > 1000.
>
> I should have added that I like that idea though.
OK, so I looked at how others implement this. It turns out there's
only one other: the atheros rng and this is what it does:
drivers/net/wireless/ath/ath9k/rng.c
so rather than redoing the entirety of the TPM rng like this, I thought
it's easier to keep what we have (direct hwrng device) and plug our
tpm_get_random() function into the kernel rng like the below.
James
---
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 3d6d394a8661..0794521c0784 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -536,7 +536,7 @@ static int tpm_hwrng_read(struct hwrng *rng, void *data,
size_t max, bool wait)
{
struct tpm_chip *chip = container_of(rng, struct tpm_chip, hwrng);
- return tpm_get_random(chip, data, max);
+ return __tpm_get_random(chip, data, max);
}
static int tpm_add_hwrng(struct tpm_chip *chip)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index d7a3888ad80f..14631cba000c 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#include
#include "tpm.h"
@@ -424,15 +425,11 @@ int tpm_pm_resume(struct device *dev)
}
EXPORT_SYMBOL_GPL(tpm_pm_resume);
-/**
- * tpm_get_random() - get random bytes from the TPM's RNG
- * @chip: a &struct tpm_chip instance, %NULL for the default chip
- * @out: destination buffer for the random bytes
- * @max: the max number of bytes to write to @out
- *
- * Return: number of random b
Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()
On Fri, 2019-10-04 at 11:33 -0700, Jerry Snitselaar wrote: > On Fri Oct 04 19, James Bottomley wrote: > > On Fri, 2019-10-04 at 21:22 +0300, Jarkko Sakkinen wrote: > > > On Thu, Oct 03, 2019 at 04:59:37PM -0700, James Bottomley wrote: > > > > I think the principle of using multiple RNG sources for strong > > > > keys is a sound one, so could I propose a compromise: We have > > > > a tpm subsystem random number generator that, when asked for > > > > random bytes first extracts bytes from the TPM RNG and > > > > places it into the kernel entropy pool and then asks for > > > > random bytes from the kernel RNG? That way, it will always have > > > > the entropy to satisfy the request and in the worst case, where > > > > the kernel has picked up no other entropy sources at all it > > > > will be equivalent to what we have now (single entropy source) > > > > but usually it will be a much better mixed entropy source. > > > > > > I think we should rely the existing architecture where TPM is > > > contributing to the entropy pool as hwrng. > > > > That doesn't seem to work: when I trace what happens I see us > > inject 32 bytes of entropy at boot time, but never again. I think > > the problem is the kernel entropy pool is push not pull and we have > > no triggering event in the TPM to get us to push. I suppose we > > could set a timer to do this or perhaps there is a pull hook and we > > haven't wired it up correctly? > > > > James > > > > Shouldn't hwrng_fillfn be pulling from it? It should, but the problem seems to be it only polls the "current" hw rng ... it doesn't seem to have a concept that there may be more than one. What happens, according to a brief reading of the code, is when multiple are registered, it determines what the "best" one is and then only pulls from that. What I think it should be doing is filling from all of them using the entropy quality to adjust how many bits we get. James
Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()
On Fri, 2019-10-04 at 21:22 +0300, Jarkko Sakkinen wrote: > On Thu, Oct 03, 2019 at 04:59:37PM -0700, James Bottomley wrote: > > I think the principle of using multiple RNG sources for strong keys > > is a sound one, so could I propose a compromise: We have a tpm > > subsystem random number generator that, when asked for random > > bytes first extracts bytes from the TPM RNG and places it into > > the kernel entropy pool and then asks for random bytes from the > > kernel RNG? That way, it will always have the entropy to satisfy > > the request and in the worst case, where the kernel has picked up > > no other entropy sources at all it will be equivalent to what we > > have now (single entropy source) but usually it will be a much > > better mixed entropy source. > > I think we should rely the existing architecture where TPM is > contributing to the entropy pool as hwrng. That doesn't seem to work: when I trace what happens I see us inject 32 bytes of entropy at boot time, but never again. I think the problem is the kernel entropy pool is push not pull and we have no triggering event in the TPM to get us to push. I suppose we could set a timer to do this or perhaps there is a pull hook and we haven't wired it up correctly? James
Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()
On Thu, 2019-10-03 at 18:08 -0400, Mimi Zohar wrote: > On Fri, 2019-10-04 at 00:57 +0300, Jarkko Sakkinen wrote: > > On Fri, Oct 04, 2019 at 12:51:25AM +0300, Jarkko Sakkinen wrote: > > > On Thu, Oct 03, 2019 at 02:53:47PM -0400, Mimi Zohar wrote: > > > > [Cc'ing David Safford] > > > > > > > > On Thu, 2019-10-03 at 20:58 +0300, Jarkko Sakkinen wrote: > > > > > On Thu, Oct 03, 2019 at 09:02:32AM -0400, Mimi Zohar wrote: > > > > > > On Thu, 2019-10-03 at 14:41 +0300, Jarkko Sakkinen wrote: > > > > > > > On Wed, Oct 02, 2019 at 10:00:19AM -0400, Mimi Zohar > > > > > > > wrote: > > > > > > > > On Thu, 2019-09-26 at 20:16 +0300, Jarkko Sakkinen > > > > > > > > wrote: > > > > > > > > > Only the kernel random pool should be used for > > > > > > > > > generating random numbers. > > > > > > > > > TPM contributes to that pool among the other sources > > > > > > > > > of entropy. In here it > > > > > > > > > is not, agreed, absolutely critical because TPM is > > > > > > > > > what is trusted anyway > > > > > > > > > but in order to remove tpm_get_random() we need to > > > > > > > > > first remove all the > > > > > > > > > call sites. > > > > > > > > > > > > > > > > At what point during boot is the kernel random pool > > > > > > > > available? Does this imply that you're planning on > > > > > > > > changing trusted keys as well? > > > > > > > > > > > > > > Well trusted keys *must* be changed to use it. It is not > > > > > > > a choice because using a proprietary random number > > > > > > > generator instead of defacto one in the kernel can be > > > > > > > categorized as a *regression*. > > > > > > > > > > > > I really don't see how using the TPM random number for TPM > > > > > > trusted keys would be considered a regression. That by > > > > > > definition is a trusted key. If anything, changing what is > > > > > > currently being done would be the regression. > > > > > > > > > > It is really not a TPM trusted key. It trusted key that gets > > > > > sealed with the TPM. The key itself is used in clear by > > > > > kernel. The random number generator exists in the kernel to > > > > > for a reason. > > > > > > > > > > It is without doubt a regression. > > > > > > > > You're misusing the term "regression" here. A regression is > > > > something that previously worked and has stopped working. In > > > > this case, trusted keys has always been based on the TPM random > > > > number generator. Before changing this, there needs to be some > > > > guarantees that the kernel random number generator has a pool > > > > of random numbers early, on all systems including embedded > > > > devices, not just servers. > > > > > > I'm not using the term regression incorrectly here. Wrong > > > function was used to generate random numbers for the payload > > > here. It is an obvious bug. > > > > At the time when trusted keys was introduced I'd say that it was a > > wrong design decision and badly implemented code. But you are right > > in that as far that code is considered it would unfair to speak of > > a regression. I think that's a bit unfair: when the code was written, a decade ago, the TPM was thought to be the state of the art in terms of random number generators. Of course, since then the kernel RNG has become way better and we've become more aware of nation state actors thinking that influencing the RNG is the best way to compromise strong crypto. > > asym-tpm.c on the other hand this is fresh new code. There has been > > *countless* of discussions over the years that random numbers > > should come from multiple sources of entropy. There is no other > > categorization than a bug for the tpm_get_random() there. > > This week's LWN article on "5.4 Merge window, part 2" discusses > "boot- time entropy". This article couldn't have been more perfectly > timed. I think the principle of using multiple RNG sources for strong keys is a sound one, so could I propose a compromise: We have a tpm subsystem random number generator that, when asked for random bytes first extracts bytes from the TPM RNG and places it into the kernel entropy pool and then asks for random bytes from the kernel RNG? That way, it will always have the entropy to satisfy the request and in the worst case, where the kernel has picked up no other entropy sources at all it will be equivalent to what we have now (single entropy source) but usually it will be a much better mixed entropy source. James
Re: [PATCH v6 05/12] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
On Fri, 2019-09-20 at 17:35 +0300, Jarkko Sakkinen wrote: > On Fri, Sep 20, 2019 at 05:34:00PM +0300, Jarkko Sakkinen wrote: > > On Mon, Sep 09, 2019 at 01:20:57PM +0100, James Bottomley wrote: > > Forgot to ask: what is the new field handles? You mean for the null seed or for the virtual handles? For the former, there isn't one since the null seed is maintained as a context when not in use, although since the null seed context is loaded before an operation it will mostly get 8000 for the brief time it is used. For the latter, there's no change in the way virtual handles are processed. James
Re: [PATCH v6 02/12] tpm-buf: add handling for TPM2B types
On Fri, 2019-09-20 at 17:18 +0300, Jarkko Sakkinen wrote:
> On Mon, Sep 09, 2019 at 01:18:35PM +0100, James Bottomley wrote:
> > Most complex TPM commands require appending TPM2B buffers to the
> > command body. Since TPM2B types are essentially variable size
> > arrays,it makes it impossible to represent these complex command
> > arguments as structures and we simply have to build them up using
> > append primitives like these.
> >
> > Signed-off-by: James Bottomley > om>
>
> I think a better idea would be to have headerless TPM buffers
I thought about that. The main problem is that most of the
construct/append functions use the header, and these are the functions
most useful to the TPM2B operation.
The other thing that argues against this is that the TPM2B case would
save nothing if we eliminated the header, because we allocate a page
for all the data regardless.
> and also it makes sense to have a separate length field in the
> struct to keep the code sane given that sometimes the buffer does not
> store the length.
I'm really not sure about that one. The header length has to be filled
in for the non-TPM2B case but right at the moment we have no finish
function for the buf where it could be, so we'd end up having to
maintain two lengths in every update operation on non-TPM2B buffers.
That seems inefficient and the only slight efficiency we get in the
TPM2B case is not having to do the big endian conversion from the
header which doesn't seem to be worth the added complexity.
James
> E.g.
>
> enum tpm_buf_flags {
> TPM_BUF_OVERFLOW= BIT(0),
> TPM_BUF_HEADERLESS = BIT(1),
> };
>
> struct tpm_buf {
> unsigned int length;
> struct page *data_page;
> unsigned int flags;
> u8 *data;
> };
>
> /Jarkko
>
Re: [PATCH v6 01/12] tpm-buf: move from static inlines to real functions
On Fri, 2019-09-20 at 17:06 +0300, Jarkko Sakkinen wrote: > On Fri, Sep 20, 2019 at 05:06:15PM +0300, Jarkko Sakkinen wrote: > > On Mon, Sep 09, 2019 at 01:17:56PM +0100, James Bottomley wrote: > > > This separates out the old tpm_buf_... handling functions from > > > static > > > inlines in tpm.h and makes them their own tpm-buf.c file. This > > > is a > > > precursor so we can add new functions for other TPM type handling > > > > > > Signed-off-by: James Bottomley > > .com> > > > > What about TPM_BUF_2B that gets added in this commit? > > Probably just a glitch in rebasing/squashing? Well a glitch in splitting one patch into three, yes. I'll fix it up. James
Re: [PATCH v6 00/12] add integrity and security to TPM2 transactions
On Tue, 2019-09-10 at 17:21 +0100, Jarkko Sakkinen wrote: > On Mon, Sep 09, 2019 at 01:16:48PM +0100, James Bottomley wrote: > > Link to previous cover letter: > > > > https://lore.kernel.org/linux-integrity/1540193596.3202.7.camel@Han > > senPartnership.com/ > > > > This is marked v6 instead of v5 because I did a v5 after feedback > > on v4 > > but didn't get around to posting it and then had to rework the > > whole of > > the kernel space handling while I was on holiday. I also added the > > documentation of how the whole thing works and the rationale for > > doing > > it in tpm-security.rst (patch 11). The main reason for doing this > > now > > is so we have something to discuss at Plumbers. > > > > The new patch set implements the various splits requested, but the > > main > > changes are that the kernel space is gone and is replaced by a > > context > > save and restore of the generated null seed. This is easier to > > handle > > than a full kernel space given the new threading for TPM spaces, > > but > > conceptually it is still very like a space. I've also made whether > > integrity and encryption is turned on a Kconfig option. > > > > James > > So... is there a changelog for the revisions? Well, yes, standard way: they're in the individual patches under the '- --' prefixed with v6: James
[PATCH v6 12/12] tpm2-sessions: NOT FOR COMMITTING add sessions testing
This runs through a preset sequence using sessions to demonstrate that
the session handling code functions. It does both HMAC, encryption
and decryption by testing an encrypted sealing operation with
authority and proving that the same sealed data comes back again via
an HMAC and response encryption. It also does policy unsealing which
mimics the more complex of the trusted key scenarios.
Signed-off-by: James Bottomley
---
v3: add policy unseal testing with two sessions
v6: move to new null seed framework
---
drivers/char/tpm/Makefile | 2 +
drivers/char/tpm/tpm-chip.c | 1 +
drivers/char/tpm/tpm.h| 1 +
drivers/char/tpm/tpm2-cmd.c | 7 +
drivers/char/tpm/tpm2-sessions-test.c | 795 ++
drivers/char/tpm/tpm2-sessions.c | 1 +
drivers/char/tpm/tpm2-sessions.h | 3 +-
7 files changed, 809 insertions(+), 1 deletion(-)
create mode 100644 drivers/char/tpm/tpm2-sessions-test.c
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 8f9e58317048..f6577c9e3138 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -35,3 +35,5 @@ obj-$(CONFIG_TCG_TIS_ST33ZP24) += st33zp24/
obj-$(CONFIG_TCG_XEN) += xen-tpmfront.o
obj-$(CONFIG_TCG_CRB) += tpm_crb.o
obj-$(CONFIG_TCG_VTPM_PROXY) += tpm_vtpm_proxy.o
+
+obj-m += tpm2-sessions-test.o
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 4838c6a9f0f2..5dda37047a71 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -252,6 +252,7 @@ struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip)
return NULL;
return chip;
}
+EXPORT_SYMBOL(tpm_find_get_ops);
/**
* tpm_dev_release() - free chip memory and the device number
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index c88eee6376e4..06f63874e833 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -117,6 +117,7 @@ enum tpm2_command_codes {
TPM2_CC_CONTEXT_LOAD= 0x0161,
TPM2_CC_CONTEXT_SAVE= 0x0162,
TPM2_CC_FLUSH_CONTEXT = 0x0165,
+ TPM2_CC_POLICY_COMMAND_CODE = 0x16c,
TPM2_CC_READ_PUBLIC = 0x0173,
TPM2_CC_START_AUTH_SESS = 0x0176,
TPM2_CC_VERIFY_SIGNATURE= 0x0177,
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 2c798f18282f..d4bbe19170d1 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -222,6 +222,7 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
tpm_buf_destroy(&buf);
return rc;
}
+EXPORT_SYMBOL_GPL(tpm2_pcr_read);
/**
* tpm2_pcr_extend() - extend a PCR value
@@ -269,6 +270,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
return rc;
}
+EXPORT_SYMBOL_GPL(tpm2_pcr_extend);
struct tpm2_get_random_out {
__be16 size;
@@ -353,6 +355,7 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t
max)
tpm_buf_destroy(&buf);
return err;
}
+EXPORT_SYMBOL_GPL(tpm2_get_random);
/**
* tpm2_flush_context() - execute a TPM2_FlushContext command
@@ -376,6 +379,7 @@ void tpm2_flush_context(struct tpm_chip *chip, u32 handle)
tpm_transmit_cmd(chip, &buf, 0, "flushing context");
tpm_buf_destroy(&buf);
}
+EXPORT_SYMBOL_GPL(tpm2_flush_context);
/**
* tpm_buf_append_auth() - append TPMS_AUTH_COMMAND to the buffer.
@@ -406,6 +410,7 @@ void tpm2_buf_append_auth(struct tpm_buf *buf, u32
session_handle,
if (hmac && hmac_len)
tpm_buf_append(buf, hmac, hmac_len);
}
+EXPORT_SYMBOL_GPL(tpm2_buf_append_auth);
/**
* tpm2_seal_trusted() - seal the payload of a trusted key
@@ -532,6 +537,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
return rc;
}
+EXPORT_SYMBOL_GPL(tpm2_seal_trusted);
/**
* tpm2_load_cmd() - execute a TPM2_Load command
@@ -715,6 +721,7 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
tpm2_flush_context(chip, blob_handle);
return rc;
}
+EXPORT_SYMBOL_GPL(tpm2_unseal_trusted);
struct tpm2_get_cap_out {
u8 more_data;
diff --git a/drivers/char/tpm/tpm2-sessions-test.c
b/drivers/char/tpm/tpm2-sessions-test.c
new file mode 100644
index ..834811ab3466
--- /dev/null
+++ b/drivers/char/tpm/tpm2-sessions-test.c
@@ -0,0 +1,795 @@
+/* run a set of tests of the sessions code */
+#include "tpm.h"
+#include "tpm2-sessions.h"
+
+#include
+
+#include
+
+#include
+#include
+
+#include
+
+/* simple policy: command code must be TPM2_CC_UNSEAL */
+static u8 policy[] = {
+ 0xe6, 0x13, 0x13, 0x70, 0x76, 0x52, 0x4b, 0xde,
+ 0x48, 0x75, 0x33, 0x86, 0x58, 0x84, 0xe9, 0x73,
+ 0x2e, 0xbe, 0xe3, 0xaa, 0xcb, 0x09, 0x5d, 0x94,
+ 0xa6, 0xde, 0x49, 0x2e, 0xc0, 0x6c, 0x46, 0xfa,
+};
+
+static struct crypto_shash *sha256_hash;
+
+static int parse_create
[PATCH v6 11/12] Documentation: add tpm-security.rst
Document how the new encrypted secure interface for TPM2 works and how security can be assured after boot by certifying the NULL seed. Signed-off-by: James Bottomley --- v6: replace kernel space with null seed context save --- Documentation/security/tpm/tpm-security.rst | 204 1 file changed, 204 insertions(+) create mode 100644 Documentation/security/tpm/tpm-security.rst diff --git a/Documentation/security/tpm/tpm-security.rst b/Documentation/security/tpm/tpm-security.rst new file mode 100644 index ..3411030505a1 --- /dev/null +++ b/Documentation/security/tpm/tpm-security.rst @@ -0,0 +1,204 @@ +TPM Security + + +The object of this document is to describe how we make the kernel's +use of the TPM reasonably robust in the face of external snooping and +packet alteration attacks. The current security document is for TPM +2.0. + +Introduction + + +The TPM is usually a discrete chip attached to a PC via some type of +low bandwidth bus. There are exceptions to this such as the Intel +PTT, which is a software TPM running inside a software environment +close to the CPU, which are subject to different attacks, but right at +the moment, most hardened security environments require a discrete +hardware TPM, which is the use case discussed here. + +Snooping and Alteration Attacks against the bus +--- + +The current state of the art for snooping the TPM Genie hardware +interposer https://www.nccgroup.trust/us/our-research/tpm-genie/ which +is a simple external device that can be installed in a couple of +seconds on any system or laptop. However, the next phase of research +seems to be hacking existing devices on the bus to act as interposers, +so the fact that the attacker requires physical access for a few +seconds might evaporate. However, the goal of this document is to +protect TPM secrets and integrity as far as we are able in this +environment and to try to insure that if we can't prevent the attack +then at least we can detect it. + +Unfortunately, most of the TPM functionality, including the hardware +reset capability can be controlled by an attacker who has access to +the bus, so we'll discuss some of the disruption possibilities below. + +Measurement (PCR) Integrity +--- + +Since the attacker can send their own commands to the TPM, they can +send arbitrary PCR extends and thus disrupt the measurement system, +which would be an annoying denial of service attack. However, there +are two, more serious, classes of attack aimed at entities sealed to +trust measurements. + +1. The attacker could intercept all PCR extends coming from the system + and completely substitute their own values, producing a replay of + an untampered state that would cause PCR measurements to attest to + a trusted state and release secrets + +2. At some point in time the attacker could reset the TPM, clearing + the PCRs and then send down their own measurements which would + effectively overwrite the boot time measurements the TPM has + already done. + +The first can be thwarted by always doing HMAC protection of the PCR +extend and read command meaning measurement values cannot be +substituted without producing a detectable HMAC failure in the +response. However, the second can only really be detected by relying +on some sort of mechanism for protection which would change over TPM +reset. + +Secrets Guarding + + +Certain information passing in and out of the TPM, such as key sealing +and private key import and random number generation, is vulnerable to +interception which HMAC protection alone cannot protect, so for these +types of command we must also employ request and response encryption +to prevent the loss of secret information. + +Establishing Initial Trust with the TPM +--- + +In order to provide security from the beginning, an initial shared or +asymmetric secret must be established which must also be unknown to +the attacker. The most obvious avenues for this are the endorsement +and storage seeds, which can be used to derive asymmetric keys. +However, using these keys is difficult because the only way to pass +them into the kernel would be on the command line, which requires +extensive support in the boot system, and there's no guarantee that +either hierarchy would not have some type of authorization. + +The mechanism chosen for the Linux Kernel is to derive the primary +elliptic curve key from the null seed using the standard storage seed +parameters. The null seed has two advantages: firstly the hierarchy +physically cannot have an authorization, so we are always able to use +it and secondly, the null seed changes across TPM resets, meaning if +we establish trust on the null seed at start of day, all sessions +salted with the derived key will fail if the TPM is reset and the seed +changes. + +Obvi
[PATCH v6 10/12] tpm: add the null key name as a tpm2 sysfs variable
This is the last component of encrypted tpm2 session handling that
allows us to verify from userspace that the key derived from the NULL
seed genuinely belongs to the TPM and has not been spoofed.
The procedure for doing this involves creating an attestation identity
key (which requires verification of the TPM EK certificate) and then
using that AIK to sign a certification of the Elliptic Curve key over
the NULL seed. Userspace must create this EC Key using the parameters
prescribed in TCG TPM v2.0 Provisioning Guidance for the SRK ECC; if
this is done correctly the names will match and the TPM can then run a
TPM2_Certify operation on this derived primary key using the newly
created AIK.
Signed-off-by: James Bottomley
---
drivers/char/tpm/tpm-sysfs.c | 27 ++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index d9caedda075b..07aa8f427b96 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -309,6 +309,19 @@ static ssize_t timeouts_show(struct device *dev, struct
device_attribute *attr,
}
static DEVICE_ATTR_RO(timeouts);
+static ssize_t null_name_show(struct device *dev, struct device_attribute
*attr,
+ char *buf)
+{
+ struct tpm_chip *chip = to_tpm_chip(dev);
+ int size = TPM2_NAME_SIZE;
+
+ bin2hex(buf, chip->tpmkeyname, size);
+ size *= 2;
+ buf[size++] = '\n';
+ return size;
+}
+static DEVICE_ATTR_RO(null_name);
+
static struct attribute *tpm_dev_attrs[] = {
&dev_attr_pubek.attr,
&dev_attr_pcrs.attr,
@@ -323,17 +336,29 @@ static struct attribute *tpm_dev_attrs[] = {
NULL,
};
+static struct attribute *tpm2_dev_attrs[] = {
+ &dev_attr_null_name.attr,
+ NULL,
+};
+
static const struct attribute_group tpm_dev_group = {
.attrs = tpm_dev_attrs,
};
+static const struct attribute_group tpm2_dev_group = {
+ .attrs = tpm2_dev_attrs,
+};
+
void tpm_sysfs_add_device(struct tpm_chip *chip)
{
/* XXX: If you wish to remove this restriction, you must first update
* tpm_sysfs to explicitly lock chip->ops.
*/
- if (chip->flags & TPM_CHIP_FLAG_TPM2)
+ if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+ WARN_ON(chip->groups_cnt != 0);
+ chip->groups[chip->groups_cnt++] = &tpm2_dev_group;
return;
+ }
/* The sysfs routines rely on an implicit tpm_try_get_ops, device_del
* is called before ops is null'd and the sysfs core synchronizes this
--
2.16.4
[PATCH v6 09/12] trusted keys: Add session encryption protection to the seal/unseal path
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 | 124 ++--
1 file changed, 85 insertions(+), 39 deletions(-)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 572d05966b77..2c798f18282f 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -422,7 +422,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;
@@ -436,45 +438,56 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
if (i == ARRAY_SIZE(tpm2_hash_map))
return -EINVAL;
- rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
+ rc = tpm2_start_auth_session(chip, &auth);
if (rc)
return rc;
- tpm_buf_append_u32(&buf, options->keyhandle);
- tpm2_buf_append_auth(&buf, TPM2_RS_PW,
-NULL /* nonce */, 0,
-0 /* session_attributes */,
-options->keyauth /* hmac */,
-TPM_DIGEST_SIZE);
+ rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
+ if (rc) {
+ tpm2_end_auth_session(auth);
+ return rc;
+ }
+
+ rc = tpm_buf_init_2b(&t2b);
+ if (rc) {
+ tpm_buf_destroy(&buf);
+ tpm2_end_auth_session(auth);
+ return rc;
+ }
+ tpm_buf_append_name(&buf, auth, options->keyhandle, NULL);
+ tpm_buf_append_hmac_session(&buf, auth, TPM2_SA_DECRYPT,
+ options->keyauth, TPM_DIGEST_SIZE);
/* sensitive */
- tpm_buf_append_u16(&buf, 4 + TPM_DIGEST_SIZE + payload->key_len + 1);
+ tpm_buf_append_u16(&t2b, TPM_DIGEST_SIZE);
+ tpm_buf_append(&t2b, options->blobauth, TPM_DIGEST_SIZE);
+ tpm_buf_append_u16(&t2b, payload->key_len + 1);
+ tpm_buf_append(&t2b, payload->key, payload->key_len);
+ tpm_buf_append_u8(&t2b, payload->migratable);
- tpm_buf_append_u16(&buf, TPM_DIGEST_SIZE);
- tpm_buf_append(&buf, options->blobauth, TPM_DIGEST_SIZE);
- tpm_buf_append_u16(&buf, payload->key_len + 1);
- tpm_buf_append(&buf, payload->key, payload->key_len);
- tpm_buf_append_u8(&buf, payload->migratable);
+ tpm_buf_append_2b(&buf, &t2b);
/* public */
- tpm_buf_append_u16(&buf, 14 + options->policydigest_len);
- tpm_buf_append_u16(&buf, TPM_ALG_KEYEDHASH);
- tpm_buf_append_u16(&buf, hash);
+ tpm_buf_append_u16(&t2b, TPM_ALG_KEYEDHASH);
+ tpm_buf_append_u16(&t2b, hash);
/* policy */
if (options->policydigest_len) {
- tpm_buf_append_u32(&buf, 0);
- tpm_buf_append_u16(&buf, options->policydigest_len);
- tpm_buf_append(&buf, options->policydigest,
+ tpm_buf_append_u32(&t2b, 0);
+ tpm_buf_append_u16(&t2b, options->policydigest_len);
+ tpm_buf_append(&t2b, options->policydigest,
options->policydigest_len);
} else {
- tpm_buf_append_u32(&buf, TPM2_OA_USER_WITH_AUTH);
- tpm_buf_append_u16(&buf, 0);
+ tpm_buf_append_u32(&t2b, TPM2_OA_USER_WITH_AUTH);
+ tpm_buf_append_u16(&t2b, 0);
}
/* public parameters */
- tpm_buf_append_u16(&buf, TPM_ALG_NULL);
- tpm_buf_append_u16(&buf, 0);
+ tpm_buf_append_u16(&t2b, TPM_ALG_NULL);
+ /* unique (zero) */
+ tpm_buf_append_u16(&t2b, 0);
+
+ tpm_buf_append_2b(&buf, &t2b);
/* outside info */
tpm_buf_append_u16(&buf, 0);
@@ -487,7 +500,9 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
goto out;
}
+ tpm_buf_fill_hmac_session(&buf, auth);
rc = tpm_transmit_cmd(chip, &buf, 4, "sealing data");
+ rc = tpm_buf_check_hmac_response(&buf, auth, rc
[PATCH v6 08/12] tpm2: add session encryption protection to tpm2_get_random()
If some entity is snooping the TPM bus, they can see the random
numbers we're extracting from the TPM and do prediction attacks
against their consumers. Foil this attack by using response
encryption to prevent the attacker from seeing the random sequence.
Signed-off-by: James Bottomley
---
v3: add error handling to sessions and redo to be outside loop
---
drivers/char/tpm/tpm2-cmd.c | 21 +
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 0012657d3617..572d05966b77 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -296,29 +296,40 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest,
size_t max)
int total = 0;
int retries = 5;
u8 *dest_ptr = dest;
+ struct tpm2_auth *auth;
if (!num_bytes || max > TPM_MAX_RNG_DATA)
return -EINVAL;
- err = tpm_buf_init(&buf, 0, 0);
+ err = tpm2_start_auth_session(chip, &auth);
if (err)
return err;
+ err = tpm_buf_init(&buf, 0, 0);
+ if (err) {
+ tpm2_end_auth_session(auth);
+ return err;
+ }
+
do {
- tpm_buf_reset(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_GET_RANDOM);
+ tpm_buf_reset(&buf, TPM2_ST_SESSIONS, TPM2_CC_GET_RANDOM);
+ tpm_buf_append_hmac_session_opt(&buf, auth, TPM2_SA_ENCRYPT
+ | TPM2_SA_CONTINUE_SESSION,
+ NULL, 0);
tpm_buf_append_u16(&buf, num_bytes);
+ tpm_buf_fill_hmac_session(&buf, auth);
err = tpm_transmit_cmd(chip, &buf,
offsetof(struct tpm2_get_random_out,
buffer),
"attempting get random");
+ err = tpm_buf_check_hmac_response(&buf, auth, err);
if (err) {
if (err > 0)
err = -EIO;
goto out;
}
- out = (struct tpm2_get_random_out *)
- &buf.data[TPM_HEADER_SIZE];
+ out = (struct tpm2_get_random_out *)tpm_buf_parameters(&buf);
recd = min_t(u32, be16_to_cpu(out->size), num_bytes);
if (tpm_buf_length(&buf) <
TPM_HEADER_SIZE +
@@ -335,6 +346,8 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t
max)
} while (retries-- && total < max);
tpm_buf_destroy(&buf);
+ tpm2_end_auth_session(auth);
+
return total ? total : -EIO;
out:
tpm_buf_destroy(&buf);
--
2.16.4
[PATCH v6 07/12] tpm2: add hmac checks to tpm2_pcr_extend()
We use tpm2_pcr_extend() in trusted keys to extend a PCR to prevent a
key from being re-loaded until the next reboot. To use this
functionality securely, that extend must be protected by a session
hmac.
Signed-off-by: James Bottomley
---
v3: add error handling to sessions
---
drivers/char/tpm/tpm2-cmd.c | 28 +++-
1 file changed, 11 insertions(+), 17 deletions(-)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index d120b0a260eb..0012657d3617 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -223,13 +223,6 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
return rc;
}
-struct tpm2_null_auth_area {
- __be32 handle;
- __be16 nonce_size;
- u8 attributes;
- __be16 auth_size;
-} __packed;
-
/**
* tpm2_pcr_extend() - extend a PCR value
*
@@ -243,24 +236,23 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
struct tpm_digest *digests)
{
struct tpm_buf buf;
- struct tpm2_null_auth_area auth_area;
+ struct tpm2_auth *auth;
int rc;
int i;
- rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
+ rc = tpm2_start_auth_session(chip, &auth);
if (rc)
return rc;
- tpm_buf_append_u32(&buf, pcr_idx);
+ rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
+ if (rc) {
+ tpm2_end_auth_session(auth);
+ return rc;
+ }
- auth_area.handle = cpu_to_be32(TPM2_RS_PW);
- auth_area.nonce_size = 0;
- auth_area.attributes = 0;
- auth_area.auth_size = 0;
+ tpm_buf_append_name(&buf, auth, pcr_idx, NULL);
+ tpm_buf_append_hmac_session(&buf, auth, 0, NULL, 0);
- tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area));
- tpm_buf_append(&buf, (const unsigned char *)&auth_area,
- sizeof(auth_area));
tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
for (i = 0; i < chip->nr_allocated_banks; i++) {
@@ -269,7 +261,9 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
chip->allocated_banks[i].digest_size);
}
+ tpm_buf_fill_hmac_session(&buf, auth);
rc = tpm_transmit_cmd(chip, &buf, 0, "attempting extend a PCR value");
+ rc = tpm_buf_check_hmac_response(&buf, auth, rc);
tpm_buf_destroy(&buf);
--
2.16.4
[PATCH v6 06/12] tpm-buf: add tpm_buf_parameters()
Introducing encryption sessions changes where the return parameters
are located in the buffer because if a return session is present
they're 4 bytes beyond the header with those 4 bytes showing the
parameter length. If there is no return session, then they're in the
usual place immediately after the header. The tpm_buf_parameters()
encapsulates this calculation and should be used everywhere
&buf.data[TPM_HEADER_SIZE] is used now
Signed-off-by: James Bottomley
---
drivers/char/tpm/tpm-buf.c | 10 ++
drivers/char/tpm/tpm.h | 2 ++
2 files changed, 12 insertions(+)
diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
index f56350123a08..a5d793d8180d 100644
--- a/drivers/char/tpm/tpm-buf.c
+++ b/drivers/char/tpm/tpm-buf.c
@@ -190,3 +190,13 @@ u32 tpm_get_inc_u32(const u8 **ptr)
return val;
}
EXPORT_SYMBOL_GPL(tpm_get_inc_u32);
+
+u8 *tpm_buf_parameters(struct tpm_buf *buf)
+{
+ int offset = TPM_HEADER_SIZE;
+
+ if (tpm_buf_tag(buf) == TPM2_ST_SESSIONS)
+ offset += 4;
+
+ return &buf->data[offset];
+}
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index ebead8e4c3fe..c88eee6376e4 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -311,6 +311,8 @@ u8 tpm_get_inc_u8(const u8 **ptr);
u16 tpm_get_inc_u16(const u8 **ptr);
u32 tpm_get_inc_u32(const u8 **ptr);
+u8 *tpm_buf_parameters(struct tpm_buf *buf);
+
/* opaque structure, holds auth session parameters like the session key */
struct tpm2_auth;
--
2.16.4
[PATCH v6 05/12] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
This code adds true session based HMAC authentication plus parameter decryption and response encryption using AES. 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 * tpm_buf_append_hmac_session() where tpm2_append_auth() would go * 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. 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 save its context in the tpm_chip structure. The context is loaded on demand into an available volatile handle when tpm_start_auth_session() is called, but is flushed before that function exits to conserve handles. Signed-off-by: James Bottomley Reviewed-by: Ard Biesheuvel # crypto API parts --- 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 v5: Move to sync_skcipher API v6: eliminate kernel space and use context save for null seed and make feature conditional on CONFIG_TPM_BUS_SECURITY --- drivers/char/tpm/Kconfig | 11 + drivers/char/tpm/Makefile|1 + drivers/char/tpm/tpm-buf.c |1 + drivers/char/tpm/tpm.h | 20 +- drivers/char/tpm/tpm2-cmd.c | 22 +- drivers/char/tpm/tpm2-sessions.c | 1203 ++ drivers/char/tpm/tpm2-sessions.h | 137 + include/linux/tpm.h | 29 + 8 files changed, 1411 insertions(+), 13 deletions(-) create mode 100644 drivers/char/tpm/tpm2-sessions.c create mode 100644 drivers/char/tpm/tpm2-sessions.h diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig index 88a3c06fc153..96b09adfc163 100644 --- a/drivers/char/tpm/Kconfig +++ b/drivers/char/tpm/Kconfig @@ -9,6 +9,9 @@ menuconfig TCG_TPM imply 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, @@ -27,6 +30,14 @@ menuconfig TCG_TPM if TCG_TPM +config TPM_BUS_SECURITY + bool "Use secure transactions on the TPM bus" + default y + ---help--- + Setting this causes us to deploy a tamper resistent scheme +for communicating with the TPM to prevent or detect bus snooping +attacks like TPM Genie. Saying Y here adds some encryption overhead +to all kernel to TPM transactions. config HW_RANDOM_TPM bool "TPM HW Random Number Generator support" depends on TCG_TPM && HW_RANDOM && !(TCG_TPM=y && HW_RANDOM=m) diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile index 78bd025b808a..8f9e58317048 100644 --- a/drivers/char/tpm/Makefile +++ b/drivers/char/tpm/Makefile @@ -17,6 +17,7 @@ tpm-y += eventlog/tpm1.o tpm-y += eventlog/tpm2.o tpm-y += tpm-buf.o +tpm-$(CONFIG_TPM_BUS_SECURITY) += 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-buf.c b/drivers/char/tpm/tpm-buf.c index 553adb84b0ac..f56350123a08 100644 --- a/drivers/char/tpm/tpm-buf.c +++ b/drivers/char/tpm/tpm-buf.c @@ -31,6 +31,7 @@ void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal) head->tag = cpu_to_be16(tag);
[PATCH v6 04/12] tpm2-space: export the context save and load commands
The TPM2 session handling code needs to save and restore a single
volatile context for the elliptic curve version of the NULL seed, so
export the APIs which do this for internal use.
Signed-off-by: James Bottomley
---
drivers/char/tpm/tpm.h| 4
drivers/char/tpm/tpm2-space.c | 8
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index d942188debc9..85a7302ddfeb 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -399,6 +399,10 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct
tpm_space *space, u8 *cmd,
size_t cmdsiz);
int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space, void
*buf,
size_t *bufsiz);
+int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
+ unsigned int buf_size, unsigned int *offset);
+int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
+ unsigned int *offset, u32 *handle);
int tpm_bios_log_setup(struct tpm_chip *chip);
void tpm_bios_log_teardown(struct tpm_chip *chip);
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 982d341d8837..ca1cb56ccc51 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -65,8 +65,8 @@ void tpm2_del_space(struct tpm_chip *chip, struct tpm_space
*space)
kfree(space->session_buf);
}
-static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
-unsigned int *offset, u32 *handle)
+int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
+ unsigned int *offset, u32 *handle)
{
struct tpm_buf tbuf;
struct tpm2_context *ctx;
@@ -116,8 +116,8 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
return 0;
}
-static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
-unsigned int buf_size, unsigned int *offset)
+int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
+ unsigned int buf_size, unsigned int *offset)
{
struct tpm_buf tbuf;
unsigned int body_size;
--
2.16.4
[PATCH v6 03/12] tpm-buf: add cursor based functions for response parsing
It's very convenient when parsing responses to have a cursor you
simply move over the response extracting the data. Add such cursor
functions for the TPM unsigned integer types.
Signed-off-by: James Bottomley
---
drivers/char/tpm/tpm-buf.c | 26 ++
drivers/char/tpm/tpm.h | 4
2 files changed, 30 insertions(+)
diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
index 8c1ed8a14e01..553adb84b0ac 100644
--- a/drivers/char/tpm/tpm-buf.c
+++ b/drivers/char/tpm/tpm-buf.c
@@ -163,3 +163,29 @@ void tpm_buf_append_2b(struct tpm_buf *buf, struct tpm_buf
*tpm2b)
}
EXPORT_SYMBOL_GPL(tpm_buf_append_2b);
+/* functions for unmarshalling data and moving the cursor */
+u8 tpm_get_inc_u8(const u8 **ptr)
+{
+ return *((*ptr)++);
+}
+EXPORT_SYMBOL_GPL(tpm_get_inc_u8);
+
+u16 tpm_get_inc_u16(const u8 **ptr)
+{
+ u16 val;
+
+ val = get_unaligned_be16(*ptr);
+ *ptr += sizeof(val);
+ return val;
+}
+EXPORT_SYMBOL_GPL(tpm_get_inc_u16);
+
+u32 tpm_get_inc_u32(const u8 **ptr)
+{
+ u32 val;
+
+ val = get_unaligned_be32(*ptr);
+ *ptr += sizeof(val);
+ return val;
+}
+EXPORT_SYMBOL_GPL(tpm_get_inc_u32);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 7627917db345..d942188debc9 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -302,6 +302,10 @@ void tpm_buf_append_u16(struct tpm_buf *buf, const u16
value);
void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value);
void tpm_buf_append_2b(struct tpm_buf *buf, struct tpm_buf *tpm2b);
+u8 tpm_get_inc_u8(const u8 **ptr);
+u16 tpm_get_inc_u16(const u8 **ptr);
+u32 tpm_get_inc_u32(const u8 **ptr);
+
extern struct class *tpm_class;
extern struct class *tpmrm_class;
extern dev_t tpm_devt;
--
2.16.4
[PATCH v6 02/12] tpm-buf: add handling for TPM2B types
Most complex TPM commands require appending TPM2B buffers to the
command body. Since TPM2B types are essentially variable size arrays,
it makes it impossible to represent these complex command arguments as
structures and we simply have to build them up using append primitives
like these.
Signed-off-by: James Bottomley
---
drivers/char/tpm/tpm-buf.c | 47 ++
drivers/char/tpm/tpm.h | 2 ++
2 files changed, 49 insertions(+)
diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
index 9fa8a9cb0fdf..8c1ed8a14e01 100644
--- a/drivers/char/tpm/tpm-buf.c
+++ b/drivers/char/tpm/tpm-buf.c
@@ -8,6 +8,8 @@
#include
+#include
+
static int __tpm_buf_init(struct tpm_buf *buf)
{
buf->data_page = alloc_page(GFP_HIGHUSER);
@@ -46,6 +48,24 @@ int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
}
EXPORT_SYMBOL_GPL(tpm_buf_init);
+int tpm_buf_init_2b(struct tpm_buf *buf)
+{
+ struct tpm_header *head;
+ int rc;
+
+ rc = __tpm_buf_init(buf);
+ if (rc)
+ return rc;
+
+ head = (struct tpm_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);
@@ -53,6 +73,13 @@ void tpm_buf_destroy(struct tpm_buf *buf)
}
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_header *head = (struct tpm_header *)buf->data;
@@ -116,3 +143,23 @@ void tpm_buf_append_u32(struct tpm_buf *buf, const u32
value)
tpm_buf_append(buf, (u8 *) &value2, 4);
}
EXPORT_SYMBOL_GPL(tpm_buf_append_u32);
+
+static void tpm_buf_reset_int(struct tpm_buf *buf)
+{
+ struct tpm_header *head;
+
+ head = (struct tpm_header *)buf->data;
+ head->length = cpu_to_be32(sizeof(*head));
+}
+
+void tpm_buf_append_2b(struct tpm_buf *buf, struct tpm_buf *tpm2b)
+{
+ u16 len = tpm_buf_length(tpm2b);
+
+ tpm_buf_append_u16(buf, len);
+ tpm_buf_append(buf, tpm_buf_data(tpm2b), len);
+ /* clear the buf for reuse */
+ tpm_buf_reset_int(tpm2b);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append_2b);
+
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 8c5b8bba60d2..7627917db345 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -292,6 +292,7 @@ struct tpm_buf {
int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal);
void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal);
+int tpm_buf_init_2b(struct tpm_buf *buf);
void tpm_buf_destroy(struct tpm_buf *buf);
u32 tpm_buf_length(struct tpm_buf *buf);
void tpm_buf_append(struct tpm_buf *buf, const unsigned char *new_data,
@@ -299,6 +300,7 @@ void tpm_buf_append(struct tpm_buf *buf, const unsigned
char *new_data,
void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value);
void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value);
void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value);
+void tpm_buf_append_2b(struct tpm_buf *buf, struct tpm_buf *tpm2b);
extern struct class *tpm_class;
extern struct class *tpmrm_class;
--
2.16.4
[PATCH v6 01/12] tpm-buf: move from static inlines to real functions
This separates out the old tpm_buf_... handling functions from static
inlines in tpm.h and makes them their own tpm-buf.c file. This is a
precursor so we can add new functions for other TPM type handling
Signed-off-by: James Bottomley
---
v2: added this patch to separate out the API changes
v3: added tpm_buf_reset_cmd()
v6: extract first then add functions
---
drivers/char/tpm/Makefile | 1 +
drivers/char/tpm/tpm-buf.c | 118 +
drivers/char/tpm/tpm.h | 90 --
3 files changed, 129 insertions(+), 80 deletions(-)
create mode 100644 drivers/char/tpm/tpm-buf.c
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index a01c4cab902a..78bd025b808a 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -15,6 +15,7 @@ tpm-y += tpm-sysfs.o
tpm-y += eventlog/common.o
tpm-y += eventlog/tpm1.o
tpm-y += eventlog/tpm2.o
+tpm-y += tpm-buf.o
tpm-$(CONFIG_ACPI) += tpm_ppi.o eventlog/acpi.o
tpm-$(CONFIG_EFI) += eventlog/efi.o
diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
new file mode 100644
index ..9fa8a9cb0fdf
--- /dev/null
+++ b/drivers/char/tpm/tpm-buf.c
@@ -0,0 +1,118 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Handing for tpm_buf structures to facilitate the building of commands
+ */
+
+#include "tpm.h"
+
+#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_header *head;
+
+ head = (struct tpm_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);
+
+void tpm_buf_destroy(struct tpm_buf *buf)
+{
+ kunmap(buf->data_page);
+ __free_page(buf->data_page);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_destroy);
+
+u32 tpm_buf_length(struct tpm_buf *buf)
+{
+ struct tpm_header *head = (struct tpm_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_header *head = (struct tpm_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_header *head = (struct tpm_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(&buf->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, &value, 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 *) &value2, 2);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append_u16);
+
+void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
+{
+ __be32 value2 = cpu_to_be32(value);
+
+ tpm_buf_append(buf, (u8 *) &value2, 4);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append_u32);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a7fea3e0ca86..8c5b8bba60d2 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -281,6 +281,7 @@ enum tpm_sub_capabilities {
enum tpm_buf_flags {
TPM_BUF_OVERFLOW= BIT(0),
+ TPM_BUF_2B = BIT(1),
};
struct tpm_buf {
@@ -289,86 +290,15 @@ struct tpm_buf {
u8 *data;
};
-static inline void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
-{
- struct tpm_header *head = (struct tpm_header *)buf->data;
-
- head->tag = cpu_to_be16(tag);
- head->length = cpu_to_be32(sizeof(*head));
- head->ordinal = cpu_to_be32(ordinal);
-}
-
-static inline int tpm_buf_init
[PATCH v6 00/12] add integrity and security to TPM2 transactions
Link to previous cover letter: https://lore.kernel.org/linux-integrity/1540193596.3202.7.ca...@hansenpartnership.com/ This is marked v6 instead of v5 because I did a v5 after feedback on v4 but didn't get around to posting it and then had to rework the whole of the kernel space handling while I was on holiday. I also added the documentation of how the whole thing works and the rationale for doing it in tpm-security.rst (patch 11). The main reason for doing this now is so we have something to discuss at Plumbers. The new patch set implements the various splits requested, but the main changes are that the kernel space is gone and is replaced by a context save and restore of the generated null seed. This is easier to handle than a full kernel space given the new threading for TPM spaces, but conceptually it is still very like a space. I've also made whether integrity and encryption is turned on a Kconfig option. James --- James Bottomley (12): tpm-buf: move from static inlines to real functions tpm-buf: add handling for TPM2B types tpm-buf: add cursor based functions for response parsing tpm2-space: export the context save and load commands tpm2-sessions: Add full HMAC and encrypt/decrypt session handling tpm-buf: add tpm_buf_parameters() 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 Documentation: add tpm-security.rst tpm2-sessions: NOT FOR COMMITTING add sessions testing Documentation/security/tpm/tpm-security.rst | 204 + drivers/char/tpm/Kconfig| 11 + drivers/char/tpm/Makefile |4 + drivers/char/tpm/tpm-buf.c | 202 + drivers/char/tpm/tpm-chip.c |1 + drivers/char/tpm/tpm-sysfs.c| 27 +- drivers/char/tpm/tpm.h | 117 +-- drivers/char/tpm/tpm2-cmd.c | 202 +++-- drivers/char/tpm/tpm2-sessions-test.c | 795 ++ drivers/char/tpm/tpm2-sessions.c| 1204 +++ drivers/char/tpm/tpm2-sessions.h| 138 +++ drivers/char/tpm/tpm2-space.c |8 +- include/linux/tpm.h | 29 + 13 files changed, 2787 insertions(+), 155 deletions(-) create mode 100644 Documentation/security/tpm/tpm-security.rst 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 v7 09/16] fscrypt: add an HKDF-SHA512 implementation
On Mon, 2019-07-29 at 13:29 -0700, Eric Biggers wrote:
> On Sun, Jul 28, 2019 at 03:39:49PM -0400, Theodore Y. Ts'o wrote:
> > On Fri, Jul 26, 2019 at 03:41:34PM -0700, Eric Biggers wrote:
> > > From: Eric Biggers
[...]
> > > HKDF solves all the above problems.
> > >
> > > Signed-off-by: Eric Biggers
> >
> > Unless I missed something there's nothing here which is fscrypt
> > specific. Granted that it's somewhat unlikely that someone would
> > want to implement (the very bloated) IKE from IPSEC in the kernel,
> > I wonder if there might be other users of HKDF, and whether this
> > would be better placed in lib/ or crypto/ instead of fs/crypto?
>
> This is standard HKDF-SHA512; only the choice of parameters is
> fscrypt-specific. So it could indeed use a common implementation of
> HKDF if one were available.
>
> However, I don't think there are any other HKDF users in the kernel
> currently.
Well, I'm still trying to add the TPM ones, but they're based on SP800-
108 for arbitrary keys and SP800-56A for elliptic curve ones. These
are similar to the RFC5869 except that they do extract/expand in a
single operation. Plus, of course, the TPM mandates we use the name
algorithm (usually sha256, which is what I hardcoded) as the hash.
Note: since you don't use the extract step either in your
implementation, effectively you're equivalent to SP800-108 as well.
This is effectively the same reason as the TPM: we need deterministic
keys, so we've nowhere to get the salt from that would persist.
> Also, while there was a patch to support HKDF via the crypto_rng API,
> there was no consensus about whether this was actually the best way
> to add KDF support:
> https://lore.kernel.org/linux-crypto/2423373.zd5thvq...@positron.chro
> nox.de
>
> So for now, to avoid unnecessarily blocking this patchset I think we
> should just go with this implementation in fs/crypto/. It can always
> be changed later, once we decide on the best way to add KDFs to the
> crypto API.
>
> [To be clear: this patch already uses "hmac(sha512)" from the crypto
> API. It's only the actual HKDF part that we're talking about here.
Right, once you have the hmac + hash available, the rest is easy, so
this is what we have for the TPM KDFa:
static void KDFa(u8 *key, int keylen, const char *label, u8 *u,
u8 *v, int bytes, u8 *out)
{
u32 counter;
const __be32 bits = cpu_to_be32(bytes * 8);
for (counter = 1; bytes > 0; bytes -= SHA256_DIGEST_SIZE, counter++,
out += SHA256_DIGEST_SIZE) {
SHASH_DESC_ON_STACK(desc, sha256_hash);
__be32 c = cpu_to_be32(counter);
hmac_init(desc, key, keylen);
crypto_shash_update(desc, (u8 *)&c, sizeof(c));
crypto_shash_update(desc, label, strlen(label)+1);
crypto_shash_update(desc, u, SHA256_DIGEST_SIZE);
crypto_shash_update(desc, v, SHA256_DIGEST_SIZE);
crypto_shash_update(desc, (u8 *)&bits, sizeof(bits));
hmac_final(desc, key, keylen, out);
}
}
I honestly think these things are so simplistic with the correct hmac
that it would make it more confusing to try to produce a general KDF
than it would simply to hard code them where we need them.
James
Re: [PATCH 4/6] crypto: hkdf - RFC5869 Key Derivation Function
On Sun, 2019-01-13 at 08:56 +0100, Stephan Müller wrote: > The question may arise why to plug the KDFs into RNGs. The answer is > quite simple: KDFs are a form of random number generator. In that > they take some input for initialization (aka seed, salt, key, > personalization string). Then they produce pseudo-random bit > sequences of arbitrary length. Possibly the generation operation can > be modified by providing some additional input to be used by the > generation process (aka label, context, info string, additional > information string). Thus, the RNG interface is a natural fit for the > KDFs. Philosophically, that's quite wrong. KDFs are a class of pseudorandom functions (PRFs). PRFs are designed so that the output is indistinguishable from a random number generator to observers who don't know the input but is deterministically useful for participants who do. That means the're definitely not RNGs they're functions whose output is designed to look like the output of an RNG. I suppose the mathematical thing that distinguishes PRFs and RNGs is entropy: PRFs have zero entropy because given the same inputs you expect the same output. Now whether it makes sense to use the RNG API or not I'll leave that up to the crypto people. I would have expected any cryptographic RNG API to be mostly about entropy management (the Linux core internal one certainly is), but it appears that the one in crypto isn't. James
Re: [PATCH v4 2/7] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
On Wed, 2018-10-24 at 02:48 +0300, Jarkko Sakkinen wrote:
> On Mon, 22 Oct 2018, James Bottomley wrote:
> > [...]
I'll tidy up the descriptions.
> 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).
You mean move tpm2_buf_append_auth() into the new sessions file as well
... sure, I can do that.
[...]
> > +
> > +/*
> > + * assume hash sha256 and nonces u, v of size SHA256_DIGEST_SIZE
> > but
> > + * otherwise standard KDFa. Note output is in bytes not bits.
> > + */
> > +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).
> > +{
> > + u32 counter;
> > + const __be32 bits = cpu_to_be32(bytes * 8);
> > +
> > + for (counter = 1; bytes > 0; bytes -= SHA256_DIGEST_SIZE,
> > counter++,
> > +out += SHA256_DIGEST_SIZE) {
>
> Only one counter is actually used for anything so this is overly
> complicated and IMHO it is ok to call the counter just 'i'. Maybe
> just:
>
> for (i = 1; (bytes - (i - 1) * SHA256_DIGEST_SIZE) > 0; i++) {
>
> > + SHASH_DESC_ON_STACK(desc, sha256_hash);
> > + __be32 c = cpu_to_be32(counter);
> > +
> > + hmac_init(desc, key, keylen);
> > + crypto_shash_update(desc, (u8 *)&c, sizeof(c));
> > + crypto_shash_update(desc, label, strlen(label)+1);
> > + crypto_shash_update(desc, u, SHA256_DIGEST_SIZE);
> > + crypto_shash_update(desc, v, SHA256_DIGEST_SIZE);
> > + crypto_shash_update(desc, (u8 *)&bits,
> > sizeof(bits));
> > + hmac_final(desc, key, keylen, out);
> > + }
> > +}
> > +
> > +/*
> > + * Somewhat of a bastardization of the real KDFe. We're assuming
> > + * we're working with known point sizes for the input parameters
> > and
> > + * the hash algorithm is fixed at sha256. Because we know that
> > the
> > + * point size is 32 bytes like the hash size, there's no need to
> > loop
> > + * in this KDF.
> > + */
> > +static void KDFe(u8 z[EC_PT_SZ], const char *str, u8 *pt_u, u8
> > *pt_v,
> > +u8 *keyout)
> > +{
> > + SHASH_DESC_ON_STACK(desc, sha256_hash);
> > + /*
> > +* this should be an iterative counter, but because we
> > know
> > +* we're only taking 32 bytes for the point using a
> > sha256
> > +* hash which is also 32 bytes, there's only one loop
> > +*/
> > + __be32 c = cpu_to_be32(1);
> > +
> > + desc->tfm = sha256_hash;
> > + desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
> > +
> > + crypto_shash_init(desc);
> > + /* counter (BE) */
> > + crypto_shash_update(desc, (u8 *)&c, sizeof(c));
> > + /* secret value */
> > + crypto_shash_update(desc, z, EC_PT_SZ);
> > + /* string including trailing zero */
> > + crypto_shash_update(desc, str, strlen(str)+1);
> > + crypto_shash_update(desc, pt_u, EC_PT_SZ);
> > + crypto_shash_update(desc, pt_v, EC_PT_SZ);
> > + crypto_shash_final(desc, keyout);
> > +}
> > +
> > +static void tpm_buf_append_salt(struct tpm_buf *buf, struct
> > tpm_chip *chip,
> > + struct tpm2_auth *auth)
>
> Given the complexity of this function and some not that obvious
> choices in the implementation (coordinates), it would make sense to
> document this function.
I'll try to beef up the salting description
> > +{
> > + struct crypto_kpp *kpp;
> > + struct kpp_request *req;
> > + struct scatterlist s[2], d[1];
> > + struct ecdh p = {0};
> > + u8 encoded_key[EC_PT_SZ], *x, *y;
>
> Why you use one character variable name 'p' and longer name
> 'encoded_key'?
>
> > + unsigned int buf_len;
> > + u8 *secret;
> > +
> > + secret = kmalloc(EC_PT_SZ, GFP_KERNEL);
> > + if (!secret)
> > + return;
> > +
> > + p.curve_id = ECC_CURVE_NIST_P256;
>
Re: [PATCH v4 0/7] add integrity and security to TPM2 transactions
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
Re: [PATCH v4 2/7] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
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.
> However, I take it this means that hmac_init() is never called in
> contexts where sleeping is not allowed? For the relevance of that,
> please see below.
Right, these routines are always called as an adjunct to a TPM
operation and TPM operations can sleep, so we must at least have kernel
thread context.
[...]
> > + /* encrypt before HMAC */
> > + if (auth->attrs & TPM2_SA_DECRYPT) {
> > + struct scatterlist sg[1];
> > + u16 len;
> > + SKCIPHER_REQUEST_ON_STACK(req, auth->aes);
> > +
> > + skcipher_request_set_tfm(req, auth->aes);
> > + skcipher_request_set_callback(req,
> > CRYPTO_TFM_REQ_MAY_SLEEP,
> > + NULL, NULL);
> > +
>
> Your crypto_alloc_skcipher() call [further down] does not mask out
> CRYPTO_ALG_ASYNC, and so it permits asynchronous implementations to
> be selected. Your generic cfb template only permits synchronous
> implementations, since it wraps the cipher directly (which is always
> synchronous). However, we have support in the tree for some
> accelerators that implement cfb(aes), and those will return
> -EINPROGRESS when invoking crypto_skcipher_en/decrypt(req), which you
> are not set up to handle.
>
> So the simple solution is to call 'crypto_alloc_skcipher("cfb(aes)",
> 0, CRYPTO_ALG_ASYNC)' below instead.
>
> However, I would prefer it if we permit asynchronous skciphers here.
> The reason is that, on a given platform, the accelerator may be the
> only truly time invariant AES implementation that is available, and
> since we are dealing with the TPM, a bit of paranoia does not hurt.
> It also makes it easier to implement it as a [time invariant] SIMD
> based asynchronous skcipher, which are simpler than synchronous ones
> since they don't require a non-SIMD fallback path for calls from
> contexts where the SIMD unit may not be used.
>
> I have already implemented cfb(aes) for arm64/NEON. I will post the
> patch after the merge window closes.
>
> > + /* need key and IV */
> > + KDFa(auth->session_key, SHA256_DIGEST_SIZE
> > ++ auth->passphraselen, "CFB", auth->our_nonce,
> > +auth->tpm_nonce, AES_KEYBYTES +
> > AES_BLOCK_SIZE,
> > +auth->scratch);
> > + crypto_skcipher_setkey(auth->aes, auth->scratch,
> > AES_KEYBYTES);
> > + len = tpm_get_inc_u16(&p);
> > + sg_init_one(sg, p, len);
> > + skcipher_request_set_crypt(req, sg, sg, len,
> > + auth->scratch +
> > AES_KEYBYTES);
> > + crypto_skcipher_encrypt(req);
>
> So please consider replacing this with something like.
>
> DECLARE_CRYPTO_WAIT(wait); [further up]
> skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP,
> crypto_req_done, &wait);
> crypto_wait_req(crypto_skcipher_encrypt(req), &wait);
Sure, I can do this ... the crypto skcipher handling was also cut and
paste, but I forget where from this time. So what I think you're
asking for is below as the incremental diff? I've tested this out and
it all works fine in my session testing environment (and on my real
laptop) ... although since I'm fully sync, I won't really have tested
the -EINPROGRESS do the wait case.
James
---
diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index 422c3ec64f8c..bbd3e8580954 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -165,7 +165,6 @@ static void hmac_init(struct shash_desc *desc, u8 *key, int
keylen)
int i;
desc->tfm = sha256_hash;
- desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
crypto_shash_init(desc);
for (i = 0; i < sizeof(pad); i++) {
if (i < keylen)
@@ -244,7 +243,6 @@ static void KDFe(u8 z[EC_PT_SZ], const char *str, u8 *pt_u,
u8 *pt_v,
__be32 c = cpu_to_be32(1);
desc->tfm = sha256_hash;
- desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
crypto_shash_init(desc);
/* counter (BE) */
@@ -445,7 +443,6 @@ void tpm_buf_fill_hmac_session(struct tpm_buf *buf, struct
tpm2_auth *auth)
auth->ordinal = head->ord
[PATCH v4 7/7] tpm2-sessions: NOT FOR COMMITTING add sessions testing
This runs through a preset sequence using sessions to demonstrate that
the session handling code functions. It does both HMAC, encryption
and decryption by testing an encrypted sealing operation with
authority and proving that the same sealed data comes back again via
an HMAC and response encryption. It also does policy unsealing which
mimics the more complex of the trusted key scenarios.
Signed-off-by: James Bottomley
---
v3: add policy unseal testing with two sessions
---
drivers/char/tpm/Makefile | 1 +
drivers/char/tpm/tpm-chip.c | 1 +
drivers/char/tpm/tpm.h| 1 +
drivers/char/tpm/tpm2-cmd.c | 2 +
drivers/char/tpm/tpm2-sessions-test.c | 360 ++
5 files changed, 365 insertions(+)
create mode 100644 drivers/char/tpm/tpm2-sessions-test.c
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 1b5f6ccbb86d..b3f5e95679f9 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -6,6 +6,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 tpm2-sessions.o
+obj-m += tpm2-sessions-test.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-chip.c b/drivers/char/tpm/tpm-chip.c
index 46caadca916a..ef7ff32bf827 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -142,6 +142,7 @@ struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip)
return NULL;
return chip;
}
+EXPORT_SYMBOL(tpm_find_get_ops);
/**
* tpm_dev_release() - free chip memory and the device number
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index d39065d9995d..8cb8b32a8418 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -163,6 +163,7 @@ enum tpm2_command_codes {
TPM2_CC_CONTEXT_LOAD= 0x0161,
TPM2_CC_CONTEXT_SAVE= 0x0162,
TPM2_CC_FLUSH_CONTEXT = 0x0165,
+ TPM2_CC_POLICY_COMMAND_CODE = 0x16c,
TPM2_CC_READ_PUBLIC = 0x0173,
TPM2_CC_START_AUTH_SESS = 0x0176,
TPM2_CC_GET_CAPABILITY = 0x017A,
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index a8655cd535d1..7afe0f50dcce 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -379,6 +379,7 @@ void tpm2_flush_context_cmd(struct tpm_chip *chip, u32
handle,
tpm_buf_destroy(&buf);
}
+EXPORT_SYMBOL_GPL(tpm2_flush_context_cmd);
/**
* tpm_buf_append_auth() - append TPMS_AUTH_COMMAND to the buffer.
@@ -409,6 +410,7 @@ void tpm2_buf_append_auth(struct tpm_buf *buf, u32
session_handle,
if (hmac && hmac_len)
tpm_buf_append(buf, hmac, hmac_len);
}
+EXPORT_SYMBOL_GPL(tpm2_buf_append_auth);
/**
* tpm2_seal_trusted() - seal the payload of a trusted key
diff --git a/drivers/char/tpm/tpm2-sessions-test.c
b/drivers/char/tpm/tpm2-sessions-test.c
new file mode 100644
index ..7b4a01ad3745
--- /dev/null
+++ b/drivers/char/tpm/tpm2-sessions-test.c
@@ -0,0 +1,360 @@
+/* run a set of tests of the sessions code */
+#include "tpm.h"
+#include "tpm2-sessions.h"
+
+#include
+
+#include
+
+/* simple policy: command code must be TPM2_CC_UNSEAL */
+static u8 policy[] = {
+ 0xe6, 0x13, 0x13, 0x70, 0x76, 0x52, 0x4b, 0xde,
+ 0x48, 0x75, 0x33, 0x86, 0x58, 0x84, 0xe9, 0x73,
+ 0x2e, 0xbe, 0xe3, 0xaa, 0xcb, 0x09, 0x5d, 0x94,
+ 0xa6, 0xde, 0x49, 0x2e, 0xc0, 0x6c, 0x46, 0xfa,
+};
+
+static u32 get_policy(struct tpm_chip *chip)
+{
+ struct tpm_buf buf;
+ u8 nonce[SHA256_DIGEST_SIZE];
+ u32 h;
+ int rc;
+
+ rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_START_AUTH_SESS);
+ if (rc)
+ return 0;
+
+ /* salt key */
+ tpm_buf_append_u32(&buf, TPM2_RH_NULL);
+ /* bind key */
+ tpm_buf_append_u32(&buf, TPM2_RH_NULL);
+ /* zero nonce */
+ memset(nonce, 0, sizeof(nonce));
+ tpm_buf_append_u16(&buf, sizeof(nonce));
+ tpm_buf_append(&buf, nonce, sizeof(nonce));
+ /* encrypted salt (empty) */
+ tpm_buf_append_u16(&buf, 0);
+ /* session type (HMAC, audit or policy) */
+ tpm_buf_append_u8(&buf, TPM2_SE_POLICY);
+ /* symmetric encryption parameters */
+ /* symmetric algorithm */
+ tpm_buf_append_u16(&buf, TPM2_ALG_NULL);
+
+ /* hash algorithm for session */
+ tpm_buf_append_u16(&buf, TPM2_ALG_SHA256);
+
+ rc = tpm_transmit_cmd(chip, &chip->kernel_space, buf.data, PAGE_SIZE,
+ 0, 0, "start policy session");
+
+ h = get_unaligned_be32(&buf.data[TPM_HEADER_SIZE]);
+
+ t
[PATCH v4 6/7] tpm: add the null key name as a tpm2 sysfs variable
This is the last component of encrypted tpm2 session handling that
allows us to verify from userspace that the key derived from the NULL
seed genuinely belongs to the TPM and has not been spoofed.
The procedure for doing this involves creating an attestation identity
key (which requires verification of the TPM EK certificate) and then
using that AIK to sign a certification of the Elliptic Curve key over
the NULL seed. Userspace must create this EC Key using the parameters
prescribed in TCG TPM v2.0 Provisioning Guidance for the SRK ECC; if
this is done correctly the names will match and the TPM can then run a
TPM2_Certify operation on this derived primary key using the newly
created AIK.
Signed-off-by: James Bottomley
---
drivers/char/tpm/tpm-sysfs.c | 27 ++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index 83a77a445538..7e901d9e893e 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -284,6 +284,19 @@ static ssize_t timeouts_show(struct device *dev, struct
device_attribute *attr,
}
static DEVICE_ATTR_RO(timeouts);
+static ssize_t null_name_show(struct device *dev, struct device_attribute
*attr,
+ char *buf)
+{
+ struct tpm_chip *chip = to_tpm_chip(dev);
+ int size = TPM2_NAME_SIZE;
+
+ bin2hex(buf, chip->tpmkeyname, size);
+ size *= 2;
+ buf[size++] = '\n';
+ return size;
+}
+static DEVICE_ATTR_RO(null_name);
+
static struct attribute *tpm_dev_attrs[] = {
&dev_attr_pubek.attr,
&dev_attr_pcrs.attr,
@@ -298,17 +311,29 @@ static struct attribute *tpm_dev_attrs[] = {
NULL,
};
+static struct attribute *tpm2_dev_attrs[] = {
+ &dev_attr_null_name.attr,
+ NULL,
+};
+
static const struct attribute_group tpm_dev_group = {
.attrs = tpm_dev_attrs,
};
+static const struct attribute_group tpm2_dev_group = {
+ .attrs = tpm2_dev_attrs,
+};
+
void tpm_sysfs_add_device(struct tpm_chip *chip)
{
/* XXX: If you wish to remove this restriction, you must first update
* tpm_sysfs to explicitly lock chip->ops.
*/
- if (chip->flags & TPM_CHIP_FLAG_TPM2)
+ if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+ WARN_ON(chip->groups_cnt != 0);
+ chip->groups[chip->groups_cnt++] = &tpm2_dev_group;
return;
+ }
/* The sysfs routines rely on an implicit tpm_try_get_ops, device_del
* is called before ops is null'd and the sysfs core synchronizes this
--
2.16.4
[PATCH v4 5/7] trusted keys: Add session encryption protection to the seal/unseal path
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(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
+ rc = tpm2_start_auth_session(chip, &auth);
if (rc)
return rc;
- tpm_buf_append_u32(&buf, options->keyhandle);
- tpm2_buf_append_auth(&buf, TPM2_RS_PW,
-NULL /* nonce */, 0,
-0 /* session_attributes */,
-options->keyauth /* hmac */,
-TPM_DIGEST_SIZE);
+ rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
+ if (rc) {
+ tpm2_end_auth_session(auth);
+ return rc;
+ }
+
+ rc = tpm_buf_init_2b(&t2b);
+ if (rc) {
+ tpm_buf_destroy(&buf);
+ tpm2_end_auth_session(auth);
+ return rc;
+ }
+ tpm_buf_append_name(&buf, auth, options->keyhandle, NULL);
+ tpm_buf_append_hmac_session(&buf, auth, TPM2_SA_DECRYPT,
+ options->keyauth, TPM_DIGEST_SIZE);
/* sensitive */
- tpm_buf_append_u16(&buf, 4 + TPM_DIGEST_SIZE + payload->key_len + 1);
+ tpm_buf_append_u16(&t2b, TPM_DIGEST_SIZE);
+ tpm_buf_append(&t2b, options->blobauth, TPM_DIGEST_SIZE);
+ tpm_buf_append_u16(&t2b, payload->key_len + 1);
+ tpm_buf_append(&t2b, payload->key, payload->key_len);
+ tpm_buf_append_u8(&t2b, payload->migratable);
- tpm_buf_append_u16(&buf, TPM_DIGEST_SIZE);
- tpm_buf_append(&buf, options->blobauth, TPM_DIGEST_SIZE);
- tpm_buf_append_u16(&buf, payload->key_len + 1);
- tpm_buf_append(&buf, payload->key, payload->key_len);
- tpm_buf_append_u8(&buf, payload->migratable);
+ tpm_buf_append_2b(&buf, &t2b);
/* public */
- tpm_buf_append_u16(&buf, 14 + options->policydigest_len);
- tpm_buf_append_u16(&buf, TPM2_ALG_KEYEDHASH);
- tpm_buf_append_u16(&buf, hash);
+ tpm_buf_append_u16(&t2b, TPM2_ALG_KEYEDHASH);
+ tpm_buf_append_u16(&t2b, hash);
/* policy */
if (options->policydigest_len) {
- tpm_buf_append_u32(&buf, 0);
- tpm_buf_append_u16(&buf, options->policydigest_len);
- tpm_buf_append(&buf, options->policydigest,
+ tpm_buf_append_u32(&t2b, 0);
+ tpm_buf_append_u16(&t2b, options->policydigest_len);
+ tpm_buf_append(&t2b, options->policydigest,
options->policydigest_len);
} else {
- tpm_buf_append_u32(&buf, TPM2_OA_USER_WITH_AUTH);
- tpm_buf_append_u16(&buf, 0);
+ tpm_buf_append_u32(&t2b, TPM2_OA_USER_WITH_AUTH);
+ tpm_buf_append_u16(&t2b, 0);
}
/* public parameters */
- tpm_buf_append_u16(&buf, TPM2_ALG_NULL);
- tpm_buf_append_u16(&buf, 0);
+ tpm_buf_append_u16(&t2b, TPM2_ALG_NULL);
+ /* unique (zero) */
+ tpm_buf_append_u16(&t2b, 0);
+
+ tpm_buf_append_2b(&buf, &t2b);
/* outside info */
tpm_buf_append_u16(&buf, 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(&buf, auth);
+
+ rc = tp
[PATCH v4 4/7] tpm2: add session encryption protection to tpm2_get_random()
If some entity is snooping the TPM bus, they can see the random
numbers we're extracting from the TPM and do prediction attacks
against their consumers. Foil this attack by using response
encryption to prevent the attacker from seeing the random sequence.
Signed-off-by: James Bottomley
---
v3: add error handling to sessions and redo to be outside loop
---
drivers/char/tpm/tpm2-cmd.c | 24 ++--
1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 332b34b347f1..22f1c7bee173 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -266,7 +266,6 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32
count,
return rc;
}
-
struct tpm2_get_random_out {
__be16 size;
u8 buffer[TPM_MAX_RNG_DATA];
@@ -293,21 +292,32 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest,
size_t max)
int total = 0;
int retries = 5;
u8 *dest_ptr = dest;
+ struct tpm2_auth *auth;
if (!num_bytes || max > TPM_MAX_RNG_DATA)
return -EINVAL;
- err = tpm_buf_init(&buf, 0, 0);
+ err = tpm2_start_auth_session(chip, &auth);
if (err)
return err;
+ err = tpm_buf_init(&buf, 0, 0);
+ if (err) {
+ tpm2_end_auth_session(auth);
+ return err;
+ }
+
do {
- tpm_buf_reset(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_GET_RANDOM);
+ tpm_buf_reset(&buf, TPM2_ST_SESSIONS, TPM2_CC_GET_RANDOM);
+ tpm_buf_append_hmac_session(&buf, auth, TPM2_SA_ENCRYPT
+ | TPM2_SA_CONTINUE_SESSION,
+ NULL, 0);
tpm_buf_append_u16(&buf, num_bytes);
- err = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
- offsetof(struct tpm2_get_random_out,
- buffer),
+ tpm_buf_fill_hmac_session(&buf, auth);
+ err = tpm_transmit_cmd(chip, &chip->kernel_space, buf.data,
+ PAGE_SIZE, TPM_HEADER_SIZE + 2,
0, "attempting get random");
+ err = tpm_buf_check_hmac_response(&buf, auth, err);
if (err)
goto out;
@@ -327,6 +337,8 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t
max)
} while (retries-- && total < max);
tpm_buf_destroy(&buf);
+ tpm2_end_auth_session(auth);
+
return total ? total : -EIO;
out:
tpm_buf_destroy(&buf);
--
2.16.4
[PATCH v4 3/7] tpm2: add hmac checks to tpm2_pcr_extend()
We use tpm2_pcr_extend() in trusted keys to extend a PCR to prevent a
key from being re-loaded until the next reboot. To use this
functionality securely, that extend must be protected by a session
hmac.
Signed-off-by: James Bottomley
---
v3: add error handling to sessions
---
drivers/char/tpm/tpm2-cmd.c | 33 +
1 file changed, 13 insertions(+), 20 deletions(-)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index a17e5c573c4e..332b34b347f1 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -209,13 +209,6 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8
*res_buf)
return rc;
}
-struct tpm2_null_auth_area {
- __be32 handle;
- __be16 nonce_size;
- u8 attributes;
- __be16 auth_size;
-} __packed;
-
/**
* tpm2_pcr_extend() - extend a PCR value
*
@@ -230,7 +223,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32
count,
struct tpm2_digest *digests)
{
struct tpm_buf buf;
- struct tpm2_null_auth_area auth_area;
+ struct tpm2_auth *auth;
int rc;
int i;
int j;
@@ -238,20 +231,19 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx,
u32 count,
if (count > ARRAY_SIZE(chip->active_banks))
return -EINVAL;
- rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
+ rc = tpm2_start_auth_session(chip, &auth);
if (rc)
return rc;
- tpm_buf_append_u32(&buf, pcr_idx);
+ rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
+ if (rc) {
+ tpm2_end_auth_session(auth);
+ return rc;
+ }
- auth_area.handle = cpu_to_be32(TPM2_RS_PW);
- auth_area.nonce_size = 0;
- auth_area.attributes = 0;
- auth_area.auth_size = 0;
+ tpm_buf_append_name(&buf, auth, pcr_idx, NULL);
+ tpm_buf_append_hmac_session(&buf, auth, 0, NULL, 0);
- tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area));
- tpm_buf_append(&buf, (const unsigned char *)&auth_area,
- sizeof(auth_area));
tpm_buf_append_u32(&buf, count);
for (i = 0; i < count; i++) {
@@ -264,9 +256,10 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx,
u32 count,
hash_digest_size[tpm2_hash_map[j].crypto_id]);
}
}
-
- rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
- "attempting extend a PCR value");
+ tpm_buf_fill_hmac_session(&buf, auth);
+ rc = tpm_transmit_cmd(chip, &chip->kernel_space, buf.data, PAGE_SIZE,
+ 0, 0, "attempting extend a PCR value");
+ rc = tpm_buf_check_hmac_response(&buf, auth, rc);
tpm_buf_destroy(&buf);
--
2.16.4
[PATCH v4 2/7] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
This code adds true session based HMAC authentication plus parameter
decryption and response encryption using AES.
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
* tpm_buf_append_hmac_session() where tpm2_append_auth() would go
* 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.
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
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
+
enum tpm_const {
TPM_MINOR = 224,/* officially assigned */
TPM_BUFSIZE = 4096,
@@ -103,6 +112,7 @@ enum tpm2_timeouts {
enum tpm2_structures {
TPM2_ST_NO_SESSIONS = 0x8001,
TPM2_ST_SESSIONS= 0x8002,
+ TPM2_ST_CREATION= 0x8021,
};
/* Indicates from what layer of the software stack the error comes from */
@@ -125,12 +135,20 @@ enum tpm2_return_codes {
enum tpm2_algorithms {
TPM2_ALG_ERROR = 0x,
TPM2_ALG_SHA1 = 0x0004,
+ TPM2_ALG_AES= 0x0006,
TPM2_ALG_KEYEDHASH = 0x0008,
TPM2_ALG_SHA256 = 0x000B,
[PATCH v4 1/7] tpm-buf: create new functions for handling TPM buffers
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
---
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(&buf->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, &value, 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 *) &value2, 2);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append_u16);
+
+void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
+{
+ __be32 value2 = cpu_to_be32(value);
+
[PATCH v4 0/7] add integrity and security to TPM2 transactions
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 1/2] crypto: fix cfb mode decryption
On October 21, 2018 9:58:04 AM GMT, Ard Biesheuvel
wrote:
>On 21 October 2018 at 10:07, James Bottomley
> wrote:
>> On Sun, 2018-10-21 at 09:05 +0200, Ard Biesheuvel wrote:
>>> (+ James)
>>
>> Thanks!
>>
>>> On 20 October 2018 at 01:01, Dmitry Eremin-Solenikov
>>> wrote:
>>> > crypto_cfb_decrypt_segment() incorrectly XOR'ed generated
>keystream
>>> > with
>>> > IV, rather than with data stream, resulting in incorrect
>>> > decryption.
>>> > Test vectors will be added in the next patch.
>>> >
>>> > Signed-off-by: Dmitry Eremin-Solenikov
>>> > Cc: sta...@vger.kernel.org
>>> > ---
>>> > crypto/cfb.c | 2 +-
>>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>>> >
>>> > diff --git a/crypto/cfb.c b/crypto/cfb.c
>>> > index a0d68c09e1b9..fd4e8500e121 100644
>>> > --- a/crypto/cfb.c
>>> > +++ b/crypto/cfb.c
>>> > @@ -144,7 +144,7 @@ static int crypto_cfb_decrypt_segment(struct
>>> > skcipher_walk *walk,
>>> >
>>> > do {
>>> > crypto_cfb_encrypt_one(tfm, iv, dst);
>>> > - crypto_xor(dst, iv, bsize);
>>> > + crypto_xor(dst, src, bsize);
>>
>> This does look right. I think the reason the TPM code works is that
>it
>> always does encrypt/decrypt in-place, which is a separate piece of
>the
>> code which appears to be correct.
>>
>
>Yeah I figured that.
>
>So where is the TPM code that actually uses this code?
It was posted to the integrity list a while ago. I'm planning a repost
shortly.
James
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [PATCH 1/2] crypto: fix cfb mode decryption
On Sun, 2018-10-21 at 09:05 +0200, Ard Biesheuvel wrote:
> (+ James)
Thanks!
> On 20 October 2018 at 01:01, Dmitry Eremin-Solenikov
> wrote:
> > crypto_cfb_decrypt_segment() incorrectly XOR'ed generated keystream
> > with
> > IV, rather than with data stream, resulting in incorrect
> > decryption.
> > Test vectors will be added in the next patch.
> >
> > Signed-off-by: Dmitry Eremin-Solenikov
> > Cc: sta...@vger.kernel.org
> > ---
> > crypto/cfb.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/crypto/cfb.c b/crypto/cfb.c
> > index a0d68c09e1b9..fd4e8500e121 100644
> > --- a/crypto/cfb.c
> > +++ b/crypto/cfb.c
> > @@ -144,7 +144,7 @@ static int crypto_cfb_decrypt_segment(struct
> > skcipher_walk *walk,
> >
> > do {
> > crypto_cfb_encrypt_one(tfm, iv, dst);
> > - crypto_xor(dst, iv, bsize);
> > + crypto_xor(dst, src, bsize);
This does look right. I think the reason the TPM code works is that it
always does encrypt/decrypt in-place, which is a separate piece of the
code which appears to be correct.
James
Re: [PATCH v1 0/3] WireGuard: Secure Network Tunnel
On Mon, 2018-08-13 at 10:55 -0700, Jason A. Donenfeld wrote: > > but it's very hard for a flow classifier because you have to > > The construction and identifier strings might not obviously help with > the extremely narrow idea you've brought up, but it is very important > for safely introducing additional versions. Namely, it prevents > against cross-protocol key reuse attacks and type confusion bugs. So > don't be too quick to dismiss the importance of these for > accomplishing what we're after. I'm not saying a hash check isn't important for safety; I'm saying that if you only have a hash of a dynamic part plus the protocol identifier to go on it makes far more work for the flow classifier. You can see this easily if you contemplate the idea that the hash might be the algorithm being changed. > > so lets pick one of the above and try it out. > > We have, multiple times, and it's absolutely trivial to do and works > well. The exact thing you're concerned about has already been > researched and worked with on live systems quite a bit over the last > 3 years, and it works in a pretty straight forward way. I'm not sure > there's much more to add here: the thing you want is already there > and has been tested extensively. At this point the "pick one and > let's try it out!" is an old story, and the focus now is on making > sure the code quality and netdev api usage is correct for merging Great, thanks, I'll look forward to seeing it in v2 then. James
Re: [PATCH v1 0/3] WireGuard: Secure Network Tunnel
On Mon, 2018-08-13 at 10:02 -0700, Jason A. Donenfeld wrote: > > Could we please build planning for this crypto failure day into > > wireguard now rather than have to do it later? It doesn't need to > > be full cipher agility, it just needs to be the ability to handle > > multiple protocol versions ... two should do it because that gives > > a template to follow (and test version to try to find bugs in the > > implementation). It looks like the protocol could simply be updated > > to put the version into one (or more) of the three reserved bytes > > in the handshake headers, so perhaps doing this before they get > > used for something else would be a good first step? > > > > James > > > > > > Indeed the answer is in fact along the lines of what you've suggested > in your question: the protocol is very strictly versioned. This means > that while there intentionally isn't negotiation of ciphers -- > something historically very bug-prone -- there is ample room for > updating the protocol. This is enabled via 4 aspects of the protocol: > > - An explicit "identifier" string is hashed in as part of the first > step of cryptographic operations, containing a "v1" as well as the > protocol designer's email. > - An explicit "construction" string is hashed in as part of the first > step of cryptographic operations, containing the Noise handshake > pattern and a list of the cryptographic primitives used. Any hash involving other parameters allows you to check for a version mismatch, but it's very hard for a flow classifier because you have to do the hash at the point you classify. If we're running concurrent versions we need an easy way to separate them. > - A type field at the beginning of each message. Newer message types > (corresponding with newer versions) can easily be introduced via this > field, and they can even coexist with older ones need be. > - Three unused reserved fields ready to be utilised in the event > they're needed. Either of these will work for easy classification. > In other words, there's ample room for such contingency measures > within the protocol. I have a preference for explicit versioning, having dealt with some protocol issues before. However, I'm much less concerned with *how* it's done than that it *be* done in the kernel patch so we can test out rolling the version number to change the algorithms in a backward compatible way, so lets pick one of the above and try it out. Regards, James
Re: [PATCH v1 0/3] WireGuard: Secure Network Tunnel
> Ample information, including documentation, installation > instructions, > and project details, is available at: > > * https://www.wireguard.com/ > * https://www.wireguard.com/papers/wireguard.pdf In your paper you say this: > Finally, WireGuard is cryptographically opinionated. It intentionally > lacks cipher and protocol agility. If > holes are found in the underlying primitives, all endpoints will be > required to update. The only thing that's certain (beyond death and taxes) is that your crypto choice will one day need updating; either in response to an urgent CVE because an algorithm is compromised or in response to a less urgent one because it is deprecated. Assuming wireguard is reasonably successful we'll have a large ecosystem dependent on it. On this day, we're going to have the choice of either breaking the entire ecosystem by rolling out a change that can't connect to lower protocol versions or trying to wedge version agility into wireguard in a hurry. The former is too awful to contemplate because of the almost universal ecosystem breakage it would cause and the latter is going to lead to additional bugs because people in a hurry aren't as careful as they should be. Could we please build planning for this crypto failure day into wireguard now rather than have to do it later? It doesn't need to be full cipher agility, it just needs to be the ability to handle multiple protocol versions ... two should do it because that gives a template to follow (and test version to try to find bugs in the implementation). It looks like the protocol could simply be updated to put the version into one (or more) of the three reserved bytes in the handshake headers, so perhaps doing this before they get used for something else would be a good first step? James
Re: CAAM and IMA/EVM : caam_rsa_enc: DECO: desc idx 7: Protocol Size Error
On Tue, 2018-04-10 at 23:01 +0100, Martin Townsend wrote: > Using openssl to get the signature in my x509 cert > > Signature Algorithm: sha256WithRSAEncryption > 68:82:cc:5d:f9:ee:fb:1a:77:72:a6:a9:c6:4c:cc:d7:f6:2a: > 17:a5:db:bf:5a:2b:8d:39:60:dc:a0:93:39:45:0f:bc:a7:e8: > 7f:6c:06:84:2d:f3:c1:94:0a:60:56:1c:50:78:dc:34:d1:87: > > So there's an extra 0x00 and the signature is 257 bytes so I guess > this is upsetting CAAM, just need to work out where it's coming from, > or whether it's valid and CAAM should be handling it. A signature is just a bignum so leading zeros are permitted because it's the same numeric value; however, there are normalization procedures that require stripping the leading zeros, say before doing a hash or other operation which would be affected by them. CAAM should definitely handle it on the "be liberal in what you accept" principle. The kernel should probably remove the leading zeros on the "be conservative in what you do" part of the same principle. > I notice that in my stack trace I have pkcs1pad_verify which > suggests some sort of padding? Yes, RSA has various forms of padding because the information being encrypted is usually much smaller than the encryption unit; PKCS1 is the most common (although its now deprecated in favour of OAEP because of all the padding oracle problems). James
Re: [tpmdd-devel] in-kernel user of ecdsa
On Mon, 2018-03-12 at 20:56 +0100, Stephan Mueller wrote: > Am Montag, 12. März 2018, 19:09:18 CET schrieb James Bottomley: > > Hi James, > > > > > On Mon, 2018-03-12 at 19:07 +0200, Tudor Ambarus wrote: > > > > > > Hi, > > > > > > Would you consider using ECDSA in the kernel module signing > > > facility? When compared with RSA, ECDSA has shorter keys, the key > > > generation process is faster, the sign operation is faster, but > > > the verify operation is slower than with RSA. > > > > You missed the keyrings list, which is where the module signing > > utility is discussed. > > > > First question is, have you actually tried? It looks like sign- > > file doesn't do anything RSA specific so if you give it an EC X.509 > > certificate it will produce an ECDSA signature. > > > > I think our kernel internal x509 parsers don't have the EC OIDs, so > > signature verification will fail; but, especially since we have the > > rest of the EC machinery in the crypto subsystem, that looks to be > > simply fixable. > > ECDSA is not implemented currently in the kernel crypto API. an ECDSA signature is produced as a ECDH operation using the DSA algorithm instead of KDFe, so it's trivial with what we have; signature verification involves a separate point addition but we have all the primitives for this in crypto/ecc.c so adding it isn't really difficult, is it? James
Re: [tpmdd-devel] in-kernel user of ecdsa
On Mon, 2018-03-12 at 19:07 +0200, Tudor Ambarus wrote: > Hi, > > Would you consider using ECDSA in the kernel module signing facility? > When compared with RSA, ECDSA has shorter keys, the key generation > process is faster, the sign operation is faster, but the verify > operation is slower than with RSA. You missed the keyrings list, which is where the module signing utility is discussed. First question is, have you actually tried? It looks like sign-file doesn't do anything RSA specific so if you give it an EC X.509 certificate it will produce an ECDSA signature. I think our kernel internal x509 parsers don't have the EC OIDs, so signature verification will fail; but, especially since we have the rest of the EC machinery in the crypto subsystem, that looks to be simply fixable. James
Re: [PATCH v3 1/6] tpm-buf: create new functions for handling TPM buffers
On Mon, 2018-03-12 at 09:00 -0700, J Freyensee wrote:
> >
> > +int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
> > +{
> > + int rc;
> > +
> > + rc = __tpm_buf_init(buf);
>
>
> Assuming that functions like tpm_buf_init() are the top-level API
> being defined in this patch, shouldn't it check if buf is valid
> before passing into the internal functions like __tpm_buf_init(buf)
> (maybe WARN()/BUG_ON()?). Or does __tpm_buf_init(buf) do this check?
These are kernel internal APIs designed for on stack struct tpm_buf
usage, so I can't think of a viable threat model that would require
this type of checking ... do you have one?
James
Re: [PATCH v3 0/6] add integrity and security to TPM2 transactions
On Mon, 2018-03-12 at 12:58 +0200, Jarkko Sakkinen wrote: > 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&m=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. Yes, v3 is a more complete version of v2 with a couple of sessions API additions. 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. 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. > 2. Do you know in which kernel version will the crypto additions > land? They're here: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/log/ So I'd guess next merge window. You can do what we do in SCSI and create a "postmerge" branch based on the cryptodev one (we often have SCSI stuff with block tree precursors). The way I run it is that I don't send the merge window pull request until I see the merge-base against Linus master move to the base of the patches (meaning all the precursors are upstream). > /Jarkko >
[PATCH v3 6/6] tpm2-sessions: NOT FOR COMMITTING add sessions testing
This runs through a preset sequence using sessions to demonstrate that
the session handling code functions. It does both HMAC, encryption
and decryption by testing an encrypted sealing operation with
authority and proving that the same sealed data comes back again via
an HMAC and response encryption. It also does policy unsealing which
mimics the more complex of the trusted key scenarios.
Signed-off-by: James Bottomley
---
v3: add policy unseal testing with two sessions
---
drivers/char/tpm/Makefile | 1 +
drivers/char/tpm/tpm-chip.c | 1 +
drivers/char/tpm/tpm.h| 1 +
drivers/char/tpm/tpm2-cmd.c | 2 +
drivers/char/tpm/tpm2-sessions-test.c | 359 ++
5 files changed, 364 insertions(+)
create mode 100644 drivers/char/tpm/tpm2-sessions-test.c
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index b83737ccaa81..1ac7a4046630 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -6,6 +6,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 tpm1_eventlog.o tpm2_eventlog.o \
tpm2-space.o tpm-buf.o tpm2-sessions.o
+obj-m += tpm2-sessions-test.o
tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_eventlog_acpi.o
tpm-$(CONFIG_EFI) += tpm_eventlog_efi.o
tpm-$(CONFIG_OF) += tpm_eventlog_of.o
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 0a62c19937b6..ca174ee1e670 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -118,6 +118,7 @@ struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip)
return res;
}
+EXPORT_SYMBOL(tpm_chip_find_get);
/**
* tpm_dev_release() - free chip memory and the device number
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index b1eee56cbbb5..8a652d36939d 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -146,6 +146,7 @@ enum tpm2_command_codes {
TPM2_CC_CONTEXT_LOAD= 0x0161,
TPM2_CC_CONTEXT_SAVE= 0x0162,
TPM2_CC_FLUSH_CONTEXT = 0x0165,
+ TPM2_CC_POLICY_COMMAND_CODE = 0x16c,
TPM2_CC_READ_PUBLIC = 0x0173,
TPM2_CC_START_AUTH_SESS = 0x0176,
TPM2_CC_GET_CAPABILITY = 0x017A,
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 8b164b7347de..3f47d8b3d361 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -418,6 +418,7 @@ void tpm2_flush_context_cmd(struct tpm_chip *chip, u32
handle,
tpm_buf_destroy(&buf);
}
+EXPORT_SYMBOL_GPL(tpm2_flush_context_cmd);
/**
* tpm_buf_append_auth() - append TPMS_AUTH_COMMAND to the buffer.
@@ -448,6 +449,7 @@ void tpm2_buf_append_auth(struct tpm_buf *buf, u32
session_handle,
if (hmac && hmac_len)
tpm_buf_append(buf, hmac, hmac_len);
}
+EXPORT_SYMBOL_GPL(tpm2_buf_append_auth);
/**
* tpm2_seal_trusted() - seal the payload of a trusted key
diff --git a/drivers/char/tpm/tpm2-sessions-test.c
b/drivers/char/tpm/tpm2-sessions-test.c
new file mode 100644
index ..4559e1a5f4d8
--- /dev/null
+++ b/drivers/char/tpm/tpm2-sessions-test.c
@@ -0,0 +1,359 @@
+/* run a set of tests of the sessions code */
+#include "tpm.h"
+#include "tpm2-sessions.h"
+
+#include
+
+#include
+
+/* simple policy: command code must be TPM2_CC_UNSEAL */
+static u8 policy[] = {
+ 0xe6, 0x13, 0x13, 0x70, 0x76, 0x52, 0x4b, 0xde,
+ 0x48, 0x75, 0x33, 0x86, 0x58, 0x84, 0xe9, 0x73,
+ 0x2e, 0xbe, 0xe3, 0xaa, 0xcb, 0x09, 0x5d, 0x94,
+ 0xa6, 0xde, 0x49, 0x2e, 0xc0, 0x6c, 0x46, 0xfa,
+};
+
+static u32 get_policy(struct tpm_chip *chip)
+{
+ struct tpm_buf buf;
+ u8 nonce[SHA256_DIGEST_SIZE];
+ u32 h;
+ int rc;
+
+ rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_START_AUTH_SESS);
+ if (rc)
+ return 0;
+
+ /* salt key */
+ tpm_buf_append_u32(&buf, TPM2_RH_NULL);
+ /* bind key */
+ tpm_buf_append_u32(&buf, TPM2_RH_NULL);
+ /* zero nonce */
+ memset(nonce, 0, sizeof(nonce));
+ tpm_buf_append_u16(&buf, sizeof(nonce));
+ tpm_buf_append(&buf, nonce, sizeof(nonce));
+ /* encrypted salt (empty) */
+ tpm_buf_append_u16(&buf, 0);
+ /* session type (HMAC, audit or policy) */
+ tpm_buf_append_u8(&buf, TPM2_SE_POLICY);
+ /* symmetric encryption parameters */
+ /* symmetric algorithm */
+ tpm_buf_append_u16(&buf, TPM2_ALG_NULL);
+
+ /* hash algorithm for session */
+ tpm_buf_append_u16(&buf, TPM2_ALG_SHA256);
+
+ rc = tpm_transmit_cmd(chip, &chip->kernel_space, buf.data, PAGE_SIZE,
+ 0, 0, "start policy session");
+
+ h = get_unaligned_be32(&buf.data[TPM_HEADER_SIZE]);
+
+ tpm_buf_reset_cmd(&buf, TPM2_ST_NO_SESSION
[PATCH v3 5/6] trusted keys: Add session encryption protection to the seal/unseal path
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 | 156
1 file changed, 98 insertions(+), 58 deletions(-)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 47395c455ae1..8b164b7347de 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -463,8 +463,9 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
struct trusted_key_options *options)
{
unsigned int blob_len;
- struct tpm_buf buf;
+ struct tpm_buf buf, t2b;
u32 hash, rlength;
+ struct tpm2_auth *auth;
int i;
int rc;
@@ -478,45 +479,56 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
if (i == ARRAY_SIZE(tpm2_hash_map))
return -EINVAL;
- rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
+ rc = tpm2_start_auth_session(chip, &auth);
if (rc)
return rc;
- tpm_buf_append_u32(&buf, options->keyhandle);
- tpm2_buf_append_auth(&buf, TPM2_RS_PW,
-NULL /* nonce */, 0,
-0 /* session_attributes */,
-options->keyauth /* hmac */,
-TPM_DIGEST_SIZE);
+ rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
+ if (rc) {
+ tpm2_end_auth_session(auth);
+ return rc;
+ }
+ rc = tpm_buf_init_2b(&t2b);
+ if (rc) {
+ tpm_buf_destroy(&buf);
+ tpm2_end_auth_session(auth);
+ return rc;
+ }
+
+ tpm_buf_append_name(&buf, auth, options->keyhandle, NULL);
+ tpm_buf_append_hmac_session(&buf, auth, TPM2_SA_DECRYPT,
+ options->keyauth, TPM_DIGEST_SIZE);
/* sensitive */
- tpm_buf_append_u16(&buf, 4 + TPM_DIGEST_SIZE + payload->key_len + 1);
+ tpm_buf_append_u16(&t2b, TPM_DIGEST_SIZE);
+ tpm_buf_append(&t2b, options->blobauth, TPM_DIGEST_SIZE);
+ tpm_buf_append_u16(&t2b, payload->key_len + 1);
+ tpm_buf_append(&t2b, payload->key, payload->key_len);
+ tpm_buf_append_u8(&t2b, payload->migratable);
- tpm_buf_append_u16(&buf, TPM_DIGEST_SIZE);
- tpm_buf_append(&buf, options->blobauth, TPM_DIGEST_SIZE);
- tpm_buf_append_u16(&buf, payload->key_len + 1);
- tpm_buf_append(&buf, payload->key, payload->key_len);
- tpm_buf_append_u8(&buf, payload->migratable);
+ tpm_buf_append_2b(&buf, &t2b);
/* public */
- tpm_buf_append_u16(&buf, 14 + options->policydigest_len);
- tpm_buf_append_u16(&buf, TPM2_ALG_KEYEDHASH);
- tpm_buf_append_u16(&buf, hash);
+ tpm_buf_append_u16(&t2b, TPM2_ALG_KEYEDHASH);
+ tpm_buf_append_u16(&t2b, hash);
/* policy */
if (options->policydigest_len) {
- tpm_buf_append_u32(&buf, 0);
- tpm_buf_append_u16(&buf, options->policydigest_len);
- tpm_buf_append(&buf, options->policydigest,
+ tpm_buf_append_u32(&t2b, 0);
+ tpm_buf_append_u16(&t2b, options->policydigest_len);
+ tpm_buf_append(&t2b, options->policydigest,
options->policydigest_len);
} else {
- tpm_buf_append_u32(&buf, TPM2_OA_USER_WITH_AUTH);
- tpm_buf_append_u16(&buf, 0);
+ tpm_buf_append_u32(&t2b, TPM2_OA_USER_WITH_AUTH);
+ tpm_buf_append_u16(&t2b, 0);
}
/* public parameters */
- tpm_buf_append_u16(&buf, TPM2_ALG_NULL);
- tpm_buf_append_u16(&buf, 0);
+ tpm_buf_append_u16(&t2b, TPM2_ALG_NULL);
+ /* unique (zero) */
+ tpm_buf_append_u16(&t2b, 0);
+
+ tpm_buf_append_2b(&buf, &t2b);
/* outside info */
tpm_buf_append_u16(&buf, 0);
@@ -529,8 +541,11 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
goto out;
}
- rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 4, 0,
- "sealing dat
[PATCH v3 4/6] tpm2: add session encryption protection to tpm2_get_random()
If some entity is snooping the TPM bus, they can see the random
numbers we're extracting from the TPM and do prediction attacks
against their consumers. Foil this attack by using response
encryption to prevent the attacker from seeing the random sequence.
Signed-off-by: James Bottomley
---
v3: add error handling to sessions and redo to be outside loop
---
drivers/char/tpm/tpm2-cmd.c | 73 +++--
1 file changed, 38 insertions(+), 35 deletions(-)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 6ed07ca4a5e8..47395c455ae1 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -38,10 +38,6 @@ struct tpm2_get_tpm_pt_out {
__be32 value;
} __packed;
-struct tpm2_get_random_in {
- __be16 size;
-} __packed;
-
struct tpm2_get_random_out {
__be16 size;
u8 buffer[TPM_MAX_RNG_DATA];
@@ -51,8 +47,6 @@ union tpm2_cmd_params {
struct tpm2_startup_in startup_in;
struct tpm2_get_tpm_pt_in get_tpm_pt_in;
struct tpm2_get_tpm_pt_out get_tpm_pt_out;
- struct tpm2_get_random_in getrandom_in;
- struct tpm2_get_random_out getrandom_out;
};
struct tpm2_cmd {
@@ -304,17 +298,6 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx,
u32 count,
return rc;
}
-
-#define TPM2_GETRANDOM_IN_SIZE \
- (sizeof(struct tpm_input_header) + \
-sizeof(struct tpm2_get_random_in))
-
-static const struct tpm_input_header tpm2_getrandom_header = {
- .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
- .length = cpu_to_be32(TPM2_GETRANDOM_IN_SIZE),
- .ordinal = cpu_to_be32(TPM2_CC_GET_RANDOM)
-};
-
/**
* tpm2_get_random() - get random bytes from the TPM RNG
*
@@ -327,44 +310,64 @@ static const struct tpm_input_header
tpm2_getrandom_header = {
*/
int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max)
{
- struct tpm2_cmd cmd;
- u32 recd, rlength;
+ u32 recd;
u32 num_bytes;
int err;
int total = 0;
int retries = 5;
u8 *dest = out;
+ struct tpm_buf buf;
+ struct tpm2_get_random_out *rout;
+ struct tpm2_auth *auth;
- num_bytes = min_t(u32, max, sizeof(cmd.params.getrandom_out.buffer));
+ num_bytes = min_t(u32, max, TPM_MAX_RNG_DATA);
- if (!out || !num_bytes ||
- max > sizeof(cmd.params.getrandom_out.buffer))
+ if (!out || !num_bytes
+ || max > TPM_MAX_RNG_DATA)
return -EINVAL;
- do {
- cmd.header.in = tpm2_getrandom_header;
- cmd.params.getrandom_in.size = cpu_to_be16(num_bytes);
+ err = tpm2_start_auth_session(chip, &auth);
+ if (err)
+ return err;
+
+ err = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_GET_RANDOM);
+ if (err) {
+ tpm2_end_auth_session(auth);
+ return err;
+ }
- err = tpm_transmit_cmd(chip, NULL, &cmd, sizeof(cmd),
- offsetof(struct tpm2_get_random_out,
- buffer),
+ do {
+ tpm_buf_append_hmac_session(&buf, auth, TPM2_SA_ENCRYPT
+ | TPM2_SA_CONTINUE_SESSION,
+ NULL, 0);
+ tpm_buf_append_u16(&buf, num_bytes);
+ tpm_buf_fill_hmac_session(&buf, auth);
+ err = tpm_transmit_cmd(chip, &chip->kernel_space, buf.data,
+ PAGE_SIZE, TPM_HEADER_SIZE + 2,
0, "attempting get random");
+ err = tpm_buf_check_hmac_response(&buf, auth, err);
if (err)
break;
- recd = min_t(u32, be16_to_cpu(cmd.params.getrandom_out.size),
-num_bytes);
- rlength = be32_to_cpu(cmd.header.out.length);
- if (rlength < offsetof(struct tpm2_get_random_out, buffer) +
- recd)
- return -EFAULT;
- memcpy(dest, cmd.params.getrandom_out.buffer, recd);
+ rout = (struct tpm2_get_random_out *)&buf.data[TPM_HEADER_SIZE
+ 4];
+ recd = be16_to_cpu(rout->size);
+ recd = min_t(u32, recd, num_bytes);
+ if (tpm_buf_length(&buf) < TPM_HEADER_SIZE + 4
+ + 2 + recd) {
+ total = -EFAULT;
+ break;
+ }
+ memcpy(dest, rout->buffer, recd);
dest += recd;
total += recd;
num_bytes -= recd;
+ tpm_buf_reset_cmd(&buf, TPM2_ST_SESSIONS, TPM2_CC_GET_RANDOM);
} while (retries-- && total < max);
+
[PATCH v3 3/6] tpm2: add hmac checks to tpm2_pcr_extend()
We use tpm2_pcr_extend() in trusted keys to extend a PCR to prevent a
key from being re-loaded until the next reboot. To use this
functionality securely, that extend must be protected by a session
hmac.
Signed-off-by: James Bottomley
---
v3: add error handling to sessions
---
drivers/char/tpm/tpm2-cmd.c | 33 +
1 file changed, 13 insertions(+), 20 deletions(-)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index c0ebfc4efd4d..6ed07ca4a5e8 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -247,13 +247,6 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8
*res_buf)
return rc;
}
-struct tpm2_null_auth_area {
- __be32 handle;
- __be16 nonce_size;
- u8 attributes;
- __be16 auth_size;
-} __packed;
-
/**
* tpm2_pcr_extend() - extend a PCR value
*
@@ -268,7 +261,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32
count,
struct tpm2_digest *digests)
{
struct tpm_buf buf;
- struct tpm2_null_auth_area auth_area;
+ struct tpm2_auth *auth;
int rc;
int i;
int j;
@@ -276,20 +269,19 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx,
u32 count,
if (count > ARRAY_SIZE(chip->active_banks))
return -EINVAL;
- rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
+ rc = tpm2_start_auth_session(chip, &auth);
if (rc)
return rc;
- tpm_buf_append_u32(&buf, pcr_idx);
+ rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
+ if (rc) {
+ tpm2_end_auth_session(auth);
+ return rc;
+ }
- auth_area.handle = cpu_to_be32(TPM2_RS_PW);
- auth_area.nonce_size = 0;
- auth_area.attributes = 0;
- auth_area.auth_size = 0;
+ tpm_buf_append_name(&buf, auth, pcr_idx, NULL);
+ tpm_buf_append_hmac_session(&buf, auth, 0, NULL, 0);
- tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area));
- tpm_buf_append(&buf, (const unsigned char *)&auth_area,
- sizeof(auth_area));
tpm_buf_append_u32(&buf, count);
for (i = 0; i < count; i++) {
@@ -302,9 +294,10 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx,
u32 count,
hash_digest_size[tpm2_hash_map[j].crypto_id]);
}
}
-
- rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
- "attempting extend a PCR value");
+ tpm_buf_fill_hmac_session(&buf, auth);
+ rc = tpm_transmit_cmd(chip, &chip->kernel_space, buf.data, PAGE_SIZE,
+ 0, 0, "attempting extend a PCR value");
+ rc = tpm_buf_check_hmac_response(&buf, auth, rc);
tpm_buf_destroy(&buf);
--
2.12.3
[PATCH v3 2/6] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
This code adds true session based HMAC authentication plus parameter
decryption and response encryption using AES.
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
* tpm_buf_append_hmac_session() where tpm2_append_auth() would go
* 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.
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
---
drivers/char/tpm/Kconfig |3 +
drivers/char/tpm/Makefile|2 +-
drivers/char/tpm/tpm.h | 27 +
drivers/char/tpm/tpm2-cmd.c | 34 +-
drivers/char/tpm/tpm2-sessions.c | 1166 ++
drivers/char/tpm/tpm2-sessions.h | 57 ++
6 files changed, 1273 insertions(+), 16 deletions(-)
create mode 100644 drivers/char/tpm/tpm2-sessions.c
create mode 100644 drivers/char/tpm/tpm2-sessions.h
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 41b2482b97c3..b83737ccaa81 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 tpm1_eventlog.o tpm2_eventlog.o \
- tpm2-space.o tpm-buf.o
+ tpm2-space.o tpm-buf.o tpm2-sessions.o
tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_eventlog_acpi.o
tpm-$(CONFIG_EFI) += tpm_eventlog_efi.o
tpm-$(CONFIG_OF) += tpm_eventlog_of.o
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 2fca263d4ca3..b1eee56cbbb5 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -42,6 +42,9 @@
#include
#endif
+/* fixed define for the curve we use which is NIST_P256 */
+#define EC_PT_SZ 32
+
enum tpm_const {
TPM_MINOR = 224,/* officially assigned */
TPM_BUFSIZE = 4096,
@@ -93,6 +96,7 @@ enum tpm2_const {
enum tpm2_structures {
TPM2_ST_NO_SESSIONS = 0x8001,
TPM2_ST_SESSIONS= 0x8002,
+ TPM2_ST_CREATION= 0x8021,
};
/* Indicates from what layer of the software stack the error comes from */
@@ -114,16 +118,25 @@ enum tpm2_return_codes {
enum tpm2_algorithms {
TPM2_ALG_ERROR = 0x,
TPM2_ALG_SHA1 = 0x0004,
+ TPM2_ALG_AES= 0x0006,
TPM2_ALG_KEYEDHASH = 0x0008,
TPM2_ALG_SHA256 = 0x000B,
TPM2_ALG_SHA384 = 0x000C,
TPM2_ALG_SHA512 = 0x000D,
TPM2_ALG_NULL = 0x0010,
TPM2_ALG_SM3_256= 0x0012,
+ TPM2_ALG_ECC= 0x0023,
+ TPM2_ALG_CFB= 0x0043,
+};
+
+enum tpm2_curves {
+ TPM2_ECC_NONE = 0x,
+ TPM2_ECC_NIST_P256 = 0x00
[PATCH v3 1/6] tpm-buf: create new functions for handling TPM buffers
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
---
v2: added this patch to separate out the API changes
v3: added tpm_buf_reset_cmd()
---
drivers/char/tpm/Makefile | 2 +-
drivers/char/tpm/tpm-buf.c | 191 +
drivers/char/tpm/tpm.h | 95 --
3 files changed, 208 insertions(+), 80 deletions(-)
create mode 100644 drivers/char/tpm/tpm-buf.c
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index d37c4a1748f5..41b2482b97c3 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 tpm1_eventlog.o tpm2_eventlog.o \
- tpm2-space.o
+ tpm2-space.o tpm-buf.o
tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_eventlog_acpi.o
tpm-$(CONFIG_EFI) += tpm_eventlog_efi.o
tpm-$(CONFIG_OF) += tpm_eventlog_of.o
diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
new file mode 100644
index ..146a71cec067
--- /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_cmd(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_cmd);
+
+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_cmd(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(&buf->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, &value, 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 *) &value2, 2);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append_u16);
+
+void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
+{
+ __be32 value2 = cpu_to_be32(value);
+
+ tpm_buf_append(buf, (u8 *) &value2, 4)
[PATCH v3 0/6] add integrity and security to TPM2 transactions
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&m=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 --- James Bottomley (6): 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 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.h| 123 ++-- drivers/char/tpm/tpm2-cmd.c | 298 + drivers/char/tpm/tpm2-sessions-test.c | 359 ++ drivers/char/tpm/tpm2-sessions.c | 1166 + drivers/char/tpm/tpm2-sessions.h | 57 ++ 9 files changed, 1993 insertions(+), 208 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.12.3
Re: [RFC 0/5] add integrity and security to TPM2 transactions
On Sat, 2018-03-10 at 14:49 +0200, Jarkko Sakkinen wrote: > 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&m=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. 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 .. Thanks, James
[RFC v2 5/5] tpm2-sessions: NOT FOR COMMITTING add sessions testing
>From f69d2ec1bdddefa87c7130699c797cd5e24fcaf2 Mon Sep 17 00:00:00 2001
This runs through a preset sequence using sessions to demonstrate that
the session handling code functions. It does both HMAC, encryption
and decryption by testing an encrypted sealing operation with
authority and proving that the same sealed data comes back again via
an HMAC and response encryption.
Signed-off-by: James Bottomley
---
drivers/char/tpm/Makefile | 1 +
drivers/char/tpm/tpm-chip.c | 1 +
drivers/char/tpm/tpm2-sessions-test.c | 177 ++
3 files changed, 179 insertions(+)
create mode 100644 drivers/char/tpm/tpm2-sessions-test.c
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index b83737ccaa81..1ac7a4046630 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -6,6 +6,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 tpm1_eventlog.o tpm2_eventlog.o \
tpm2-space.o tpm-buf.o tpm2-sessions.o
+obj-m += tpm2-sessions-test.o
tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_eventlog_acpi.o
tpm-$(CONFIG_EFI) += tpm_eventlog_efi.o
tpm-$(CONFIG_OF) += tpm_eventlog_of.o
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 0a62c19937b6..ca174ee1e670 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -118,6 +118,7 @@ struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip)
return res;
}
+EXPORT_SYMBOL(tpm_chip_find_get);
/**
* tpm_dev_release() - free chip memory and the device number
diff --git a/drivers/char/tpm/tpm2-sessions-test.c
b/drivers/char/tpm/tpm2-sessions-test.c
new file mode 100644
index ..bd599648c971
--- /dev/null
+++ b/drivers/char/tpm/tpm2-sessions-test.c
@@ -0,0 +1,177 @@
+/* run a set of tests of the sessions code */
+#include "tpm.h"
+#include "tpm2-sessions.h"
+
+#include
+
+int tpm2_sessions_test(void)
+{
+ struct tpm2_auth *auth;
+ struct tpm_buf buf, b1;
+ struct tpm_buf t2b;
+ struct tpm_chip *chip;
+ int rc;
+ char payload[29];
+ char *password = "Passw0Rd";
+ const u8 *p;
+ u32 h;
+ u8 name[34];
+ u16 len;
+ int ret = -EINVAL;
+
+ chip = tpm_chip_find_get(NULL);
+ if (!chip)
+ return -ENODEV;
+
+ if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
+ return -ENODEV;
+
+ get_random_bytes(payload, sizeof(payload));
+
+ /* precursor: get a session */
+ rc = tpm2_start_auth_session(chip, &auth);
+ dev_info(&chip->dev, "TPM: start auth session returned %d\n", rc);
+ if (rc)
+ goto out;
+
+ /* first test: get random bytes from TPM */
+ tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_GET_RANDOM);
+ tpm_buf_append_hmac_session(&buf, auth, TPM2_SA_ENCRYPT
+ | TPM2_SA_CONTINUE_SESSION, NULL, 0);
+ tpm_buf_append_u16(&buf, 29);
+ tpm_buf_fill_hmac_session(&buf, auth);
+ rc = tpm_transmit_cmd(chip, &chip->kernel_space, buf.data, PAGE_SIZE,
+ 0, 0, "get random");
+ rc = tpm_buf_check_hmac_response(&buf, auth, rc);
+ dev_info(&chip->dev, "TPM: check hmac response returned %d\n", rc);
+ tpm_buf_destroy(&buf);
+
+ /*
+* second test, seal random data protecting sensitive by
+* encryption and also doing response encryption (not
+* necessary) The encrypted payload has two components: an
+* authorization password which must be presented on useal and
+* the actual data (the random payload)
+*/
+ tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
+ tpm_buf_append_name(&buf, auth, chip->tpmkey, chip->tpmkeyname);
+ tpm_buf_append_hmac_session(&buf, auth, TPM2_SA_DECRYPT
+ | TPM2_SA_ENCRYPT
+ | TPM2_SA_CONTINUE_SESSION, NULL, 0);
+ /* sensitive */
+ tpm_buf_init_2b(&t2b);
+ /* the authorization */
+ tpm_buf_append_u16(&t2b, strlen(password));
+ tpm_buf_append(&t2b, password, strlen(password));
+ /* the payload */
+ tpm_buf_append_u16(&t2b, sizeof(payload));
+ tpm_buf_append(&t2b, payload, sizeof(payload));
+ tpm_buf_append_2b(&buf, &t2b);
+ /* the public */
+ /* type */
+ tpm_buf_append_u16(&t2b, TPM2_ALG_KEYEDHASH);
+ /* name hash */
+ tpm_buf_append_u16(&t2b, TPM2_ALG_SHA256);
+ /* object properties */
+ tpm_buf_append_u32(&t2b, TPM2_OA_USER_WITH_AUTH | TPM2_OA_NO_DA);
+ /* auth policy (empty) */
+ tpm_buf_append_u16(&t2b, 0);
+ /* keyed hash parameters (we
[RFC v2 4/5] tpm2: add session encryption protection to tpm2_get_random()
If some entity is snooping the TPM bus, they can see the random
numbers we're extracting from the TPM and do prediction attacks
against their consumers. Foil this attack by using response
encryption to prevent the attacker from seeing the random sequence.
Signed-off-by: James Bottomley
---
drivers/char/tpm/tpm2-cmd.c | 76 +
1 file changed, 35 insertions(+), 41 deletions(-)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index a56cdd5d55ff..ad2a7e72bacf 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -38,10 +38,6 @@ struct tpm2_get_tpm_pt_out {
__be32 value;
} __packed;
-struct tpm2_get_random_in {
- __be16 size;
-} __packed;
-
struct tpm2_get_random_out {
__be16 size;
u8 buffer[TPM_MAX_RNG_DATA];
@@ -51,8 +47,6 @@ union tpm2_cmd_params {
struct tpm2_startup_in startup_in;
struct tpm2_get_tpm_pt_in get_tpm_pt_in;
struct tpm2_get_tpm_pt_out get_tpm_pt_out;
- struct tpm2_get_random_in getrandom_in;
- struct tpm2_get_random_out getrandom_out;
};
struct tpm2_cmd {
@@ -302,17 +296,6 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx,
u32 count,
return rc;
}
-
-#define TPM2_GETRANDOM_IN_SIZE \
- (sizeof(struct tpm_input_header) + \
-sizeof(struct tpm2_get_random_in))
-
-static const struct tpm_input_header tpm2_getrandom_header = {
- .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
- .length = cpu_to_be32(TPM2_GETRANDOM_IN_SIZE),
- .ordinal = cpu_to_be32(TPM2_CC_GET_RANDOM)
-};
-
/**
* tpm2_get_random() - get random bytes from the TPM RNG
*
@@ -325,42 +308,53 @@ static const struct tpm_input_header
tpm2_getrandom_header = {
*/
int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max)
{
- struct tpm2_cmd cmd;
- u32 recd, rlength;
+ u32 recd;
u32 num_bytes;
int err;
int total = 0;
int retries = 5;
u8 *dest = out;
+ struct tpm_buf buf;
+ struct tpm2_get_random_out *rout;
+ struct tpm2_auth *auth;
- num_bytes = min_t(u32, max, sizeof(cmd.params.getrandom_out.buffer));
+ num_bytes = min_t(u32, max, TPM_MAX_RNG_DATA);
- if (!out || !num_bytes ||
- max > sizeof(cmd.params.getrandom_out.buffer))
+ if (!out || !num_bytes
+ || max > TPM_MAX_RNG_DATA)
return -EINVAL;
do {
- cmd.header.in = tpm2_getrandom_header;
- cmd.params.getrandom_in.size = cpu_to_be16(num_bytes);
-
- err = tpm_transmit_cmd(chip, NULL, &cmd, sizeof(cmd),
- offsetof(struct tpm2_get_random_out,
- buffer),
- 0, "attempting get random");
+ err = tpm2_start_auth_session(chip, &auth);
if (err)
break;
-
- recd = min_t(u32, be16_to_cpu(cmd.params.getrandom_out.size),
-num_bytes);
- rlength = be32_to_cpu(cmd.header.out.length);
- if (rlength < offsetof(struct tpm2_get_random_out, buffer) +
- recd)
- return -EFAULT;
- memcpy(dest, cmd.params.getrandom_out.buffer, recd);
-
- dest += recd;
- total += recd;
- num_bytes -= recd;
+ tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_GET_RANDOM);
+ tpm_buf_append_hmac_session(&buf, auth, TPM2_SA_ENCRYPT,
+ NULL, 0);
+ tpm_buf_append_u16(&buf, num_bytes);
+ tpm_buf_fill_hmac_session(&buf, auth);
+ err = tpm_transmit_cmd(chip, &chip->kernel_space, buf.data,
+ PAGE_SIZE, TPM_HEADER_SIZE + 2,
+ 0, "attempting get random");
+ err = tpm_buf_check_hmac_response(&buf, auth, err);
+ if (!err) {
+ rout = (struct tpm2_get_random_out
*)&buf.data[TPM_HEADER_SIZE + 4];
+ recd = be16_to_cpu(rout->size);
+ recd = min_t(u32, recd, num_bytes);
+ if (tpm_buf_length(&buf) < TPM_HEADER_SIZE + 4
+ + 2 + recd) {
+ total = -EFAULT;
+ } else {
+ memcpy(dest, rout->buffer, recd);
+
+ dest += recd;
+ total += recd;
+ num_bytes -= recd;
+ }
+ }
+ tpm_buf_destroy(&buf);
+ if (err || total < 0)
+
[RFC v2 3/5] tpm2: add hmac checks to tpm2_pcr_extend()
We use tpm2_pcr_extend() in trusted keys to extend a PCR to prevent a
key from being re-loaded until the next reboot. To use this
functionality securely, that extend must be protected by a session
hmac.
Signed-off-by: James Bottomley
---
drivers/char/tpm/tpm2-cmd.c | 31 +++
1 file changed, 11 insertions(+), 20 deletions(-)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 2042d4008b9c..a56cdd5d55ff 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -247,13 +247,6 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8
*res_buf)
return rc;
}
-struct tpm2_null_auth_area {
- __be32 handle;
- __be16 nonce_size;
- u8 attributes;
- __be16 auth_size;
-} __packed;
-
/**
* tpm2_pcr_extend() - extend a PCR value
*
@@ -268,7 +261,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32
count,
struct tpm2_digest *digests)
{
struct tpm_buf buf;
- struct tpm2_null_auth_area auth_area;
+ struct tpm2_auth *auth;
int rc;
int i;
int j;
@@ -276,20 +269,17 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx,
u32 count,
if (count > ARRAY_SIZE(chip->active_banks))
return -EINVAL;
- rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
+ rc = tpm2_start_auth_session(chip, &auth);
if (rc)
return rc;
- tpm_buf_append_u32(&buf, pcr_idx);
+ rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
+ if (rc)
+ return rc;
- auth_area.handle = cpu_to_be32(TPM2_RS_PW);
- auth_area.nonce_size = 0;
- auth_area.attributes = 0;
- auth_area.auth_size = 0;
+ tpm_buf_append_name(&buf, auth, pcr_idx, NULL);
+ tpm_buf_append_hmac_session(&buf, auth, 0, NULL, 0);
- tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area));
- tpm_buf_append(&buf, (const unsigned char *)&auth_area,
- sizeof(auth_area));
tpm_buf_append_u32(&buf, count);
for (i = 0; i < count; i++) {
@@ -302,9 +292,10 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx,
u32 count,
hash_digest_size[tpm2_hash_map[j].crypto_id]);
}
}
-
- rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
- "attempting extend a PCR value");
+ tpm_buf_fill_hmac_session(&buf, auth);
+ rc = tpm_transmit_cmd(chip, &chip->kernel_space, buf.data, PAGE_SIZE,
+ 0, 0, "attempting extend a PCR value");
+ rc = tpm_buf_check_hmac_response(&buf, auth, rc);
tpm_buf_destroy(&buf);
--
2.12.3
[RFC v2 2/5] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
This code adds true session based HMAC authentication plus parameter
decryption and response encryption using AES.
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
* tpm_buf_append_hmac_session() where tpm2_append_auth() would go
* 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.
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
---
drivers/char/tpm/Kconfig |3 +
drivers/char/tpm/Makefile|2 +-
drivers/char/tpm/tpm.h | 26 +
drivers/char/tpm/tpm2-cmd.c | 22 +-
drivers/char/tpm/tpm2-sessions.c | 1030 ++
drivers/char/tpm/tpm2-sessions.h | 56 +++
6 files changed, 1126 insertions(+), 13 deletions(-)
create mode 100644 drivers/char/tpm/tpm2-sessions.c
create mode 100644 drivers/char/tpm/tpm2-sessions.h
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 41b2482b97c3..b83737ccaa81 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 tpm1_eventlog.o tpm2_eventlog.o \
- tpm2-space.o tpm-buf.o
+ tpm2-space.o tpm-buf.o tpm2-sessions.o
tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_eventlog_acpi.o
tpm-$(CONFIG_EFI) += tpm_eventlog_efi.o
tpm-$(CONFIG_OF) += tpm_eventlog_of.o
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 51c1fab2354c..1b495d748f90 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -42,6 +42,9 @@
#include
#endif
+/* fixed define for the curve we use which is NIST_P256 */
+#define EC_PT_SZ 32
+
enum tpm_const {
TPM_MINOR = 224,/* officially assigned */
TPM_BUFSIZE = 4096,
@@ -93,6 +96,7 @@ enum tpm2_const {
enum tpm2_structures {
TPM2_ST_NO_SESSIONS = 0x8001,
TPM2_ST_SESSIONS= 0x8002,
+ TPM2_ST_CREATION= 0x8021,
};
/* Indicates from what layer of the software stack the error comes from */
@@ -114,16 +118,25 @@ enum tpm2_return_codes {
enum tpm2_algorithms {
TPM2_ALG_ERROR = 0x,
TPM2_ALG_SHA1 = 0x0004,
+ TPM2_ALG_AES= 0x0006,
TPM2_ALG_KEYEDHASH = 0x0008,
TPM2_ALG_SHA256 = 0x000B,
TPM2_ALG_SHA384 = 0x000C,
TPM2_ALG_SHA512 = 0x000D,
TPM2_ALG_NULL = 0x0010,
TPM2_ALG_SM3_256= 0x0012,
+ TPM2_ALG_ECC= 0x0023,
+ TPM2_ALG_CFB= 0x0043,
+};
+
+enum tpm2_curves {
+ TPM2_ECC_NONE = 0x,
+ TPM2_ECC_NIST_P256 = 0x0003,
};
enum tpm2_command_codes {
TPM2_CC_FIRST = 0x011F,
+ TPM2_CC_CREATE_PRIMARY = 0x0131,
TP
[RFC v2 1/5] tpm-buf: create new functions for handling TPM buffers
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
---
v2: added this patch to separate out the API changes
---
drivers/char/tpm/Makefile | 2 +-
drivers/char/tpm/tpm-buf.c | 184 +
drivers/char/tpm/tpm.h | 94 ---
3 files changed, 200 insertions(+), 80 deletions(-)
create mode 100644 drivers/char/tpm/tpm-buf.c
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index d37c4a1748f5..41b2482b97c3 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 tpm1_eventlog.o tpm2_eventlog.o \
- tpm2-space.o
+ tpm2-space.o tpm-buf.o
tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_eventlog_acpi.o
tpm-$(CONFIG_EFI) += tpm_eventlog_efi.o
tpm-$(CONFIG_OF) += tpm_eventlog_of.o
diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
new file mode 100644
index ..8e674f410823
--- /dev/null
+++ b/drivers/char/tpm/tpm-buf.c
@@ -0,0 +1,184 @@
+// 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;
+}
+
+int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
+{
+ struct tpm_input_header *head;
+ int rc;
+
+ rc = __tpm_buf_init(buf);
+ if (rc)
+ return rc;
+
+ 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);
+
+ 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(&buf->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, &value, 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 *) &value2, 2);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append_u16);
+
+void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
+{
+ __be32 value2 = cpu_to_be32(value);
+
+ tpm_buf_append(buf, (u8 *) &value2, 4);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append_u32);
+
+static void tpm_buf_reset(struct tpm_buf *buf)
+{
+ struct tpm_input_header *head;
+
+ head = (struct tpm_input_header *)buf->data;
+
[RFC 0/5] add integrity and security to TPM2 transactions
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&m=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 --- James Bottomley (5): 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() tpm2-sessions: NOT FOR COMMITTING add sessions testing drivers/char/tpm/Kconfig |3 + drivers/char/tpm/Makefile |3 +- drivers/char/tpm/tpm-buf.c| 184 ++ drivers/char/tpm/tpm-chip.c |1 + drivers/char/tpm/tpm.h| 116 ++-- drivers/char/tpm/tpm2-cmd.c | 129 ++--- drivers/char/tpm/tpm2-sessions-test.c | 177 ++ drivers/char/tpm/tpm2-sessions.c | 1030 + drivers/char/tpm/tpm2-sessions.h | 56 ++ 9 files changed, 1548 insertions(+), 151 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.12.3
Re: [RFC 0/2] add integrity and security to TPM2 transactions
On Mon, 2018-03-05 at 07:04 -0700, Jason Gunthorpe wrote: > On Fri, Mar 02, 2018 at 10:04:54PM -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. > > We have the same issue for TPM1 then right? Sort of. HMAC authentication isn't optional in TPM1 like it is in TPM2, so we do already use it (in the trusted keys code, for instance), so we have less of a problem becasuse it doesn't have the insecure TPM_RS_PW option. However, TPM1 also has a specific weakness here in that if you don't have authority in the object (i.e. no shared secret), the HMAC provides no protection against an intelligent attacker. The only way to get the same security as we have with TPM2 in this situation is to use transport encryption. Given that sha1 is already compromised, TPM1 has a strictly limited shelf life, especially as all laptops are being shipped with TPM2 now, so I don't think it's unreasonable to say if you're worried about this compromise you should use TPM2. James
Re: [PATCH 1/2] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
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->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->buf, data, len);
> > +}
> > +
> > +#define TPM2B_APPEND(type) \
> > + static inline void tpm2b_append_##type(struct tpm2b *buf,
> > const type value) { tpm_buf_append_##type(&buf->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->buf) - sizeof(struct
> > tpm_input_header);
> > +}
> > +
> > +static inline void tpm2b_destroy(struct tpm2b *buf)
> > +{
> > + tpm_buf_destroy(&buf->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).
> * 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.
> * Why there is no RFC tag given that these are so quite large
> changes?
There is an RFC tag on 0/2
> * 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.
James
[PATCH 2/2] tpm2-sessions: NOT FOR COMMITTING add sessions testing
This runs through a preset sequence using sessions to demonstrate that
the session handling code functions. It does both HMAC, encryption
and decryption by testing an encrypted sealing operation with
authority and proving that the same sealed data comes back again via
an HMAC and response encryption.
Signed-off-by: James Bottomley
---
drivers/char/tpm/Makefile | 1 +
drivers/char/tpm/tpm-chip.c | 1 +
drivers/char/tpm/tpm2-sessions-test.c | 178 ++
3 files changed, 180 insertions(+)
create mode 100644 drivers/char/tpm/tpm2-sessions-test.c
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 95ef2b10cc8d..b9eb70f1aee6 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -6,6 +6,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 tpm1_eventlog.o tpm2_eventlog.o \
tpm2-space.o tpm2-sessions.o
+obj-m += tpm2-sessions-test.o
tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_eventlog_acpi.o
tpm-$(CONFIG_EFI) += tpm_eventlog_efi.o
tpm-$(CONFIG_OF) += tpm_eventlog_of.o
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 0a62c19937b6..ca174ee1e670 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -118,6 +118,7 @@ struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip)
return res;
}
+EXPORT_SYMBOL(tpm_chip_find_get);
/**
* tpm_dev_release() - free chip memory and the device number
diff --git a/drivers/char/tpm/tpm2-sessions-test.c
b/drivers/char/tpm/tpm2-sessions-test.c
new file mode 100644
index ..a0e8a8b62cf1
--- /dev/null
+++ b/drivers/char/tpm/tpm2-sessions-test.c
@@ -0,0 +1,178 @@
+/* run a set of tests of the sessions code */
+#include "tpm.h"
+#include "tpm2b.h"
+#include "tpm2-sessions.h"
+
+#include
+
+int tpm2_sessions_test(void)
+{
+ struct tpm2_auth *auth;
+ struct tpm_buf buf, b1;
+ struct tpm2b t2b;
+ struct tpm_chip *chip;
+ int rc;
+ char payload[29];
+ char *password = "Passw0Rd";
+ const u8 *p;
+ u32 h;
+ u8 name[34];
+ u16 len;
+ int ret = -EINVAL;
+
+ chip = tpm_chip_find_get(NULL);
+ if (!chip)
+ return -ENODEV;
+
+ if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
+ return -ENODEV;
+
+ get_random_bytes(payload, sizeof(payload));
+
+ /* precursor: get a session */
+ rc = tpm2_start_auth_session(chip, &auth);
+ dev_info(&chip->dev, "TPM: start auth session returned %d\n", rc);
+ if (rc)
+ goto out;
+
+ /* first test: get random bytes from TPM */
+ tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_GET_RANDOM);
+ tpm_buf_append_hmac_session(&buf, auth, TPM2_SA_ENCRYPT
+ | TPM2_SA_CONTINUE_SESSION, NULL, 0);
+ tpm_buf_append_u16(&buf, 29);
+ tpm_buf_fill_hmac_session(&buf, auth);
+ rc = tpm_transmit_cmd(chip, &chip->kernel_space, buf.data, PAGE_SIZE,
+ 0, 0, "get random");
+ rc = tpm_buf_check_hmac_response(&buf, auth);
+ dev_info(&chip->dev, "TPM: check hmac response returned %d\n", rc);
+ tpm_buf_destroy(&buf);
+
+ /*
+* second test, seal random data protecting sensitive by
+* encryption and also doing response encryption (not
+* necessary) The encrypted payload has two components: an
+* authorization password which must be presented on useal and
+* the actual data (the random payload)
+*/
+ tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
+ tpm_buf_append_name(&buf, auth, chip->tpmkey, chip->tpmkeyname);
+ tpm_buf_append_hmac_session(&buf, auth, TPM2_SA_DECRYPT
+ | TPM2_SA_ENCRYPT
+ | TPM2_SA_CONTINUE_SESSION, NULL, 0);
+ /* sensitive */
+ tpm2b_init(&t2b);
+ /* the authorization */
+ tpm2b_append_u16(&t2b, strlen(password));
+ tpm2b_append(&t2b, password, strlen(password));
+ /* the payload */
+ tpm2b_append_u16(&t2b, sizeof(payload));
+ tpm2b_append(&t2b, payload, sizeof(payload));
+ tpm_buf_append_2b(&buf, &t2b);
+ /* the public */
+ /* type */
+ tpm2b_append_u16(&t2b, TPM2_ALG_KEYEDHASH);
+ /* name hash */
+ tpm2b_append_u16(&t2b, TPM2_ALG_SHA256);
+ /* object properties */
+ tpm2b_append_u32(&t2b, TPM2_OA_USER_WITH_AUTH | TPM2_OA_NO_DA);
+ /* auth policy (empty) */
+ tpm2b_append_u16(&t2b, 0);
+ /* keyed hash parameters (we're null for a non-HMAC data blob) */
+ tpm2b_append_u16(&t2b, TPM2_ALG_NULL)
[PATCH 1/2] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
This code adds true session based HMAC authentication plus parameter
decryption and response encryption using AES.
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
* tpm_buf_append_hmac_session() where tpm2_append_auth() would go
* 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.
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
---
drivers/char/tpm/Kconfig | 3 +
drivers/char/tpm/Makefile| 2 +-
drivers/char/tpm/tpm.h | 22 +
drivers/char/tpm/tpm2-cmd.c | 22 +-
drivers/char/tpm/tpm2-sessions.c | 907 +++
drivers/char/tpm/tpm2-sessions.h | 55 +++
drivers/char/tpm/tpm2b.h | 82
7 files changed, 1080 insertions(+), 13 deletions(-)
create mode 100644 drivers/char/tpm/tpm2-sessions.c
create mode 100644 drivers/char/tpm/tpm2-sessions.h
create mode 100644 drivers/char/tpm/tpm2b.h
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 d37c4a1748f5..95ef2b10cc8d 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 tpm1_eventlog.o tpm2_eventlog.o \
- tpm2-space.o
+ tpm2-space.o tpm2-sessions.o
tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_eventlog_acpi.o
tpm-$(CONFIG_EFI) += tpm_eventlog_efi.o
tpm-$(CONFIG_OF) += tpm_eventlog_of.o
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 3e083a30a108..95a0d5288d6a 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -42,6 +42,9 @@
#include
#endif
+/* fixed define for the curve we use which is NIST_P256 */
+#define EC_PT_SZ 32
+
enum tpm_const {
TPM_MINOR = 224,/* officially assigned */
TPM_BUFSIZE = 4096,
@@ -114,16 +117,25 @@ enum tpm2_return_codes {
enum tpm2_algorithms {
TPM2_ALG_ERROR = 0x,
TPM2_ALG_SHA1 = 0x0004,
+ TPM2_ALG_AES= 0x0006,
TPM2_ALG_KEYEDHASH = 0x0008,
TPM2_ALG_SHA256 = 0x000B,
TPM2_ALG_SHA384 = 0x000C,
TPM2_ALG_SHA512 = 0x000D,
TPM2_ALG_NULL = 0x0010,
TPM2_ALG_SM3_256= 0x0012,
+ TPM2_ALG_ECC= 0x0023,
+ TPM2_ALG_CFB= 0x0043,
+};
+
+enum tpm2_curves {
+ TPM2_ECC_NONE = 0x,
+ TPM2_ECC_NIST_P256 = 0x0003,
};
enum tpm2_command_codes {
TPM2_CC_FIRST = 0x011F,
+ TPM2_CC_CREATE_PRIMARY = 0x0131,
TPM2_CC_SELF_TEST = 0x0143,
TPM2_CC_STARTUP = 0x0144,
TPM2_CC_SHUTDOWN= 0x0145,
@@ -133,6 +145,7 @@ enum tpm2_command_codes {
TPM2_CC_CONTEXT_LOAD= 0x0161,
TPM2_CC_CONTEXT_SAVE= 0x0162,
TPM2_C
[RFC 0/2] add integrity and security to TPM2 transactions
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&m=151994371015475 --- James Bottomley (2): tpm2-sessions: Add full HMAC and encrypt/decrypt session handling tpm2-sessions: NOT FOR COMMITTING add sessions testing drivers/char/tpm/Kconfig | 3 + drivers/char/tpm/Makefile | 3 +- drivers/char/tpm/tpm-chip.c | 1 + drivers/char/tpm/tpm.h| 22 + drivers/char/tpm/tpm2-cmd.c | 22 +- drivers/char/tpm/tpm2-sessions-test.c | 178 +++ drivers/char/tpm/tpm2-sessions.c | 907 ++ drivers/char/tpm/tpm2-sessions.h | 55 +++ drivers/char/tpm/tpm2b.h | 82 +++ 9 files changed, 1260 insertions(+), 13 deletions(-) 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 create mode 100644 drivers/char/tpm/tpm2b.h -- 2.12.3
[PATCH 2/2] crypto: ecdh: fix to allow multi segment scatterlists
Apparently the ecdh use case was in bluetooth which always has single
element scatterlists, so the ecdh module was hard coded to expect
them. Now we're using this in TPM, we need multi-element
scatterlists, so remove this limitation.
Signed-off-by: James Bottomley
---
crypto/ecdh.c | 23 +--
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/crypto/ecdh.c b/crypto/ecdh.c
index 3aca0933ec44..d2ec33f0e098 100644
--- a/crypto/ecdh.c
+++ b/crypto/ecdh.c
@@ -89,12 +89,19 @@ static int ecdh_compute_value(struct kpp_request *req)
if (!shared_secret)
goto free_pubkey;
- copied = sg_copy_to_buffer(req->src, 1, public_key,
- public_key_sz);
- if (copied != public_key_sz) {
- ret = -EINVAL;
+ /* from here on it's invalid parameters */
+ ret = -EINVAL;
+
+ /* must have exactly two points to be on the curve */
+ if (public_key_sz != req->src_len)
+ goto free_all;
+
+ copied = sg_copy_to_buffer(req->src,
+ sg_nents_for_len(req->src,
+ public_key_sz),
+ public_key, public_key_sz);
+ if (copied != public_key_sz)
goto free_all;
- }
ret = crypto_ecdh_shared_secret(ctx->curve_id, ctx->ndigits,
ctx->private_key, public_key,
@@ -111,7 +118,11 @@ static int ecdh_compute_value(struct kpp_request *req)
if (ret < 0)
goto free_all;
- copied = sg_copy_from_buffer(req->dst, 1, buf, nbytes);
+ /* might want less than we've got */
+ nbytes = min_t(size_t, nbytes, req->dst_len);
+ copied = sg_copy_from_buffer(req->dst, sg_nents_for_len(req->dst,
+ nbytes),
+buf, nbytes);
if (copied != nbytes)
ret = -EINVAL;
--
2.12.3
