[PATCH 1/2] sha1: use unit32_t and uint8_t

[PATCH 1/2] sha1: use unit32_t and uint8_t

[Jean-Christophe PLAGNIOL-VILLARD]

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
 lib/sha1.c |   52 ++++++++++++++++++++++++++--------------------------
 1 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/lib/sha1.c b/lib/sha1.c
index 00dcc78..0e8aed1 100644
--- a/lib/sha1.c
+++ b/lib/sha1.c
@@ -35,9 +35,9 @@
 
 typedef struct
 {
-    unsigned long total[2];	/*!< number of bytes processed	*/
-    unsigned long state[5];	/*!< intermediate digest state	*/
-    unsigned char buffer[64];	/*!< data block being processed */
+	uint32_t total[2];	/*!< number of bytes processed	*/
+	uint32_t state[5];	/*!< intermediate digest state	*/
+	uint8_t buffer[64];	/*!< data block being processed */
 }
 sha1_context;
 
@@ -46,12 +46,13 @@ sha1_context;
  */
 #ifndef GET_UINT32_BE
 #define GET_UINT32_BE(n,b,i) {				\
-	(n) = ( (unsigned long) (b)[(i)    ] << 24 )	\
-	    | ( (unsigned long) (b)[(i) + 1] << 16 )	\
-	    | ( (unsigned long) (b)[(i) + 2] <<  8 )	\
-	    | ( (unsigned long) (b)[(i) + 3]       );	\
+	(n) = ( (uint32_t) (b)[(i)    ] << 24 )	\
+	    | ( (uint32_t) (b)[(i) + 1] << 16 )	\
+	    | ( (uint32_t) (b)[(i) + 2] <<  8 )	\
+	    | ( (uint32_t) (b)[(i) + 3]       );	\
 }
 #endif
+
 #ifndef PUT_UINT32_BE
 #define PUT_UINT32_BE(n,b,i) {				\
 	(b)[(i)    ] = (unsigned char) ( (n) >> 24 );	\
@@ -76,9 +77,9 @@ static void sha1_starts (sha1_context * ctx)
 	ctx->state[4] = 0xC3D2E1F0;
 }
 
-static void sha1_process (sha1_context * ctx, unsigned char data[64])
+static void sha1_process (sha1_context * ctx, uint8_t data[64])
 {
-	unsigned long temp, W[16], A, B, C, D, E;
+	uint32_t temp, W[16], A, B, C, D, E;
 
 	GET_UINT32_BE (W[0], data, 0);
 	GET_UINT32_BE (W[1], data, 4);
@@ -233,10 +234,9 @@ static void sha1_process (sha1_context * ctx, unsigned char data[64])
 /*
  * SHA-1 process buffer
  */
-static void sha1_update (sha1_context * ctx, unsigned char *input, int ilen)
+static void sha1_update (sha1_context * ctx, uint8_t *input, uint32_t ilen)
 {
-	int fill;
-	unsigned long left;
+	uint32_t fill, left;
 
 	if (ilen <= 0)
 		return;
@@ -247,7 +247,7 @@ static void sha1_update (sha1_context * ctx, unsigned char *input, int ilen)
 	ctx->total[0] += ilen;
 	ctx->total[0] &= 0xFFFFFFFF;
 
-	if (ctx->total[0] < (unsigned long) ilen)
+	if (ctx->total[0] < ilen)
 		ctx->total[1]++;
 
 	if (left && ilen >= fill) {
@@ -269,7 +269,7 @@ static void sha1_update (sha1_context * ctx, unsigned char *input, int ilen)
 	}
 }
 
-static const unsigned char sha1_padding[64] = {
+static uint8_t sha1_padding[64] = {
 	0x80, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 	   0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 	   0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
@@ -279,11 +279,11 @@ static const unsigned char sha1_padding[64] = {
 /*
  * SHA-1 final digest
  */
-static void sha1_finish (sha1_context * ctx, unsigned char output[20])
+static void sha1_finish (sha1_context * ctx, uint8_t output[20])
 {
-	unsigned long last, padn;
-	unsigned long high, low;
-	unsigned char msglen[8];
+	uint32_t last, padn;
+	uint32_t high, low;
+	uint8_t msglen[8];
 
 	high = (ctx->total[0] >> 29)
 		| (ctx->total[1] << 3);
@@ -295,7 +295,7 @@ static void sha1_finish (sha1_context * ctx, unsigned char output[20])
 	last = ctx->total[0] & 0x3F;
 	padn = (last < 56) ? (56 - last) : (120 - last);
 
-	sha1_update (ctx, (unsigned char *) sha1_padding, padn);
+	sha1_update (ctx, sha1_padding, padn);
 	sha1_update (ctx, msglen, 8);
 
 	PUT_UINT32_BE (ctx->state[0], output, 0);
@@ -308,14 +308,14 @@ static void sha1_finish (sha1_context * ctx, unsigned char output[20])
 /*
  * Output = HMAC-SHA-1( input buffer, hmac key )
  */
-void sha1_hmac (unsigned char *key, int keylen,
-		unsigned char *input, int ilen, unsigned char output[20])
+void sha1_hmac (uint8_t *key, uint32_t keylen,
+		uint8_t *input, uint32_t ilen, uint8_t output[20])
 {
-	int i;
+	uint32_t i;
 	sha1_context ctx;
-	unsigned char k_ipad[64];
-	unsigned char k_opad[64];
-	unsigned char tmpbuf[20];
+	uint8_t k_ipad[64];
+	uint8_t k_opad[64];
+	uint8_t tmpbuf[20];
 
 	memset (k_ipad, 0x36, 64);
 	memset (k_opad, 0x5C, 64);
@@ -363,7 +363,7 @@ static int digest_sha1_update(struct digest *d, const void *data,
 {
 	struct sha1 *m = container_of(d, struct sha1, d);
 
-	sha1_update(&m->context, (unsigned char*)data, len);
+	sha1_update(&m->context, (uint8_t*)data, len);
 
 	return 0;
 }
-- 
1.7.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/2] sha1/sha256: use be32_to_cpu and cpu_to_be32

[Jean-Christophe PLAGNIOL-VILLARD]

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
 lib/sha1.c   |   20 +++-----------------
 lib/sha256.c |   19 +++----------------
 2 files changed, 6 insertions(+), 33 deletions(-)

diff --git a/lib/sha1.c b/lib/sha1.c
index 0e8aed1..b4e2abc 100644
--- a/lib/sha1.c
+++ b/lib/sha1.c
@@ -29,6 +29,7 @@
 #include <digest.h>
 #include <init.h>
 #include <linux/string.h>
+#include <asm/byteorder.h>
 
 #define SHA1_SUM_POS	-0x20
 #define SHA1_SUM_LEN	20
@@ -44,23 +45,8 @@ sha1_context;
 /*
  * 32-bit integer manipulation macros (big endian)
  */
-#ifndef GET_UINT32_BE
-#define GET_UINT32_BE(n,b,i) {				\
-	(n) = ( (uint32_t) (b)[(i)    ] << 24 )	\
-	    | ( (uint32_t) (b)[(i) + 1] << 16 )	\
-	    | ( (uint32_t) (b)[(i) + 2] <<  8 )	\
-	    | ( (uint32_t) (b)[(i) + 3]       );	\
-}
-#endif
-
-#ifndef PUT_UINT32_BE
-#define PUT_UINT32_BE(n,b,i) {				\
-	(b)[(i)    ] = (unsigned char) ( (n) >> 24 );	\
-	(b)[(i) + 1] = (unsigned char) ( (n) >> 16 );	\
-	(b)[(i) + 2] = (unsigned char) ( (n) >>  8 );	\
-	(b)[(i) + 3] = (unsigned char) ( (n)       );	\
-}
-#endif
+#define GET_UINT32_BE(n,b,i) (n) = be32_to_cpu(((uint32_t*)(b))[i / 4])
+#define PUT_UINT32_BE(n,b,i) ((uint32_t*)(b))[i / 4] = cpu_to_be32(n)
 
 /*
  * SHA-1 context setup
diff --git a/lib/sha256.c b/lib/sha256.c
index 78064da..975ebe9 100644
--- a/lib/sha256.c
+++ b/lib/sha256.c
@@ -22,6 +22,7 @@
 #include <digest.h>
 #include <init.h>
 #include <linux/string.h>
+#include <asm/byteorder.h>
 
 #define SHA256_SUM_LEN	32
 
@@ -34,22 +35,8 @@ typedef struct {
 /*
  * 32-bit integer manipulation macros (big endian)
  */
-#ifndef GET_UINT32_BE
-#define GET_UINT32_BE(n,b,i) {				\
-	(n) = ( (unsigned long) (b)[(i)    ] << 24 )	\
-	    | ( (unsigned long) (b)[(i) + 1] << 16 )	\
-	    | ( (unsigned long) (b)[(i) + 2] <<  8 )	\
-	    | ( (unsigned long) (b)[(i) + 3]       );	\
-}
-#endif
-#ifndef PUT_UINT32_BE
-#define PUT_UINT32_BE(n,b,i) {				\
-	(b)[(i)    ] = (unsigned char) ( (n) >> 24 );	\
-	(b)[(i) + 1] = (unsigned char) ( (n) >> 16 );	\
-	(b)[(i) + 2] = (unsigned char) ( (n) >>  8 );	\
-	(b)[(i) + 3] = (unsigned char) ( (n)       );	\
-}
-#endif
+#define GET_UINT32_BE(n,b,i) (n) = be32_to_cpu(((uint32_t*)(b))[i / 4])
+#define PUT_UINT32_BE(n,b,i) ((uint32_t*)(b))[i / 4] = cpu_to_be32(n)
 
 static void sha256_starts(sha256_context * ctx)
 {
-- 
1.7.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] sha1/sha256: use be32_to_cpu and cpu_to_be32

[Andre]

On 09/21/2010 06:28 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:

> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD<plagnioj at jcrosoft.com>
> ---
>   lib/sha1.c   |   20 +++-----------------
>   lib/sha256.c |   19 +++----------------
>   2 files changed, 6 insertions(+), 33 deletions(-)
>
> diff --git a/lib/sha1.c b/lib/sha1.c
> index 0e8aed1..b4e2abc 100644
> --- a/lib/sha1.c
> +++ b/lib/sha1.c
> @@ -29,6 +29,7 @@
>   #include<digest.h>
>   #include<init.h>
>   #include<linux/string.h>
> +#include<asm/byteorder.h>
>
>   #define SHA1_SUM_POS	-0x20
>   #define SHA1_SUM_LEN	20
> @@ -44,23 +45,8 @@ sha1_context;
>   /*
>    * 32-bit integer manipulation macros (big endian)
>    */
> -#ifndef GET_UINT32_BE
> -#define GET_UINT32_BE(n,b,i) {				\
> -	(n) = ( (uint32_t) (b)[(i)    ]<<  24 )	\
> -	    | ( (uint32_t) (b)[(i) + 1]<<  16 )	\
> -	    | ( (uint32_t) (b)[(i) + 2]<<   8 )	\
> -	    | ( (uint32_t) (b)[(i) + 3]       );	\
> -}
> -#endif
> -
> -#ifndef PUT_UINT32_BE
> -#define PUT_UINT32_BE(n,b,i) {				\
> -	(b)[(i)    ] = (unsigned char) ( (n)>>  24 );	\
> -	(b)[(i) + 1] = (unsigned char) ( (n)>>  16 );	\
> -	(b)[(i) + 2] = (unsigned char) ( (n)>>   8 );	\
> -	(b)[(i) + 3] = (unsigned char) ( (n)       );	\
> -}
> -#endif
> +#define GET_UINT32_BE(n,b,i) (n) = be32_to_cpu(((uint32_t*)(b))[i / 4])
> +#define PUT_UINT32_BE(n,b,i) ((uint32_t*)(b))[i / 4] = cpu_to_be32(n)
>
>    

The previous macros served two purposes: endian swapping and performing 
the memory accesses byte-by-byte. New versions are unsafe for CPUs which 
do not support misaligned 32bit memory accesses.

Andre
--



_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] sha1/sha256: use be32_to_cpu and cpu_to_be32

[Jean-Christophe PLAGNIOL-VILLARD]

On 00:00 Fri 24 Sep     , Andre wrote:
> On 09/21/2010 06:28 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> 
> >Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD<plagnioj at jcrosoft.com>
> >---
> >  lib/sha1.c   |   20 +++-----------------
> >  lib/sha256.c |   19 +++----------------
> >  2 files changed, 6 insertions(+), 33 deletions(-)
> >
> >diff --git a/lib/sha1.c b/lib/sha1.c
> >index 0e8aed1..b4e2abc 100644
> >--- a/lib/sha1.c
> >+++ b/lib/sha1.c
> >@@ -29,6 +29,7 @@
> >  #include<digest.h>
> >  #include<init.h>
> >  #include<linux/string.h>
> >+#include<asm/byteorder.h>
> >
> >  #define SHA1_SUM_POS	-0x20
> >  #define SHA1_SUM_LEN	20
> >@@ -44,23 +45,8 @@ sha1_context;
> >  /*
> >   * 32-bit integer manipulation macros (big endian)
> >   */
> >-#ifndef GET_UINT32_BE
> >-#define GET_UINT32_BE(n,b,i) {				\
> >-	(n) = ( (uint32_t) (b)[(i)    ]<<  24 )	\
> >-	    | ( (uint32_t) (b)[(i) + 1]<<  16 )	\
> >-	    | ( (uint32_t) (b)[(i) + 2]<<   8 )	\
> >-	    | ( (uint32_t) (b)[(i) + 3]       );	\
> >-}
> >-#endif
> >-
> >-#ifndef PUT_UINT32_BE
> >-#define PUT_UINT32_BE(n,b,i) {				\
> >-	(b)[(i)    ] = (unsigned char) ( (n)>>  24 );	\
> >-	(b)[(i) + 1] = (unsigned char) ( (n)>>  16 );	\
> >-	(b)[(i) + 2] = (unsigned char) ( (n)>>   8 );	\
> >-	(b)[(i) + 3] = (unsigned char) ( (n)       );	\
> >-}
> >-#endif
> >+#define GET_UINT32_BE(n,b,i) (n) = be32_to_cpu(((uint32_t*)(b))[i / 4])
> >+#define PUT_UINT32_BE(n,b,i) ((uint32_t*)(b))[i / 4] = cpu_to_be32(n)
> >
> 
> The previous macros served two purposes: endian swapping and
> performing the memory accesses byte-by-byte. New versions are unsafe
> for CPUs which do not support misaligned 32bit memory accesses.
be32_to_cpu and cpu_to_be32 are supposed to handle this

and if they need byte-by-byte you will handle there no in each file that need
to do it

Best Regards,
J.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] sha1/sha256: use be32_to_cpu and cpu_to_be32

[Sascha Hauer]

On Fri, Sep 24, 2010 at 12:00:42AM -0700, Andre wrote:
> On 09/21/2010 06:28 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>
>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD<plagnioj at jcrosoft.com>
>> ---
>>   lib/sha1.c   |   20 +++-----------------
>>   lib/sha256.c |   19 +++----------------
>>   2 files changed, 6 insertions(+), 33 deletions(-)
>>
>> diff --git a/lib/sha1.c b/lib/sha1.c
>> index 0e8aed1..b4e2abc 100644
>> --- a/lib/sha1.c
>> +++ b/lib/sha1.c
>> @@ -29,6 +29,7 @@
>>   #include<digest.h>
>>   #include<init.h>
>>   #include<linux/string.h>
>> +#include<asm/byteorder.h>
>>
>>   #define SHA1_SUM_POS	-0x20
>>   #define SHA1_SUM_LEN	20
>> @@ -44,23 +45,8 @@ sha1_context;
>>   /*
>>    * 32-bit integer manipulation macros (big endian)
>>    */
>> -#ifndef GET_UINT32_BE
>> -#define GET_UINT32_BE(n,b,i) {				\
>> -	(n) = ( (uint32_t) (b)[(i)    ]<<  24 )	\
>> -	    | ( (uint32_t) (b)[(i) + 1]<<  16 )	\
>> -	    | ( (uint32_t) (b)[(i) + 2]<<   8 )	\
>> -	    | ( (uint32_t) (b)[(i) + 3]       );	\
>> -}
>> -#endif
>> -
>> -#ifndef PUT_UINT32_BE
>> -#define PUT_UINT32_BE(n,b,i) {				\
>> -	(b)[(i)    ] = (unsigned char) ( (n)>>  24 );	\
>> -	(b)[(i) + 1] = (unsigned char) ( (n)>>  16 );	\
>> -	(b)[(i) + 2] = (unsigned char) ( (n)>>   8 );	\
>> -	(b)[(i) + 3] = (unsigned char) ( (n)       );	\
>> -}
>> -#endif
>> +#define GET_UINT32_BE(n,b,i) (n) = be32_to_cpu(((uint32_t*)(b))[i / 4])
>> +#define PUT_UINT32_BE(n,b,i) ((uint32_t*)(b))[i / 4] = cpu_to_be32(n)
>>
>>    
>
> The previous macros served two purposes: endian swapping and performing  
> the memory accesses byte-by-byte. New versions are unsafe for CPUs which  
> do not support misaligned 32bit memory accesses.

Indeed. We have get_unaligned_be32() / put_unaligned_be32(). These should be
the correct functions, right?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] sha1/sha256: use be32_to_cpu and cpu_to_be32

[Jean-Christophe PLAGNIOL-VILLARD]

On 09:43 Fri 24 Sep     , Sascha Hauer wrote:
> On Fri, Sep 24, 2010 at 12:00:42AM -0700, Andre wrote:
> > On 09/21/2010 06:28 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >
> >> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD<plagnioj at jcrosoft.com>
> >> ---
> >>   lib/sha1.c   |   20 +++-----------------
> >>   lib/sha256.c |   19 +++----------------
> >>   2 files changed, 6 insertions(+), 33 deletions(-)
> >>
> >> diff --git a/lib/sha1.c b/lib/sha1.c
> >> index 0e8aed1..b4e2abc 100644
> >> --- a/lib/sha1.c
> >> +++ b/lib/sha1.c
> >> @@ -29,6 +29,7 @@
> >>   #include<digest.h>
> >>   #include<init.h>
> >>   #include<linux/string.h>
> >> +#include<asm/byteorder.h>
> >>
> >>   #define SHA1_SUM_POS	-0x20
> >>   #define SHA1_SUM_LEN	20
> >> @@ -44,23 +45,8 @@ sha1_context;
> >>   /*
> >>    * 32-bit integer manipulation macros (big endian)
> >>    */
> >> -#ifndef GET_UINT32_BE
> >> -#define GET_UINT32_BE(n,b,i) {				\
> >> -	(n) = ( (uint32_t) (b)[(i)    ]<<  24 )	\
> >> -	    | ( (uint32_t) (b)[(i) + 1]<<  16 )	\
> >> -	    | ( (uint32_t) (b)[(i) + 2]<<   8 )	\
> >> -	    | ( (uint32_t) (b)[(i) + 3]       );	\
> >> -}
> >> -#endif
> >> -
> >> -#ifndef PUT_UINT32_BE
> >> -#define PUT_UINT32_BE(n,b,i) {				\
> >> -	(b)[(i)    ] = (unsigned char) ( (n)>>  24 );	\
> >> -	(b)[(i) + 1] = (unsigned char) ( (n)>>  16 );	\
> >> -	(b)[(i) + 2] = (unsigned char) ( (n)>>   8 );	\
> >> -	(b)[(i) + 3] = (unsigned char) ( (n)       );	\
> >> -}
> >> -#endif
> >> +#define GET_UINT32_BE(n,b,i) (n) = be32_to_cpu(((uint32_t*)(b))[i / 4])
> >> +#define PUT_UINT32_BE(n,b,i) ((uint32_t*)(b))[i / 4] = cpu_to_be32(n)
> >>
> >>    
> >
> > The previous macros served two purposes: endian swapping and performing  
> > the memory accesses byte-by-byte. New versions are unsafe for CPUs which  
> > do not support misaligned 32bit memory accesses.
> 
> Indeed. We have get_unaligned_be32() / put_unaligned_be32(). These should be
> the correct functions, right?
no-nned IIRC as be32_to_cpu and cpu_to_be32 already handle this
depending on the arch

Best Regards,
J.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] sha1/sha256: use be32_to_cpu and cpu_to_be32

[Andre]

On 09/24/2010 01:34 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 09:43 Fri 24 Sep     , Sascha Hauer wrote:
>> On Fri, Sep 24, 2010 at 12:00:42AM -0700, Andre wrote:
>>> On 09/21/2010 06:28 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>
>>>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD<plagnioj at jcrosoft.com>
>>>> ---
>>>>    lib/sha1.c   |   20 +++-----------------
>>>>    lib/sha256.c |   19 +++----------------
>>>>    2 files changed, 6 insertions(+), 33 deletions(-)
>>>>
>>>> diff --git a/lib/sha1.c b/lib/sha1.c
>>>> index 0e8aed1..b4e2abc 100644
>>>> --- a/lib/sha1.c
>>>> +++ b/lib/sha1.c
>>>> @@ -29,6 +29,7 @@
>>>>    #include<digest.h>
>>>>    #include<init.h>
>>>>    #include<linux/string.h>
>>>> +#include<asm/byteorder.h>
>>>>
>>>>    #define SHA1_SUM_POS	-0x20
>>>>    #define SHA1_SUM_LEN	20
>>>> @@ -44,23 +45,8 @@ sha1_context;
>>>>    /*
>>>>     * 32-bit integer manipulation macros (big endian)
>>>>     */
>>>> -#ifndef GET_UINT32_BE
>>>> -#define GET_UINT32_BE(n,b,i) {				\
>>>> -	(n) = ( (uint32_t) (b)[(i)    ]<<   24 )	\
>>>> -	    | ( (uint32_t) (b)[(i) + 1]<<   16 )	\
>>>> -	    | ( (uint32_t) (b)[(i) + 2]<<    8 )	\
>>>> -	    | ( (uint32_t) (b)[(i) + 3]       );	\
>>>> -}
>>>> -#endif
>>>> -
>>>> -#ifndef PUT_UINT32_BE
>>>> -#define PUT_UINT32_BE(n,b,i) {				\
>>>> -	(b)[(i)    ] = (unsigned char) ( (n)>>   24 );	\
>>>> -	(b)[(i) + 1] = (unsigned char) ( (n)>>   16 );	\
>>>> -	(b)[(i) + 2] = (unsigned char) ( (n)>>    8 );	\
>>>> -	(b)[(i) + 3] = (unsigned char) ( (n)       );	\
>>>> -}
>>>> -#endif
>>>> +#define GET_UINT32_BE(n,b,i) (n) = be32_to_cpu(((uint32_t*)(b))[i / 4])
>>>> +#define PUT_UINT32_BE(n,b,i) ((uint32_t*)(b))[i / 4] = cpu_to_be32(n)
>>>>
>>>>
>>>
>>> The previous macros served two purposes: endian swapping and performing
>>> the memory accesses byte-by-byte. New versions are unsafe for CPUs which
>>> do not support misaligned 32bit memory accesses.
>>
>> Indeed. We have get_unaligned_be32() / put_unaligned_be32(). These should be
>> the correct functions, right?
 >
> no-nned IIRC as be32_to_cpu and cpu_to_be32 already handle this
> depending on the arch
>

I think get_unaligned_be32() / put_unaligned_be32() are correct in this 
case. be32_to_cpu / cpu_to_be32 perform endian swapping (if required) 
with source and destination both being 32bit variables, not memory 
locations ?

Of course the easy way to test any version is to build for an 
architecture which cares about alignment and look at the disassembly. If 
the compiler generates one 32bit load/store instruction instead of 4 
byte accesses then the code is wrong.

In any case, this looks dubious:

#define PUT_UINT32_BE(n,b,i) ((uint32_t*)(b))[i / 4] = cpu_to_be32(n)

Behaviour when i == 0 is the same as when i == 1, which wasn't the case 
with the old macros. Also, if b is not 32bit aligned, store will be 
misaligned regardless of having cpu_to_be32(), or anything else, on the rhs.

Andre
--

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] sha1/sha256: use be32_to_cpu and cpu_to_be32

[Jean-Christophe PLAGNIOL-VILLARD]

On 03:16 Fri 24 Sep     , Andre wrote:
> On 09/24/2010 01:34 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >On 09:43 Fri 24 Sep     , Sascha Hauer wrote:
> >>On Fri, Sep 24, 2010 at 12:00:42AM -0700, Andre wrote:
> >>>On 09/21/2010 06:28 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >>>
> >>>>Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD<plagnioj at jcrosoft.com>
> >>>>---
> >>>>   lib/sha1.c   |   20 +++-----------------
> >>>>   lib/sha256.c |   19 +++----------------
> >>>>   2 files changed, 6 insertions(+), 33 deletions(-)
> >>>>
> >>>>diff --git a/lib/sha1.c b/lib/sha1.c
> >>>>index 0e8aed1..b4e2abc 100644
> >>>>--- a/lib/sha1.c
> >>>>+++ b/lib/sha1.c
> >>>>@@ -29,6 +29,7 @@
> >>>>   #include<digest.h>
> >>>>   #include<init.h>
> >>>>   #include<linux/string.h>
> >>>>+#include<asm/byteorder.h>
> >>>>
> >>>>   #define SHA1_SUM_POS	-0x20
> >>>>   #define SHA1_SUM_LEN	20
> >>>>@@ -44,23 +45,8 @@ sha1_context;
> >>>>   /*
> >>>>    * 32-bit integer manipulation macros (big endian)
> >>>>    */
> >>>>-#ifndef GET_UINT32_BE
> >>>>-#define GET_UINT32_BE(n,b,i) {				\
> >>>>-	(n) = ( (uint32_t) (b)[(i)    ]<<   24 )	\
> >>>>-	    | ( (uint32_t) (b)[(i) + 1]<<   16 )	\
> >>>>-	    | ( (uint32_t) (b)[(i) + 2]<<    8 )	\
> >>>>-	    | ( (uint32_t) (b)[(i) + 3]       );	\
> >>>>-}
> >>>>-#endif
> >>>>-
> >>>>-#ifndef PUT_UINT32_BE
> >>>>-#define PUT_UINT32_BE(n,b,i) {				\
> >>>>-	(b)[(i)    ] = (unsigned char) ( (n)>>   24 );	\
> >>>>-	(b)[(i) + 1] = (unsigned char) ( (n)>>   16 );	\
> >>>>-	(b)[(i) + 2] = (unsigned char) ( (n)>>    8 );	\
> >>>>-	(b)[(i) + 3] = (unsigned char) ( (n)       );	\
> >>>>-}
> >>>>-#endif
> >>>>+#define GET_UINT32_BE(n,b,i) (n) = be32_to_cpu(((uint32_t*)(b))[i / 4])
> >>>>+#define PUT_UINT32_BE(n,b,i) ((uint32_t*)(b))[i / 4] = cpu_to_be32(n)
> >>>>
> >>>>
> >>>
> >>>The previous macros served two purposes: endian swapping and performing
> >>>the memory accesses byte-by-byte. New versions are unsafe for CPUs which
> >>>do not support misaligned 32bit memory accesses.
> >>
> >>Indeed. We have get_unaligned_be32() / put_unaligned_be32(). These should be
> >>the correct functions, right?
> >
> >no-nned IIRC as be32_to_cpu and cpu_to_be32 already handle this
> >depending on the arch
> >
> 
> I think get_unaligned_be32() / put_unaligned_be32() are correct in
> this case. be32_to_cpu / cpu_to_be32 perform endian swapping (if
> required) with source and destination both being 32bit variables,
> not memory locations ?
no the arch have to handle this
> 
> Of course the easy way to test any version is to build for an
> architecture which cares about alignment and look at the
> disassembly. If the compiler generates one 32bit load/store
> instruction instead of 4 byte accesses then the code is wrong.
> 
> In any case, this looks dubious:
> 
> #define PUT_UINT32_BE(n,b,i) ((uint32_t*)(b))[i / 4] = cpu_to_be32(n)
> 
> Behaviour when i == 0 is the same as when i == 1, which wasn't the
> case with the old macros. Also, if b is not 32bit aligned, store
> will be misaligned regardless of having cpu_to_be32(), or anything
> else, on the rhs.
except here i is always a multiple of 4

Best Regards,
J.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] sha1/sha256: use be32_to_cpu and cpu_to_be32

[Sascha Hauer]

On Fri, Sep 24, 2010 at 01:00:45PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > >>>>-	    | ( (uint32_t) (b)[(i) + 3]       );	\
> > >>>>-}
> > >>>>-#endif
> > >>>>-
> > >>>>-#ifndef PUT_UINT32_BE
> > >>>>-#define PUT_UINT32_BE(n,b,i) {				\
> > >>>>-	(b)[(i)    ] = (unsigned char) ( (n)>>   24 );	\
> > >>>>-	(b)[(i) + 1] = (unsigned char) ( (n)>>   16 );	\
> > >>>>-	(b)[(i) + 2] = (unsigned char) ( (n)>>    8 );	\
> > >>>>-	(b)[(i) + 3] = (unsigned char) ( (n)       );	\
> > >>>>-}
> > >>>>-#endif
> > >>>>+#define GET_UINT32_BE(n,b,i) (n) = be32_to_cpu(((uint32_t*)(b))[i / 4])
> > >>>>+#define PUT_UINT32_BE(n,b,i) ((uint32_t*)(b))[i / 4] = cpu_to_be32(n)
> > >>>>
> > >>>>
> > >>>
> > >>>The previous macros served two purposes: endian swapping and performing
> > >>>the memory accesses byte-by-byte. New versions are unsafe for CPUs which
> > >>>do not support misaligned 32bit memory accesses.
> > >>
> > >>Indeed. We have get_unaligned_be32() / put_unaligned_be32(). These should be
> > >>the correct functions, right?
> > >
> > >no-nned IIRC as be32_to_cpu and cpu_to_be32 already handle this
> > >depending on the arch
> > >
> > 
> > I think get_unaligned_be32() / put_unaligned_be32() are correct in
> > this case. be32_to_cpu / cpu_to_be32 perform endian swapping (if
> > required) with source and destination both being 32bit variables,
> > not memory locations ?
> no the arch have to handle this

No. As Andre mentioned cpu_to_be32 operates on variables, not on
pointers. with ((uint32_t*)(b))[i / 4] you cast b to a pointer to
uint32_t which you dereference with [i / 4]. This has nothing to do
with cpu_to_be32 and will crash if the architecture does not allow
unaligned accesses.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] sha1/sha256: use be32_to_cpu and cpu_to_be32

[Jean-Christophe PLAGNIOL-VILLARD]

On 13:15 Fri 24 Sep     , Sascha Hauer wrote:
> On Fri, Sep 24, 2010 at 01:00:45PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > >>>>-	    | ( (uint32_t) (b)[(i) + 3]       );	\
> > > >>>>-}
> > > >>>>-#endif
> > > >>>>-
> > > >>>>-#ifndef PUT_UINT32_BE
> > > >>>>-#define PUT_UINT32_BE(n,b,i) {				\
> > > >>>>-	(b)[(i)    ] = (unsigned char) ( (n)>>   24 );	\
> > > >>>>-	(b)[(i) + 1] = (unsigned char) ( (n)>>   16 );	\
> > > >>>>-	(b)[(i) + 2] = (unsigned char) ( (n)>>    8 );	\
> > > >>>>-	(b)[(i) + 3] = (unsigned char) ( (n)       );	\
> > > >>>>-}
> > > >>>>-#endif
> > > >>>>+#define GET_UINT32_BE(n,b,i) (n) = be32_to_cpu(((uint32_t*)(b))[i / 4])
> > > >>>>+#define PUT_UINT32_BE(n,b,i) ((uint32_t*)(b))[i / 4] = cpu_to_be32(n)
> > > >>>>
> > > >>>>
> > > >>>
> > > >>>The previous macros served two purposes: endian swapping and performing
> > > >>>the memory accesses byte-by-byte. New versions are unsafe for CPUs which
> > > >>>do not support misaligned 32bit memory accesses.
> > > >>
> > > >>Indeed. We have get_unaligned_be32() / put_unaligned_be32(). These should be
> > > >>the correct functions, right?
> > > >
> > > >no-nned IIRC as be32_to_cpu and cpu_to_be32 already handle this
> > > >depending on the arch
> > > >
> > > 
> > > I think get_unaligned_be32() / put_unaligned_be32() are correct in
> > > this case. be32_to_cpu / cpu_to_be32 perform endian swapping (if
> > > required) with source and destination both being 32bit variables,
> > > not memory locations ?
> > no the arch have to handle this
> 
> No. As Andre mentioned cpu_to_be32 operates on variables, not on
> pointers. with ((uint32_t*)(b))[i / 4] you cast b to a pointer to
> uint32_t which you dereference with [i / 4]. This has nothing to do
> with cpu_to_be32 and will crash if the architecture does not allow
> unaligned accesses.
except cpu_to_be 32 do a swapb so it's safe

Best Regards,
J.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] sha1/sha256: use be32_to_cpu and cpu_to_be32

[Sascha Hauer]

On Fri, Sep 24, 2010 at 01:43:50PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 13:15 Fri 24 Sep     , Sascha Hauer wrote:
> > On Fri, Sep 24, 2010 at 01:00:45PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > >>>>-	    | ( (uint32_t) (b)[(i) + 3]       );	\
> > > > >>>>-}
> > > > >>>>-#endif
> > > > >>>>-
> > > > >>>>-#ifndef PUT_UINT32_BE
> > > > >>>>-#define PUT_UINT32_BE(n,b,i) {				\
> > > > >>>>-	(b)[(i)    ] = (unsigned char) ( (n)>>   24 );	\
> > > > >>>>-	(b)[(i) + 1] = (unsigned char) ( (n)>>   16 );	\
> > > > >>>>-	(b)[(i) + 2] = (unsigned char) ( (n)>>    8 );	\
> > > > >>>>-	(b)[(i) + 3] = (unsigned char) ( (n)       );	\
> > > > >>>>-}
> > > > >>>>-#endif
> > > > >>>>+#define GET_UINT32_BE(n,b,i) (n) = be32_to_cpu(((uint32_t*)(b))[i / 4])
> > > > >>>>+#define PUT_UINT32_BE(n,b,i) ((uint32_t*)(b))[i / 4] = cpu_to_be32(n)
> > > > >>>>
> > > > >>>>
> > > > >>>
> > > > >>>The previous macros served two purposes: endian swapping and performing
> > > > >>>the memory accesses byte-by-byte. New versions are unsafe for CPUs which
> > > > >>>do not support misaligned 32bit memory accesses.
> > > > >>
> > > > >>Indeed. We have get_unaligned_be32() / put_unaligned_be32(). These should be
> > > > >>the correct functions, right?
> > > > >
> > > > >no-nned IIRC as be32_to_cpu and cpu_to_be32 already handle this
> > > > >depending on the arch
> > > > >
> > > > 
> > > > I think get_unaligned_be32() / put_unaligned_be32() are correct in
> > > > this case. be32_to_cpu / cpu_to_be32 perform endian swapping (if
> > > > required) with source and destination both being 32bit variables,
> > > > not memory locations ?
> > > no the arch have to handle this
> > 
> > No. As Andre mentioned cpu_to_be32 operates on variables, not on
> > pointers. with ((uint32_t*)(b))[i / 4] you cast b to a pointer to
> > uint32_t which you dereference with [i / 4]. This has nothing to do
> > with cpu_to_be32 and will crash if the architecture does not allow
> > unaligned accesses.
> except cpu_to_be 32 do a swapb so it's safe

cpu_to_be32 does a __swap32 and not a swapb.

And I can only repeat myself: The problem is *not* cpu_to_be32 but the
argument you pass *to* cpu_to_be32. Let me put it in other words:

x = be32_to_cpu(((uint32_t*)(b))[i / 4])

can be written as

u32 y = ((uint32_t*)(b))[i / 4];
x = be32_to_cpu(y);

which can also be written as:

u32 *z = (uint32_t *)b;
u32 y = z[i / 4];
x = be32_to_cpu(y);

And this will crash in the first line if b is not aligned.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] sha1/sha256: use be32_to_cpu and cpu_to_be32

[Sascha Hauer]

On Fri, Sep 24, 2010 at 02:52:55PM +0200, Sascha Hauer wrote:
> On Fri, Sep 24, 2010 at 01:43:50PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 13:15 Fri 24 Sep     , Sascha Hauer wrote:
> > > On Fri, Sep 24, 2010 at 01:00:45PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > >>>>-	    | ( (uint32_t) (b)[(i) + 3]       );	\
> > > > > >>>>-}
> > > > > >>>>-#endif
> > > > > >>>>-
> > > > > >>>>-#ifndef PUT_UINT32_BE
> > > > > >>>>-#define PUT_UINT32_BE(n,b,i) {				\
> > > > > >>>>-	(b)[(i)    ] = (unsigned char) ( (n)>>   24 );	\
> > > > > >>>>-	(b)[(i) + 1] = (unsigned char) ( (n)>>   16 );	\
> > > > > >>>>-	(b)[(i) + 2] = (unsigned char) ( (n)>>    8 );	\
> > > > > >>>>-	(b)[(i) + 3] = (unsigned char) ( (n)       );	\
> > > > > >>>>-}
> > > > > >>>>-#endif
> > > > > >>>>+#define GET_UINT32_BE(n,b,i) (n) = be32_to_cpu(((uint32_t*)(b))[i / 4])
> > > > > >>>>+#define PUT_UINT32_BE(n,b,i) ((uint32_t*)(b))[i / 4] = cpu_to_be32(n)
> > > > > >>>>
> > > > > >>>>
> > > > > >>>
> > > > > >>>The previous macros served two purposes: endian swapping and performing
> > > > > >>>the memory accesses byte-by-byte. New versions are unsafe for CPUs which
> > > > > >>>do not support misaligned 32bit memory accesses.
> > > > > >>
> > > > > >>Indeed. We have get_unaligned_be32() / put_unaligned_be32(). These should be
> > > > > >>the correct functions, right?
> > > > > >
> > > > > >no-nned IIRC as be32_to_cpu and cpu_to_be32 already handle this
> > > > > >depending on the arch
> > > > > >
> > > > > 
> > > > > I think get_unaligned_be32() / put_unaligned_be32() are correct in
> > > > > this case. be32_to_cpu / cpu_to_be32 perform endian swapping (if
> > > > > required) with source and destination both being 32bit variables,
> > > > > not memory locations ?
> > > > no the arch have to handle this
> > > 
> > > No. As Andre mentioned cpu_to_be32 operates on variables, not on
> > > pointers. with ((uint32_t*)(b))[i / 4] you cast b to a pointer to
> > > uint32_t which you dereference with [i / 4]. This has nothing to do
> > > with cpu_to_be32 and will crash if the architecture does not allow
> > > unaligned accesses.
> > except cpu_to_be 32 do a swapb so it's safe
> 
> cpu_to_be32 does a __swap32 and not a swapb.
> 
> And I can only repeat myself: The problem is *not* cpu_to_be32 but the
> argument you pass *to* cpu_to_be32. Let me put it in other words:
> 
> x = be32_to_cpu(((uint32_t*)(b))[i / 4])
> 
> can be written as
> 
> u32 y = ((uint32_t*)(b))[i / 4];
> x = be32_to_cpu(y);
> 
> which can also be written as:
> 
> u32 *z = (uint32_t *)b;
> u32 y = z[i / 4];
> x = be32_to_cpu(y);
> 
> And this will crash in the first line if b is not aligned.

I mean in the second line because that's where the pointer is
dereferenced.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox