[PATCH 0/3] replace asn1_encode_oid with encode_OID

[PATCH 0/3] replace asn1_encode_oid with encode_OID

[James Bottomley]

The relacement removes the requirement for consumers to maintain free
form OIDs in their code and instead requires all OIDs encoded to be in
the linux OID registry.

Note patch 2/3 needs fixing up for the tpm2_key_encode memory leak fix.

James

---

James Bottomley (3):
  lib/oid_registry: add ability to ASN.1 encode OIDs
  KEYS: trusted: use encode_OID for OID encoding
  lib: asn1_encode: remove obsolete asn1_encode_oid

 include/linux/asn1_encoder.h              |  3 -
 include/linux/oid_registry.h              |  1 +
 lib/asn1_encoder.c                        | 91 -----------------------
 lib/oid_registry.c                        | 29 ++++++++
 security/keys/trusted-keys/trusted_tpm2.c |  9 ++-
 5 files changed, 35 insertions(+), 98 deletions(-)

-- 
2.35.3

[PATCH 1/3] lib/oid_registry: add ability to ASN.1 encode OIDs

[James Bottomley]

Consumers of the ASN.1 encoder occasionally need to insert OIDs into
the ASN.1 stream.  The existing interface in lib/asn1_encoder.c is
clunky in that it directly encodes the u32 array form of the OID.
Instead introduce a function, encode_OID() which takes the OID enum
and returns the ASN.1 encoding.  This is easy because the OID registry
table already has the binary encoded form for comparison.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 include/linux/oid_registry.h |  1 +
 lib/oid_registry.c           | 29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

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..adbc287875c1 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 + 2;
+}
+EXPORT_SYMBOL_GPL(encode_OID);
-- 
2.35.3

[PATCH 2/3] KEYS: trusted: use encode_OID for OID encoding

[James Bottomley]

The new routine takes the OID enum instead of needing the u32 OID
array explicitly which reduces duplication and the potential for
mistakes.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 security/keys/trusted-keys/trusted_tpm2.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

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;
-- 
2.35.3

[PATCH 3/3] lib: asn1_encode: remove obsolete asn1_encode_oid

[James Bottomley]

This has been replaced by encode_OID from the OID_registry.  To use,
consumers must make sure the OID is present in enum OID in
oid_registry.h and then use encode_OID with the enum.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 include/linux/asn1_encoder.h |  3 --
 lib/asn1_encoder.c           | 91 ------------------------------------
 2 files changed, 94 deletions(-)

diff --git a/include/linux/asn1_encoder.h b/include/linux/asn1_encoder.h
index 08cd0c2ad34f..5f8cf47ede59 100644
--- a/include/linux/asn1_encoder.h
+++ b/include/linux/asn1_encoder.h
@@ -13,9 +13,6 @@ unsigned char *
 asn1_encode_integer(unsigned char *data, const unsigned char *end_data,
 		    s64 integer);
 unsigned char *
-asn1_encode_oid(unsigned char *data, const unsigned char *end_data,
-		u32 oid[], int oid_len);
-unsigned char *
 asn1_encode_tag(unsigned char *data, const unsigned char *end_data,
 		u32 tag, const unsigned char *string, int len);
 unsigned char *
diff --git a/lib/asn1_encoder.c b/lib/asn1_encoder.c
index 0fd3c454a468..c0db3cbebe89 100644
--- a/lib/asn1_encoder.c
+++ b/lib/asn1_encoder.c
@@ -85,97 +85,6 @@ asn1_encode_integer(unsigned char *data, const unsigned char *end_data,
 }
 EXPORT_SYMBOL_GPL(asn1_encode_integer);
 
-/* calculate the base 128 digit values setting the top bit of the first octet */
-static int asn1_encode_oid_digit(unsigned char **_data, int *data_len, u32 oid)
-{
-	unsigned char *data = *_data;
-	int start = 7 + 7 + 7 + 7;
-	int ret = 0;
-
-	if (*data_len < 1)
-		return -EINVAL;
-
-	/* quick case */
-	if (oid == 0) {
-		*data++ = 0x80;
-		(*data_len)--;
-		goto out;
-	}
-
-	while (oid >> start == 0)
-		start -= 7;
-
-	while (start > 0 && *data_len > 0) {
-		u8 byte;
-
-		byte = oid >> start;
-		oid = oid - (byte << start);
-		start -= 7;
-		byte |= 0x80;
-		*data++ = byte;
-		(*data_len)--;
-	}
-
-	if (*data_len > 0) {
-		*data++ = oid;
-		(*data_len)--;
-	} else {
-		ret = -EINVAL;
-	}
-
- out:
-	*_data = data;
-	return ret;
-}
-
-/**
- * asn1_encode_oid() - encode an oid to ASN.1
- * @data:	position to begin encoding at
- * @end_data:	end of data pointer, points one beyond last usable byte in @data
- * @oid:	array of oids
- * @oid_len:	length of oid array
- *
- * this encodes an OID up to ASN.1 when presented as an array of OID values
- */
-unsigned char *
-asn1_encode_oid(unsigned char *data, const unsigned char *end_data,
-		u32 oid[], int oid_len)
-{
-	int data_len = end_data - data;
-	unsigned char *d = data + 2;
-	int i, ret;
-
-	if (WARN(oid_len < 2, "OID must have at least two elements"))
-		return ERR_PTR(-EINVAL);
-
-	if (WARN(oid_len > 32, "OID is too large"))
-		return ERR_PTR(-EINVAL);
-
-	if (IS_ERR(data))
-		return data;
-
-
-	/* need at least 3 bytes for tag, length and OID encoding */
-	if (data_len < 3)
-		return ERR_PTR(-EINVAL);
-
-	data[0] = _tag(UNIV, PRIM, OID);
-	*d++ = oid[0] * 40 + oid[1];
-
-	data_len -= 3;
-
-	for (i = 2; i < oid_len; i++) {
-		ret = asn1_encode_oid_digit(&d, &data_len, oid[i]);
-		if (ret < 0)
-			return ERR_PTR(ret);
-	}
-
-	data[1] = d - data - 2;
-
-	return d;
-}
-EXPORT_SYMBOL_GPL(asn1_encode_oid);
-
 /**
  * asn1_encode_length() - encode a length to follow an ASN.1 tag
  * @data: pointer to encode at
-- 
2.35.3

Re: [PATCH 1/3] lib/oid_registry: add ability to ASN.1 encode OIDs

[Jarkko Sakkinen]

On Fri May 24, 2024 at 3:59 PM EEST, James Bottomley wrote:
> Consumers of the ASN.1 encoder occasionally need to insert OIDs into
> the ASN.1 stream.  The existing interface in lib/asn1_encoder.c is
> clunky in that it directly encodes the u32 array form of the OID.
> Instead introduce a function, encode_OID() which takes the OID enum
> and returns the ASN.1 encoding.  This is easy because the OID registry
> table already has the binary encoded form for comparison.
>
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>  include/linux/oid_registry.h |  1 +
>  lib/oid_registry.c           | 29 +++++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+)
>
> 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..adbc287875c1 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

nit: "encode_OID()"

https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt

> + * @oid: The OID to encode
> + * @buffer: The buffer to encode to
> + * @bufsize: the maximum size of the buffer

Align with tab characters.

Hmm just for sake of consistency s/the/The/

> + *
> + * Returns: negative error or encoded size in the buffer.

"Return:"

> + */
> +ssize_t encode_OID(enum OID oid, u8 *buffer, size_t bufsize)
> +{
> +	int oid_size;
> +
> +	BUG_ON(oid >= OID__NR);

Please use neither WARN's nor BUG_ON's as some sort of assertions.

It neither need pr_err() given it has enum type which AFAIK will
be detected by static analysis, but at most pr_err().


> +
> +	oid_size = oid_index[oid + 1] - oid_index[oid];
> +
> +	if (bufsize < oid_size + 2)
> +		return -EINVAL;

Hmm... maybe -E2BIG? It would overflow.

Here it would make actually sense since it is not enum typed
parameter to issue pr_err() because it is clearly a programming
error.

> +
> +	buffer[0] = _tag(UNIV, PRIM, OID);
> +	buffer[1] = oid_size;
> +
> +	memcpy(&buffer[2], &oid_data[oid_index[oid]], oid_size);
> +
> +	return oid_size + 2;
> +}
> +EXPORT_SYMBOL_GPL(encode_OID);

Yep, makes more sense than the old code for sure.

BR, Jarkko

Re: [PATCH 2/3] KEYS: trusted: use encode_OID for OID encoding

[Jarkko Sakkinen]

On Fri May 24, 2024 at 3:59 PM EEST, James Bottomley wrote:
> The new routine takes the OID enum instead of needing the u32 OID
> array explicitly which reduces duplication and the potential for
> mistakes.
>
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>  security/keys/trusted-keys/trusted_tpm2.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> 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;

Yupe, it's better this way.

BR, Jarkko

Re: [PATCH 3/3] lib: asn1_encode: remove obsolete asn1_encode_oid

[Jarkko Sakkinen]

On Fri May 24, 2024 at 3:59 PM EEST, James Bottomley wrote:
> This has been replaced by encode_OID from the OID_registry.  To use,
> consumers must make sure the OID is present in enum OID in
> oid_registry.h and then use encode_OID with the enum.
>
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>  include/linux/asn1_encoder.h |  3 --
>  lib/asn1_encoder.c           | 91 ------------------------------------
>  2 files changed, 94 deletions(-)
>
> diff --git a/include/linux/asn1_encoder.h b/include/linux/asn1_encoder.h
> index 08cd0c2ad34f..5f8cf47ede59 100644
> --- a/include/linux/asn1_encoder.h
> +++ b/include/linux/asn1_encoder.h
> @@ -13,9 +13,6 @@ unsigned char *
>  asn1_encode_integer(unsigned char *data, const unsigned char *end_data,
>  		    s64 integer);
>  unsigned char *
> -asn1_encode_oid(unsigned char *data, const unsigned char *end_data,
> -		u32 oid[], int oid_len);
> -unsigned char *
>  asn1_encode_tag(unsigned char *data, const unsigned char *end_data,
>  		u32 tag, const unsigned char *string, int len);
>  unsigned char *
> diff --git a/lib/asn1_encoder.c b/lib/asn1_encoder.c
> index 0fd3c454a468..c0db3cbebe89 100644
> --- a/lib/asn1_encoder.c
> +++ b/lib/asn1_encoder.c
> @@ -85,97 +85,6 @@ asn1_encode_integer(unsigned char *data, const unsigned char *end_data,
>  }
>  EXPORT_SYMBOL_GPL(asn1_encode_integer);
>  
> -/* calculate the base 128 digit values setting the top bit of the first octet */
> -static int asn1_encode_oid_digit(unsigned char **_data, int *data_len, u32 oid)
> -{
> -	unsigned char *data = *_data;
> -	int start = 7 + 7 + 7 + 7;
> -	int ret = 0;
> -
> -	if (*data_len < 1)
> -		return -EINVAL;
> -
> -	/* quick case */
> -	if (oid == 0) {
> -		*data++ = 0x80;
> -		(*data_len)--;
> -		goto out;
> -	}
> -
> -	while (oid >> start == 0)
> -		start -= 7;
> -
> -	while (start > 0 && *data_len > 0) {
> -		u8 byte;
> -
> -		byte = oid >> start;
> -		oid = oid - (byte << start);
> -		start -= 7;
> -		byte |= 0x80;
> -		*data++ = byte;
> -		(*data_len)--;
> -	}
> -
> -	if (*data_len > 0) {
> -		*data++ = oid;
> -		(*data_len)--;
> -	} else {
> -		ret = -EINVAL;
> -	}
> -
> - out:
> -	*_data = data;
> -	return ret;
> -}
> -
> -/**
> - * asn1_encode_oid() - encode an oid to ASN.1
> - * @data:	position to begin encoding at
> - * @end_data:	end of data pointer, points one beyond last usable byte in @data
> - * @oid:	array of oids
> - * @oid_len:	length of oid array
> - *
> - * this encodes an OID up to ASN.1 when presented as an array of OID values
> - */
> -unsigned char *
> -asn1_encode_oid(unsigned char *data, const unsigned char *end_data,
> -		u32 oid[], int oid_len)
> -{
> -	int data_len = end_data - data;
> -	unsigned char *d = data + 2;
> -	int i, ret;
> -
> -	if (WARN(oid_len < 2, "OID must have at least two elements"))
> -		return ERR_PTR(-EINVAL);
> -
> -	if (WARN(oid_len > 32, "OID is too large"))
> -		return ERR_PTR(-EINVAL);
> -
> -	if (IS_ERR(data))
> -		return data;
> -
> -
> -	/* need at least 3 bytes for tag, length and OID encoding */
> -	if (data_len < 3)
> -		return ERR_PTR(-EINVAL);
> -
> -	data[0] = _tag(UNIV, PRIM, OID);
> -	*d++ = oid[0] * 40 + oid[1];
> -
> -	data_len -= 3;
> -
> -	for (i = 2; i < oid_len; i++) {
> -		ret = asn1_encode_oid_digit(&d, &data_len, oid[i]);
> -		if (ret < 0)
> -			return ERR_PTR(ret);
> -	}
> -
> -	data[1] = d - data - 2;
> -
> -	return d;
> -}
> -EXPORT_SYMBOL_GPL(asn1_encode_oid);
> -
>  /**
>   * asn1_encode_length() - encode a length to follow an ASN.1 tag
>   * @data: pointer to encode at

Obvious change but I skip R-y's before the first patch has been
fixed (no use for anyone to tag them).

BR, Jarkko

Re: [PATCH 1/3] lib/oid_registry: add ability to ASN.1 encode OIDs

[James Bottomley]

On Fri, 2024-05-24 at 16:34 +0300, Jarkko Sakkinen wrote:
> On Fri May 24, 2024 at 3:59 PM EEST, James Bottomley wrote:
[...]
> > 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..adbc287875c1 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
> 
> nit: "encode_OID()"

will fix.

> https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
> 
> > + * @oid: The OID to encode
> > + * @buffer: The buffer to encode to
> > + * @bufsize: the maximum size of the buffer
> 
> Align with tab characters.

The kernel convention is to follow the style in the file, which isn't
currently tab aligned.

> Hmm just for sake of consistency s/the/The/

Yes, sure.

> > + *
> > + * Returns: negative error or encoded size in the buffer.
> 
> "Return:"
> 
> > + */
> > +ssize_t encode_OID(enum OID oid, u8 *buffer, size_t bufsize)
> > +{
> > +       int oid_size;
> > +
> > +       BUG_ON(oid >= OID__NR);
> 
> Please use neither WARN's nor BUG_ON's as some sort of assertions.
> 
> It neither need pr_err() given it has enum type which AFAIK will
> be detected by static analysis, but at most pr_err().

So this also is the style of the file.  It seems to be because the
internal OID enum is always a fed in constant from kernel code (no user
space exposure) so it's designed to trip in a developer test boot to
alert the developer to bad code.

> 
> 
> > +
> > +       oid_size = oid_index[oid + 1] - oid_index[oid];
> > +
> > +       if (bufsize < oid_size + 2)
> > +               return -EINVAL;
> 
> Hmm... maybe -E2BIG? It would overflow.

Technically it's an underflow (provided buffer is too small) and we
don't have an E2SMALL error ...

> Here it would make actually sense since it is not enum typed
> parameter to issue pr_err() because it is clearly a programming
> error.

Sure, I can add that.

> > +
> > +       buffer[0] = _tag(UNIV, PRIM, OID);
> > +       buffer[1] = oid_size;
> > +
> > +       memcpy(&buffer[2], &oid_data[oid_index[oid]], oid_size);
> > +
> > +       return oid_size + 2;
> > +}
> > +EXPORT_SYMBOL_GPL(encode_OID);
> 
> Yep, makes more sense than the old code for sure.

Thanks,

James

Re: [PATCH 1/3] lib/oid_registry: add ability to ASN.1 encode OIDs

[Jarkko Sakkinen]

On Fri May 24, 2024 at 5:02 PM EEST, James Bottomley wrote:
> On Fri, 2024-05-24 at 16:34 +0300, Jarkko Sakkinen wrote:
> > On Fri May 24, 2024 at 3:59 PM EEST, James Bottomley wrote:
> [...]
> > > 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..adbc287875c1 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
> > 
> > nit: "encode_OID()"
>
> will fix.
>
> > https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
> > 
> > > + * @oid: The OID to encode
> > > + * @buffer: The buffer to encode to
> > > + * @bufsize: the maximum size of the buffer
> > 
> > Align with tab characters.
>
> The kernel convention is to follow the style in the file, which isn't
> currently tab aligned.
>
> > Hmm just for sake of consistency s/the/The/
>
> Yes, sure.
>
> > > + *
> > > + * Returns: negative error or encoded size in the buffer.
> > 
> > "Return:"
> > 
> > > + */
> > > +ssize_t encode_OID(enum OID oid, u8 *buffer, size_t bufsize)
> > > +{
> > > +       int oid_size;
> > > +
> > > +       BUG_ON(oid >= OID__NR);
> > 
> > Please use neither WARN's nor BUG_ON's as some sort of assertions.
> > 
> > It neither need pr_err() given it has enum type which AFAIK will
> > be detected by static analysis, but at most pr_err().
>
> So this also is the style of the file.  It seems to be because the
> internal OID enum is always a fed in constant from kernel code (no user
> space exposure) so it's designed to trip in a developer test boot to
> alert the developer to bad code.
>
> > 
> > 
> > > +
> > > +       oid_size = oid_index[oid + 1] - oid_index[oid];
> > > +
> > > +       if (bufsize < oid_size + 2)
> > > +               return -EINVAL;
> > 
> > Hmm... maybe -E2BIG? It would overflow.
>
> Technically it's an underflow (provided buffer is too small) and we
> don't have an E2SMALL error ...

Let's stick to -EINVAL.

>
> > Here it would make actually sense since it is not enum typed
> > parameter to issue pr_err() because it is clearly a programming
> > error.
>
> Sure, I can add that.
>
> > > +
> > > +       buffer[0] = _tag(UNIV, PRIM, OID);
> > > +       buffer[1] = oid_size;
> > > +
> > > +       memcpy(&buffer[2], &oid_data[oid_index[oid]], oid_size);
> > > +
> > > +       return oid_size + 2;
> > > +}
> > > +EXPORT_SYMBOL_GPL(encode_OID);
> > 
> > Yep, makes more sense than the old code for sure.
>
> Thanks,
>
> James

Yeah, this is definitely something that can be picked as soon as
it is done!

BR, Jarkko

Re: [PATCH 1/3] lib/oid_registry: add ability to ASN.1 encode OIDs

[Jarkko Sakkinen]

On Fri May 24, 2024 at 5:26 PM EEST, Jarkko Sakkinen wrote:
> On Fri May 24, 2024 at 5:02 PM EEST, James Bottomley wrote:
> > On Fri, 2024-05-24 at 16:34 +0300, Jarkko Sakkinen wrote:
> > > On Fri May 24, 2024 at 3:59 PM EEST, James Bottomley wrote:
> > [...]
> > > > 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..adbc287875c1 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
> > > 
> > > nit: "encode_OID()"
> >
> > will fix.
> >
> > > https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
> > > 
> > > > + * @oid: The OID to encode
> > > > + * @buffer: The buffer to encode to
> > > > + * @bufsize: the maximum size of the buffer
> > > 
> > > Align with tab characters.
> >
> > The kernel convention is to follow the style in the file, which isn't
> > currently tab aligned.
> >
> > > Hmm just for sake of consistency s/the/The/
> >
> > Yes, sure.
> >
> > > > + *
> > > > + * Returns: negative error or encoded size in the buffer.
> > > 
> > > "Return:"
> > > 
> > > > + */
> > > > +ssize_t encode_OID(enum OID oid, u8 *buffer, size_t bufsize)
> > > > +{
> > > > +       int oid_size;
> > > > +
> > > > +       BUG_ON(oid >= OID__NR);
> > > 
> > > Please use neither WARN's nor BUG_ON's as some sort of assertions.
> > > 
> > > It neither need pr_err() given it has enum type which AFAIK will
> > > be detected by static analysis, but at most pr_err().
> >
> > So this also is the style of the file.  It seems to be because the
> > internal OID enum is always a fed in constant from kernel code (no user
> > space exposure) so it's designed to trip in a developer test boot to
> > alert the developer to bad code.
> >
> > > 
> > > 
> > > > +
> > > > +       oid_size = oid_index[oid + 1] - oid_index[oid];
> > > > +
> > > > +       if (bufsize < oid_size + 2)
> > > > +               return -EINVAL;
> > > 
> > > Hmm... maybe -E2BIG? It would overflow.
> >
> > Technically it's an underflow (provided buffer is too small) and we
> > don't have an E2SMALL error ...
>
> Let's stick to -EINVAL.
>
> >
> > > Here it would make actually sense since it is not enum typed
> > > parameter to issue pr_err() because it is clearly a programming
> > > error.
> >
> > Sure, I can add that.
> >
> > > > +
> > > > +       buffer[0] = _tag(UNIV, PRIM, OID);
> > > > +       buffer[1] = oid_size;
> > > > +
> > > > +       memcpy(&buffer[2], &oid_data[oid_index[oid]], oid_size);
> > > > +
> > > > +       return oid_size + 2;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(encode_OID);
> > > 
> > > Yep, makes more sense than the old code for sure.
> >
> > Thanks,
> >
> > James
>
> Yeah, this is definitely something that can be picked as soon as
> it is done!

Yeah, and other series definitely worth of *eventually* taking in
most likely :-) Don't mean to be impolite but I think it is more
fair to say that you're ignoring something now, than just ignore
it.

BR, Jarkko

Re: [PATCH 1/3] lib/oid_registry: add ability to ASN.1 encode OIDs

[Ben Boeckel]

On Fri, May 24, 2024 at 08:59:53 -0400, James Bottomley wrote:
> Consumers of the ASN.1 encoder occasionally need to insert OIDs into
> the ASN.1 stream.  The existing interface in lib/asn1_encoder.c is
> clunky in that it directly encodes the u32 array form of the OID.
> Instead introduce a function, encode_OID() which takes the OID enum
> and returns the ASN.1 encoding.  This is easy because the OID registry
> table already has the binary encoded form for comparison.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>  include/linux/oid_registry.h |  1 +
>  lib/oid_registry.c           | 29 +++++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+)
> 
> 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..adbc287875c1 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

"provided"

--Ben

> + * @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 + 2;
> +}
> +EXPORT_SYMBOL_GPL(encode_OID);
> -- 
> 2.35.3
> 
>