When allocating or enqueuing the response-ready interrupt IN buffer
fails in ec_host_cmd_backend_send(), the error messages refer to an
"INT OUT" endpoint, and the allocation failure log prints ret, which is
always 0 from the preceding bulk IN enqueue. Fix both messages to refer
to the IN INT endpoint, matching ec_host_cmd_signal_event().
Signed-off-by: Firas Sammoura <fsammoura@google.com>
ec_host_cmd_signal_event() allocates the interrupt IN buffer with
net_buf_alloc() via ec_host_cmd_buf_alloc_int(), and frees it with
usbd_ep_buf_free() if usbd_ep_enqueue() fails. Because the buffer was
never queued to the USB stack, release it with net_buf_unref() instead,
matching the other usbd_ep_enqueue() failure paths in this file, and
drop the unused uds_ctx variable.
Signed-off-by: Firas Sammoura <fsammoura@google.com>
Fixes 2 issues with transport management using bluetooth whereby a
connection attempt failed but no failure was returned, meaning that
clients would think the connection was actually successful
Signed-off-by: Jamie McCrae <spam@helper3000.net>
Fixes this parameter wrongly being set as required, it is not a
required parameter and should only be set when attempting to
connect over LE coded PHY
Signed-off-by: Jamie McCrae <spam@helper3000.net>
ctx->usb_rx_buf is only used between allocating an OUT buffer and
enqueuing it. Once enqueued, the buffer belongs to the USB stack and is
freed in the request callback, so the context never needs the pointer.
Use a local variable instead, and release the buffer with
net_buf_unref() if enqueuing fails: with no context pointer left,
there is nothing for net_buf_drop() to clear.
Signed-off-by: Firas Sammoura <fsammoura@google.com>
pending_event is set by ec_host_cmd_backend_usb_trigger_event() in
the caller's thread and cleared from the USB stack thread once the
event notification is queued. Rename class_state to flags and track
the pending event as an atomic bit in it instead of a plain bool
shared between threads.
Signed-off-by: Firas Sammoura <fsammoura@google.com>
ec_host_cmd_reset() dequeues the OUT, bulk IN and interrupt IN
endpoints with three copies of the same call and error check. Loop
over the endpoint addresses instead. The error message now identifies
the endpoint by its address, as the request callback already does.
Signed-off-by: Firas Sammoura <fsammoura@google.com>
k_thread_stack_space_get() takes a size_t pointer, but stack_unused was
declared as unsigned int. On 64-bit targets such as native_sim/64 these
types differ in size, triggering -Wincompatible-pointer-types and
letting the kernel write 8 bytes into a 4-byte variable.
Declare stack_unused as size_t to match the API.
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Martin Stumpf <finomnis@gmail.com>
Interrupt notifications are EP_INT_SIZE bytes long, but only the
type byte was appended with net_buf_add_u8() before buf->len was set
to the full size. Bytes 1-3 were never written: they only read as
zero because the buffer pool is static memory cleared at boot.
Setting buf->len directly also bypasses the net_buf tailroom check.
Append the notification with net_buf_add_le32(), which writes the
type into byte 0 and zeroes bytes 1-3, and check at build time that
this matches the interrupt endpoint size. The data sent to the host
is unchanged.
Signed-off-by: Firas Sammoura <fsammoura@google.com>
usbd_ep_buf_free() ends in udc_ep_buf_free(), which always returns 0,
so the error branch after freeing a completed OUT buffer is never
taken. Drop it and discard the return value explicitly. Do the same
for the other usbd_ep_buf_free() callers in this file, which already
ignore it, for consistency.
Signed-off-by: Firas Sammoura <fsammoura@google.com>
bulk_out_buf and the bulk_out_buf context field are never used: OUT
transfers are received directly into the handler's rx_ctx buffer.
Remove them, saving EP_BULK_SIZE bytes of RAM.
Signed-off-by: Firas Sammoura <fsammoura@google.com>
The macro holds the interrupt endpoint packet size; the doubled _SIZE
suffix reads like a typo. Rename it to EP_INT_SIZE to match
EP_BULK_SIZE. No functional change.
Signed-off-by: Firas Sammoura <fsammoura@google.com>
These pointers are always assigned before they are read, so the NULL
initializers are dead stores. In ec_host_cmd_request(), initialize bi
directly from udc_get_buf_info() instead. No functional change.
Signed-off-by: Firas Sammoura <fsammoura@google.com>
Use a compound assignment instead of repeating ctx->rx_ctx->len on
both sides. No functional change.
Signed-off-by: Firas Sammoura <fsammoura@google.com>
rx_ctx->len and expected_len are size_t but were logged with %d,
which expects an int. On 64-bit targets the types differ, and
-Wformat reports all three arguments wherever log format strings are
type-checked.
Signed-off-by: Firas Sammoura <fsammoura@google.com>
expected_request_len() returns 0 for an invalid header. rx_ctx->len
and expected_len are both size_t, so rx_ctx->len >= expected_len is
then always true and the separate expected_len == 0 test is redundant.
Mention in the comment that the first check covers invalid headers.
Signed-off-by: Firas Sammoura <fsammoura@google.com>
EC_HOST_CMD_SEND_TIMEOUT_MS is not referenced anywhere; the response
timeout is IN_TRANSFER_TIMEOUT_MS.
Signed-off-by: Firas Sammoura <fsammoura@google.com>
Nothing from <assert.h>, <stdarg.h>, <string.h>, <zephyr/device.h> or
<zephyr/devicetree.h> is used by the USB backend: __ASSERT() and
BUILD_ASSERT() come in through <zephyr/kernel.h>, and the backend has
no device or devicetree references.
Signed-off-by: Firas Sammoura <fsammoura@google.com>
transport_mgmt_get_other_transport() checks for the outgoing direction
in both branches. Asking for the other side of a bridge's incoming
transport with the incoming direction returns NULL, while asking with
the outgoing direction returns it.
The direction is the role of the given transport in the bridge, as for
transport_mgmt_is_bridged() and transport_mgmt_get_bridge(), so check
for the incoming direction in the first branch.
Fixes#121953
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Aric Radzin <aric.radzin@gmail.com>
transport_mgmt_disconnect_bridge() returns -ENOTCONN for a bridge that
is not connected without releasing the transport management lock.
With CONFIG_MCUMGR_GRP_TRANSPORT_LOCKING, which is enabled by default,
the next lock then blocks forever. The SMP layer takes the lock for
every request and response when transport management is enabled, so
MCUmgr stops processing.
Release the lock before returning.
Fixes#121952
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Aric Radzin <aric.radzin@gmail.com>
Fixes an issue whereby the list size was set to 0 when using the
get details for groups command as it did not count the number of
groups it would be returning
Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
The file of an upload or download was only closed after the transfer
when its length was non-zero, because a length of 0 also stands for an
unknown length, e.g. for an upload resumed at a non-zero offset. Empty
files were therefore kept open until the inactivity timeout. Whether
the length is known is now tracked separately.
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Martin Stumpf <finomnis@gmail.com>
64-bit statistics were truncated to 32 bits, because they were encoded
with zcbor_uint32_put(). They are now encoded with zcbor_uint64_put().
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Martin Stumpf <finomnis@gmail.com>
stat_mgmt_count() passed a pointer to its counter pointer to
stats_walk(), so the counter of the caller was never incremented.
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Martin Stumpf <finomnis@gmail.com>
The "& 0xff" mask for unsigned taskstat priorities was applied to the
result of zcbor_uint32_put() instead of to the priority. Because of
operator precedence it also bound the conditional expression the wrong
way. Negative priorities are now reported as 0-255 again.
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Martin Stumpf <finomnis@gmail.com>
Uploading an empty file did not truncate an existing file, because the
file was only opened and truncated when the request carried data. A
request at offset 0 is now always processed, and fs_write() is skipped
for zero-length data.
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Martin Stumpf <finomnis@gmail.com>
mcumgr_serial_process_frag_raw() takes a struct smp_hdr pointer that it
only ever overwrites with the start of the received data before reading
it, and its caller passes an uninitialised variable. Clang reports this
with -Wuninitialized, which fails the build as soon as the raw framing
code is compiled with the LLVM toolchain.
Make the header pointer a local variable of the function, and drop the
parameter and the unused variable in its caller.
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Aric Radzin <aric.radzin@gmail.com>
Use the offset returned by the target to determine how many image bytes
were actually written instead of assuming that the complete requested
payload was accepted.
An SMP frame commonly carries up to 340 bytes of image data, but a target
may accept fewer bytes, for example 336 bytes, when the flash write size
must be aligned to a 16-byte boundary. Account for this behavior so the
client advances its source buffer by the target-confirmed amount and
continues the upload without losing data.
Signed-off-by: Martin Meyer <meyer.m90@gmail.com>
CONFIG_MCUMGR_TRANSPORT_UART_ASYNC=y fails to build unless the
unrelated CONFIG_UART_CONSOLE_MCUMGR=y is also set. The RX timeout added
by commit 3b9cd7b9cb ("drivers: console: uart_mcumgr: Add async
timeout") is defined under UART_CONSOLE_MCUMGR, the UART console's
MCUmgr passthrough, so CONFIG_UART_CONSOLE_MCUMGR_ASYNC_RX_TIMEOUT_US
is undefined in any other configuration.
Move the option next to the other MCUmgr UART async options as
MCUMGR_TRANSPORT_UART_ASYNC_RX_TIMEOUT_US, under
MCUMGR_TRANSPORT_UART_ASYNC, which guards the driver code that uses
it. The prompt, default and help text are unchanged. The option has
not been in a release, so it is renamed without a deprecation period.
Tested by building samples/subsys/mgmt/mcumgr/smp_svr for
nrf52840dk/nrf52840 with serial.conf and the asynchronous UART API,
which fails without this change.
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Aric Radzin <aric.radzin@gmail.com>
Enable the MCUboot bootutil library when the MCUmgr image management
server or client is enabled.
Add a Zephyr-side Kconfig option for linking applications with the
MCUboot bootutil headers and select the bootutil library from the
MCUmgr image management Kconfig symbols.
This ensures the required bootutil dependency is enabled automatically
for MCUmgr image management functionality.
Signed-off-by: Martin Meyer <meyer.m90@gmail.com>
In ec_host_cmd_log_request(), dbg_raw was sized as sizeof(dbg_fmt) (14
bytes) under the assumption that rx_header->cmd_ver (%d) takes at most
2 characters. Since cmd_ver is a uint8_t (0..255), it can take up to 3
decimal digits, requiring up to 15 bytes including the null terminator
and triggering -Werror=format-truncation= when
CONFIG_EC_HOST_CMD_LOG_DBG_BUFFERS is enabled.
Increase the debug log buffer size to accommodate 3-digit versions and
add boundary test coverage.
Signed-off-by: Firas Sammoura <fsammoura@google.com>
In ec_host_cmd_send_response(), LOG_HEXDUMP_DBG(args->output_buf,
args->output_buf_size, "HC resp:") was called before prepare_response()
validated that args->output_buf_size + TX_HEADER_SIZE fits within
tx->len_max. When CONFIG_EC_HOST_CMD_LOG_DBG_BUFFERS is enabled and a
handler sets an oversized output_buf_size, this triggered an
out-of-bounds read past hc_tx_buffer before prepare_response() rejected
the response with EC_HOST_CMD_INVALID_RESPONSE.
Move the CONFIG_EC_HOST_CMD_LOG_DBG_BUFFERS block after
prepare_response() succeeds and add a simulator unit test
(test_no_log_on_invalid_response) verifying that rejected responses do
not trigger a response hexdump.
Signed-off-by: Firas Sammoura <fsammoura@google.com>
When verify_rx() fails (hc->rx_status != EC_HOST_CMD_SUCCESS),
ec_host_cmd_send_response() previously read rx_header->cmd_id from
hc->rx_ctx.buf to log 'HC 0x%04x err %d'. If fewer than 4 bytes were
received or the header was invalid, this logged stale or uninitialized
bytes from hc_rx_buffer.
Only log rx_header->cmd_id when hc->rx_status == EC_HOST_CMD_SUCCESS,
and log 'HC rx err %d' otherwise. Add a simulator unit test
(test_err_log_on_unverified_rx) verifying both unverified RX errors and
handler-level errors.
Signed-off-by: Firas Sammoura <fsammoura@google.com>
In verify_rx() and prepare_response(), the valid data size was computed
using uint16_t, which could wrap around if data_len was close to
UINT16_MAX. Additionally, incoming request size was not explicitly
bounded against rx->len_max.
Compute valid data sizes using size_t and reject incoming packets whose
expected size exceeds rx->len_max or available rx->len. Also update
the eSPI backend and cal_checksum() size type to prevent potential
truncation. Add unit tests covering large data lengths and response
wrapping, and replace the skipped test_no_log_on_rx_data_len_overflow
with test_rx_data_len_bounds.
Signed-off-by: Firas Sammoura <fsammoura@google.com>
Previously, incoming packets were logged before their length and
integrity were verified. When debug logging is enabled, a malformed
packet (such as a truncated header or unverified payload length) could
cause out-of-bounds reads and trigger a hexdump of uninitialized memory.
Defer debug logging until after the incoming packet passes header and
length validation so that only safely verified payloads are read.
Signed-off-by: Firas Sammoura <fsammoura@google.com>
Unexpected CS assertions during PROCESSING or SENDING states previously
fell through to the spi_bad_rx label, clobbering the backend state to
RX_BAD. A subsequent CS deassertion would then fail the state check and
immediately re-arm RX DMA into the shared buffer while the handler thread
was still processing the active request.
Fix this by:
- Ignoring unexpected CS assertions during PROCESSING and SENDING states
with a warning log, preserving the current state and avoiding hardware
TX register corruption.
- Extending the CS deassertion deferral check to cover the SENDING state
in addition to PROCESSING, ensuring RX re-arming is safely postponed
until the TX transfer completes.
- Adding native_sim unit tests covering unexpected CS toggles during both
PROCESSING and SENDING states.
Signed-off-by: Firas Sammoura <fsammoura@google.com>
In ec_host_cmd_espi_send(), the backend unconditionally sets the state
back to ESPI_STATE_READY_TO_RECV after espi_write_lpc_request(). If a new
command is received via IBF interrupt right after the request finishes,
the ISR transitions the state to ESPI_STATE_PROCESSING, which can then
be inadvertently clobbered by the send function returning to READY_TO_RECV.
Switch the backend state to atomic operations and use atomic_cas() when
completing send to only transition from ESPI_STATE_SENDING to
ESPI_STATE_READY_TO_RECV if no new request was accepted in the interim.
Assisted-by: Antigravity:gemini-1.5-pro
Signed-off-by: Al Semjonovs <asemjonovs@google.com>
Utilize a code spell-checking tool to scan for and correct spelling errors
in all files within:
- `subsys/mgmt/mcumgr`
Assisted-by: Claude:claude-sonnet-5-5 codespell
Signed-off-by: Pisit Sawangvonganan <pisit@ndrsolution.com>
Utilize a code spell-checking tool to scan for and correct spelling errors
in all files within:
- `subsys/mgmt/ec_host_cmd`
Assisted-by: Claude:claude-sonnet-5-5 codespell
Signed-off-by: Pisit Sawangvonganan <pisit@ndrsolution.com>
smp_bt_bridge_config_details() returned MGMT_RETURN_CHECK(ok) as its
bool result, which is false on success, and never set the return code.
transport_mgmt_config_details() therefore treated every successful
encode as a failure and returned its uninitialized rc as the SMP
result, leaving the configs list unterminated. The function also
reported the mode 0 configuration for any requested mode.
Set the return code and return the encode status, and reject modes
other than 0 with TRANSPORT_MGMT_ERR_INVALID_MODE, as the other
transports do.
Introduced in commit e095e47105 ("mgmt: mcumgr: transport: smp_bt:
Add support for transport mgmt").
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: JP Hutchins <jp@intercreate.io>
Fixes an issue where builds would fail if transport management or
SMP client support were enabled in general because the MCUmgr
client code for Bluetooth could not be disabled, this adds a new
Kconfig to enable it ``CONFIG_MCUMGR_TRANSPORT_BT_CLIENT``
Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
ZCBOR_MAP_DECODE_KEY_DECODER stores every decoder as a zcbor_decoder_t,
whose second parameter is void *, while the function behind the pointer
declares its own - struct zcbor_string * for zcbor_tstr_decode, for
instance. zcbor_map_decode_bulk() calls through the generic type, so
UBSan's function check reports every request that matches a key:
zcbor_bulk.c:46:10: runtime error: call to function
(unknown) through pointer to incorrect function type
'_Bool (*)(zcbor_state_t *, void *)'
That fires on nearly every MCUmgr command, which masks whatever else a
sanitized or fuzzed build would have reported. The parameter is a
pointer either way, and issue #117167 settled on suppressing the check
rather than giving every decoder a wrapper, so the attribute goes on
the one function that makes the call. It is guarded on clang: GCC has
no function sanitizer and warns on the name.
Verified on native_sim/native/64 with host/llvm and CONFIG_UBSAN=y.
tests/subsys/mgmt/mcumgr/zcbor_bulk reports the error above without the
attribute and runs clean with it.
Fixes#117167
Assisted-by: Claude:claude-opus-5
Signed-off-by: Arkadiusz Grzelka <devitwise@gmail.com>
CONFIG_MCUMGR_TRANSPORT_UART_MTU and CONFIG_MCUMGR_TRANSPORT_SHELL_MTU
only set the value returned by the get_mtu callbacks of the UART and
shell transports, which the SMP layer does not use. Serial frames are
limited to MCUMGR_SERIAL_MAX_FRAME (127) bytes and the reassembled
packet size is limited by CONFIG_MCUMGR_TRANSPORT_NETBUF_SIZE.
Deprecate them along with the callback. They keep their default and
keep working until removal, and emit a deprecation warning when changed
from the default (on a separate symbol, due to Kconfig limitations).
Also fix the UART_MCUMGR_RX_BUF_COUNT help text, which related the
buffer count to the UART MTU. Each receive buffer holds one line until
it is decoded into the packet buffer, so the count only limits how many
fragments can be queued.
Fixes#54106
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Aric Radzin <aric.radzin@gmail.com>
Nothing has called the get_mtu callback of struct smp_transport_api_t
since commit c56b45c3e8 ("mgmt: mcumgr: Fix output packet to frame
issue on serial transport") removed SMP-level response fragmentation.
Each transport fragments outgoing packets itself in its output
function, using its own link limits.
Deprecate the smp_transport_get_mtu_fn type and the get_mtu member,
and document on smp_transport_out_fn that fragmentation and reassembly
are the transport's responsibility. In-tree transports keep providing
the callback until it is removed, with the deprecation warning
suppressed where they set it. The test stubs no longer set it.
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Aric Radzin <aric.radzin@gmail.com>
Checks how much free space is left in the buffer to prevent
processing invalid packets further
Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
The SMP shell transport hardcodes UART backend lookup in TX path,
causing mcumgr responses to always go to UART regardless of which
backend received the request.
Store a pointer to the shell_uart_common context in smp_shell_data
during init and use it in smp_shell_process() to set the TX target.
This allows SMP responses to route back to the correct backend
(UART, CDC-ACM, etc.).
Co-Developed-by: qwen3.6:27b via llama.cpp <assistant@opencode.ai>
Signed-off-by: Kuldeep Dhaka <kuldeepdhaka9@gmail.com>
Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
Add a library that downloads a signed image from any HTTP server
and streams it into the MCUboot secondary slot, then lets the
application request the swap, confirm or erase it. Unlike hawkBit
or UpdateHub no management protocol or update server is needed.
Downloads run either on the caller thread or, optionally, on a
library work queue with a completion callback, and can be
cancelled. Redirects are followed, interrupted transfers can
resume with a Range request, and the MCUboot header is checked
after the download with optional SHA-256 and downgrade checks.
Multi-image layouts and https with TLS 1.2 or 1.3 are supported.
A shell command exposes the whole cycle from the console.
Assisted-by: Claude:opus-5
Signed-off-by: Sylvio Alves <sylvio.alves@espressif.com>
