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

["Jarkko Sakkinen" <jarkko@kernel.org>]

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