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