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
