[1/3] CIFS: Use OID registry facility

Commit Message

David Howells Oct. 22, 2012, 2:23 p.m. UTC

Use the OID registry facility from fs/cifs/asn1.c as that converts known OIDs
into an enum which should speed up comparison.

Signed-off-by: David Howells <[e-mail]>
---

 fs/cifs/Kconfig              |    1 
 fs/cifs/asn1.c               |  156 ++++++++----------------------------------
 include/linux/oid_registry.h |    6 ++
 3 files changed, 36 insertions(+), 127 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Jeff Layton Oct. 22, 2012, 6:36 p.m. UTC | #1

On Mon, 22 Oct 2012 15:23:26 +0100
David Howells <[e-mail]> wrote:

> Use the OID registry facility from fs/cifs/asn1.c as that converts known OIDs
> into an enum which should speed up comparison.
> 
> Signed-off-by: David Howells <[e-mail]>
> ---
> 
>  fs/cifs/Kconfig              |    1 
>  fs/cifs/asn1.c               |  156 ++++++++----------------------------------
>  include/linux/oid_registry.h |    6 ++
>  3 files changed, 36 insertions(+), 127 deletions(-)
> 
> diff --git a/fs/cifs/Kconfig b/fs/cifs/Kconfig
> index 2075ddf..0f7a0a7 100644
> --- a/fs/cifs/Kconfig
> +++ b/fs/cifs/Kconfig
> @@ -10,6 +10,7 @@ config CIFS
>  	select CRYPTO_ECB
>  	select CRYPTO_DES
>  	select CRYPTO_SHA256
> +	select OID_REGISTRY
>  	help
>  	  This is the client VFS module for the Common Internet File System
>  	  (CIFS) protocol which is the successor to the Server Message Block
> diff --git a/fs/cifs/asn1.c b/fs/cifs/asn1.c
> index cfd1ce3..3a7b2b6 100644
> --- a/fs/cifs/asn1.c
> +++ b/fs/cifs/asn1.c
> @@ -22,6 +22,7 @@
>  #include <linux/kernel.h>
>  #include <linux/mm.h>
>  #include <linux/slab.h>
> +#include <linux/oid_registry.h>
>  #include "cifspdu.h"
>  #include "cifsglob.h"
>  #include "cifs_debug.h"
> @@ -76,17 +77,6 @@
>  #define ASN1_ERR_DEC_LENGTH_MISMATCH	4
>  #define ASN1_ERR_DEC_BADVALUE		5
>  
> -#define SPNEGO_OID_LEN 7
> -#define NTLMSSP_OID_LEN  10
> -#define KRB5_OID_LEN  7
> -#define KRB5U2U_OID_LEN  8
> -#define MSKRB5_OID_LEN  7
> -static unsigned long SPNEGO_OID[7] = { 1, 3, 6, 1, 5, 5, 2 };
> -static unsigned long NTLMSSP_OID[10] = { 1, 3, 6, 1, 4, 1, 311, 2, 2, 10 };
> -static unsigned long KRB5_OID[7] = { 1, 2, 840, 113554, 1, 2, 2 };
> -static unsigned long KRB5U2U_OID[8] = { 1, 2, 840, 113554, 1, 2, 2, 3 };
> -static unsigned long MSKRB5_OID[7] = { 1, 2, 840, 48018, 1, 2, 2 };
> -
>  /*
>   * ASN.1 context.
>   */
> @@ -397,95 +387,10 @@ asn1_octets_decode(struct asn1_ctx *ctx,
>  	return 1;
>  } */
>  
> -static unsigned char
> -asn1_subid_decode(struct asn1_ctx *ctx, unsigned long *subid)
> +static enum OID
> +asn1_oid_decode(struct asn1_ctx *ctx, unsigned char *eoc)
>  {
> -	unsigned char ch;
> -
> -	*subid = 0;
> -
> -	do {
> -		if (!asn1_octet_decode(ctx, &ch))
> -			return 0;
> -
> -		*subid <<= 7;
> -		*subid |= ch & 0x7F;
> -	} while ((ch & 0x80) == 0x80);
> -	return 1;
> -}
> -
> -static int
> -asn1_oid_decode(struct asn1_ctx *ctx,
> -		unsigned char *eoc, unsigned long **oid, unsigned int *len)
> -{
> -	unsigned long subid;
> -	unsigned int size;
> -	unsigned long *optr;
> -
> -	size = eoc - ctx->pointer + 1;
> -
> -	/* first subid actually encodes first two subids */
> -	if (size < 2 || size > UINT_MAX/sizeof(unsigned long))
> -		return 0;
> -
> -	*oid = kmalloc(size * sizeof(unsigned long), GFP_ATOMIC);
> -	if (*oid == NULL)
> -		return 0;
> -
> -	optr = *oid;
> -
> -	if (!asn1_subid_decode(ctx, &subid)) {
> -		kfree(*oid);
> -		*oid = NULL;
> -		return 0;
> -	}
> -
> -	if (subid < 40) {
> -		optr[0] = 0;
> -		optr[1] = subid;
> -	} else if (subid < 80) {
> -		optr[0] = 1;
> -		optr[1] = subid - 40;
> -	} else {
> -		optr[0] = 2;
> -		optr[1] = subid - 80;
> -	}
> -
> -	*len = 2;
> -	optr += 2;
> -
> -	while (ctx->pointer < eoc) {
> -		if (++(*len) > size) {
> -			ctx->error = ASN1_ERR_DEC_BADVALUE;
> -			kfree(*oid);
> -			*oid = NULL;
> -			return 0;
> -		}
> -
> -		if (!asn1_subid_decode(ctx, optr++)) {
> -			kfree(*oid);
> -			*oid = NULL;
> -			return 0;
> -		}
> -	}
> -	return 1;
> -}
> -
> -static int
> -compare_oid(unsigned long *oid1, unsigned int oid1len,
> -	    unsigned long *oid2, unsigned int oid2len)
> -{
> -	unsigned int i;
> -
> -	if (oid1len != oid2len)
> -		return 0;
> -	else {
> -		for (i = 0; i < oid1len; i++) {
> -			if (oid1[i] != oid2[i])
> -				return 0;
> -		}
> -		return 1;
> -	}
> +	return look_up_OID(ctx->pointer, eoc - ctx->pointer);
>  }
>  
>  	/* BB check for endian conversion issues here */
> @@ -497,8 +402,7 @@ decode_negTokenInit(unsigned char *security_blob, int length,
>  	struct asn1_ctx ctx;
>  	unsigned char *end;
>  	unsigned char *sequence_end;
> -	unsigned long *oid = NULL;
> -	unsigned int cls, con, tag, oidlen, rc;
> +	unsigned int cls, con, tag, rc;
>  
>  	/* cifs_dump_mem(" Received SecBlob ", security_blob, length); */
>  
> @@ -519,12 +423,8 @@ decode_negTokenInit(unsigned char *security_blob, int length,
>  	if (rc) {
>  		if ((tag == ASN1_OJI) && (con == ASN1_PRI) &&
>  		    (cls == ASN1_UNI)) {
> -			rc = asn1_oid_decode(&ctx, end, &oid, &oidlen);
> -			if (rc) {
> -				rc = compare_oid(oid, oidlen, SPNEGO_OID,
> -						 SPNEGO_OID_LEN);
> -				kfree(oid);
> -			}
> +			if (asn1_oid_decode(&ctx, end) != OID_SPNEGO)
> +				rc = 0;
>  		} else
>  			rc = 0;
>  	}
> @@ -588,26 +488,28 @@ decode_negTokenInit(unsigned char *security_blob, int length,
>  			return 0;
>  		}
>  		if ((tag == ASN1_OJI) && (con == ASN1_PRI)) {
> -			if (asn1_oid_decode(&ctx, end, &oid, &oidlen)) {
> -
> -				cFYI(1, "OID len = %d oid = 0x%lx 0x%lx "
> -					"0x%lx 0x%lx", oidlen, *oid,
> -					*(oid + 1), *(oid + 2), *(oid + 3));
> -
> -				if (compare_oid(oid, oidlen, MSKRB5_OID,
> -						MSKRB5_OID_LEN))
> -					server->sec_mskerberos = true;
> -				else if (compare_oid(oid, oidlen, KRB5U2U_OID,
> -						     KRB5U2U_OID_LEN))
> -					server->sec_kerberosu2u = true;
> -				else if (compare_oid(oid, oidlen, KRB5_OID,
> -						     KRB5_OID_LEN))
> -					server->sec_kerberos = true;
> -				else if (compare_oid(oid, oidlen, NTLMSSP_OID,
> -						     NTLMSSP_OID_LEN))
> -					server->sec_ntlmssp = true;
> -
> -				kfree(oid);
> +			enum OID oid = asn1_oid_decode(&ctx, end);
> +			if (oid != OID__NR)
> +				cFYI(1, "OID oid = %u", oid);
> +			switch (oid) {
> +			case OID_MSKRB5:
> +				server->sec_mskerberos = true;
> +				break;
> +			case OID_KRB5U2U:
> +				server->sec_kerberosu2u = true;
> +				break;
> +			case OID_KRB5:
> +				server->sec_kerberos = true;
> +				break;
> +			case OID_NTLMSSP:
> +				server->sec_ntlmssp = true;
> +				break;
> +			case OID__NR:
> +				cFYI(1, "OID len = %ld oid = %*pX",
> +				     end - ctx.pointer,
> +				     (int)(end - ctx.pointer), ctx.pointer);
> +			default:
> +				break;
>  			}
>  		} else {
>  			cFYI(1, "Should be an oid what is going on?");
> diff --git a/include/linux/oid_registry.h b/include/linux/oid_registry.h
> index 6926db7..4bff9c5 100644
> --- a/include/linux/oid_registry.h
> +++ b/include/linux/oid_registry.h
> @@ -82,6 +82,12 @@ enum OID {
>  	OID_authorityKeyIdentifier,	/* 2.5.29.35 */
>  	OID_extKeyUsage,		/* 2.5.29.37 */
>  
> +	OID_SPNEGO,			/* 1.3.6.1.5.5.2 */
> +	OID_NTLMSSP,			/* 1.3.6.1.4.1.311.2.2.10 */
> +	OID_KRB5,			/* 1.2.840.113554.1.2.2 */
> +	OID_KRB5U2U,			/* 1.2.840.113554.1.2.2.3 */
> +	OID_MSKRB5,			/* 1.2.840.48018.1.2.2 */
> +
>  	OID__NR
>  };
>  
> 

In general, I'm all for moving CIFS out of the business of implementing
ASN.1 like this. This last bit of this patch was a bit confusing though.
It left me wondering where the actual definitions of these OIDs go.

It looks though like you're using a script to scrape the comments out
of the above enum to generate C files. That would certainly work, but
it seems like a fragile solution. Minor whitespace munging (like an
extra newline in there) could throw off the parsing...

Shirish Pargaonkar Oct. 22, 2012, 6:48 p.m. UTC | #2

On Mon, Oct 22, 2012 at 9:23 AM, David Howells <[e-mail]> wrote:
> Use the OID registry facility from fs/cifs/asn1.c as that converts known OIDs
> into an enum which should speed up comparison.
>
> Signed-off-by: David Howells <[e-mail]>
> ---
>
>  fs/cifs/Kconfig              |    1
>  fs/cifs/asn1.c               |  156 ++++++++----------------------------------
>  include/linux/oid_registry.h |    6 ++
>  3 files changed, 36 insertions(+), 127 deletions(-)
>
> diff --git a/fs/cifs/Kconfig b/fs/cifs/Kconfig
> index 2075ddf..0f7a0a7 100644
> --- a/fs/cifs/Kconfig
> +++ b/fs/cifs/Kconfig
> @@ -10,6 +10,7 @@ config CIFS
>         select CRYPTO_ECB
>         select CRYPTO_DES
>         select CRYPTO_SHA256
> +       select OID_REGISTRY

How do I incorporate/add OID_REGISTRY as a config option?
I do not see that in my .config (3.7)

>         help
>           This is the client VFS module for the Common Internet File System
>           (CIFS) protocol which is the successor to the Server Message Block
> diff --git a/fs/cifs/asn1.c b/fs/cifs/asn1.c
> index cfd1ce3..3a7b2b6 100644
> --- a/fs/cifs/asn1.c
> +++ b/fs/cifs/asn1.c
> @@ -22,6 +22,7 @@
>  #include <linux/kernel.h>
>  #include <linux/mm.h>
>  #include <linux/slab.h>
> +#include <linux/oid_registry.h>
>  #include "cifspdu.h"
>  #include "cifsglob.h"
>  #include "cifs_debug.h"
> @@ -76,17 +77,6 @@
>  #define ASN1_ERR_DEC_LENGTH_MISMATCH   4
>  #define ASN1_ERR_DEC_BADVALUE          5
>
> -#define SPNEGO_OID_LEN 7
> -#define NTLMSSP_OID_LEN  10
> -#define KRB5_OID_LEN  7
> -#define KRB5U2U_OID_LEN  8
> -#define MSKRB5_OID_LEN  7
> -static unsigned long SPNEGO_OID[7] = { 1, 3, 6, 1, 5, 5, 2 };
> -static unsigned long NTLMSSP_OID[10] = { 1, 3, 6, 1, 4, 1, 311, 2, 2, 10 };
> -static unsigned long KRB5_OID[7] = { 1, 2, 840, 113554, 1, 2, 2 };
> -static unsigned long KRB5U2U_OID[8] = { 1, 2, 840, 113554, 1, 2, 2, 3 };
> -static unsigned long MSKRB5_OID[7] = { 1, 2, 840, 48018, 1, 2, 2 };
> -
>  /*
>   * ASN.1 context.
>   */
> @@ -397,95 +387,10 @@ asn1_octets_decode(struct asn1_ctx *ctx,
>         return 1;
>  } */
>
> -static unsigned char
> -asn1_subid_decode(struct asn1_ctx *ctx, unsigned long *subid)
> +static enum OID
> +asn1_oid_decode(struct asn1_ctx *ctx, unsigned char *eoc)
>  {
> -       unsigned char ch;
> -
> -       *subid = 0;
> -
> -       do {
> -               if (!asn1_octet_decode(ctx, &ch))
> -                       return 0;
> -
> -               *subid <<= 7;
> -               *subid |= ch & 0x7F;
> -       } while ((ch & 0x80) == 0x80);
> -       return 1;
> -}
> -
> -static int
> -asn1_oid_decode(struct asn1_ctx *ctx,
> -               unsigned char *eoc, unsigned long **oid, unsigned int *len)
> -{
> -       unsigned long subid;
> -       unsigned int size;
> -       unsigned long *optr;
> -
> -       size = eoc - ctx->pointer + 1;
> -
> -       /* first subid actually encodes first two subids */
> -       if (size < 2 || size > UINT_MAX/sizeof(unsigned long))
> -               return 0;
> -
> -       *oid = kmalloc(size * sizeof(unsigned long), GFP_ATOMIC);
> -       if (*oid == NULL)
> -               return 0;
> -
> -       optr = *oid;
> -
> -       if (!asn1_subid_decode(ctx, &subid)) {
> -               kfree(*oid);
> -               *oid = NULL;
> -               return 0;
> -       }
> -
> -       if (subid < 40) {
> -               optr[0] = 0;
> -               optr[1] = subid;
> -       } else if (subid < 80) {
> -               optr[0] = 1;
> -               optr[1] = subid - 40;
> -       } else {
> -               optr[0] = 2;
> -               optr[1] = subid - 80;
> -       }
> -
> -       *len = 2;
> -       optr += 2;
> -
> -       while (ctx->pointer < eoc) {
> -               if (++(*len) > size) {
> -                       ctx->error = ASN1_ERR_DEC_BADVALUE;
> -                       kfree(*oid);
> -                       *oid = NULL;
> -                       return 0;
> -               }
> -
> -               if (!asn1_subid_decode(ctx, optr++)) {
> -                       kfree(*oid);
> -                       *oid = NULL;
> -                       return 0;
> -               }
> -       }
> -       return 1;
> -}
> -
> -static int
> -compare_oid(unsigned long *oid1, unsigned int oid1len,
> -           unsigned long *oid2, unsigned int oid2len)
> -{
> -       unsigned int i;
> -
> -       if (oid1len != oid2len)
> -               return 0;
> -       else {
> -               for (i = 0; i < oid1len; i++) {
> -                       if (oid1[i] != oid2[i])
> -                               return 0;
> -               }
> -               return 1;
> -       }
> +       return look_up_OID(ctx->pointer, eoc - ctx->pointer);
>  }
>
>         /* BB check for endian conversion issues here */
> @@ -497,8 +402,7 @@ decode_negTokenInit(unsigned char *security_blob, int length,
>         struct asn1_ctx ctx;
>         unsigned char *end;
>         unsigned char *sequence_end;
> -       unsigned long *oid = NULL;
> -       unsigned int cls, con, tag, oidlen, rc;
> +       unsigned int cls, con, tag, rc;
>
>         /* cifs_dump_mem(" Received SecBlob ", security_blob, length); */
>
> @@ -519,12 +423,8 @@ decode_negTokenInit(unsigned char *security_blob, int length,
>         if (rc) {
>                 if ((tag == ASN1_OJI) && (con == ASN1_PRI) &&
>                     (cls == ASN1_UNI)) {
> -                       rc = asn1_oid_decode(&ctx, end, &oid, &oidlen);
> -                       if (rc) {
> -                               rc = compare_oid(oid, oidlen, SPNEGO_OID,
> -                                                SPNEGO_OID_LEN);
> -                               kfree(oid);
> -                       }
> +                       if (asn1_oid_decode(&ctx, end) != OID_SPNEGO)
> +                               rc = 0;
>                 } else
>                         rc = 0;
>         }
> @@ -588,26 +488,28 @@ decode_negTokenInit(unsigned char *security_blob, int length,
>                         return 0;
>                 }
>                 if ((tag == ASN1_OJI) && (con == ASN1_PRI)) {
> -                       if (asn1_oid_decode(&ctx, end, &oid, &oidlen)) {
> -
> -                               cFYI(1, "OID len = %d oid = 0x%lx 0x%lx "
> -                                       "0x%lx 0x%lx", oidlen, *oid,
> -                                       *(oid + 1), *(oid + 2), *(oid + 3));
> -
> -                               if (compare_oid(oid, oidlen, MSKRB5_OID,
> -                                               MSKRB5_OID_LEN))
> -                                       server->sec_mskerberos = true;
> -                               else if (compare_oid(oid, oidlen, KRB5U2U_OID,
> -                                                    KRB5U2U_OID_LEN))
> -                                       server->sec_kerberosu2u = true;
> -                               else if (compare_oid(oid, oidlen, KRB5_OID,
> -                                                    KRB5_OID_LEN))
> -                                       server->sec_kerberos = true;
> -                               else if (compare_oid(oid, oidlen, NTLMSSP_OID,
> -                                                    NTLMSSP_OID_LEN))
> -                                       server->sec_ntlmssp = true;
> -
> -                               kfree(oid);
> +                       enum OID oid = asn1_oid_decode(&ctx, end);
> +                       if (oid != OID__NR)
> +                               cFYI(1, "OID oid = %u", oid);
> +                       switch (oid) {
> +                       case OID_MSKRB5:
> +                               server->sec_mskerberos = true;
> +                               break;
> +                       case OID_KRB5U2U:
> +                               server->sec_kerberosu2u = true;
> +                               break;
> +                       case OID_KRB5:
> +                               server->sec_kerberos = true;
> +                               break;
> +                       case OID_NTLMSSP:
> +                               server->sec_ntlmssp = true;
> +                               break;
> +                       case OID__NR:
> +                               cFYI(1, "OID len = %ld oid = %*pX",
> +                                    end - ctx.pointer,
> +                                    (int)(end - ctx.pointer), ctx.pointer);
> +                       default:
> +                               break;
>                         }
>                 } else {
>                         cFYI(1, "Should be an oid what is going on?");
> diff --git a/include/linux/oid_registry.h b/include/linux/oid_registry.h
> index 6926db7..4bff9c5 100644
> --- a/include/linux/oid_registry.h
> +++ b/include/linux/oid_registry.h
> @@ -82,6 +82,12 @@ enum OID {
>         OID_authorityKeyIdentifier,     /* 2.5.29.35 */
>         OID_extKeyUsage,                /* 2.5.29.37 */
>
> +       OID_SPNEGO,                     /* 1.3.6.1.5.5.2 */
> +       OID_NTLMSSP,                    /* 1.3.6.1.4.1.311.2.2.10 */
> +       OID_KRB5,                       /* 1.2.840.113554.1.2.2 */
> +       OID_KRB5U2U,                    /* 1.2.840.113554.1.2.2.3 */
> +       OID_MSKRB5,                     /* 1.2.840.48018.1.2.2 */
> +
>         OID__NR
>  };
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Howells Oct. 22, 2012, 7:13 p.m. UTC | #3

Shirish Pargaonkar <[e-mail]> wrote:

> How do I incorporate/add OID_REGISTRY as a config option?
> I do not see that in my .config (3.7)

What do you mean?

David
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Howells Oct. 22, 2012, 7:19 p.m. UTC | #4

Jeff Layton <[e-mail]> wrote:

> In general, I'm all for moving CIFS out of the business of implementing
> ASN.1 like this. This last bit of this patch was a bit confusing though.
> It left me wondering where the actual definitions of these OIDs go.
> 
> It looks though like you're using a script to scrape the comments out
> of the above enum to generate C files.

That is correct.  See the comment preceding enum OID.  I should add something
to Documentation/ about this too.

> That would certainly work, but it seems like a fragile solution. Minor
> whitespace munging (like an extra newline in there) could throw off the
> parsing...

Extra whitespace and extra newlines shouldn't hurt, unless you mean splitting
a comment over multiple lines.

I could generate the enum too, though it's easy enough for anyone to check
that the OID they've just added is present in the table.

Actually, it can be made a lot more robust by assuming that every line with
"OID_" in it is an OID declaration, and everyone of those lines that doesn't
match the expected pattern gets an error.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Shirish Pargaonkar Oct. 22, 2012, 7:57 p.m. UTC | #5

On Mon, Oct 22, 2012 at 2:13 PM, David Howells <[e-mail]> wrote:
> Shirish Pargaonkar <[e-mail]> wrote:
>
>> How do I incorporate/add OID_REGISTRY as a config option?
>> I do not see that in my .config (3.7)
>
> What do you mean?
>
> David

David, I do not see CONFIG_OID_RRGISTRY in .config.

Regards,

Shirish
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jeff Layton Oct. 22, 2012, 7:57 p.m. UTC | #6

On Mon, 22 Oct 2012 20:19:25 +0100
David Howells <[e-mail]> wrote:

> Jeff Layton <[e-mail]> wrote:
> 
> > In general, I'm all for moving CIFS out of the business of implementing
> > ASN.1 like this. This last bit of this patch was a bit confusing though.
> > It left me wondering where the actual definitions of these OIDs go.
> > 
> > It looks though like you're using a script to scrape the comments out
> > of the above enum to generate C files.
> 
> That is correct.  See the comment preceding enum OID.  I should add something
> to Documentation/ about this too.
> 
> > That would certainly work, but it seems like a fragile solution. Minor
> > whitespace munging (like an extra newline in there) could throw off the
> > parsing...
> 
> Extra whitespace and extra newlines shouldn't hurt, unless you mean splitting
> a comment over multiple lines.
> 
> I could generate the enum too, though it's easy enough for anyone to check
> that the OID they've just added is present in the table.
> 
> Actually, it can be made a lot more robust by assuming that every line with
> "OID_" in it is an OID declaration, and everyone of those lines that doesn't
> match the expected pattern gets an error.
> 
> David

I guess it just looked a little weird to me. The comment that matches a
enum element is actually part of the following element since it comes
after the comma. Based on the script, what matters is that the enum and
the comment are on the same line.

But...as long as it's all clearly documented, I guess I can't complain
too much. ;)

David Howells Oct. 27, 2012, 1:04 p.m. UTC | #7

Shirish Pargaonkar <[e-mail]> wrote:

> >> How do I incorporate/add OID_REGISTRY as a config option?
> >> I do not see that in my .config (3.7)
> >
> > What do you mean?
> >
> > David
> 
> David, I do not see CONFIG_OID_RRGISTRY in .config.

Do you see OID_REGISTRY in lib/Kconfig (probably near the end)?

David
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Shirish Pargaonkar Oct. 27, 2012, 10:53 p.m. UTC | #8

On Sat, Oct 27, 2012 at 8:04 AM, David Howells <[e-mail]> wrote:
> Shirish Pargaonkar <[e-mail]> wrote:
>
>> >> How do I incorporate/add OID_REGISTRY as a config option?
>> >> I do not see that in my .config (3.7)
>> >
>> > What do you mean?
>> >
>> > David
>>
>> David, I do not see CONFIG_OID_RRGISTRY in .config.
>
> Do you see OID_REGISTRY in lib/Kconfig (probably near the end)?
>
> David

Yes, I do it

config OID_REGISTRY
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jeff Layton Oct. 28, 2012, 11:22 a.m. UTC | #9

On Mon, 22 Oct 2012 15:23:26 +0100
David Howells <[e-mail]> wrote:

> Use the OID registry facility from fs/cifs/asn1.c as that converts known OIDs
> into an enum which should speed up comparison.
> 
> Signed-off-by: David Howells <[e-mail]>
> ---
> 
>  fs/cifs/Kconfig              |    1 
>  fs/cifs/asn1.c               |  156 ++++++++----------------------------------
>  include/linux/oid_registry.h |    6 ++
>  3 files changed, 36 insertions(+), 127 deletions(-)
> 
> diff --git a/fs/cifs/Kconfig b/fs/cifs/Kconfig
> index 2075ddf..0f7a0a7 100644
> --- a/fs/cifs/Kconfig
> +++ b/fs/cifs/Kconfig
> @@ -10,6 +10,7 @@ config CIFS
>  	select CRYPTO_ECB
>  	select CRYPTO_DES
>  	select CRYPTO_SHA256
> +	select OID_REGISTRY
>  	help
>  	  This is the client VFS module for the Common Internet File System
>  	  (CIFS) protocol which is the successor to the Server Message Block
> diff --git a/fs/cifs/asn1.c b/fs/cifs/asn1.c
> index cfd1ce3..3a7b2b6 100644
> --- a/fs/cifs/asn1.c
> +++ b/fs/cifs/asn1.c
> @@ -22,6 +22,7 @@
>  #include <linux/kernel.h>
>  #include <linux/mm.h>
>  #include <linux/slab.h>
> +#include <linux/oid_registry.h>
>  #include "cifspdu.h"
>  #include "cifsglob.h"
>  #include "cifs_debug.h"
> @@ -76,17 +77,6 @@
>  #define ASN1_ERR_DEC_LENGTH_MISMATCH	4
>  #define ASN1_ERR_DEC_BADVALUE		5
>  
> -#define SPNEGO_OID_LEN 7
> -#define NTLMSSP_OID_LEN  10
> -#define KRB5_OID_LEN  7
> -#define KRB5U2U_OID_LEN  8
> -#define MSKRB5_OID_LEN  7
> -static unsigned long SPNEGO_OID[7] = { 1, 3, 6, 1, 5, 5, 2 };
> -static unsigned long NTLMSSP_OID[10] = { 1, 3, 6, 1, 4, 1, 311, 2, 2, 10 };
> -static unsigned long KRB5_OID[7] = { 1, 2, 840, 113554, 1, 2, 2 };
> -static unsigned long KRB5U2U_OID[8] = { 1, 2, 840, 113554, 1, 2, 2, 3 };
> -static unsigned long MSKRB5_OID[7] = { 1, 2, 840, 48018, 1, 2, 2 };
> -
>  /*
>   * ASN.1 context.
>   */
> @@ -397,95 +387,10 @@ asn1_octets_decode(struct asn1_ctx *ctx,
>  	return 1;
>  } */
>  
> -static unsigned char
> -asn1_subid_decode(struct asn1_ctx *ctx, unsigned long *subid)
> +static enum OID
> +asn1_oid_decode(struct asn1_ctx *ctx, unsigned char *eoc)
>  {
> -	unsigned char ch;
> -
> -	*subid = 0;
> -
> -	do {
> -		if (!asn1_octet_decode(ctx, &ch))
> -			return 0;
> -
> -		*subid <<= 7;
> -		*subid |= ch & 0x7F;
> -	} while ((ch & 0x80) == 0x80);
> -	return 1;
> -}
> -
> -static int
> -asn1_oid_decode(struct asn1_ctx *ctx,
> -		unsigned char *eoc, unsigned long **oid, unsigned int *len)
> -{
> -	unsigned long subid;
> -	unsigned int size;
> -	unsigned long *optr;
> -
> -	size = eoc - ctx->pointer + 1;
> -
> -	/* first subid actually encodes first two subids */
> -	if (size < 2 || size > UINT_MAX/sizeof(unsigned long))
> -		return 0;
> -
> -	*oid = kmalloc(size * sizeof(unsigned long), GFP_ATOMIC);
> -	if (*oid == NULL)
> -		return 0;
> -
> -	optr = *oid;
> -
> -	if (!asn1_subid_decode(ctx, &subid)) {
> -		kfree(*oid);
> -		*oid = NULL;
> -		return 0;
> -	}
> -
> -	if (subid < 40) {
> -		optr[0] = 0;
> -		optr[1] = subid;
> -	} else if (subid < 80) {
> -		optr[0] = 1;
> -		optr[1] = subid - 40;
> -	} else {
> -		optr[0] = 2;
> -		optr[1] = subid - 80;
> -	}
> -
> -	*len = 2;
> -	optr += 2;
> -
> -	while (ctx->pointer < eoc) {
> -		if (++(*len) > size) {
> -			ctx->error = ASN1_ERR_DEC_BADVALUE;
> -			kfree(*oid);
> -			*oid = NULL;
> -			return 0;
> -		}
> -
> -		if (!asn1_subid_decode(ctx, optr++)) {
> -			kfree(*oid);
> -			*oid = NULL;
> -			return 0;
> -		}
> -	}
> -	return 1;
> -}
> -
> -static int
> -compare_oid(unsigned long *oid1, unsigned int oid1len,
> -	    unsigned long *oid2, unsigned int oid2len)
> -{
> -	unsigned int i;
> -
> -	if (oid1len != oid2len)
> -		return 0;
> -	else {
> -		for (i = 0; i < oid1len; i++) {
> -			if (oid1[i] != oid2[i])
> -				return 0;
> -		}
> -		return 1;
> -	}
> +	return look_up_OID(ctx->pointer, eoc - ctx->pointer);
>  }
>  
>  	/* BB check for endian conversion issues here */
> @@ -497,8 +402,7 @@ decode_negTokenInit(unsigned char *security_blob, int length,
>  	struct asn1_ctx ctx;
>  	unsigned char *end;
>  	unsigned char *sequence_end;
> -	unsigned long *oid = NULL;
> -	unsigned int cls, con, tag, oidlen, rc;
> +	unsigned int cls, con, tag, rc;
>  
>  	/* cifs_dump_mem(" Received SecBlob ", security_blob, length); */
>  
> @@ -519,12 +423,8 @@ decode_negTokenInit(unsigned char *security_blob, int length,
>  	if (rc) {
>  		if ((tag == ASN1_OJI) && (con == ASN1_PRI) &&
>  		    (cls == ASN1_UNI)) {
> -			rc = asn1_oid_decode(&ctx, end, &oid, &oidlen);
> -			if (rc) {
> -				rc = compare_oid(oid, oidlen, SPNEGO_OID,
> -						 SPNEGO_OID_LEN);
> -				kfree(oid);
> -			}
> +			if (asn1_oid_decode(&ctx, end) != OID_SPNEGO)
> +				rc = 0;
>  		} else
>  			rc = 0;
>  	}
> @@ -588,26 +488,28 @@ decode_negTokenInit(unsigned char *security_blob, int length,
>  			return 0;
>  		}
>  		if ((tag == ASN1_OJI) && (con == ASN1_PRI)) {
> -			if (asn1_oid_decode(&ctx, end, &oid, &oidlen)) {
> -
> -				cFYI(1, "OID len = %d oid = 0x%lx 0x%lx "
> -					"0x%lx 0x%lx", oidlen, *oid,
> -					*(oid + 1), *(oid + 2), *(oid + 3));
> -
> -				if (compare_oid(oid, oidlen, MSKRB5_OID,
> -						MSKRB5_OID_LEN))
> -					server->sec_mskerberos = true;
> -				else if (compare_oid(oid, oidlen, KRB5U2U_OID,
> -						     KRB5U2U_OID_LEN))
> -					server->sec_kerberosu2u = true;
> -				else if (compare_oid(oid, oidlen, KRB5_OID,
> -						     KRB5_OID_LEN))
> -					server->sec_kerberos = true;
> -				else if (compare_oid(oid, oidlen, NTLMSSP_OID,
> -						     NTLMSSP_OID_LEN))
> -					server->sec_ntlmssp = true;
> -
> -				kfree(oid);
> +			enum OID oid = asn1_oid_decode(&ctx, end);
> +			if (oid != OID__NR)
> +				cFYI(1, "OID oid = %u", oid);
> +			switch (oid) {
> +			case OID_MSKRB5:
> +				server->sec_mskerberos = true;
> +				break;
> +			case OID_KRB5U2U:
> +				server->sec_kerberosu2u = true;
> +				break;
> +			case OID_KRB5:
> +				server->sec_kerberos = true;
> +				break;
> +			case OID_NTLMSSP:
> +				server->sec_ntlmssp = true;
> +				break;
> +			case OID__NR:
> +				cFYI(1, "OID len = %ld oid = %*pX",
> +				     end - ctx.pointer,
> +				     (int)(end - ctx.pointer), ctx.pointer);
> +			default:
> +				break;
>  			}
>  		} else {
>  			cFYI(1, "Should be an oid what is going on?");
> diff --git a/include/linux/oid_registry.h b/include/linux/oid_registry.h
> index 6926db7..4bff9c5 100644
> --- a/include/linux/oid_registry.h
> +++ b/include/linux/oid_registry.h
> @@ -82,6 +82,12 @@ enum OID {
>  	OID_authorityKeyIdentifier,	/* 2.5.29.35 */
>  	OID_extKeyUsage,		/* 2.5.29.37 */
>  
> +	OID_SPNEGO,			/* 1.3.6.1.5.5.2 */
> +	OID_NTLMSSP,			/* 1.3.6.1.4.1.311.2.2.10 */
> +	OID_KRB5,			/* 1.2.840.113554.1.2.2 */
> +	OID_KRB5U2U,			/* 1.2.840.113554.1.2.2.3 */
> +	OID_MSKRB5,			/* 1.2.840.48018.1.2.2 */
> +
>  	OID__NR
>  };
>  
> 

I forgot to mention that this patch looks fine to me after my earlier comments:

Reviewed-by: Jeff Layton <[e-mail]>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/cifs/Kconfig b/fs/cifs/Kconfig
index 2075ddf..0f7a0a7 100644
--- a/fs/cifs/Kconfig
+++ b/fs/cifs/Kconfig
@@ -10,6 +10,7 @@  config CIFS
 	select CRYPTO_ECB
 	select CRYPTO_DES
 	select CRYPTO_SHA256
+	select OID_REGISTRY
 	help
 	  This is the client VFS module for the Common Internet File System
 	  (CIFS) protocol which is the successor to the Server Message Block
diff --git a/fs/cifs/asn1.c b/fs/cifs/asn1.c
index cfd1ce3..3a7b2b6 100644
--- a/fs/cifs/asn1.c
+++ b/fs/cifs/asn1.c
@@ -22,6 +22,7 @@ 
 #include <linux/kernel.h>
 #include <linux/mm.h>
 #include <linux/slab.h>
+#include <linux/oid_registry.h>
 #include "cifspdu.h"
 #include "cifsglob.h"
 #include "cifs_debug.h"
@@ -76,17 +77,6 @@ 
 #define ASN1_ERR_DEC_LENGTH_MISMATCH	4
 #define ASN1_ERR_DEC_BADVALUE		5
 
-#define SPNEGO_OID_LEN 7
-#define NTLMSSP_OID_LEN  10
-#define KRB5_OID_LEN  7
-#define KRB5U2U_OID_LEN  8
-#define MSKRB5_OID_LEN  7
-static unsigned long SPNEGO_OID[7] = { 1, 3, 6, 1, 5, 5, 2 };
-static unsigned long NTLMSSP_OID[10] = { 1, 3, 6, 1, 4, 1, 311, 2, 2, 10 };
-static unsigned long KRB5_OID[7] = { 1, 2, 840, 113554, 1, 2, 2 };
-static unsigned long KRB5U2U_OID[8] = { 1, 2, 840, 113554, 1, 2, 2, 3 };
-static unsigned long MSKRB5_OID[7] = { 1, 2, 840, 48018, 1, 2, 2 };
-
 /*
  * ASN.1 context.
  */
@@ -397,95 +387,10 @@  asn1_octets_decode(struct asn1_ctx *ctx,
 	return 1;
 } */
 
-static unsigned char
-asn1_subid_decode(struct asn1_ctx *ctx, unsigned long *subid)
+static enum OID
+asn1_oid_decode(struct asn1_ctx *ctx, unsigned char *eoc)
 {
-	unsigned char ch;
-
-	*subid = 0;
-
-	do {
-		if (!asn1_octet_decode(ctx, &ch))
-			return 0;
-
-		*subid <<= 7;
-		*subid |= ch & 0x7F;
-	} while ((ch & 0x80) == 0x80);
-	return 1;
-}
-
-static int
-asn1_oid_decode(struct asn1_ctx *ctx,
-		unsigned char *eoc, unsigned long **oid, unsigned int *len)
-{
-	unsigned long subid;
-	unsigned int size;
-	unsigned long *optr;
-
-	size = eoc - ctx->pointer + 1;
-
-	/* first subid actually encodes first two subids */
-	if (size < 2 || size > UINT_MAX/sizeof(unsigned long))
-		return 0;
-
-	*oid = kmalloc(size * sizeof(unsigned long), GFP_ATOMIC);
-	if (*oid == NULL)
-		return 0;
-
-	optr = *oid;
-
-	if (!asn1_subid_decode(ctx, &subid)) {
-		kfree(*oid);
-		*oid = NULL;
-		return 0;
-	}
-
-	if (subid < 40) {
-		optr[0] = 0;
-		optr[1] = subid;
-	} else if (subid < 80) {
-		optr[0] = 1;
-		optr[1] = subid - 40;
-	} else {
-		optr[0] = 2;
-		optr[1] = subid - 80;
-	}
-
-	*len = 2;
-	optr += 2;
-
-	while (ctx->pointer < eoc) {
-		if (++(*len) > size) {
-			ctx->error = ASN1_ERR_DEC_BADVALUE;
-			kfree(*oid);
-			*oid = NULL;
-			return 0;
-		}
-
-		if (!asn1_subid_decode(ctx, optr++)) {
-			kfree(*oid);
-			*oid = NULL;
-			return 0;
-		}
-	}
-	return 1;
-}
-
-static int
-compare_oid(unsigned long *oid1, unsigned int oid1len,
-	    unsigned long *oid2, unsigned int oid2len)
-{
-	unsigned int i;
-
-	if (oid1len != oid2len)
-		return 0;
-	else {
-		for (i = 0; i < oid1len; i++) {
-			if (oid1[i] != oid2[i])
-				return 0;
-		}
-		return 1;
-	}
+	return look_up_OID(ctx->pointer, eoc - ctx->pointer);
 }
 
 	/* BB check for endian conversion issues here */
@@ -497,8 +402,7 @@  decode_negTokenInit(unsigned char *security_blob, int length,
 	struct asn1_ctx ctx;
 	unsigned char *end;
 	unsigned char *sequence_end;
-	unsigned long *oid = NULL;
-	unsigned int cls, con, tag, oidlen, rc;
+	unsigned int cls, con, tag, rc;
 
 	/* cifs_dump_mem(" Received SecBlob ", security_blob, length); */
 
@@ -519,12 +423,8 @@  decode_negTokenInit(unsigned char *security_blob, int length,
 	if (rc) {
 		if ((tag == ASN1_OJI) && (con == ASN1_PRI) &&
 		    (cls == ASN1_UNI)) {
-			rc = asn1_oid_decode(&ctx, end, &oid, &oidlen);
-			if (rc) {
-				rc = compare_oid(oid, oidlen, SPNEGO_OID,
-						 SPNEGO_OID_LEN);
-				kfree(oid);
-			}
+			if (asn1_oid_decode(&ctx, end) != OID_SPNEGO)
+				rc = 0;
 		} else
 			rc = 0;
 	}
@@ -588,26 +488,28 @@  decode_negTokenInit(unsigned char *security_blob, int length,
 			return 0;
 		}
 		if ((tag == ASN1_OJI) && (con == ASN1_PRI)) {
-			if (asn1_oid_decode(&ctx, end, &oid, &oidlen)) {
-
-				cFYI(1, "OID len = %d oid = 0x%lx 0x%lx "
-					"0x%lx 0x%lx", oidlen, *oid,
-					*(oid + 1), *(oid + 2), *(oid + 3));
-
-				if (compare_oid(oid, oidlen, MSKRB5_OID,
-						MSKRB5_OID_LEN))
-					server->sec_mskerberos = true;
-				else if (compare_oid(oid, oidlen, KRB5U2U_OID,
-						     KRB5U2U_OID_LEN))
-					server->sec_kerberosu2u = true;
-				else if (compare_oid(oid, oidlen, KRB5_OID,
-						     KRB5_OID_LEN))
-					server->sec_kerberos = true;
-				else if (compare_oid(oid, oidlen, NTLMSSP_OID,
-						     NTLMSSP_OID_LEN))
-					server->sec_ntlmssp = true;
-
-				kfree(oid);
+			enum OID oid = asn1_oid_decode(&ctx, end);
+			if (oid != OID__NR)
+				cFYI(1, "OID oid = %u", oid);
+			switch (oid) {
+			case OID_MSKRB5:
+				server->sec_mskerberos = true;
+				break;
+			case OID_KRB5U2U:
+				server->sec_kerberosu2u = true;
+				break;
+			case OID_KRB5:
+				server->sec_kerberos = true;
+				break;
+			case OID_NTLMSSP:
+				server->sec_ntlmssp = true;
+				break;
+			case OID__NR:
+				cFYI(1, "OID len = %ld oid = %*pX",
+				     end - ctx.pointer,
+				     (int)(end - ctx.pointer), ctx.pointer);
+			default:
+				break;
 			}
 		} else {
 			cFYI(1, "Should be an oid what is going on?");
diff --git a/include/linux/oid_registry.h b/include/linux/oid_registry.h
index 6926db7..4bff9c5 100644
--- a/include/linux/oid_registry.h
+++ b/include/linux/oid_registry.h
@@ -82,6 +82,12 @@  enum OID {
 	OID_authorityKeyIdentifier,	/* 2.5.29.35 */
 	OID_extKeyUsage,		/* 2.5.29.37 */
 
+	OID_SPNEGO,			/* 1.3.6.1.5.5.2 */
+	OID_NTLMSSP,			/* 1.3.6.1.4.1.311.2.2.10 */
+	OID_KRB5,			/* 1.2.840.113554.1.2.2 */
+	OID_KRB5U2U,			/* 1.2.840.113554.1.2.2.3 */
+	OID_MSKRB5,			/* 1.2.840.48018.1.2.2 */
+
 	OID__NR
 };