tests/sys_crypto: remove use of BOARD_BLACKLIST variable

[aabadie]

Contribution description

This PR tries to get rid of the BOARD_BLACKLIST variable in tests/sys_crypto application. The approach is to try to fix the integer length issues raised when building on 8/16 bit architectures instead of just blacklisting the corresponding features.

While doing, I think I found a potential bug in one of the decrypt function, see 4d214cc. On 8/16 bit architectures, the compiler was failing with "error: comparison is always false due to limited range of data type". Which makes sense when looking at the code.

After all these integer length issues were fixed, it was just a matter of moving some boards to the insufficient memory variable.

The test application now builds fine on some msp430 and AVR based boards.

I haven't tested the test script on MSP430/AVR8 boards because I don't have the hardware with me.

Testing procedure

The following commands should build and work fine (if test is supported):

$ make BOARD=z1 -C test/sys_crypto flash test
$ make BOARD=mega-xplained -C test/sys_crypto flash test
$ make BOARD=microduino-corerf -C test/sys_crypto flash test

Issues/PRs references

None

[aabadie]

I could test on an 8bit board (from #12639) and one of the tests is broken.

[aabadie]

With the latest version of this PR, tests/sys_crypto works on AVR. Needs careful review though because it's crypto code and because it contains a small API change (I haven't found anything better).

[bergzand]

What's the exact error on the AVR platform when using a size_t for the input length?

[aabadie]

What's the exact error on the AVR platform when using a size_t for the input length?

The problem is raising when testing upper bounds of the input_len parameter. If it's above 65535, an error is returned. The problem on 8/16bit architecture is that size_t is not a 32 bit integer, so the parameter value is 0 in this case, resulting in a valid value returned.
That's why using an input_len type with explicit length fixes the issue.

[aabadie]

Here is the issue on AVR:

.........................
crypto_modes_ccm_tests.test_crypto_modes_ccm_check_len (tests/sys_crypto/tests-crypto-modes-ccm.c 1015) exp -4 was 8
.........
run 34 failures 1

The comes from the following function

RIOT/sys/crypto/modes/ccm.c

Line 139 in 4d279ae

static inline int _fits_in_nbytes(size_t value, uint8_t num_bytes)

that is not returning 0 when it should e.g. when passing it a value of 65536 (UINT16_MAX + 1): on non 32bit architecture this value is converted to 0 because of the use of size_t.

Do you have suggestions for a better fix ?

[bergzand]

I strongly prefer to keep the length field here size_t. My main reason here is that size_t is meant to be used for sizes of arrays etc…, the result of a sizeof() call is of type size_t, exactly for what it is used here.

Do you have suggestions for a better fix ?

I'm wondering if the failing tests are valid for the platforms where size_t is 16 bit.

Rationale:

CCM mode contains a variable length field (length_encoding) containing the length in octets of the message. This variable length field is between 2 - 8 octets in size. The implementation checks if the supplied message length fits within the length_encoding octets. For the Cortex-M platforms, size_t is 32 bit wide, it could be enough to have length_encoding == 2, but it might require more octets for an arbitrary message. The failing test here validates whether the crypto algo correctly rejects the encrypt/decrypt call when the message size and the length_encoding argument are incompatible.

On the AVR platform (and probably also on the 16 bit platforms) size_t is 16 bit. The minimum length for length_encoding of 2 octets is thus always able to contain the maximum message size. The encrypt/decrypt call can not fail based on the length_encoding being too small for the message size.

What I would do is wrap the tests in:

/* Tests only required when the length_encoding could require more than 2 octets */
`#if SIZE_MAX > UINT16_MAX` 

And maybe add an test to verify that the call returns an error when length_encoding is smaller than 2.

[aabadie]

What I would do is wrap the tests in:

/* Tests only required when the length_encoding could require more than 2 octets */
#if SIZE_MAX > UINT16_MAX

And maybe add an test to verify that the call returns an error when length_encoding is smaller than 2.

Sounds good, I'll update the PR accordingly.

[aabadie]

@bergzand, changed the PR following your suggestion. Directly rebased and squashed. Also tested on atmega256rfr2-xpro and now it works.

[bergzand]

ACK¸ I haven't tested it personally, but I'm willing to trust your tests for this. Changes look good.

[aabadie]

@bergzand, may I squash this one ?

[maribu]

On the ATmega1284P I get:

main(): This is RIOT! (Version: 2020.01-devel-768-g7aace-alex)
..................................
OK (34 tests)

[maribu]

@bergzand: I think this is ready to go in 😉

[aabadie]

@bergzand, OK to squash ?

[bergzand]

@bergzand, OK to squash ?

Yes, please squash

[aabadie]

Done!

[bergzand]

Ack, changes look good.

[aabadie]

Thanks for reviewing and testing @bergzand @maribu!