Fix (some ?) memory alignment issues on the crypto part - resulting in crashes or strange behavior

[TheMalkavien]

Hi,

As discussed into the #4855 issue, here is my humble PR.

It fixes #4855 by avoiding the use of non aligned pointers. On some hardware, we do not care at all, but for some the result is not guaranteed, and it makes some hardware crash (rp2040 crashes, Heltecv3 PKI_FAILED, ...).

There is 2 main changes :

• Firstly, avoiding to dereference the extraNonce pointer, wich is pointing to a bytebuffer, at an address we cannot be sure is properly aligned (it depends on the size of the buffer, and the alignment of the buffer itself).
• Secondly, the two crypto bytebuffers are now aligned at declaration, it makes the use of pointers inside, a little more safe (and it fixes multiples crashes during the crypto algorithm which use pointers to the buffers).

As discussed, these two changes are a quick way to fix crashes or bugs linked to the non-alignment of some objects. It will probably not fix all the alignment problems in the future (or even in the present), but it should be considered to avoid such difficult to debug behavior.
Maybe the compiler can provide some useful flags for that, and of course be very careful when using pointers (as usual ? :p ).

Tested and working on RP2040-lora and Heltecv3

Thanks !

[CLAassistant]

All committers have signed the CLA.

[jp-bennett]

It looks sane to me, though memory alignment isn't something I'm used to worrying about (obviously). I should have a rp2040 platform by the end of today to test with.

[GUVWAF]

I'm wondering whether we should align the radiobuf (used when receiving) as well:

firmware/src/mesh/RadioInterface.h

Line 94 in 9bebad2

uint8_t radiobuf[MAX_LORA_PAYLOAD_LEN + 1];

As we're doing pointer arithmetic for the payload afterwards:

firmware/src/mesh/RadioLibInterface.cpp

Line 392 in 9bebad2

const uint8_t *payload = radiobuf + sizeof(PacketHeader);

[TheMalkavien]

Hi,

I'd say, every structure should be aligned for performance at least. I'm rusty, but I found it strange that the compiler does not align global variables by default.

Anyway, in your example, even if the start of the buffer is properly aligned, there is a risk that if sizeof(PacketHeader) is not modulo "word", your pointer will be not aligned.
I'ts not a problem if you use it byte by byte, but using it with a dereferrence will mostly cause problems.

Everything I say must be verified, I'm not myself an expert in memory alignment or gcc specificities.

[GUVWAF]

@TheMalkavien Thanks, makes sense. We force sizeof(PacketHeader) to be 16, but that might thus be an issue as well. Not sure how far we should go with this, but at least this PR looks good to me.

[jp-bennett]

#4878 adds a test case that crashes the rp2040. I've not yet been able to trigger the Heltec v3 flaw. This patch does fix that crash and get the test suite passing again on rp2040.