More Zephyr fixes - #11431
Conversation
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11431
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-src, wolfssl-bugs
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
34b05cf to
7f0bbd0
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11431
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-src, wolfssl-bugs
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11431
Scan targets checked: none
Unchanged since last review (not re-run): wolfcrypt-src, wolfcrypt-bugs, wolfssl-src, wolfssl-bugs
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
|
Jenkins retest this please |
wc_port.h guarded a sys_clock_gettime()/sys_clock_settime() remap with
"#ifndef CLOCK_REALTIME". On Zephyr 4.3 and newer the same header includes
<time.h>, and both picolibc and newlib define CLOCK_REALTIME as 1. The guard was
therefore always false, the whole block was skipped, and z_time() called
clock_gettime() - a symbol Zephyr only defines when the application enables
CONFIG_POSIX_TIMERS, which is itself gated behind CONFIG_POSIX_SYSTEM_INTERFACES.
A build without them failed:
test.c:3657:9: error: implicit declaration of function 'clock_settime';
did you mean 'sys_clock_settime'?
Making the remap unconditional fixes that build but hands every consumer a
library-wide rewrite of two POSIX names. wc_port.h is reached from every
wolfCrypt header, so an application calling clock_gettime() would silently get
sys_clock_gettime(), which reports failure as a negative errno instead of -1
plus errno, and code that stores a function pointer under either name would stop
compiling depending on its include order.
Drop the macros and name the Zephyr API at the three places that need it -
z_time(), the wolfCrypt test's dummy wallclock and the threaded TLS sample -
keeping the POSIX call for Zephyr versions without SYS_CLOCK_REALTIME.
sys_clock_gettime() lives in lib/os/clock.c and is core Zephyr rather than
POSIX, so a Zephyr build no longer has to enable the POSIX layer just to satisfy
wc_port.c.
The module ended with target_link_libraries(wolfSSL INTERFACE zephyr_interface) which links the two the wrong way round. It makes anything that links wolfSSL inherit Zephyr's flags - which every zephyr_library() already has - and puts wolfSSL's own WOLFSSL_USER_SETTINGS and WOLFSSL_ZEPHYR definitions onto nothing. The app target picks them up anyway through CONFIG_APP_LINK_WITH_WOLFSSL, so the omission only shows up for other libraries. Consequently every Zephyr library or module that includes a wolfSSL header has to remember zephyr_library_link_libraries(wolfSSL) of its own. One that forgets still compiles, because the headers fall back to their unconfigured defaults - and then disagrees with libwolfssl about structure layout at run time. Use zephyr_link_libraries(wolfSSL) instead, so the interface properties reach every Zephyr library. This is what the mbedTLS module does. The replacement only adds. Building zephyr/samples/wolfssl_tls_sock for qemu_x86 both ways and comparing every entry in compile_commands.json, no translation unit loses a define or an include path; 175 of them gain WOLFSSL_USER_SETTINGS, WOLFSSL_ZEPHYR, WOLFSSL_HAVE_MIN and WOLFSSL_HAVE_MAX. The link line is unchanged and the resulting image has the same size and the same symbols.
Zephyr 4.1 removed CONFIG_NET_SOCKETS_POSIX_NAMES, so settings.h took over the
job itself and defined socket, bind, connect, listen, accept, send, recv,
sendto, recvfrom, setsockopt, getsockopt, shutdown, getpeername and getsockname
as object-like macros onto their zsock_ counterparts.
settings.h is reached from every wolfSSL header, so those macros rewrite any
matching token in any translation unit that includes one - including members of
structures that have nothing to do with wolfSSL. Zephyr's own
struct socket_op_vtable declares bind, connect, listen, accept, shutdown,
sendto, recvfrom, getsockopt and setsockopt, and whether a file compiles at all
comes down to the order of its includes:
#include <zephyr/net/socket.h> /* members declared without the macros */
#include <wolfssl/ssl.h> /* macros arrive */
static const struct socket_op_vtable vt = { .bind = my_bind };
error: 'const struct socket_op_vtable' has no member named 'zsock_bind'
The compiler then drops the designated initializer and falls back to positional
initialization, so the handler is installed in the wrong slot. Reversing the two
includes builds cleanly. Zephyr's own TLS socket layer hits this and carries a
block of #undefs to defuse it.
Call Zephyr's API by its own name instead. send/recv already go through
SEND_FUNCTION/RECV_FUNCTION and sendto/recvfrom through
DTLS_SENDTO_FUNCTION/DTLS_RECVFROM_FUNCTION, so those only needed a value;
socket and accept run through wc_socket_cloexec()/wc_accept_cloexec(); close and
inet_pton/inet_ntop were already handled by CloseSocket and XINET_PTON/XINET_NTOP.
That leaves bind, connect, listen, getsockopt, setsockopt and getpeername, which
get XSOCKET_* wrappers in wolfio.h. They default to the BSD names, so every other
port preprocesses exactly as before.
zsock_ is also the only spelling that works across configurations from 4.4 on:
the POSIX aliases in <zephyr/posix/sys/socket.h> need
CONFIG_NET_NAMESPACE_COMPAT_MODE, and CONFIG_NET_NAMESPACE_COMPAT_MODE itself
only restores types and constants, never the function names.
wolfssl/test.h keeps a remapping of its own, placed after its system includes
where no later declaration can be caught by it, since the harness calls the
socket API directly in many more places.
Nothing in the module exposed HAVE_OCSP or the certificate status request extension, so a Kconfig-driven build had no way to ask a TLS server to staple an OCSP response for its own certificate. Both stay off by default.
WOLF_CRYPTO_CB reached a Kconfig-driven build only through wolfTPM, so an application with a crypto device of its own had no way to enable the callback interface, and no way to reach WOLF_PRIVATE_KEY_ID with it - which is what lets a TLS private key stay on that device rather than sit in the application's memory. A port that brings its own device should keep defining WOLF_CRYPTO_CB from its own option rather than selecting this one: a select reaches a settings-file build, where the settings file is authoritative over anything that changes struct layouts.
bench_gmac_internal() passed a literal 0 as useDeviceID to bench_stats_sym_finish(), so with BENCH_DEVID defined both the software and the hardware run printed "SW", and bench_stats_add() merged them into one entry because it keys on that value too. On an accelerator where GMAC goes from 146 KiB/s to 8.7 MiB/s the mislabelling is easy to spot, but the table said the two were the same measurement.
0cd4af4 to
461d244
Compare
|
Jenkins retest this please |
Five independent Zephyr fixes and one benchmark fix, one per commit. Items 1-3 were the original PR; 4-6 were added since.
1. Call Zephyr's clock API instead of remapping the POSIX names.
wc_port.hguarded asys_clock_gettime()/sys_clock_settime()remap with#ifndef CLOCK_REALTIME, but picolibc and newlib both defineCLOCK_REALTIMEthemselves. The guard was always false, soz_time()calledclock_gettime(), which exists only when the application enablesCONFIG_POSIX_TIMERS:Making the remap unconditional fixes that build but hands every consumer a library-wide rewrite of two POSIX names, the same shape as item 3 below:
wc_port.his reached from every wolfCrypt header, so an application callingclock_gettime()would silently getsys_clock_gettime(), which reports failure as a negative errno instead of-1pluserrno.So the macros are gone and the three places that need the Zephyr call name it -
z_time(), the wolfCrypt test's dummy wallclock, and the threaded TLS sample - each keeping the POSIX call for Zephyr versions withoutSYS_CLOCK_REALTIME.sys_clock_gettime()is core Zephyr rather than POSIX and is always present; no new public macro is needed, since<zephyr/kernel.h>already pulls insys/clock.handsettings.halready includeskernel.h.2. Put the wolfSSL interface library on
zephyr_interface.target_link_libraries(wolfSSL INTERFACE zephyr_interface)links the two the wrong way round, so wolfSSL'sWOLFSSL_USER_SETTINGSreached nothing. Any Zephyr library including a wolfSSL header had to rememberzephyr_library_link_libraries(wolfSSL); one that forgets still compiles, then disagrees with libwolfssl about structure layout at run time.zephyr_link_libraries(wolfSSL)is what the mbedTLS module does, and it only adds: comparing every entry incompile_commands.jsonboth ways, no translation unit loses a define or include path, 175 gain them, and the image has the same size and symbols.3. Stop redefining the POSIX socket names library-wide. Since Zephyr 4.1 removed
CONFIG_NET_SOCKETS_POSIX_NAMES,settings.hdefined 14 socket names as object-like macros.settings.hreaches every wolfSSL header, so those rewrite any matching token anywhere, including structure members: Zephyr'sstruct socket_op_vtabledeclaresbind,connect,listen,accept,shutdownand more, and whether a file compiles depends on its include order.The compiler then falls back to positional initialization, installing the handler in the wrong slot. Zephyr's own TLS socket layer carries a block of
#undefs to defuse this.wolfSSL now calls Zephyr's API by name.
send/recv,sendto/recvfrom,socket/accept,closeandinet_pton/inet_ntopalready had wrappers that only needed a Zephyr value;bind,connect,listen,getsockopt,setsockoptandgetpeernameget newXSOCKET_*macros inwolfio.h. They default to the BSD names, so every other port preprocesses exactly as before.wolfssl/test.hkeeps a remapping of its own, placed after its system includes and function-like so it cannot rewrite a structure member.zsock_is the only spelling that works in every configuration from 4.4 on, since the POSIX aliases needCONFIG_NET_NAMESPACE_COMPAT_MODE, which restores types and constants but never function names.4. Add Kconfig options for OCSP and OCSP stapling. Nothing in the module exposed
HAVE_OCSPor the certificate status request extension, so a Kconfig-driven build had no way to ask a TLS server to staple an OCSP response for its own certificate.WOLFSSL_OCSPandWOLFSSL_OCSP_STAPLINGboth default to off, so no existing configuration changes.5. Add a Kconfig option for the crypto callback interface.
WOLF_CRYPTO_CBreached a Kconfig-driven build only through wolfTPM, so an application with a crypto device of its own had no way to enable the callback interface, and no way to reachWOLF_PRIVATE_KEY_IDwith it - which is what lets a TLS private key stay on that device rather than sit in the application's memory.A port that brings its own device should keep defining
WOLF_CRYPTO_CBfrom its own option rather than selecting this one. Aselectalso reaches a settings-file build, and there the settings file is authoritative over anything that changes structure layouts. wolfTPM's existing define inuser_settings.his now#ifndef-guarded so the two cannot collide.6. Label the GMAC benchmark rows by the device that ran them. Not Zephyr-specific, but found while benchmarking a Zephyr crypto-callback port, and a one-line fix.
bench_gmac_internal()passed a literal0asuseDeviceIDtobench_stats_sym_finish(), so withBENCH_DEVIDdefined both the software and the hardware run printedSW, andbench_stats_add()merged them into one entry because it keys on that value too. On an accelerator where GMAC goes from 146 KiB/s to 8.7 MiB/s the mislabelling is easy to spot, but the table said the two were the same measurement.