feat: use nrf52 hardware crypto where we can

[NickDunklee]

First PR, advert-only

While working on some sensor code implementations, I ran into some hard crashes that root-caused to the 4KB loop task stack being exhausted.

Looking for various optimization schemes resulted in this relatively low-lift fix. RAK3401 and RAK4631 both support hardware crypto. Edit: Heltec t096 and Seeed t1000-e support this as well, so I have tested them and added them.

Rather than loading in one of the two software crypto libs, we can just use the onboard hardware. This is faster, should consume less power, and in testing used a scant up to 700 bytes in the run loop vs 2.5-3KB per advert.

This change only affects advert verification processing, which currently consumes a significant chunk of the 4KB run loop.

I figured such a change should likely be implemented in phases.

After soaking, this hardware crypto verification
process could be implemented across the entire MeshCore cryptographic function on RAK nodes. Also possible other nodes have available hardware crypto, however, I have not checked, so future improvements may also exist there.

Tested on:

• RAK3401 RAK 1W
• RAK4631 19001
• Heltec t096
• Seeed t1000-e

Addendum, more crypto added:

feat: add more crypto

After soaking for a bit on the adverts without issue on multiple nodes, I added more hardware crypto. While I haven't a chance to do a debug comparison of the run loop like I did with the advert code, moving more crypto out of software and into hardware should further reduce the memory footprint in the run loop by a couple to three hundred bytes which should contribute to overall stability, especially in devices with sensors/GPS consuming more run loop memory. (Possibly some minor performance and/or power improvements.)

Supported nodes is unchanged in this PR addition, but if others can verify, they can easily be added.

Some info on the CC310: https://docs.nordicsemi.com/r/bundle/ps_nrf9151/page/cryptocell.html

Added:

• AES-128 packet encryption/decryption now use hardware crypto
• HMAC-SHA-256 authentication now uses hardware crypto
• ACK hash computation and channel ID derivation now use hardware crypto
• RNG (random number generator) now uses hardware crypto rather than radio noise + weak software RNG (which can have issues if there's no surrounding radio noise.) NIST SP 800-90B certified.
  • Runs hardware self-tests on startup
  • Runs continuous health tests during operation
  • Uses thermal noise/shot noise for randomness

Unchanged:

• calcSharedSecret remains software - it would be a split hw/sw solution and added complexity for likely not a lot of gains. This only happens when establishing a new contact, so not too frequent to be worth it.
• ed25519_create_keypair remains software. This is only called when a node is first initialized. It does use the hardware RNG change, however, so better randomization.

Tested on (so far):

• Heltec t096 ble companion
• rak19001 sensor
• seeed t1000-e companion
• rak19007 repeater

@Magalex2x14 tested on:

• RAK4631 companion
• RAK3401 repeater
• WisMesh Tag
• Build test with Lilligo T-Beam Supreme SX1262

@entr0p1 tested on:

• Xiao nRF52840

Build test on:

• Heltec t096 companion ble
• t1000e companion ble
• RAK 4631 repeater
• RAK 3401 companion BLE
• Heltec v3 companion wifi

[lbibass]

Shouldn't this work on every NRF52840?

[NickDunklee]

@lbibass very likely! Although anything's possible to break with different hardware designs. So I added types that I could confirm work thus far. If you or anyone else can confirm this works on other node types, happy to add them.

[entr0p1]

This is really cool. I'll try to remember to give it a test on some other boards and see how it plays out.

[Magalex2x14]

I just wanted to say that I flashed my RAK4631 companion and RAK3401 repeater with this code - I don’t see any problems. Nice PR!

[Magalex2x14]

Also tested with WisMesh Tag - I don’t see any problems.
Build test with Lilligo T-Beam Supreme SX1262.

[entr0p1]

Tested on Xiao nRF52840 (Repeater) - adverts sent, parse successfully on the receiving side. I'll test some more boards tomorrow if I can get time.

Great work :)

[entr0p1]

Worth noting - given the CC310 is in the nRF52840 MCU itself (and much much more), with enough board tests I think we shouldn't gate it by board and have it globally enabled. The maintainers might have a different view, but to keep things consistent that'd be my $0.02.

Further tested boards - all OK:

• GAT562 30s Mesh Kit (Companion BLE)
• Heltec T114 (Repeater)
• Lilygo T-Echo (Companion BLE)
• Lilygo T-Echo Lite (Repeater)
• Thinknode M1 (Companion BLE)

Tests performed:
Repeater:

• Advert send and receive successfully decoded

Companion BLE:

• Advert send and receive successfully decoded
• Direct message (both directions) sent and received

[NickDunklee]

@entr0p1 very good point, and all the testing thus far (awesome y'all!) seems to indicate this should work just dandy.

I pushed a change that removes the individual board-type enabled flags and enabled globally for nrf52.

If a specific board ends up having issues, that board type can have it disabled with -U USE_CC310_HW_CRYPTO for that specific board type.

[Avamander]

Has anyone actually benchmarked if this is faster? Clocking extra silicon, the extra flash consumption, it might not be worth it.

[Magalex2x14]

@Avamander I didn’t compare, but maybe this paper will answer your questions https://ewsn.org/file-repository/ewsn2021/Article8.pdf

[NickDunklee]

@Avamander the advantages aren't just about potential speed. It is also about reducing the footprint of the processing run loop. The run loop is only 4KB of memory on RAK nodes and evicting encryption to hardware crypto removes ~2.5KB of memory consumption from the loop. (The software crypto advert processing alone I tested as consuming 2.5KB-3KB per advert.) This will increase node stability, especially on configs like RAK + sensors that may increase run loop consumption.

Using hardware crypto (especially the RNG) also produces more durable encryption.

tl;dr: improves stability first, better randomness, theoretical performance and/or power improvements, but the first two are good enough to warrant the effort.

[entr0p1]

Agreed. I can't speak on behalf of the maintainers, but from my perspective, the more we offload to hardware the better. Especially if there's nothing to lose by doing it.

On the memory usage, I suspect we have very little headroom as it is so if we're talking nearly 50% of it being freed by this change, that's not insignificant and should be prioritised.

[Magalex2x14]

@IoTThinks Sorry to bother you, but maybe you might be interested in this PR for your powersaving fork? Perhaps you would be interested in assessing the impact of this code on energy consumption?

[IoTThinks]

@Magalex2x14 Let me test this PR on my RAK4631 and PowerSaving 16.1 this weekend.

[ripplebiz]

Thanks for this. Good work.

[entr0p1]

@NickDunklee just wanted to throw some data in the mix to support this.

Measured in a lab setting with low traffic (only strong so very little CRC errors involved). I'm running some stat probes at various points of operation in the stack, so was able to capture these in greater detail. Everything below is from actual measurements on hardware with real packets from the live mesh.

• free after boot - free at the deepest point setup() reached
• consumed by verify - how much further the first verification pushed the watermark down from wherever it already was.
• free after verify - boot minus that excursion; where verification itself bottoms out.
• free at floor - free at the deepest point ever reached, lifetime. Always <= "after verify". Equal to it when verification is the deepest thing on the stack.

Columns run left-to-right in time: boot -> verify -> lifetime worst:

build	free after boot	consumed by verify	free after verify	free at floor	headroom	gain
stock, software	1880 B	856 B	1024 B	1024 B	25 %	-
stock, CC310	1880 B	504–512 B	1368–1376 B	1368 B	33 %	+344 B

Timing-wise, it also halves the processing time:

operation	software	CC310
verify()	232 ms	114 ms

It's not about it being "faster" per-se; what it really means (I think) is less time we're unable to process another packet because we're processing a verify().

Epic stuff mate.

[mikecarper]

Exiting the cli stack and then running the command requested is another way I fixed this. I'll get a pull request for it. Setting up a huge region I was able to trigger a stack overflow.

[IoTThinks]

@IoTThinks Sorry to bother you, but maybe you might be interested in this PR for your powersaving fork? Perhaps you would be interested in assessing the impact of this code on energy consumption?

Hi @Magalex2x14
I have tested this PR with PowerSaving on RAK4631.
This PR works fine. The powersaving off is still at 8.8mA and powersaving on at 6.0mA.
In powersaving's perpective, there is not much difference with vs. without this PR if we use power meter to check.

We may need PPK2 to see the realtime difference.
Have a nice day.

[Magalex2x14]

@IoTThinks Thank you very much!

[lbibass]

@IoTThinks Sorry to bother you, but maybe you might be interested in this PR for your powersaving fork? Perhaps you would be interested in assessing the impact of this code on energy consumption?

Hi @Magalex2x14 I have tested this PR with PowerSaving on RAK4631. This PR works fine. The powersaving off is still at 8.8mA and powersaving on at 6.0mA. In powersaving's perpective, there is not much difference with vs. without this PR if we use power meter to check.

We may need PPK2 to see the realtime difference. Have a nice day.

I have a PPK2. I can look into doing some tests with one of my spare Heltec T114 later today.

[NickDunklee]

Thanks for the detailed test results @entr0p1 - that's great to see, especially the timing acceleration! I need to up my lab testing game. Thanks to everyone else that helped test this too. It was great to have such concrete feedback that it wouldn't break things. And thanks to @ripplebiz and team for allowing it into the codebase!

[lbibass]

@IoTThinks Sorry to bother you, but maybe you might be interested in this PR for your powersaving fork? Perhaps you would be interested in assessing the impact of this code on energy consumption?

Hi @Magalex2x14 I have tested this PR with PowerSaving on RAK4631. This PR works fine. The powersaving off is still at 8.8mA and powersaving on at 6.0mA. In powersaving's perpective, there is not much difference with vs. without this PR if we use power meter to check.
We may need PPK2 to see the realtime difference. Have a nice day.

I have a PPK2. I can look into doing some tests with one of my spare Heltec T114 later today.

These are the results of the test with the hardware crypto. Repeater powersaving is enabled, and I am running the most current dev commit, 8b9bee3 The first image is with it enabled, processing a flood advert from my local companion. in the main platform.io file, USE_CC310_HW_CRYPTO is set to 1.

In this second image, I set USE_CC310_HW_CRYPTO to 0, and recompiled and reflashed. I sent another advert from the same companion.

there is practically zero difference between the two in terms of power consumption, though it appears the time spent processing the packet itself is in fact lowered with the hardware crypto enabled. This lines up with expected behavior. Great job on this PR!

[fdlamotte]

seems that nRFCrypto.begin() should only be called once, during system init

One of the issues triggered by talking it every time (apart from the big overhead it adds) is that it resets RNG ...

To me, the use of hw functions should be reviewed by a specialist in hw crypto (at least for the correct use of HW RNG, which can be tricky)

[Avamander]

@entr0p1

Timing-wise, it also halves the processing time:

operation	software	CC310
verify()	232 ms	114 ms

This is very interesting. I went and benchmarked the ed25519-dalek crate I used and it came out as 47 ms for a software-based verify. So there's certainly quite a bit that can be shaved down there even without the hardware acceleration.

Unfortunately the opaque proprietary CC310 blob doesn't provide access to the low-level operations the CryptoCell can do, so that 114ms probably can't be improved upon by a lot. But I'm sure something similar is wasted there as well.

[nextgens]

@ripplebiz I am really upset that this got merged.

1. The HW crypto does not make sense on AES (benchmarked here, it probably does not make sense on SHA* for the same reasons (we use tiny payloads limited to MAX_PACKET_PAYLOAD, HW acceleration shines on large payloads or when you need throughput through concurrency).
2. nRFCrypto.begin() nRFCrypto.end() at each invocation is just stupid. It's slow if nothing else and if you did care about reentrancy (MC doesn't have to) this is obviously a terrible pattern where you may have two threads doing the init() user() deinit() dance and interfering with each other.
3. Trusting the HWRNG fully (just after initialising it) is equally as stupid. I haven't read the spec sheet but I'm fairly certain that it does not guarantee not to block. The right approach would be to feed a CSPRNG (like in Implement a PRNG based on ascon_xof & crypto tests #2280) with it as one of the sources. As of what happens in practice when you draw from it right after initialising it... your guess is as good as mine but my money is not on the secure side of the equation.
4. There are licensing concerns: see https://github.com/adafruit/Adafruit_nRFCrypto/blob/master/src/cortex-m4/license.txt and the associated binary blob.

IMHO The only thing to keep is the HW acceleration of ed25519.verify(), everything else should be reverted (or at least benchmarked). This PR tries to do too many things at once and should never have been merged.

[nextgens]

'cause yeah... it's well known that nothing could ever go wrong. Why would you ever check the return code?

https://github.com/adafruit/Adafruit_nRFCrypto/blob/master/src/nRFCrypto_Random.cpp#L87
https://github.com/DiUS/nRF5-SDK-15.3.0-reduced/blob/master/components/libraries/crypto/backend/cc310/cc310_backend_rng.c

[IoTThinks]

Let me take note of the effect of this PR.

As a plan B, we can always comment this to disable this feature.

-D USE_CC310_HW_CRYPTO=1