Add fast path patch for ChaCha20-Poly1305 noise operations - #32
Conversation
3cc4de5 to
c437f36
Compare
|
|
esphbot
left a comment
esphbot
left a comment
There was a problem hiding this comment.
Warning
Important issues found.
- Capability macro is defined even when the patches are not applied, breaking the documented generic CMake path
c437f36 to
14111c5
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces an ESPHome-specific “fast path” for the Noise protocol’s ChaCha20-Poly1305 operations on constrained MCUs, focused on reducing fixed per-message overhead (especially for small encrypted API frames).
Changes:
- Adds a port-owned public header (
sodium_esphome.h) that exposes capability macros and fast-path APIs without changing upstream libsodium public headers. - Introduces a session-persistent ChaCha20 state API to avoid redoing key setup per message and to generate the Poly1305 key block as pure keystream.
- Adds a one-pass Poly1305 AEAD transcript MAC function and replaces small tail byte loops with
memcpy/memsetin hot paths.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| port_include/sodium/sodium_esphome.h | Declares ESPHome-only fast-path APIs + capability macro for consumers (e.g., noise-c) |
| patches/05-memcpy-tails.patch | Optimizes Poly1305 and ChaCha20 tail handling by replacing byte loops with memcpy and ensuring aligned tail processing |
| patches/06-noise-session-api.patch | Adds session-oriented ChaCha20 entry points and a one-pass Poly1305 AEAD transcript MAC function |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
f2f742b to
4814c63
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (5)
patches/06-noise-session-api.patch:86
- This include uses angle brackets for a port-provided header. For consistency with the existing port patch set and to avoid include-order surprises, prefer the quoted include form here too.
#include <stdint.h>
+#include <sodium/sodium_esphome.h>
port_include/sodium/sodium_esphome.h:41
- The fallback branch defines SODIUM_ESPHOME_NOISE_FAST_PATH even when __has_include is unavailable. That can produce false-positive feature detection (e.g., an unpatched tree compiled with a toolchain lacking __has_include), leading consumers to call these symbols and fail at link time. It’s safer to not advertise the capability unless the marker header is confirmed present (or the build explicitly defines the macro).
#if defined(__has_include)
# if __has_include(<sodium/sodium_esphome_patched.h>)
# define SODIUM_ESPHOME_NOISE_FAST_PATH 1
# endif
#else
# define SODIUM_ESPHOME_NOISE_FAST_PATH 1
#endif
patches/06-noise-session-api.patch:242
- crypto_stream_chacha20_ietf_session_block0_xor() accepts nullable c/m in the signature, but the implementation unconditionally passes them to chacha20_encrypt_bytes() when mlen > 0. If a caller accidentally passes mlen>0 with c==NULL or m==NULL, this will crash; adding a small runtime guard makes the API harder to misuse (especially since the header explicitly says it skips normal public-API validation).
+ chacha20_encrypt_bytes(ctx, NULL, block0, 64);
+ /* ctx counter is now 1; the guard skips the call entirely for the
+ block0-only decrypt setup */
+ if (mlen > 0U) {
+ chacha20_encrypt_bytes(ctx, m, c, mlen);
+ }
+
patches/05-memcpy-tails.patch:34
- poly1305_update() now calls memcpy() even when want==0 (e.g., when bytes==0 but st->leftover is non-zero). The prior loop performed no reads in that case, so this subtly strengthens the requirements on m (some toolchains/UB sanitizers treat memcpy(NULL, ..., 0) as UB). Guarding the memcpy with want!=0 preserves the previous behavior while keeping the fast path for non-zero copies.
/* handle leftover */
if (st->leftover) {
unsigned long long want = (poly1305_block_size - st->leftover);
@@ -24,9 +24,7 @@ poly1305_update(poly1305_state_internal_t *st, const unsigned char *m,
if (want > bytes) {
want = bytes;
}
- for (i = 0; i < want; i++) {
- st->buffer[st->leftover + i] = m[i];
- }
+ memcpy(&st->buffer[st->leftover], m, (size_t) want);
bytes -= want;
m += want;
patches/06-noise-session-api.patch:18
- This patch includes the port header using angle brackets (<sodium/sodium_esphome.h>), but other port patches include port-provided headers using the quoted form (e.g., "sodium/esphome_yield.h"). Using the same include style here reduces the risk of accidentally picking up a different header via include search order and keeps the patch set consistent.
This issue also appears on line 84 of the same file.
+#include <sodium/sodium_esphome.h>
+
…e, document contracts, keep patch 05 upstreamable
4814c63 to
d7fa99d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
Suppressed comments (3)
port_include/sodium/sodium_esphome.h:71
- GCC/Clang-only
__attribute__((nonnull(...)))in a public header can break non-GNU toolchains (e.g. MSVC) in the plain CMake build. If you keep the annotations, they should be behind a portability macro or compiler guard.
SODIUM_EXPORT
int crypto_stream_chacha20_ietf_session_block0_xor(
crypto_stream_chacha20_ietf_session_state *st, unsigned char *block0,
unsigned char *c, const unsigned char *m, unsigned long long mlen,
uint64_t nonce)
__attribute__ ((nonnull(1, 2)));
port_include/sodium/sodium_esphome.h:81
- GCC/Clang-only
__attribute__((nonnull))in a public header can break non-GNU toolchains (e.g. MSVC) in the plain CMake build. If you keep the annotations, they should be behind a portability macro or compiler guard.
SODIUM_EXPORT
int crypto_stream_chacha20_ietf_session_xor(
crypto_stream_chacha20_ietf_session_state *st, unsigned char *c,
const unsigned char *m, unsigned long long mlen)
__attribute__ ((nonnull));
port_include/sodium/sodium_esphome.h:95
- GCC/Clang-only
__attribute__((nonnull(...)))in a public header can break non-GNU toolchains (e.g. MSVC) in the plain CMake build. If you keep the annotations, they should be behind a portability macro or compiler guard.
SODIUM_EXPORT
int crypto_onetimeauth_poly1305_aead_mac(unsigned char *mac,
const unsigned char *ad,
unsigned long long adlen,
const unsigned char *c,
unsigned long long clen,
const unsigned char *key)
__attribute__ ((nonnull(1, 4, 6)));
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (3)
patches/06-noise-session-api.patch:5
- This patch file has a free-form preamble before the first diff header. Since pack.sh applies patches using
git apply, the file needs to start with a diff header; otherwise the patch won’t apply.
ESPHome noise session API: session persistent ChaCha20 key schedule,
fused block0 plus payload pass with a NULL-input keystream mode, and a
one pass Poly1305 AEAD transcript MAC. Declarations live in
port_include/sodium/sodium_esphome.h; this patch also creates the
marker header that gates the capability macro and forces the tail
patches/05-memcpy-tails.patch:5
- This patch file has free-form prose before the first diff header. pack.sh applies patches via
git apply, which expects patches to start with a diff header; the current leading text will causegit applyto fail parsing the patch.
Portable cleanups for the chacha20 partial block tail and the poly1305
leftover buffering: copy with memcpy instead of byte loops. Standalone;
applies to pristine upstream libsodium as well as this port.
diff --git a/src/libsodium/crypto_onetimeauth/poly1305/donna/poly1305_donna.c b/src/libsodium/crypto_onetimeauth/poly1305/donna/poly1305_donna.c
port_include/sodium/sodium_esphome.h:65
- The doc comment for
crypto_stream_chacha20_ietf_session_block0_xor()impliesm/care always used, but the implementation only dereferences them whenmlen > 0. Please document the NULLability/requirements explicitly to prevent accidental NULL dereferences whenmlen > 0.
* Set the IETF nonce (4 zero bytes then the 64-bit nonce, little endian, as
* the Noise protocol specifies), write the block-0 keystream to block0 and,
* when mlen > 0, encrypt m into c starting at block counter 1. block0 must
* be at least 64 bytes: it receives the full ChaCha20 block, of which only
* the first 32 bytes are the Poly1305 key. Leaves the state's block counter
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
port_include/sodium/sodium_esphome.h:33
- The comment claims that no supported toolchains lack
__has_include, but this repo already uses__has_includeintests/kat.c, so the parenthetical “(none of the supported toolchains)” is misleading/outdated. Consider removing the claim and keeping the generic fallback wording.
* capability macro. A compiler without __has_include (none of the
* supported toolchains) also never gets the macro, which fails safe:
* consumers just take their stock libsodium code path.
PR Review — Add fast path patch for ChaCha20-Poly1305 noise operationsEvery blocking item from the previous round is genuinely fixed, and I re-verified the crypto end-to-end. Merge-ready; four non-blocking notes. I applied the full seven-patch stack to the pinned 1.0.21 submodule (all apply cleanly in glob order), built the Specific things done well in this revision:
Remaining notes, all non-blocking:
✅ Resolved since last review (4)Previously-flagged issues verified fixed
🟢 Suggestions
1. New marker header makes pack.sh non-idempotent on re-run
|
esphbot
left a comment
esphbot
left a comment
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
…ngth, document continuation semantics
|
All three suggestions adopted: pack.sh now runs git clean -fd after the reset, so the marker header from patch 06 no longer breaks a second run (verified by applying the stack twice locally); session_block0_xor returns -1 for a NULL m with nonzero mlen instead of emitting bare keystream, and the header documents that m and c may be NULL only when mlen is 0; the session_xor comment now states that only one continuation call per nonce is supported, that chaining is keystream continuous only in 64 byte multiples, and that c and m must be non NULL even for empty payloads. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
patches/06-noise-session-api.patch:244
crypto_stream_chacha20_ietf_session_block0_xor()documents thatcmay be NULL only whenmlen == 0, but the implementation only guards againstm == NULL. If a caller passesc == NULLwithmlen > 0, this will callchacha20_encrypt_bytes()with a NULL output pointer and likely crash.
+ if (mlen > 0U) {
+ if (m == NULL) {
+ return -1; /* refuse to emit bare keystream on a caller bug */
+ }
+ chacha20_encrypt_bytes(ctx, m, c, mlen);
Summary
Speeds up the ChaCha20-Poly1305 hot path used by the ESPHome noise API, split into an upstreamable patch and a port specific one.
The main consumer of this path today is log message streaming over the encrypted API, small messages where the per message fixed cost dominates, which is what these changes target. OTA over noise is planned eventually; that shifts the focus to bulk throughput, where larger noise frames amortize the fixed cost and a hand tuned chacha loop remains as a follow up.
The changes:
chacha20_encrypt_bytesand the poly1305 leftover buffering copy withmemcpyinstead of byte loops, and the tail block always takes the aligned path since it runs from the aligned stack buffer; this is the largest win for small messages, which are the common case for API traffic.port_include/sodium/sodium_esphome.h: a session persistentcrypto_stream_chacha20_ietf_session_statewhose key schedule is loaded once per session;crypto_stream_chacha20_ietf_session_block0_xorwrites the Poly1305 key block as pure keystream (no memset, no xor with zeros) and encrypts the payload in the same pass;crypto_onetimeauth_poly1305_aead_maccomputes the whole AEAD transcript in one call instead of five dispatched init/update/final calls. The declarations and capability macros live in the port owned header, so the upstream public headers are untouched and consumers can feature detect the port.No IRAM placement; it measured no best case gain and IRAM is too scarce to spend on it.
Benchmarks
noise_cipherstate_encryptwith the same pattern asAPINoiseFrameHelper::write_protobuf_messages; baseline is current main (patches 01 to 04) with noise-c main, after is this change together with the companion noise-c change.ESP32 (Xtensa LX6, 240 MHz, ESP-IDF):
ESP8266 (Xtensa LX106, 80 MHz, Arduino), same benchmark:
Memory
Measured on the same builds: ESP32 flash grows 124 bytes and static RAM is unchanged; ESP8266 flash shrinks 956 bytes and static RAM shrinks 52 bytes (the fast path uses fewer libsodium entry points, so more code is dead stripped). Each cipher state grows 32 bytes (key schedule instead of raw key), 64 bytes per connection. Peak stack per crypto call drops from about 880 to about 528 bytes because the per operation scratch, the stacked cipher context and two wrapper frames are gone.
Verification
All patches apply cleanly and the patched tree matches the benchmarked tree byte for byte; an encrypted API session (handshake plus bidirectional traffic) works against the patched build on ESP32 and ESP8266; the session functions were verified to produce output byte identical to the two call composition on device.
Companion PR: esphome-libs/noise-c#26 uses the new functions behind a capability macro, with the current implementation kept as fallback for stock libsodium.