When a bonded device reconnects and the receive state has changed
while the bonded device was offline, then per the BASS specification
the Scan Delegator shall send a notification with the receive state.
Previous implementation did this too by simply sending the notification
for all receive states regardless of whether the reconnected device
was bonded or not.
Some minor changes to prevent race conditions were also done.
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
BAP_UNICAST_CLIENT_ASE counts are per connection, and not reflect
the actual number of streams that may exist in a group. Changed to
use CONFIG_BT_BAP_UNICAST_CLIENT_GROUP_STREAM_COUNT * @ which does
reflect that exactly. The reason why it needs to be multiplied by 2,
is that CONFIG_BT_BAP_UNICAST_CLIENT_GROUP_STREAM_COUNT refers to the
number of CIS, and we may have twice as many BAP streams (as they
unidirection, and CIS may be bidirectional).
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
ATT Write commands from change unware client shall be ignored.
Previously ATT request opcode was used for request but 0 for command
when passed to att_write_rsp() which was not updated when refactored
to perform_write() which expects ATT opcode for all operations.
Also rename req to op in struct write_data to better describe what
is being keep there now.
This was affecting GATT/SR/GAS/BV-05-C qualification test case.
Signed-off-by: Szymon Janc <szymon.janc@codecoup.pl>
The CAP unicast audio start and stop procedures consist of several
subprocedures, and applications may need to know when each of these have
completed, e.g. to reconfigure the unicast group after the streams have
been codec configured but before they are QoS configured.
Add new callbacks to bt_cap_initiator_cb for each subprocedure of the
unicast audio start and unicast audio stop procedures. The callbacks are
only called if at least one request was sent to a CAP acceptor as part of
the subprocedure, so that subprocedures that are skipped because all
streams are already in the requested state do not trigger a callback. To
support this a new subproc_initiated field has been added to the common
procedure struct, which is reset whenever a new subprocedure is set.
The unit tests have been extended to verify both that the callbacks are
called when the subprocedures are performed, and that they are not called
when the subprocedures are skipped, and the BSIM CAP initiator unicast
test now waits for the new callbacks as part of the start and stop
procedures.
Assisted-by: Copilot:claude-opus-4.5
Co-authored-by: Thalley <2520413+Thalley@users.noreply.github.com>
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
Starting from QoS was not properly tested in the unit tests, and
thus did not catch a bug in the actual procedure.
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
Add a new shell command `spp rls` to allow testing of the RFCOMM Remote
Line Status (RLS) functionality through the SPP shell interface.
The command accepts optional error type parameters (overrun, parity, or
framing) to simulate serial port errors. When no parameter is provided,
it sends a no-error status.
The implementation uses the newly added `bt_rfcomm_send_rls_cmd()` API
to send RLS commands on the active SPP connection.
Signed-off-by: Lyle Zhu <lyle.zhu@nxp.com>
Add public API `bt_rfcomm_send_rls_cmd()` to allow applications to send
Remote Line Status (RLS) commands to RFCOMM DLC connections.
The new API validates the DLC connection state and sends RLS commands
with application-specified line status values to report serial port
errors such as overrun, parity, and framing errors.
Add helper macros `BT_RFCOMM_RLS_NO_ERR`, `BT_RFCOMM_RLS_ERR()`,
`BT_RFCOMM_RLS_ERR_OVERRUN_ERROR`, `BT_RFCOMM_RLS_ERR_PARITY_ERROR`,
and `BT_RFCOMM_RLS_ERR_FRAMING_ERROR` to simplify construction of line
status values.
Signed-off-by: Lyle Zhu <lyle.zhu@nxp.com>
Replace the preprocessor "#if !defined(CONFIG_BT_CTLR_SW_SWITCH_
SINGLE_TIMER)" guards with runtime "if
(!IS_ENABLED(CONFIG_BT_CTLR_SW_SWITCH_SINGLE_TIMER))" checks in
the DPPI and PPI radio HAL helpers. SW_SWITCH_TIMER and the
timer task enums are always defined, so the guarded code compiles
in both configurations and the compiler discards the disabled
branch, preserving behaviour while improving readability.
Assisted-by: CoPilot:Claude Opus 4.8
Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
Fix race in software switch timer by starting and stopping
the switch on Radio End and Radio Enable, respectively.
The Radio PHYEND event and the software-switch timer compare
event occurred at the same timer value. This allowed the
next RADIO TXEN/RXEN operation to start while PHYEND was
resetting the timer for the next software-TIFS interval,
leaving the controller in an invalid state.
Relates to commit 5fe53aaa84 ("Bluetooth: Controller: Use
PPI/DPPI to start s/w switch timer").
Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
l2cap_br_conn_req() ignores the return value of l2cap_br_chan_add(),
unlike both of its other callers. When the dynamic CID allocation
fails, the code continues as if the channel was fully set up: it sends
a connection response whose DCID is the CID that was never allocated
(0), and the channel object that the server just accepted is never
attached to the connection. Since the channel is not on the
connection's channel list, no disconnected or released callback will
ever be called for it, so the server considers the object in use
forever and one of its channels is permanently leaked.
Check the return value, give the channel back to the server through
the normal bt_l2cap_br_chan_del() lifecycle, and reply to the remote
with "no resources available".
Assisted-by: Claude:claude-fable-5
Signed-off-by: Johan Hedberg <johan.hedberg@silabs.com>
There is an issue that the receive buffer was modified while
processing the command to set the RPN.
Remove `default_rpn` to `rsp`. Simplify `rfcomm_handle_rpn()` by
consolidating response handling into a single `rsp` variable.
In the command handler path, copy the received `rpn` structure to
`rsp`, modify the param_mask, and send the response. This replaces the
previous approach of modifying the input received buffer structure
directly.
Signed-off-by: Lyle Zhu <lyle.zhu@nxp.com>
Add comprehensive error handling and buffer validation to RFCOMM
message handlers to prevent potential buffer underflow and invalid
data processing.
Remove FCS byte from buffer using `net_buf_remove_mem()` in
`rfcomm_recv()` instead of relying on length checks in data handler.
Simplify `rfcomm_handle_data()` by removing redundant FCS length check
since FCS is now removed earlier in the processing chain.
Refactor MCC message handler to properly validate buffer length and
use `net_buf_pull_mem()`/`net_buf_pull_u8()` for safe buffer access.
Add length check in TEST command handler to prevent overflow when
casting to `uint8_t`.
The change addresses a potential out-of-bounds buffer access issue,
preventing potential security issues in RFCOMM operations.
Signed-off-by: Lyle Zhu <lyle.zhu@nxp.com>
The second argument of bt_l2cap_create_pdu() is the reserve
(headroom), not the payload size. The code passed
sizeof(struct bt_hid_hdr), which consumed the whole buffer as
headroom and left tailroom = 0. The following net_buf_add() then
hits the __ASSERT() and hangs on debug builds, or writes out of
bounds on release builds.
Signed-off-by: Make Shi <make.shi@nxp.com>
hfp_hf_disconnected() leaves the HF object in a state where reusing it
is unsafe:
- The work items are canceled, but cancellation does not affect a
handler that is already running, and the handlers can block for a
long time in hfp_hf_send_cmd(), which allocates from hf_pool with
K_FOREVER. hfp_hf_create() only checks hf->acl before wiping the
object with memset(), so a new connection on the same index can wipe
the work items, the TX FIFO and the AT client state under a
still-running handler.
- Buffers still queued on tx_pending are never freed. Nothing sends
them once the DLC is gone, and the memset() on reuse then loses them
from hf_pool permanently. A fully parked pool also means a handler
blocked in the hfp_hf_send_cmd() allocation never wakes up.
- The CONNECTED flag is never cleared (only the memset() on reuse
does), so command senders keep queueing buffers for the dead DLC
between disconnection and reuse instead of failing with -ENOTCONN.
Clear the CONNECTED flag and drain tx_pending in
hfp_hf_disconnected(), and make hfp_hf_create() refuse to reuse the
object while any of the work items is still pending or running. The
refusal is a transient condition: once the handlers have finished, the
next connection attempt succeeds.
Assisted-by: Claude:claude-fable-5
Signed-off-by: Johan Hedberg <johan.hedberg@silabs.com>
Commit 6c461f291f ("Bluetooth: CAP: Handle disconnects during
procedures") added calls to active_proc_is_initiator() and
active_proc_is_commander() in bt_cap_common_disconnected(), but no
such functions exist; the actual helpers are prefixed with
bt_cap_common_. This fails the build with an implicit function
declaration error (-Werror) whenever CONFIG_BT_CAP_INITIATOR_UNICAST
or CONFIG_BT_CAP_COMMANDER is enabled, breaking the bsim audio tests
in push CI on main.
Use the correctly prefixed function names.
Signed-off-by: Anas Nashif <anas.nashif@intel.com>
Instead of support a `shell_stream` for each supported ASE,
we now just support CONFIG_BT_ISO_MAX_CHAN. Since
CONFIG_BT_ISO_MAX_CHAN is the limiting factor in actually setting
up a stream, this is a significantly better way to control the
number of streams. We could optimize further as
CONFIG_BT_ISO_MAX_CHAN contains both unicast and broadcast
streams, so we may only support fewer unicast streams
than CONFIG_BT_ISO_MAX_CHAN at some points in time.
With the default audio.conf this results in a massive RAM
savings:
Before:
Memory region Used Size Region Size %age Used
FLASH: 890368 B 1 MB 84.91%
RAM: 405410 B 448 KB 88.37%
IDT_LIST: 0 B 32 KB 0.00%
After:
Memory region Used Size Region Size %age Used
FLASH: 890480 B 1 MB 84.92%
RAM: 330674 B 448 KB 72.08%
IDT_LIST: 0 B 32 KB 0.00%
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
Add a new function to specifically cancel handover
procedures, similar to the command and initiator functions.
Additionally adds checks for the procedure type to ensure
that the right cancel function is used, to avoid any
unexpected behavior from CAP.
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
Add BT_MESH_LPN_OFFER_WAIT_TIMEOUT to configure how long the Low
Power node listens for Friend Offer messages after sending a Friend
Request. The Mesh Protocol specification section 3.6.6.4.1
recommends up to 1 second, but the optimal value depends on the
deployment, so keep 1000 ms as the default and allow tuning it.
Also introduce a separate FRIEND_CLEAR_TIMEOUT for the Friend Clear
Confirm wait. The specification defines no timeout for it, so it
must not change when the Friend Offer window is tuned.
Assisted-by: GitHub Copilot:claude-opus-5
Signed-off-by: Aleksandr Khromykh <aleksandr.khromykh@nordicsemi.no>
If the broadcast source was created and started but the handover
still failed, then we should stop and delete the broadcast source.
However bt_cap_initiator_broadcast_audio_stop is not a synchronous
function, and we cannot just stop it directly in the callback.
Instead of offloading that to another thread, we simply instruct
the shell user to manually stop and delete the source (if they
want to).
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
If int bt_cap_unicast_group_create failed then the out
group should not be assigned, but it was.
This commit ensures that on failure, the out group will
point to NULL.
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
Modify a few places to print the string and numeric values,
instead of just the numeric value.
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
Add function to convert a bt_audio_dir to a str, similar to
the other _to_str functions audio.h has.
Replaces the internal bt_audio_dir_str function.
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
The bt_bap_scan_delegator_set_bis_sync_state did not allow setting
the BIS sync value when the PA sync was not there.
This only makes sense when any of the bis_synced values
are non-zero, and should allow setting it to 0 without
a PA.
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
When there is a cp (or search cp) write and we already have
one pending, we should not attempt to send a notification.
If we do, then that notification may arrive at the client
before the notification of the first write request, so the
only thing we can really do is to reject the write.
If the opcode is invalid, we send the notification to the
system workqueue similar to all the other notifications,
which ensures that it is sent, even if we are out of ATT
TX buffers.
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
bt_conn_iso.txq is only used for TX; guard with CONFIG_BT_ISO_TX
to optimize memory for RX only builds.
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
Some controllers advertise the controller to host flow control
commands in the supported commands mask but reject them, which
fails initialisation when host flow control is enabled. Add a
no-flow-control device tree quirk that makes the host skip the
flow control commands and treat the feature as not supported.
The quirk is a build time constant, so the unreachable flow
control paths are compiled out.
Assisted-by: Claude:opus-4-8
Signed-off-by: Sylvio Alves <sylvio.alves@espressif.com>
Before initializing the metadata update procedure, we perform
a check to see if there are any streams to update.
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
If sending the HCI Accept Synchronous Connection Request command fails
in accept_sco_conn(), bt_esco_conn_req() releases the SCO connection
object with bt_sco_cleanup(), but the channel that was attached by
sco_accept() is left with chan->sco pointing to the released
connection object and with its state stuck at BT_SCO_STATE_CONNECTING.
For HFP AG this means that bt_hfp_ag_sco_accept() rejects all
subsequent incoming SCO connections for the session, since
ag->sco_chan.sco is never cleared, and hfp_ag_close_sco() may call
bt_conn_disconnect() with a stale pointer which can alias an unrelated
connection once the object is recycled.
Detach the channel and reset its state before releasing the connection
object. The disconnected callback is intentionally not called since
the connection was never established and the connected callback never
ran.
Assisted-by: Claude:claude-fable-5
Signed-off-by: Johan Hedberg <johan.hedberg@silabs.com>
The retransmission and monitor timeout handlers start with an early
return whose comment says "ACL connection is broken". The wording
suggests some sort of failure that would need to be logged or
investigated, whereas the check simply covers the benign race where the
work item runs after the ACL connection has been disconnected. Reword
the comments to describe what is actually going on.
Assisted-by: Claude:claude-fable-5
Signed-off-by: Johan Hedberg <johan.hedberg@silabs.com>
The BT_HCI_RAW_H4 and BT_HCI_RAW_H4_ENABLE Kconfig options lost their
last code references in commit 26d97164be ("Bluetooth: HCI: Use H:4
encoding for buffers"), which made the HCI raw layer use H:4 packet
encoding for all buffers unconditionally. Since then setting these
options has had no effect.
Remove the option definitions, the stale references in the sample
configurations, as well as a dead conditional default in the
BT_DRV_RX_STACK_SIZE option. Document the removal in the release
notes; since the options have been no-ops for several releases and
were never part of a deprecation cycle, there is no migration to
describe beyond dropping them.
Assisted-by: Claude:claude-fable-5
Signed-off-by: Johan Hedberg <johan.hedberg@silabs.com>
There was a missing check for when both of them were NULL,
and passing NULL to both would have resulted in an ASSERT.
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
Add LOG_DBG for missing CIG param checks to make it easier
to see exactly what check may reject the parameters.
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
Commit 22896cb8d6 ("Bluetooth: Host: Route LE host work items to the
Bluetooth workqueue") moved conn->deferred_work to the dedicated
Bluetooth workqueue, but two producers were missed and still schedule
it with plain k_work_schedule(), which targets the system workqueue:
the peripheral connection parameter update retry path in hci_core.c,
and the ISO path that retriggers the deferred work to finalize an ACL
disconnection once the last CIS reference is gone.
When either of these fires, the deferred work handler - including
disconnected() callback delivery and the connection teardown chain -
runs on the system workqueue, contradicting the documented v4.5
context change and breaking the same-queue property that makes the
non-blocking cancellations of this work item safe.
Use bt_work_schedule() in both places so that every producer of
conn->deferred_work targets the Bluetooth workqueue.
Assisted-by: Claude:claude-fable-5
Signed-off-by: Johan Hedberg <johan.hedberg@silabs.com>
monitor_ts_get() divided the cycle counter by
sys_clock_hw_cycles_per_sec() / MONITOR_TS_FREQ, truncating the
divisor to an integer. With a 32768 Hz cycle counter, common on
Bluetooth LE SoCs (both nRF52 and EFR32 use it by default), the
divisor becomes 3 instead of 3.2768, making all monitor timestamps
run 9.2% fast: one real minute displays as roughly 65.5 seconds in
btmon, and the same skew applies to the log core timestamps since
monitor_ts_get() is also registered via log_set_timestamp_func().
Convert through microseconds with k_cyc_to_us_floor64() instead,
which uses overflow-safe scaled arithmetic for any cycle rate.
Measured on nrf52dk/nrf52832 against k_uptime_get() reported in the
same monitor record: before, a record stamped at 54.9 s carried an
uptime of 50.3 s (ratio 1.0923); after, records at 16.683 s and
48.811 s carry uptimes of 16683 ms and 48810 ms (ratio 1.0000).
Assisted-by: Claude:claude-fable-5
Signed-off-by: Johan Hedberg <johan.hedberg@silabs.com>
Fix several issues in the MAP (Message Access Profile) implementation:
- Fix issues in `MAP_REQUIRED_APP_PARAM_LIST` and
`MAP_REQUIRED_HDR_LIST` macro handling.
The original implementation does not correctly detect
single-argument invocations. When only one parameter is
provided, `count` is evaluated as 0 and `tags` becomes
NULL, resulting in incorrect parsing behavior.
- Correct field name from `tags to `hdrs` in `MAP_REQUIRED_HDR_LIST`
empty case to match the actual struct member
- Add missing `BT_MAP_APPL_PARAM_TAG_ID_MAS_INST_ID` to
`SEND_EVENT_REQUIRED_AP` definition
- Fix incorrect reference from `map_mas_functions` to
`map_mns_functions` in `mce_mns_get_req_cb` function
Fix#115628.
The bug reported in #115628 was masked by the macro handling
issue and became visible only after that issue was fixed.
Signed-off-by: Can Wang <can.wang@nxp.com>
hfp_ag_disconnected() cancels the TX work and the per-call work items
(via release_call()), but leaves ongoing_call_work and slc_work
untouched. Nothing else cancels them on the disconnect path either:
ongoing_call_work is only canceled when the application reports the
ongoing calls in time, and slc_work is never canceled anywhere.
A work item that survives the disconnect is more than just a stale
timeout: hfp_ag_disconnected() clears ag->acl_conn, which marks the AG
slot as reusable, and hfp_ag_create() then wipes the whole struct with
memset() when the slot is picked up for a new connection. Zeroing a
delayable work whose timeout is still linked into the kernel timeout
list, or a work item still queued on a workqueue, corrupts the kernel's
bookkeeping.
Cancel both work items when the connection goes down, next to the
existing TX work cancellation.
Assisted-by: Claude:claude-fable-5
Signed-off-by: Johan Hedberg <johan.hedberg@silabs.com>
Every AVDTP response handler cancels session->timeout_work before
clearing the pending request, but bt_avdtp_l2cap_disconnected() only
clears the request. A command that is still outstanding when the
signalling channel goes down therefore leaves the AVDTP_TIMEOUT timer
armed.
The stale timeout itself is harmless (session->req is NULL by the time
it fires), but the session object is embedded in struct bt_a2dp, and
a2dp_get_connection() wipes that struct with memset() when the
connection slot is reused. Zeroing a delayable work whose timeout is
still linked into the kernel timeout list corrupts the kernel's timeout
bookkeeping.
Cancel the timeout work on disconnect, like the response handlers do.
To make sure the work item is always initialized by the time the
disconnected callback can run, move its initialization from
avdtp_send_cmd() to the session establishment paths (bt_avdtp_connect()
and bt_avdtp_l2cap_accept()), next to the existing session work
initialization. This also stops avdtp_send_cmd() from re-initializing
the work item on every command.
Assisted-by: Claude:claude-fable-5
Signed-off-by: Johan Hedberg <johan.hedberg@silabs.com>
The limited discoverable duration timer is only canceled by
bt_br_set_discoverable(false, ...). Nothing on the bt_disable() path
stops it.
In the normal disable path this goes unnoticed, because
hci_reset_complete() clears BT_DEV_LIMITED_DISCOVERABLE_MODE and a late
firing of the timer is a no-op. But when the HCI driver has the
no-reset quirk, the flag survives, and the timer can then fire with the
transport closed: the handler calls bt_br_set_discoverable(false,
false), which sends HCI_Write_Scan_Enable through
bt_hci_cmd_send_sync(). No response can ever arrive, so the call ends
in the 10 second "Controller unresponsive" assertion.
Canceling the timer in bt_disable() would trade this for a different
bug: disabling can fail and resume operation (the driver close op is
optional, so bt_hci_close() may return -ENOSYS, and an actual close
failure also restores the stack to an operational state), and a
canceled timer would then leave the controller in limited discoverable
mode indefinitely.
Instead, guard the timeout handler with BT_DEV_READY: a timeout that
fires while the stack is disabled (or is being disabled) becomes a
no-op, while a failed bt_disable() restores BT_DEV_READY with the timer
still armed, so the limited discoverable deadline keeps being honored.
Assisted-by: Claude:claude-fable-5
Signed-off-by: Johan Hedberg <johan.hedberg@silabs.com>
The local and remote CSRK sign counters are part of the bt_keys
storage blob, but were only ever written to persistent storage when
the bond was initially stored upon pairing completion, i.e. with a
counter value of 0. The increments done at runtime by bt_smp_sign()
and bt_smp_sign_verify() were never written back to storage.
As a consequence, after a reboot the remote sign counter would be
restored to its pairing-time value. Since ATT Signed Write Commands
are sent in plaintext over an unencrypted connection, a passive
eavesdropper could capture them and replay the entire sequence in
its original order after the reboot, passing signature verification
again. This defeats the replay protection required by the Core
Specification, which ties the sign counter to the lifetime of the
CSRK rather than to a power cycle. Security erratum 26047,
integrated in Core Spec v6.2, states this requirement explicitly:
"Conforming implementations shall be required to store the last
verified SignCounter in the security database if data signing is
used" (listed in Core Spec v6.3, Vol 1, Part C, Section 16.3).
Similarly, the local sign counter would be restored to its
pairing-time value, causing the device to reuse already-used counter
values when signing, which a compliant peer will reject as replays.
Fix this by storing the keys after each counter increment. Note that
CSRKs are only ever distributed for bonding pairings, so the keys
being stored here are guaranteed to already have a settings entry,
and bt_keys_store() is a no-op when CONFIG_BT_SETTINGS is disabled.
If storing the keys fails, fail closed: an incoming Signed Write
Command is dropped rather than executed without durable replay
protection (the in-memory increment is kept, since the peer has
already consumed that counter value), and an outgoing signed PDU is
not sent (with the increment reverted, since the counter value has
not been consumed by the peer in that case).
Signed-off-by: Johan Hedberg <johan.hedberg@silabs.com>
Assisted-by: Claude:claude-fable-5
chan_send() unreferenced the buffer when bt_smp_sign() failed, but
its callers retain ownership of the buffer on failure: process_queue()
and bt_att_chan_send_rsp() requeue it for a later retry,
bt_att_chan_req_send() puts it back into the request, and
att_req_retry() unreferences it. Consuming the buffer inside
chan_send() therefore resulted in a freed buffer being requeued (or
freed a second time) whenever signing an outgoing ATT Signed Write
Command failed, e.g. due to a missing local CSRK.
Additionally, save and restore the buffer state around the signing
call: bt_smp_sign() extends the buffer with the 12-octet signature
before it can fail, so retrying a buffer from a failed attempt would
otherwise extend it by another 12 octets each time. This mirrors what
is already done for the bt_l2cap_send_pdu() error path further down
in the same function.
That pre-signing state also needs to be kept around past the sign
call rather than re-saved afterwards: bt_smp_sign() persists its
incremented sign counter as soon as it succeeds, before the PDU is
handed to L2CAP. If bt_l2cap_send_pdu() subsequently fails, restoring
the already-signed buffer would let a later retry sign it a second
time, appending another 12-octet signature onto a buffer only sized
for one and consuming a second counter value for a single logical
send. Restoring the pre-signing state instead makes a retry sign the
buffer fresh, at the cost of skipping the counter value consumed by
the failed attempt, which is harmless since the receiver only
requires the counter to strictly increase, not stay contiguous. Note
that Zephyr's own signature verification only tolerates such skipped
counter values since the preceding commit, which made it authenticate
PDUs with the received counter value as required by the Core
Specification.
Signed-off-by: Johan Hedberg <johan.hedberg@silabs.com>
Assisted-by: Claude:claude-fable-5
bt_smp_sign() incremented the local sign counter without bound, so
after signing with the final possible value (0xFFFFFFFF) the counter
would silently wrap around to zero and start reusing counter values,
which a compliant peer rejects as replays.
Fail closed with -EOVERFLOW instead once the counter space of the
CSRK is exhausted, making it explicit that signing with this key is
over and that a new pairing needs to distribute a fresh CSRK before
signing can resume. The final possible counter value is intentionally
never used for signing, since a receiver cannot accept it without its
own next expected value wrapping around to zero (see the reasoning in
bt_smp_sign_verify()).
As the spec notes, exhausting a 32-bit sign counter takes roughly 117
years at 100,000 signed messages a day, so this is a purely defensive
measure.
Signed-off-by: Johan Hedberg <johan.hedberg@silabs.com>
Assisted-by: Claude:claude-fable-5
bt_smp_sign_verify() substituted the locally expected sign counter
value into the received PDU before running the Signing Algorithm,
which effectively requires the peer's counter to match the expected
value exactly. The Core Specification instead requires the received
counter value to be used when authenticating the PDU, with replay
protection provided by rejecting counter values that have already
been used. Core Spec v6.2, Vol 3, Part C, Section 10.4.2 (the last
version to specify the data signing feature, before its removal in
v6.3):
"The signed data shall be authenticated by performing the Signing
Algorithm where m is the Data PDU to be authenticated, k is the
stored CSRK and the SignCounter is the received counter value."
and:
"The receiving device shall protect against a replay attack by
comparing the received SignCounter with previously received
SignCounter from the same peer device. If the SignCounter was
previously used then the receiving device shall ignore the Data
PDU."
Vol 3, Part H, Section 2.4.5 states the same in terms of the last
verified counter value:
"The device performing verification shall store the last verified
SignCounter in the security database and shall compare it with a
received SignCounter to prevent replay attacks. If the received
SignCounter is greater than the stored value then the message has
not been seen by the local device before and the security database
can be updated."
Requiring an exact match permanently desynchronizes signing with the
peer as soon as the peer skips a counter value, something that can
happen for legitimate reasons: the peer may consume counter values
for signed PDUs that never end up being received, e.g. when retrying
a locally failed send with a fresh signature, or when disconnecting
with signed PDUs still queued. Once the peer's counter is ahead of
the expected value every subsequent Signed Write Command gets
rejected, with re-pairing as the only way to recover.
Fix this by authenticating the PDU against the counter value it was
received with, accepting any value not lower than the expected one,
and recording the successor of the received value as the new
expectation.
The final possible counter value (0xFFFFFFFF) is rejected as
exhausted rather than accepted, since accepting it would wrap the
next expected value around to zero, re-opening the replay window for
every previously used counter value. A peer that has consumed its
entire counter space needs to distribute a fresh CSRK through a new
pairing to resume signing. As the spec notes, this takes roughly 117
years at 100,000 signed messages a day, so this is a purely
defensive measure.
Signed-off-by: Johan Hedberg <johan.hedberg@silabs.com>
Assisted-by: Claude:claude-fable-5
