fix violation in crypto lib by GangSecurity · Pull Request #1 · GangSecurity/acrn-hypervisor

GangSecurity · 2018-12-27T08:10:17Z

No description provided.

lifeix · 2018-12-27T08:50:59Z

hypervisor/lib/crypto/crypto_api.c

+					salt, salt_len,
+					secret, secret_len,
+					info, info_len,
+					out_key, out_len) != 0) {


out_key, out_len) != 0) {
ret = 1;

changed the code

lifeix · 2018-12-27T08:51:22Z

hypervisor/lib/crypto/crypto_api.c

+			if (mbedtls_md_hmac(md,
+					secret, secret_len,
+					salt, salt_len,
+					out_key) != 0) {


lifeix · 2018-12-27T08:55:23Z

hypervisor/lib/crypto/mbedtls/hkdf.c

+                    uint8_t c = (uint8_t)(i & 0xffU);
+
+                    ret = mbedtls_md_hmac_starts( &ctx, prk, prk_len );
+                    if ( ret != 0 ) {


IMHO, could we change this style to
if (ret == 0) {
ret = ...;
}

modified them

lifeix · 2018-12-27T08:56:00Z

hypervisor/lib/crypto/mbedtls/hkdf.c

+                        break;
+                    }
+
+                    num_to_copy = (i != n) ? hash_len : (okm_len - where);


then break here if (ret != 0)

modified it

lifeix · 2018-12-27T08:57:25Z

hypervisor/lib/crypto/mbedtls/hkdf.c

    }

-exit:
    mbedtls_md_free( &ctx );


the original logic return MBEDTLS_ERR_HKDF_BAD_INPUT_DATA will not do these ?

lifeix · 2018-12-27T08:58:30Z

hypervisor/lib/crypto/mbedtls/sha256.c

 void mbedtls_sha256_free( mbedtls_sha256_context *ctx )
 {
-    if( ctx == NULL )
+    if ( ctx == NULL )


change it to
if (ctx != NULL)

lifeix · 2018-12-27T08:59:59Z

hypervisor/lib/crypto/mbedtls/sha256.c

        P( A[3], A[4], A[5], A[6], A[7], A[0], A[1], A[2], R(i+5), K[i+5] );
        P( A[2], A[3], A[4], A[5], A[6], A[7], A[0], A[1], R(i+6), K[i+6] );
        P( A[1], A[2], A[3], A[4], A[5], A[6], A[7], A[0], R(i+7), K[i+7] );
+


Why do this change ?

" for( i = 0; i < 16; i += 8 )" violate "271S: For loop incrementation is not simple."

lifeix · 2018-12-27T09:08:01Z

hypervisor/lib/crypto/mbedtls/md.c

+    }

-    return( md_info->digest_func( input, ilen, output ) );
+    return( ret );


return ret ?

some functions are removed due to not used. So the patch seems hard to review. The last function is here.
219 uint8_t mbedtls_md_get_size( const mbedtls_md_info_t *md_info )
220 {
221 uint8_t ret = 0U;
222
223 if ( md_info != NULL ) {
224 ret = (uint8_t) md_info->size;
225 }
226
227 return ( ret );
228 }

bingzhux · 2018-12-27T09:08:44Z

hypervisor/lib/crypto/mbedtls/sha256.c

+
+    ctx->buffer[used] = 0x80U;

-    ctx->buffer[used++] = 0x80;


haha, this is very bad use...

bingzhux · 2018-12-27T09:10:05Z

hypervisor/lib/crypto/mbedtls/sha256.c

+        if (ret == 0) {
+            memset( ctx->buffer, 0U, 56U );
+        } else {
+            ret = 1;


why 1 here?

how about just do nothing?

yes, will resolve it

bingzhux

see comments inline

shiqingg · 2018-12-28T06:46:04Z

hypervisor/lib/crypto/crypto_api.c

+					salt, salt_len,
+					secret, secret_len,
+					info, info_len,
+					out_key, out_len) != 0) {


Please merge these lines into one line and kindly apply this rule to the whole patch.
The line limit is 120 columns instead of 80 columns for ACRN.

shiqingg · 2018-12-28T06:54:59Z

hypervisor/lib/crypto/mbedtls/hkdf.c

-    if( ret == 0 )
-    {
-        ret = mbedtls_hkdf_expand( md, prk, mbedtls_md_get_size( md ),
+    if ( ret == 0 ) {


Please remove the unnecessary space before and after brackets and kindly apply this rule to the whole patch.
if (ret == 0)

All the code was ported from 3rd lib, the code style is different from acrn. But anyway, I will modify them since so many codes are changed.

shiqingg · 2018-12-28T06:58:51Z

hypervisor/lib/crypto/mbedtls/hkdf.c

+                 * Compute T = T(1) | T(2) | T(3) | ... | T(N)
+                 * Where T(N) is defined in RFC 5869 Section 2.3
+                 */
+                for ( i = 1; i <= n; i++ ) {


use 1U since i is unsigned.

shiqingg · 2018-12-28T07:00:13Z

hypervisor/lib/crypto/mbedtls/hkdf.c

+                 */
+                for ( i = 1; i <= n; i++ ) {
+                    size_t num_to_copy;
+                    uint8_t c = (uint8_t)(i & 0xffU);


change to
uint8_t c = (uint8_t)i;

shiqingg · 2018-12-28T07:04:22Z

hypervisor/lib/crypto/mbedtls/hkdf.c

+                    /* The constant concatenated to the end of each T(n) is a single octet.
+                     * */
+                    if ( ret == 0 ) {
+                        ret = mbedtls_md_hmac_update( &ctx, &c, 1 );


1U
Please check all the constant usages in your patches.
U for uint8_t, uint16_t, uint32_t, size_t
UL for uint64_t

shiqingg · 2018-12-28T07:12:07Z

hypervisor/lib/crypto/mbedtls/md.c

+
+            for( i = 0U; i < keylen; i++ ) {
+                *( ipad + i ) = (uint8_t)( *( ipad + i ) ^ *( temp_key + i ) );
+                *( opad + i ) = (uint8_t)( *( opad + i ) ^ *( temp_key + i ) );


change to
*(opad + i) = (uint8_t)((*(opad + i)) ^ (*(temp_key + i)));

same as previous, done

shiqingg · 2018-12-28T07:17:32Z

hypervisor/lib/crypto/mbedtls/sha256.c

-#define  SHR(x,n) ((x & 0xFFFFFFFF) >> n)
-#define ROTR(x,n) (SHR(x,n) | (x << (32 - n)))
+#define  SHR(x,n) (((x) & 0xFFFFFFFF) >> (n))
+#define ROTR(x,n) (SHR((x),(n)) | ((x) << (32 - (n))))


Please use inline functions rather than MACRO for these function-like MACRO. Same for the others.

I'd like to keep the original code what it is since all the code are ported from 3rd and this is only advisory rule.

shiqingg · 2018-12-28T07:25:42Z

hypervisor/lib/crypto/mbedtls/sha256.c

        P( A[2], A[3], A[4], A[5], A[6], A[7], A[0], A[1], W[i+6], K[i+6] );
        P( A[1], A[2], A[3], A[4], A[5], A[6], A[7], A[0], W[i+7], K[i+7] );
+
+        i += 8;


Moving the loop counter inside loop body violates another rule.
Modification of loop counter in loop body.
A loop counter used within a for loop for iteration shall not be modified in the body of the loop. Modification of the loop counter makes code difficult to read and maintain.

One suggestion:
add a local variable inside loop body to get the right index from the loop counter.

what is about keep the original code? I don't think it is complex code.

shiqingg · 2018-12-28T07:27:41Z

hypervisor/lib/crypto/mbedtls/sha256.c

+            ret = mbedtls_internal_sha256_process( ctx, ctx->buffer );
+            if (ret == 0) {
+                data += fill;
+                len  -= fill;


remove the extra space, same for the others

shiqingg · 2018-12-28T07:52:47Z

hypervisor/lib/crypto/mbedtls/sha256.c

@@ -110,64 +106,65 @@ int32_t mbedtls_sha256_starts_ret( mbedtls_sha256_context *ctx, int32_t is224 )

 static const uint32_t K[] =


it is better to use lower case for variable name

I'd like to keep it.

-remove goto -remove multiple return -Modify assignment operator in boolean expression -Modify/fix code style violations -fix attempt to change parameters passed by value -fix value need U suffix -fix use of mixed arithmetic -fix assigment in expression -other fix Tracked-On: projectacrn#861 Signed-off-by: Chen Gang G <gang.g.chen@intel.com> Reviewed-by: Bing Zhu <bing.zhu@intel.com> Acked-by: Eddie Dong <eddie.dong@intel.com>

-remove goto -remove multiple return -Modify assignment operator in boolean expression -Modify/fix code style violations -fix attempt to change parameters passed by value -fix value need U suffix -fix use of mixed arithmetic -fix assigment in expression -other fixes Tracked-On: projectacrn#861 Signed-off-by: Chen Gang G <gang.g.chen@intel.com> Reviewed-by: Bing Zhu <bing.zhu@intel.com> Acked-by: Eddie Dong <eddie.dong@intel.com>

lifeix reviewed Dec 27, 2018

View reviewed changes

hypervisor/lib/crypto/crypto_api.c Outdated

if (mbedtls_md_hmac(md,

secret, secret_len,

salt, salt_len,

out_key) != 0) {

Copy link

lifeix Dec 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

lifeix reviewed Dec 27, 2018

View reviewed changes

bingzhux reviewed Dec 27, 2018

View reviewed changes

GangSecurity force-pushed the gb1 branch from 0cee3f6 to 44dfb56 Compare December 28, 2018 05:49

shiqingg reviewed Dec 28, 2018

View reviewed changes

GangSecurity force-pushed the gb1 branch from 44dfb56 to 94bb32b Compare December 29, 2018 02:21

bingzhux approved these changes Jan 2, 2019

View reviewed changes

GangSecurity force-pushed the gb1 branch from 94bb32b to f45f218 Compare January 2, 2019 01:35

bingzhux mentioned this pull request Jan 2, 2019

fix MISRA-c violations for crypto lib projectacrn/acrn-hypervisor#2223

Closed

GangSecurity added 3 commits January 2, 2019 15:34

GangSecurity force-pushed the gb1 branch from f45f218 to da78ed4 Compare January 2, 2019 07:41

		@@ -110,64 +106,65 @@ int32_t mbedtls_sha256_starts_ret( mbedtls_sha256_context *ctx, int32_t is224 )

		static const uint32_t K[] =

Conversation

GangSecurity commented Dec 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bingzhux left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!