Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: RP2040 - Decrypt or Encrypt (during remote admin for example) crashes the device #4855

Closed
TheMalkavien opened this issue Sep 24, 2024 · 4 comments · Fixed by #4867
Closed
Labels
bug Something isn't working

Comments

@TheMalkavien
Copy link
Contributor

TheMalkavien commented Sep 24, 2024

Category

Hardware Compatibility

Hardware

Other

Firmware Version

2.5 +

Description

Hi,

Since 2.5+, my rp2040-lora was unusable. When trying to remote admin, the devices just crash.
I finally took time to do some investigations and found out it crashes just after the "Attempting PKI decryption" message.

I found out that there was some pointer dereference on "extraNonce" on the "encryptCurve25519" and "decryptCurve25519" functions causing this crash. The pointer itself is not bad (valid address) but was not word-aligned on the memory. While it may work on most hardware (with non-guaranted results), it makes rp2040 crash.

Good news is I have a fix, which you may use or not, depending on how you want to handle this situation.

Firstly, I replaced the dereferences "*extraNonce" with a memcpy, like that on 2 or 3 places :

*extraNonce = extraNonceTmp; ==> memcpy(extraNonce, &extraNonceTmp, 4); // do not use dereference on non aligned pointers

I tested, and it worked further until another crash. I again found out a lot of pointer dereference into the byte buffer in the aes code, which is not very good news (you can't replace every dereference, and don't want to touch the aes code !), but can be fixed in this case with a simple alignment directive on the buffer itself during declaration (the buffer was not aligned at all !) :

static uint8_t bytes[MAX_RHPACKETLEN] __attribute__ ((__aligned__(4)));
static uint8_t ScratchEncrypted[MAX_RHPACKETLEN] __attribute__ ((__aligned__(4)));

With these 2 tricks, my rp2040 is finally working fine on 2.5+ on decrypt AND crypt (tested on 2.5.1).
The directive is not sufficient, because the code use as nonce the last 4 bytes of the buffer, and this buffer does not always have a size modulo 4. But you can easily change by using another way to get a nonce.

Also, it may explain arbitrary crashes on other devices.

I'm not sure I explained it right, but I can answer questions :)

Thanks !

Relevant log output

No response

@TheMalkavien TheMalkavien added the bug Something isn't working label Sep 24, 2024
@TheMalkavien TheMalkavien changed the title [Bug]: RP2040 - Decrypt or Encrypt (during remote admin for exemple) crashes the device [Bug]: RP2040 - Decrypt or Encrypt (during remote admin for example) crashes the device Sep 24, 2024
@TheMalkavien
Copy link
Contributor Author

UPDATE : I also have some heltec devices.

I Had a PKI_FAILED message by trying to remote admin a 2.5.1 htv3, I checked the keys, everything were right.
So, I updated with my patched 2.5.1 (fix alignment issues), and now everything is working fine.

I'm not a professional developer, I may understand thing the wrong way, but it seems that my changes are fixing other issues related to encryption.

Hope this helps.

@jp-bennett
Copy link
Collaborator

I could hug you for this. Such excellent work. Memory alignment is something we don't have to worry about much with X86 code, so I didn't even consider it here.

@jp-bennett
Copy link
Collaborator

@TheMalkavien Do you want to go ahead and make a PR with these fixes?

@TheMalkavien
Copy link
Contributor Author

TheMalkavien commented Sep 25, 2024

Hi, of course !
I will do that as son as I can, maybe today or tomorrow.
Anyway, this should be considered more as a quick fix, an inspiration, than a definitive way to fix all memory alignment problems. I feel that some compiler flags may be used to align things depending on hardware, and of course, some coding rules.

thebentern added a commit that referenced this issue Sep 26, 2024
…n crashes or strange behavior (#4867)

* Replace multiple potentially non aligned pointer dereference (#4855)
First step to fix some Crypto crashes or strange behaviors

* Makes the two Crypto byte buffers aligned (#4855)
Fix #4855, and probably multiple Crypto problems depending on hardware

---------

Co-authored-by: Ben Meadors <[email protected]>
Co-authored-by: GUVWAF <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants