intel / isa-l_crypto

Other
275 stars 80 forks source link

Add support for big-endian architectures #58

Closed uweigand closed 4 years ago

uweigand commented 4 years ago

Create a new header endian_helper.h as a central place for byte-swap and endian helper routines:

Replace all existing byte-swap routines throughout the code base by the appropriate endian helper routine. Usually, this is to_be32 / to_be64, but in a few places in sm3_mb we need to swap unconditionally to convert the internal big-endian digest to the externally expected little-endian digest format.

Finally, add the required to_le32 / to_le64 calls in places where byte-swaps were missing, but are required on big-endian machines.

Signed-off-by: Ulrich Weigand ulrich.weigand@de.ibm.com

gbtucker commented 4 years ago

Looks ok but I think the convert structs in the reference and base functions were to satisfy unaligned arch like arm32. Without it we need an unaligned cast. @haoyu01, do you guys care about unaligned arch?

jiaqizho commented 4 years ago

Hi

In SM3 specification , defined output result must as big-endian.

As you said : "the externally expected little-endian digest format". Actully , little-endian output maybe more simple(can reduce endian-convert) , but as a standard algorithm, it have to output as big-endian.

yuhaoth commented 4 years ago

Looks ok but I think the convert structs in the reference and base functions were to satisfy unaligned arch like arm32. Without it we need an unaligned cast. @haoyu01, do you guys care about unaligned arch?

We do not care about that.

uweigand commented 4 years ago

Looks ok but I think the convert structs in the reference and base functions were to satisfy unaligned arch like arm32. Without it we need an unaligned cast. @haoyu01, do you guys care about unaligned arch?

If you prefer I could change this to use memcpy, e.g. like so:

    uint64_t convert_len = to_le64((uint64_t) ctx->total_length * 8);
    memcpy(buf + i - 8, &convert_len, 8);

Should still be optimized to the same code when possible by most compilers, and would be on the safe side alignment-wise.

uweigand commented 4 years ago

Hi

In SM3 specification , defined output result must as big-endian.

As you said : "the externally expected little-endian digest format". Actully , little-endian output maybe more simple(can reduce endian-convert) , but as a standard algorithm, it have to output as big-endian.

I do believe the current code (without my changes) always yields a little-endian output, which my patch preserves now also on big-endian platforms. As far as I can see, the algorithm itself indeed runs in big-endian mode throughout (with inputs converted to big-endian), but at the end there is a final step that converts the output from big-endian to little-endian: https://github.com/intel/isa-l_crypto/blob/master/sm3_mb/sm3_ctx_base.c#L251

As an example, according to https://8gwifi.org/MessageDigest.jsp the string "abc" has a SM3 hash of: Algo sm3 Hex Encoded [66c7f0f462eeedd9d1f2d46bdc10e4e24167c4875cf2f7a2297da02b8f4ba8e0] and the sm3_mb_test.c file verifies the output against this expected result:

         .resultDigest = {0xf4f0c766, 0xd9edee62, 0x6bd4f2d1, 0xe2e410dc,
                          0x87c46741, 0xa2f7f25c, 0x2ba07d29, 0xe0a84b8f}

which is the little-endian word representation of the above byte string.

Interestingly, in the sm3_ref_test.c file, the "expected output" for the same string is given in big-endian order:

static digest_sm3 exp_result_digest1 = { 0x66c7f0f4, 0x62eeedd9, 0xd1f2d46b, 0xdc10e4e2,
        0x4167c487, 0x5cf2f7a2, 0x297da02b, 0x8f4ba8e0
};

but those values are then actually byteswapped before being compared to the sm3_ctxmgr... result: https://github.com/intel/isa-l_crypto/blob/master/sm3_mb/sm3_ref_test.c#L92

jiaqizho commented 4 years ago

In my view , char[] text = “abc” , in memory is “00 , 63 , 62 , 61” (it means our CPU is little-endian).

in sm3_ref_test.c , exp_result_digest1 should be big-endian output, but in memory it is little endian(because CPU is little-endian) , so we need byteswap which can change it from little-endian to big-endian(in memory)

that is why we used byteswap in sm3_ref_test.

But if CPU is big-endian , “abc” in memory is “61 , 62 , 63 , 00” , I think our algorithm won’t work in that case .

Actually , we don’t have big-endian CPU anymore.

jiaqizho commented 4 years ago

in other words , in sm3_mb.h when user call sm3_ctx_mgr_submit , i think the result should not be endian change. so my point is you should change back sm3 output endian :)

otherwise , thanks again , your patch is very nice for me

uweigand commented 4 years ago

In my view , char[] text = “abc” , in memory is “00 , 63 , 62 , 61” (it means our CPU is little-endian).

I'm not sure I understand this; a char array is unaffected by endianness, char[] text = "abc" will always be "0x61 0x62 0x63 0x00" on any platform. Only the int array that the library uses to hold the digest result is affected by endianness.

in sm3_ref_test.c , exp_result_digest1 should be big-endian output, but in memory it is little endian(because CPU is little-endian) , so we need byteswap which can change it from little-endian to big-endian(in memory)

that is why we used byteswap in sm3_ref_test.

In the example above, the SM3 digest (as defined as a sequence of bytes) of the string "abc" is 0x66 0xc7 0xf0 0xf4 ... This again holds true on any platform, big-endian or little-endian. Only once you interpret the digest not a sequence of bytes, but rather as a sequence of integers, does the byte order make any difference. If the digest were interpreted as big-endian words, its first word would by 0x66c7f0f4. If it were interpreted as little-endian words, its first word would be 0xf4f0c766.

And in fact, with the current code running on a little-endian platform, the first word of digests currently returned is 0xf4f0c766. This is unchanged by my patch; after the patch is applied the digest of "abc" still has a first word of 0xf4f0c766, both on little-endian and on big-endian platforms.

in other words , in sm3_mb.h when user call sm3_ctx_mgr_submit , i think the result should not be endian change. so my point is you should change back sm3 output endian :)

Just to clarify again, my patch does not change anything about the output byte order. It returns the digest in the same byte order as it was before, which is little-endian.

gbtucker commented 4 years ago

OK, I think @jiaqizho objection was about your internal interpretation of the spec but it looks like the external API is interpreted the same for both big and little endian if we consider the in-memory order to be the standard. For example, no extra byteswap is necessary depending on platform. Then this is consistent and probably how we should keep it. So if there is no objection I'll integrate this as-is.

gbtucker commented 4 years ago

Integrated