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
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:
<Name>,<OID number>
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.
> > @ -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.
Yep, it is irrelevant IMHO, there is 8 bytes, and also its location
never changes.
> James
BR, Jarkko
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:
>
> <Name>,<OID number>
>
> 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
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:
> >
> > <Name>,<OID number>
> >
> > 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 <linux/errno.h>
#include <linux/bug.h>
#include <linux/asn1.h>
+#include <linux/asn1_ber_bytecode.h>
#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;
On Thu May 23, 2024 at 8:08 PM EEST, James Bottomley wrote:
> 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:
> > >
> > > <Name>,<OID number>
> > >
> > > 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.
This looks pretty good to me at least in this level. I could repeal
and replace the patch I did today with this if it plays out well.
BR, Jarkko