| Message ID | 20240524125955.20739-2-James.Bottomley@HansenPartnership.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | replace asn1_encode_oid with encode_OID | expand |
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
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
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
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
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 > >
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);
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(+)