Re: [PATCH 1/3] cifs: md5 cleanup - functions

Re: [PATCH 1/3] cifs: md5 cleanup - functions

[Steve French]

Alexander Nyberg <alexn@dsv.su.se> wrote on 04/11/2005 03:26:14 PM:

> > Function names and return types on same line - conform to
established 
> > fs/cifs/ style.
> > 
> > -void
> > -MD5Init(struct MD5Context *ctx)
> > +void MD5Init(struct MD5Context *ctx)
> >  {
> >     ctx->buf[0] = 0x67452301;
> >     ctx->buf[1] = 0xefcdab89;
> > @@ -60,8 +58,7 @@ MD5Init(struct MD5Context *ctx)
> >   * Update context to reflect the concatenation of another buffer
full
> >   * of bytes.
> >   */
> > -void
> > -MD5Update(struct MD5Context *ctx, unsigned char const *buf,
unsigned len)
> > +void MD5Update(struct MD5Context *ctx, unsigned char const *buf, 
> unsigned len)
> >  {
> 
> Can anyone enlighten me why CIFS is not using crypto/md5?
> Same question about md4
> 


There was a patch suggested a year or so ago to remove the older cifs
md5 implementation and have cifsencrypt.c use the newer Linux crypto
API, but since it made the code considerably more complex it did not
make any sense. The current crypto API seems to be designed for much
more complex usage patterns than cifs needs it for. The key use for this
for CIFS is the following small function (to calculate the packet
signitures on cifs packets in fs/cifs/cifsencrypt.c)

38 static int cifs_calculate_signature(const struct smb_hdr * cifs_pdu,
const char * key, char * signature)
39 {
40         struct  MD5Context context;
41 
42         if((cifs_pdu == NULL) || (signature == NULL))
43                 return -EINVAL;
44 
45         MD5Init(&context);
46         MD5Update(&context,key,CIFS_SESSION_KEY_SIZE+16);
47       
MD5Update(&context,cifs_pdu->Protocol,cifs_pdu->smb_buf_length);
48         MD5Final(signature,&context);
49         return 0;
50 }


The problem with moving this function to use crypto/md5.c is that the
three needed md5 functions (MD5Init, MD5Update, MD5Final) are not
exported (although they appear to be already implemented in close to the
right form already - but they are defined as static in crypto/md5.c) and
the only way to do the equivalent is much more complicated. I don't mind
using those equivalent three functions in crypto/md5.c directly if
someone could verify that they match the three functions of the same
name and could export them so cifs could call them - I would like to get
rid of cifs/md5.c

There was a similar issue IIRC with md4.c - the code gets more complex
rather than less complex moving to the crypto API.

Re: [PATCH 1/3] cifs: md5 cleanup - functions

[Francois Romieu]

Steve French <smfrench@austin.rr.com> :
[...]
> There was a patch suggested a year or so ago to remove the older cifs
> md5 implementation and have cifsencrypt.c use the newer Linux crypto
> API, but since it made the code considerably more complex it did not
> make any sense. The current crypto API seems to be designed for much
> more complex usage patterns than cifs needs it for. The key use for this
> for CIFS is the following small function (to calculate the packet
> signitures on cifs packets in fs/cifs/cifsencrypt.c)

If you have the patches from 10/2003 in mind, they suffered more from poor
taste than from cryptoapi imho.

Btw nobody cared about fs/cifs/connect.c::CIFSNTLMSSPNegotiateSessSetup
(indentation from Mars + unchecked allocations before dereferences).

--
Ueimor

Re: [PATCH 1/3] cifs: md5 cleanup - functions

[Steve French]

Francois Romieu wrote:

>Btw nobody cared about fs/cifs/connect.c::CIFSNTLMSSPNegotiateSessSetup
>(indentation from Mars + unchecked allocations before dereferences).
>
>--
>Ueimor
>  
>
That routine is disabled by default (as with the SPNEGO one) so it has 
not gotten much attention, it will probably go away or be significantly 
changed when someone goes through and collapses the four SessionSetup 
cases (currently each a distinct large function with only the default 
NTLM SessionSetup enabled by default) down to smaller functions with 
invoke some common functions.   A good time to do this would be when a 
SessionSetupOldStyle routine is added to handle pre-Windows NT4 
SessionSetup (OS/2, LAN Server, LAN Manager etc.).   Another possibility 
for a good time to update these routines is when SPNEGO support is 
finished - the SPNEGO SessionSetup (which is also too big a function) 
will change a lot (and get simpler) if Andrew Bartlett's idea of an 
upcall to the ntlm_auth utility (the man page is somewhat out of date 
from the better Samba 4 version of this see  
http://www.samba.org/samba/docs/man/ntlm_auth.1.html but it gives the 
general idea)  is done for that case - so that might be a good time to 
redo the session setup routines.

NTLMSSP authentication protocol is interesting (and the Davenport guys 
did a great job updating the documentation for it - see  
http://davenport.sourceforge.net/ntlm.html) and I would like to 
implement some of the cool optional features as I get time.

[PATCH 1/3] cifs: md5 cleanup - functions

[Jesper Juhl]

Function names and return types on same line - conform to established 
fs/cifs/ style.

Patch is also available at:
	http://www.linuxtux.org/~juhl/kernel_patches/fs_cifs_md5-funct.patch

Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>

--- linux-2.6.12-rc2-mm2-orig/fs/cifs/md5.c	2005-03-02 08:38:12.000000000 +0100
+++ linux-2.6.12-rc2-mm2/fs/cifs/md5.c	2005-04-09 10:34:47.000000000 +0200
@@ -28,8 +28,7 @@ static void MD5Transform(__u32 buf[4], _
 /*
  * Note: this code is harmless on little-endian machines.
  */
-static void
-byteReverse(unsigned char *buf, unsigned longs)
+static void byteReverse(unsigned char *buf, unsigned longs)
 {
 	__u32 t;
 	do {
@@ -44,8 +43,7 @@ byteReverse(unsigned char *buf, unsigned
  * Start MD5 accumulation.  Set bit count to 0 and buffer to mysterious
  * initialization constants.
  */
-void
-MD5Init(struct MD5Context *ctx)
+void MD5Init(struct MD5Context *ctx)
 {
 	ctx->buf[0] = 0x67452301;
 	ctx->buf[1] = 0xefcdab89;
@@ -60,8 +58,7 @@ MD5Init(struct MD5Context *ctx)
  * Update context to reflect the concatenation of another buffer full
  * of bytes.
  */
-void
-MD5Update(struct MD5Context *ctx, unsigned char const *buf, unsigned len)
+void MD5Update(struct MD5Context *ctx, unsigned char const *buf, unsigned len)
 {
 	register __u32 t;
 
@@ -109,8 +106,7 @@ MD5Update(struct MD5Context *ctx, unsign
  * Final wrapup - pad to 64-byte boundary with the bit pattern 
  * 1 0* (64-bit count of bits processed, MSB-first)
  */
-void
-MD5Final(unsigned char digest[16], struct MD5Context *ctx)
+void MD5Final(unsigned char digest[16], struct MD5Context *ctx)
 {
 	unsigned int count;
 	unsigned char *p;
@@ -168,8 +164,7 @@ MD5Final(unsigned char digest[16], struc
  * reflect the addition of 16 longwords of new data.  MD5Update blocks
  * the data and converts bytes into longwords for this routine.
  */
-static void
-MD5Transform(__u32 buf[4], __u32 const in[16])
+static void MD5Transform(__u32 buf[4], __u32 const in[16])
 {
 	register __u32 a, b, c, d;
 
@@ -255,9 +250,8 @@ MD5Transform(__u32 buf[4], __u32 const i
 /***********************************************************************
  the rfc 2104 version of hmac_md5 initialisation.
 ***********************************************************************/
-void
-hmac_md5_init_rfc2104(unsigned char *key, int key_len,
-		      struct HMACMD5Context *ctx)
+void hmac_md5_init_rfc2104(unsigned char *key, int key_len,
+	struct HMACMD5Context *ctx)
 {
 	int i;
 
@@ -293,9 +287,8 @@ hmac_md5_init_rfc2104(unsigned char *key
 /***********************************************************************
  the microsoft version of hmac_md5 initialisation.
 ***********************************************************************/
-void
-hmac_md5_init_limK_to_64(const unsigned char *key, int key_len,
-			 struct HMACMD5Context *ctx)
+void hmac_md5_init_limK_to_64(const unsigned char *key, int key_len,
+	struct HMACMD5Context *ctx)
 {
 	int i;
 
@@ -323,9 +316,8 @@ hmac_md5_init_limK_to_64(const unsigned 
 /***********************************************************************
  update hmac_md5 "inner" buffer
 ***********************************************************************/
-void
-hmac_md5_update(const unsigned char *text, int text_len,
-		struct HMACMD5Context *ctx)
+void hmac_md5_update(const unsigned char *text, int text_len,
+	struct HMACMD5Context *ctx)
 {
 	MD5Update(&ctx->ctx, text, text_len);	/* then text of datagram */
 }
@@ -333,8 +325,7 @@ hmac_md5_update(const unsigned char *tex
 /***********************************************************************
  finish off hmac_md5 "inner" buffer and generate outer one.
 ***********************************************************************/
-void
-hmac_md5_final(unsigned char *digest, struct HMACMD5Context *ctx)
+void hmac_md5_final(unsigned char *digest, struct HMACMD5Context *ctx)
 {
 	struct MD5Context ctx_o;
 
@@ -350,9 +341,8 @@ hmac_md5_final(unsigned char *digest, st
  single function to calculate an HMAC MD5 digest from data.
  use the microsoft hmacmd5 init method because the key is 16 bytes.
 ************************************************************/
-void
-hmac_md5(unsigned char key[16], unsigned char *data, int data_len,
-	 unsigned char *digest)
+void hmac_md5(unsigned char key[16], unsigned char *data, int data_len,
+	unsigned char *digest)
 {
 	struct HMACMD5Context ctx;
 	hmac_md5_init_limK_to_64(key, 16, &ctx);

Re: [PATCH 1/3] cifs: md5 cleanup - functions

[Alexander Nyberg]

> Function names and return types on same line - conform to established 
> fs/cifs/ style.
> 
> -void
> -MD5Init(struct MD5Context *ctx)
> +void MD5Init(struct MD5Context *ctx)
>  {
>  	ctx->buf[0] = 0x67452301;
>  	ctx->buf[1] = 0xefcdab89;
> @@ -60,8 +58,7 @@ MD5Init(struct MD5Context *ctx)
>   * Update context to reflect the concatenation of another buffer full
>   * of bytes.
>   */
> -void
> -MD5Update(struct MD5Context *ctx, unsigned char const *buf, unsigned len)
> +void MD5Update(struct MD5Context *ctx, unsigned char const *buf, unsigned len)
>  {

Can anyone enlighten me why CIFS is not using crypto/md5?
Same question about md4

Re: [PATCH 1/3] cifs: md5 cleanup - functions

[Matt Mackall]

On Mon, Apr 11, 2005 at 10:11:39PM +0200, Jesper Juhl wrote:
> 
> Function names and return types on same line - conform to established 
> fs/cifs/ style.
> 
> Patch is also available at:
> 	http://www.linuxtux.org/~juhl/kernel_patches/fs_cifs_md5-funct.patch

I think the right thing to do here is what I did with the SHA1 code
from random.c: put the favorite implementation in lib/ and replace the
cryptoapi and CIFS implementations (and any other users) with it.

If you feel like tackling this, let me know, it's been on my todo list
for a while.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH 1/3] cifs: md5 cleanup - functions

[Jesper Juhl]

On Mon, 11 Apr 2005, Matt Mackall wrote:

> Date: Mon, 11 Apr 2005 23:37:45 -0700
> From: Matt Mackall <mpm@selenic.com>
> To: Jesper Juhl <juhl-lkml@dif.dk>
> Cc: Steven French <sfrench@us.ibm.com>, Steve French <smfrench@austin.rr.com>,
>     linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/3] cifs: md5 cleanup - functions
> 
> On Mon, Apr 11, 2005 at 10:11:39PM +0200, Jesper Juhl wrote:
> > 
> > Function names and return types on same line - conform to established 
> > fs/cifs/ style.
> > 
> > Patch is also available at:
> > 	http://www.linuxtux.org/~juhl/kernel_patches/fs_cifs_md5-funct.patch
> 
> I think the right thing to do here is what I did with the SHA1 code
> from random.c: put the favorite implementation in lib/ and replace the
> cryptoapi and CIFS implementations (and any other users) with it.
> 
> If you feel like tackling this, let me know, it's been on my todo list
> for a while.
> 
I wouldn't mind having a go at it, but I don't have too much time this 
week, but next week I should have some time - I'll take a look at it then.

-- 
Jesper Juhl