| Message ID | 20240521031645.17008-5-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | None | expand |
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 000000000000..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 <linux/slab.h> > + > +/* > + * 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
On Tue May 21, 2024 at 9:18 PM EEST, James Bottomley wrote: > 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 000000000000..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 <linux/slab.h> > > + > > +/* > > + * 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). ECDSA and RSA have different space requirements. With that solution you actually max out space requirements given same cap for everything. Even tpm2_key_ecdsa should use a different value than tpm2_key_rsa to save memory. > 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. You don't save a single byte of memory with any constant that dictates the size requirements for multiple modules in two disjoint subsystems. You are maximizing the use of memory. > James BR, Jarkko
Jarkko Sakkinen <jarkko@kernel.org> 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. What the limit should be, OTOH, is up for discussion, but PAGE_SIZE seems not unreasonable. David
On Tue, 2024-05-21 at 22:44 +0100, David Howells wrote: > Jarkko Sakkinen <jarkko@kernel.org> 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
On Wed May 22, 2024 at 12:44 AM EEST, David Howells wrote: > Jarkko Sakkinen <jarkko@kernel.org> 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. What the limit > should be, OTOH, is up for discussion, but PAGE_SIZE seems not unreasonable. When the decoder for ASN.1 was part of trusted keys, the check used to be: if (ctx.priv_len + ctx.pub_len > MAX_BLOB_SIZE) return -EINVAL; And MAX_BLOB_SIZE is only 512 bytes, which does not fit event 2048 bit RSA key but that 512 bytes cap seems to be just fine for trusted keys. So the new check is: if (blob_len > max_key_len) return -E2BIG; 1. Too big value is not invalid value, thus -E2BIG. It is has also shown to be practically useful while testing this key type. 2. tpm2_key_rsa needs up to 8192 bytes for a blob to fit 4096-bit RSA key. Just saying but there is also primary null key allocated by the driver. And neither driver uses MAX_BLOB_SiZE. It uses value 8x MAX_BLOB_SIZE i.e. 4096 bytes so not really following the idea suggested. Finaly, there is three completely separate algorithms: - KEYEDHASH (trusted_keys) - RSA (tpm2_key_rsa) - ECDSA (driver)§ With all this put together it is just common sense to have parametrized cap value, and it would have no logic at all to treat them unified way. For tpm2_key_rsa I will define for clarity: #define TPM2_KEY_RSA_MAX_SIZE 8192 For tpm2_key_ecdsa you would define #define TPM2_KEY_ECDSA_MAX_SIZE 4096 So yeah, this is how I will proceed because it is really the only senseful way to proceed. > > David BR, Jarkko
On Wed May 22, 2024 at 12:59 AM EEST, James Bottomley wrote: > On Tue, 2024-05-21 at 22:44 +0100, David Howells wrote: > > Jarkko Sakkinen <jarkko@kernel.org> 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. Not true. It did return -EINVAL. This patch changes it to -E2BIG. > > > 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. MAX_BLOB_SIZE would be used to cap key blob, not the policy. And you are ignoring it yourself too in the driver. > James BR, Jarkko
On Wed May 22, 2024 at 1:45 AM EEST, Jarkko Sakkinen wrote: > On Wed May 22, 2024 at 12:59 AM EEST, James Bottomley wrote: > > On Tue, 2024-05-21 at 22:44 +0100, David Howells wrote: > > > Jarkko Sakkinen <jarkko@kernel.org> 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. > > Not true. > > It did return -EINVAL. This patch changes it to -E2BIG. > > > > > > 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. > > MAX_BLOB_SIZE would be used to cap key blob, not the policy. > > And you are ignoring it yourself too in the driver. Obviously policy is part of the key blob i.e. expected value for that. ... but that does not reduce space requirements to rsa asymmetric keys. It increases them but I think at this point 8192 is good starting point. And it cap can be scaled later. Being a parameter also allows to have even kernel-command line or sysfs parameter and stuff like that. It is robust not a bad choice. BR, Jarkko
diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig index e63a6a17793c..de2f4093c939 100644 --- a/drivers/char/tpm/Kconfig +++ b/drivers/char/tpm/Kconfig @@ -7,6 +7,7 @@ menuconfig TCG_TPM tristate "TPM Hardware Support" depends on HAS_IOMEM imply SECURITYFS + select ASN1 select CRYPTO select CRYPTO_HASH_INFO help diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile index 4c695b0388f3..071437058ef6 100644 --- a/drivers/char/tpm/Makefile +++ b/drivers/char/tpm/Makefile @@ -17,6 +17,11 @@ tpm-y += eventlog/tpm1.o tpm-y += eventlog/tpm2.o tpm-y += tpm-buf.o +# TPM2 Asymmetric Key +$(obj)/trusted_tpm2.o: $(obj)/tpm2key.asn1.h +tpm-y += tpm2key.asn1.o +tpm-y += tpm2_key.o + tpm-$(CONFIG_TCG_TPM2_HMAC) += tpm2-sessions.o tpm-$(CONFIG_ACPI) += tpm_ppi.o eventlog/acpi.o tpm-$(CONFIG_EFI) += eventlog/efi.o diff --git a/drivers/char/tpm/tpm2_key.c b/drivers/char/tpm/tpm2_key.c new file mode 100644 index 000000000000..0112362e432e --- /dev/null +++ b/drivers/char/tpm/tpm2_key.c @@ -0,0 +1,111 @@ +// SPDX-License-Identifier: GPL-2.0-only + +#include <linux/oid_registry.h> +#include <linux/slab.h> +#include <linux/types.h> +#include <crypto/tpm2_key.h> +#include <asm/unaligned.h> +#include "tpm2key.asn1.h" + +#undef pr_fmt +#define pr_fmt(fmt) "tpm2_key: "fmt + +int tpm2_key_parent(void *context, size_t hdrlen, + unsigned char tag, + const void *value, size_t vlen) +{ + struct tpm2_key *ctx = context; + const u8 *v = value; + int i; + + ctx->parent = 0; + for (i = 0; i < vlen; i++) { + ctx->parent <<= 8; + ctx->parent |= v[i]; + } + + return 0; +} + +int tpm2_key_type(void *context, size_t hdrlen, + unsigned char tag, + const void *value, size_t vlen) +{ + enum OID oid = look_up_OID(value, vlen); + + if (oid != OID_TPMSealedData) { + char buffer[50]; + + sprint_oid(value, vlen, buffer, sizeof(buffer)); + pr_debug("OID is \"%s\" which is not TPMSealedData\n", + buffer); + return -EINVAL; + } + + return 0; +} + +int tpm2_key_pub(void *context, size_t hdrlen, + unsigned char tag, + const void *value, size_t vlen) +{ + struct tpm2_key *ctx = context; + + ctx->pub = value; + ctx->pub_len = vlen; + + return 0; +} + +int tpm2_key_priv(void *context, size_t hdrlen, + unsigned char tag, + const void *value, size_t vlen) +{ + struct tpm2_key *ctx = context; + + ctx->priv = value; + ctx->priv_len = vlen; + + return 0; +} + +/** + * tpm_key_decode() - Decode TPM2 ASN.1 key. + * @src: ASN.1 source. + * @src_len: ASN.1 source length. + * @key: TPM2 asymmetric key. + * @max_key_len: Maximum length of the TPM2 asymmetric key. + * + * Decodes TPM2 ASN.1 key on success. Returns POSIX error code on failure. + */ +int tpm2_key_decode(const u8 *src, u32 src_len, struct tpm2_key *key, + u32 max_key_len) +{ + struct tpm2_key ctx; + u32 blob_len; + int ret; + + memset(&ctx, 0, sizeof(ctx)); + + ret = asn1_ber_decoder(&tpm2key_decoder, &ctx, src, src_len); + if (ret < 0) + return ret; + + blob_len = ctx.priv_len + ctx.pub_len; + if (blob_len > max_key_len) + return -E2BIG; + + ctx.blob_len = blob_len; + ctx.blob = kmalloc(blob_len, GFP_KERNEL); + if (!ctx.blob) + return -ENOMEM; + + memcpy((void *)ctx.blob, ctx.priv, ctx.priv_len); + memcpy((void *)ctx.blob + ctx.priv_len, ctx.pub, ctx.pub_len); + ctx.priv = ctx.blob; + ctx.pub = ctx.blob + ctx.priv_len; + + memcpy(key, &ctx, sizeof(ctx)); + return 0; +} +EXPORT_SYMBOL_GPL(tpm2_key_decode); diff --git a/security/keys/trusted-keys/tpm2key.asn1 b/drivers/char/tpm/tpm2key.asn1 similarity index 100% rename from security/keys/trusted-keys/tpm2key.asn1 rename to drivers/char/tpm/tpm2key.asn1 diff --git a/include/crypto/tpm2_key.h b/include/crypto/tpm2_key.h new file mode 100644 index 000000000000..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 <linux/slab.h> + +/* + * 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); + +/** + * tpm2_key_free() - Release TPM2 asymmetric key resources and reset values + * @key: TPM2 asymmetric key. + */ +static inline void tpm2_key_destroy(struct tpm2_key *key) +{ + kfree(key->blob); + memset(key, 0, sizeof(*key)); +} + +#endif /* __LINUX_TPM2_KEY_H__ */ diff --git a/security/keys/trusted-keys/Makefile b/security/keys/trusted-keys/Makefile index f0f3b27f688b..2674d5c10fc9 100644 --- a/security/keys/trusted-keys/Makefile +++ b/security/keys/trusted-keys/Makefile @@ -7,9 +7,7 @@ obj-$(CONFIG_TRUSTED_KEYS) += trusted.o trusted-y += trusted_core.o trusted-$(CONFIG_TRUSTED_KEYS_TPM) += trusted_tpm1.o -$(obj)/trusted_tpm2.o: $(obj)/tpm2key.asn1.h trusted-$(CONFIG_TRUSTED_KEYS_TPM) += trusted_tpm2.o -trusted-$(CONFIG_TRUSTED_KEYS_TPM) += tpm2key.asn1.o trusted-$(CONFIG_TRUSTED_KEYS_TEE) += trusted_tee.o diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c index ec59f9389a2d..f255388d32b8 100644 --- a/security/keys/trusted-keys/trusted_tpm2.c +++ b/security/keys/trusted-keys/trusted_tpm2.c @@ -13,11 +13,10 @@ #include <keys/trusted-type.h> #include <keys/trusted_tpm.h> +#include <crypto/tpm2_key.h> #include <asm/unaligned.h> -#include "tpm2key.asn1.h" - static struct tpm2_hash tpm2_hash_map[] = { {HASH_ALGO_SHA1, TPM_ALG_SHA1}, {HASH_ALGO_SHA256, TPM_ALG_SHA256}, @@ -28,9 +27,9 @@ static struct tpm2_hash tpm2_hash_map[] = { 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) +static int tpm2_trusted_key_encode(struct trusted_key_payload *payload, + struct trusted_key_options *options, + u8 *src, u32 len) { const int SCRATCH_SIZE = PAGE_SIZE; u8 *scratch = kmalloc(SCRATCH_SIZE, GFP_KERNEL); @@ -100,106 +99,6 @@ static int tpm2_key_encode(struct trusted_key_payload *payload, return ret; } -struct tpm2_key_context { - u32 parent; - const u8 *pub; - u32 pub_len; - const u8 *priv; - u32 priv_len; -}; - -static int tpm2_key_decode(struct trusted_key_payload *payload, - struct trusted_key_options *options, - u8 **buf) -{ - int ret; - struct tpm2_key_context ctx; - u8 *blob; - - memset(&ctx, 0, sizeof(ctx)); - - ret = asn1_ber_decoder(&tpm2key_decoder, &ctx, payload->blob, - payload->blob_len); - if (ret < 0) - return ret; - - if (ctx.priv_len + ctx.pub_len > MAX_BLOB_SIZE) - return -EINVAL; - - blob = kmalloc(ctx.priv_len + ctx.pub_len + 4, GFP_KERNEL); - if (!blob) - return -ENOMEM; - - *buf = blob; - options->keyhandle = ctx.parent; - - memcpy(blob, ctx.priv, ctx.priv_len); - blob += ctx.priv_len; - - memcpy(blob, ctx.pub, ctx.pub_len); - - return 0; -} - -int tpm2_key_parent(void *context, size_t hdrlen, - unsigned char tag, - const void *value, size_t vlen) -{ - struct tpm2_key_context *ctx = context; - const u8 *v = value; - int i; - - ctx->parent = 0; - for (i = 0; i < vlen; i++) { - ctx->parent <<= 8; - ctx->parent |= v[i]; - } - - return 0; -} - -int tpm2_key_type(void *context, size_t hdrlen, - unsigned char tag, - const void *value, size_t vlen) -{ - enum OID oid = look_up_OID(value, vlen); - - if (oid != OID_TPMSealedData) { - char buffer[50]; - - sprint_oid(value, vlen, buffer, sizeof(buffer)); - pr_debug("OID is \"%s\" which is not TPMSealedData\n", - buffer); - return -EINVAL; - } - - return 0; -} - -int tpm2_key_pub(void *context, size_t hdrlen, - unsigned char tag, - const void *value, size_t vlen) -{ - struct tpm2_key_context *ctx = context; - - ctx->pub = value; - ctx->pub_len = vlen; - - return 0; -} - -int tpm2_key_priv(void *context, size_t hdrlen, - unsigned char tag, - const void *value, size_t vlen) -{ - struct tpm2_key_context *ctx = context; - - ctx->priv = value; - ctx->priv_len = vlen; - - return 0; -} - /** * tpm2_buf_append_auth() - append TPMS_AUTH_COMMAND to the buffer. * @@ -349,7 +248,8 @@ int tpm2_seal_trusted(struct tpm_chip *chip, goto out; } - blob_len = tpm2_key_encode(payload, options, &buf.data[offset], blob_len); + blob_len = tpm2_trusted_key_encode(payload, options, &buf.data[offset], + blob_len); out: tpm_buf_destroy(&sized); @@ -389,21 +289,27 @@ static int tpm2_load_cmd(struct tpm_chip *chip, struct trusted_key_options *options, u32 *blob_handle) { - struct tpm_buf buf; unsigned int private_len; unsigned int public_len; unsigned int blob_len; - u8 *blob, *pub; + struct tpm2_key key; + struct tpm_buf buf; + const u8 *blob, *pub; int rc; u32 attrs; - rc = tpm2_key_decode(payload, options, &blob); + rc = tpm2_key_decode(payload->blob, payload->blob_len, &key, PAGE_SIZE); if (rc) { /* old form */ blob = payload->blob; payload->old_format = 1; + } else { + blob = key.blob; } + if (!blob) + return -ENOMEM; + /* new format carries keyhandle but old format doesn't */ if (!options->keyhandle) return -EINVAL; @@ -467,7 +373,8 @@ static int tpm2_load_cmd(struct tpm_chip *chip, out: if (blob != payload->blob) - kfree(blob); + tpm2_key_destroy(&key); + tpm_buf_destroy(&buf); if (rc > 0)
Move tpm2_key_decode() to the TPM driver and export the symbols to make them callable from trusted keys. It can re-used for asymmetric keys. Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> --- v2: Do not allocate blob twice. Use the one inside struct tpm2_key. --- drivers/char/tpm/Kconfig | 1 + drivers/char/tpm/Makefile | 5 + drivers/char/tpm/tpm2_key.c | 111 +++++++++++++++ .../char/tpm}/tpm2key.asn1 | 0 include/crypto/tpm2_key.h | 33 +++++ security/keys/trusted-keys/Makefile | 2 - security/keys/trusted-keys/trusted_tpm2.c | 127 +++--------------- 7 files changed, 167 insertions(+), 112 deletions(-) create mode 100644 drivers/char/tpm/tpm2_key.c rename {security/keys/trusted-keys => drivers/char/tpm}/tpm2key.asn1 (100%) create mode 100644 include/crypto/tpm2_key.h