| Message ID | 20231216211633.4000444-1-slyich@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | dpp: fix data corruption around prf_plus() call | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| prestwoj/iwd-alpine-ci-fetch | success | Fetch PR |
| prestwoj/iwd-ci-gitlint | fail | dpp: fix data corruption around prf_plus() call 9: B1 Line exceeds max length (104>80): " test-dpp: unit/test-dpp.c:514: test_pkex_key_derivation: Assertion `!memcmp(tmp, __tmp, 32)' failed." |
| prestwoj/iwd-ci-fetch | success | Fetch PR |
| prestwoj/iwd-alpine-ci-makedistcheck | success | Make Distcheck |
| prestwoj/iwd-alpine-ci-incremental_build | success | Incremental build not run PASS |
| prestwoj/iwd-ci-makedistcheck | success | Make Distcheck |
| prestwoj/iwd-ci-incremental_build | success | Incremental build not run PASS |
| prestwoj/iwd-alpine-ci-build | success | Build - Configure |
| prestwoj/iwd-ci-build | success | Build - Configure |
| prestwoj/iwd-ci-makecheckvalgrind | success | Make Check w/Valgrind |
| prestwoj/iwd-ci-makecheck | success | Make Check |
| prestwoj/iwd-ci-clang | success | clang PASS |
| prestwoj/iwd-alpine-ci-makecheckvalgrind | success | Make Check w/Valgrind |
| prestwoj/iwd-alpine-ci-makecheck | success | Make Check |
| prestwoj/iwd-ci-testrunner | success | test-runner PASS |
Hi Sergei, On 12/16/23 13:16, Sergei Trofimovich wrote: > Without the change test-dpp fails on aarch64-linux as: > > $ unit/test-dpp > TEST: DPP test responder-only key derivation > TEST: DPP test mutual key derivation > TEST: DPP test PKEX key derivation > test-dpp: unit/test-dpp.c:514: test_pkex_key_derivation: Assertion `!memcmp(tmp, __tmp, 32)' failed. > > This happens due to int/size_t type mismatch passed to vararg > parameters to prf_plus(): > > bool prf_plus(enum l_checksum_type type, const void *key, size_t key_len, > void *out, size_t out_len, > size_t n_extra, ...) > { > // ... > va_start(va, n_extra); > > for (i = 0; i < n_extra; i++) { > iov[i + 1].iov_base = va_arg(va, void *); > iov[i + 1].iov_len = va_arg(va, size_t); > // ... > > Note that varargs here could only be a sequence of `void *` / `size_t` > values. > > But in src/dpp-util.c `iwd` attempted to pass `int` there: > > prf_plus(sha, prk, bytes, z_out, bytes, 5, > mac_i, 6, // <- here > mac_r, 6, // <- and here > m_x, bytes, > n_x, bytes, > key, strlen(key)); > > aarch64 stores only 32-bit value part of the register: > > mov w7, #0x6 > str w7, [sp, #...] > > and loads full 64-bit form of the register: > > ldr x3, [x3] > > As a result higher bits of `iov[].iov_len` contain unexpected values and > sendmsg sends a lot more data than expected to the kernel. > > The change fixes test-dpp test for me. > > While at it fixed obvious `int` / `size_t` mismatch in src/erp.c. > --- > src/dpp-util.c | 5 +++-- > src/erp.c | 2 +- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/src/dpp-util.c b/src/dpp-util.c > index c805b14a..91d96297 100644 > --- a/src/dpp-util.c > +++ b/src/dpp-util.c > @@ -1376,8 +1376,9 @@ bool dpp_derive_z(const uint8_t *mac_i, const uint8_t *mac_r, > hkdf_extract(sha, NULL, 0, 1, prk, k_x, bytes); > > /* HKDF-Extract (since it doesn't take non-string arguments)*/ > - prf_plus(sha, prk, bytes, z_out, bytes, 5, mac_i, 6, mac_r, 6, m_x, > - bytes, n_x, bytes, key, strlen(key)); > + prf_plus(sha, prk, bytes, z_out, bytes, 5, > + mac_i, (size_t)6, mac_r, (size_t)6, m_x, bytes, > + n_x, bytes, key, strlen(key)); > > *z_len = bytes; > > diff --git a/src/erp.c b/src/erp.c > index 85923346..7aa80bab 100644 > --- a/src/erp.c > +++ b/src/erp.c > @@ -325,7 +325,7 @@ static bool erp_derive_reauth_keys(const uint8_t *emsk, size_t emsk_len, > if (!prf_plus(L_CHECKSUM_SHA256, r_rk, emsk_len, > r_ik, emsk_len, 3, ERP_RIK_LABEL, > strlen(ERP_RIK_LABEL) + 1, > - &cryptosuite, 1, &len, sizeof(len))) > + &cryptosuite, (size_t)1, &len, sizeof(len))) > return false; > > return true; Thanks for both finding and fixing this! Its always time consuming when we just get a report of unit tests failing and don't have the architecture to test on so thank you. Denis may just do it during merging, but we also should have: Fixes: 6320d6db0f ("crypto: remove label from prf_plus, instead use va_args") Thanks, James
Hi Sergei, On 12/16/23 15:16, Sergei Trofimovich wrote: > Without the change test-dpp fails on aarch64-linux as: > > $ unit/test-dpp > TEST: DPP test responder-only key derivation > TEST: DPP test mutual key derivation > TEST: DPP test PKEX key derivation > test-dpp: unit/test-dpp.c:514: test_pkex_key_derivation: Assertion `!memcmp(tmp, __tmp, 32)' failed. > > This happens due to int/size_t type mismatch passed to vararg > parameters to prf_plus(): > > bool prf_plus(enum l_checksum_type type, const void *key, size_t key_len, > void *out, size_t out_len, > size_t n_extra, ...) > { > // ... > va_start(va, n_extra); > > for (i = 0; i < n_extra; i++) { > iov[i + 1].iov_base = va_arg(va, void *); > iov[i + 1].iov_len = va_arg(va, size_t); > // ... > > Note that varargs here could only be a sequence of `void *` / `size_t` > values. > > But in src/dpp-util.c `iwd` attempted to pass `int` there: > > prf_plus(sha, prk, bytes, z_out, bytes, 5, > mac_i, 6, // <- here > mac_r, 6, // <- and here > m_x, bytes, > n_x, bytes, > key, strlen(key)); > > aarch64 stores only 32-bit value part of the register: > > mov w7, #0x6 > str w7, [sp, #...] > > and loads full 64-bit form of the register: > > ldr x3, [x3] > > As a result higher bits of `iov[].iov_len` contain unexpected values and > sendmsg sends a lot more data than expected to the kernel. > > The change fixes test-dpp test for me. > > While at it fixed obvious `int` / `size_t` mismatch in src/erp.c. > --- > src/dpp-util.c | 5 +++-- > src/erp.c | 2 +- > 2 files changed, 4 insertions(+), 3 deletions(-) > Applied with a slight style fixup and added a Fixes: line to the description per James' suggestion. Regards, -Denis
diff --git a/src/dpp-util.c b/src/dpp-util.c index c805b14a..91d96297 100644 --- a/src/dpp-util.c +++ b/src/dpp-util.c @@ -1376,8 +1376,9 @@ bool dpp_derive_z(const uint8_t *mac_i, const uint8_t *mac_r, hkdf_extract(sha, NULL, 0, 1, prk, k_x, bytes); /* HKDF-Extract (since it doesn't take non-string arguments)*/ - prf_plus(sha, prk, bytes, z_out, bytes, 5, mac_i, 6, mac_r, 6, m_x, - bytes, n_x, bytes, key, strlen(key)); + prf_plus(sha, prk, bytes, z_out, bytes, 5, + mac_i, (size_t)6, mac_r, (size_t)6, m_x, bytes, + n_x, bytes, key, strlen(key)); *z_len = bytes; diff --git a/src/erp.c b/src/erp.c index 85923346..7aa80bab 100644 --- a/src/erp.c +++ b/src/erp.c @@ -325,7 +325,7 @@ static bool erp_derive_reauth_keys(const uint8_t *emsk, size_t emsk_len, if (!prf_plus(L_CHECKSUM_SHA256, r_rk, emsk_len, r_ik, emsk_len, 3, ERP_RIK_LABEL, strlen(ERP_RIK_LABEL) + 1, - &cryptosuite, 1, &len, sizeof(len))) + &cryptosuite, (size_t)1, &len, sizeof(len))) return false; return true;
Without the change test-dpp fails on aarch64-linux as: $ unit/test-dpp TEST: DPP test responder-only key derivation TEST: DPP test mutual key derivation TEST: DPP test PKEX key derivation test-dpp: unit/test-dpp.c:514: test_pkex_key_derivation: Assertion `!memcmp(tmp, __tmp, 32)' failed. This happens due to int/size_t type mismatch passed to vararg parameters to prf_plus(): bool prf_plus(enum l_checksum_type type, const void *key, size_t key_len, void *out, size_t out_len, size_t n_extra, ...) { // ... va_start(va, n_extra); for (i = 0; i < n_extra; i++) { iov[i + 1].iov_base = va_arg(va, void *); iov[i + 1].iov_len = va_arg(va, size_t); // ... Note that varargs here could only be a sequence of `void *` / `size_t` values. But in src/dpp-util.c `iwd` attempted to pass `int` there: prf_plus(sha, prk, bytes, z_out, bytes, 5, mac_i, 6, // <- here mac_r, 6, // <- and here m_x, bytes, n_x, bytes, key, strlen(key)); aarch64 stores only 32-bit value part of the register: mov w7, #0x6 str w7, [sp, #...] and loads full 64-bit form of the register: ldr x3, [x3] As a result higher bits of `iov[].iov_len` contain unexpected values and sendmsg sends a lot more data than expected to the kernel. The change fixes test-dpp test for me. While at it fixed obvious `int` / `size_t` mismatch in src/erp.c. --- src/dpp-util.c | 5 +++-- src/erp.c | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-)