Skip to content

Bluetooth: Mesh: Make network msg cache netkey aware #96866

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Bluetooth: Mesh: Make network msg cache netkey aware #96866

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 18 additions & 8 deletions subsys/bluetooth/mesh/net.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ struct iv_val {

static struct {
uint32_t src : 15, /* MSb of source is always 0 */
seq : 17;
seq : 17;
const struct bt_mesh_net_cred *cred;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When cache is quite big, we can run into this situation:

  • device A receives message M from device B (in direct range) over subnet A with cred pointer A
  • right after this, Configuration Manager removes device A from subnet A and adds to subnet B
  • subnets in the code is an array, so the code will allocate first empty space in the array, which will be the address of subnet A.
  • a subnet bridge device which is in range of device A (but not in range of device B), eventually with some delay receives message M from device B and bridges it over subnet B.
  • device A receives the bridged message B over subnet B, it sees that SRC and SEQ are the same as for previously received message, cred pointer points to the same address and drops the message.
  • the first message M over subnet A was rejected by device A because there is no appkey attached to the netkey of this subnet A. But message B sent over subnet B should have been processed because it has appkey attached to the netkey of that subnet B.

This a very theoretical case, but there is a risk of using pointer to the key here as it points to an address that can be reused. Just wanted to highlight this.
key_ptr_msg_cache

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using array pointer can also let a message pass the message cache if secondary key was used to decrypt message M for the first time, but the KR procedure completed, key pointers were swapped and now the same message M relayed with some delay will go in again because cred pointers won't match.

Copy link
Contributor

@alxelax alxelax Oct 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using array pointer can also let a message pass the message cache if secondary key was used to decrypt message M for the first time, but the KR procedure completed, key pointers were swapped and now the same message M relayed with some delay will go in again because cred pointers won't match.

I guess after swapping keys, deobfuscation gives garbage address and sequence number since privacy is swapped too. There will not be relaying rings.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess after swapping keys, deobfuscation gives garbage address and sequence number since privacy is swapped too. There will not be relaying rings.

The same key will be used. It will be on a different place (keys[0] instead of keys[1]). But pointer will change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is not issues of pointer usage. Network cache should be purged after removing subnetwork (friendship termination???). It prevents considering deprecated data for networking.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Network cache should be purged after removing subnetwork (friendship termination???).

But this is not the case for key refresh.

Purging or updating message cache may work, but I guess there is some extra risk in this as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, then storing subnetwork array index instead should work. It will solve swapping keys issue.
Additionally, BT_MESH_KEY_DELETED event handler in net.c should purge network cache.
It solves your described scenarios

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use NetKey index the same way here as pointers but instead of them? If we assume that the probability of generating SEQ and SRC as garbage is unlikely by any other key, then using netkey index is the same as using pointer to keys that were used to successfully decrypt the last message.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems NetKey index should be sufficient.

Copy link
Contributor Author

@omkar3141 omkar3141 Oct 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue with NetKey index is arriving at it in the API in a way that does not break API is problematic (because of friendship and LPNs). I had made prototype with that first but discarded it for simplicity of this approach. Concern about using old pointer address as index is valid, but this really far-fetched case. The problem gets resolved as soon as a the same message with a new SEQ number arrives. So, even if message is incorrectly ignored at this case, the system continues function as expected on the next same (or different) model message from same source with different SEQ. What this solves is a systematic failure that always happens when stable network configuration exists and implementations are behaving in certain ways.

} msg_cache[CONFIG_BT_MESH_MSG_CACHE_SIZE];
static uint16_t msg_cache_next;

Expand Down Expand Up @@ -145,20 +146,22 @@ static bool check_dup(struct net_buf_simple *data)
return false;
}

static bool msg_cache_match(struct net_buf_simple *pdu)
static bool msg_cache_match(struct net_buf_simple *pdu, const struct bt_mesh_net_cred *cred)
{
uint16_t i;

for (i = msg_cache_next; i > 0U;) {
if (msg_cache[--i].src == SRC(pdu->data) &&
msg_cache[i].seq == (SEQ(pdu->data) & BIT_MASK(17))) {
msg_cache[i].seq == (SEQ(pdu->data) & BIT_MASK(17)) &&
msg_cache[i].cred == cred) {
return true;
}
}

for (i = ARRAY_SIZE(msg_cache); i > msg_cache_next;) {
if (msg_cache[--i].src == SRC(pdu->data) &&
msg_cache[i].seq == (SEQ(pdu->data) & BIT_MASK(17))) {
msg_cache[i].seq == (SEQ(pdu->data) & BIT_MASK(17)) &&
msg_cache[i].cred == cred) {
return true;
}
}
Expand All @@ -171,6 +174,7 @@ static void msg_cache_add(struct bt_mesh_net_rx *rx)
msg_cache_next %= ARRAY_SIZE(msg_cache);
msg_cache[msg_cache_next].src = rx->ctx.addr;
msg_cache[msg_cache_next].seq = rx->seq;
msg_cache[msg_cache_next].cred = rx->cred;
msg_cache_next++;
}

Expand Down Expand Up @@ -402,6 +406,7 @@ static void bt_mesh_net_local(struct k_work *work)
while ((node = sys_slist_get(&bt_mesh.local_queue))) {
struct loopback_buf *buf = CONTAINER_OF(node, struct loopback_buf, node);
struct bt_mesh_net_rx rx = {
.cred = NULL,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessary.

C99 Standard, ISO/IEC 9899:1999, Section 6.7.8 (Initialization), paragraph 21:

"If there are fewer initializers in a brace-enclosed list than there are elements or members of an aggregate, or fewer characters in a string literal used to initialize an array of known size than there are elements in the array, the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration."

.ctx = {
.net_idx = buf->sub->net_idx,
/* Initialize AppIdx to a sane value */
Expand Down Expand Up @@ -653,15 +658,20 @@ static bool net_decrypt(struct bt_mesh_net_rx *rx, struct net_buf_simple *in,
return false;
}

if (rx->net_if == BT_MESH_NET_IF_ADV && msg_cache_match(out)) {
if (rx->net_if == BT_MESH_NET_IF_ADV && msg_cache_match(out, cred)) {
LOG_DBG("Duplicate found in Network Message Cache");
return false;
}

LOG_DBG("src 0x%04x", rx->ctx.addr);

return bt_mesh_net_decrypt(&cred->enc, out, BT_MESH_NET_IVI_RX(rx),
proxy) == 0;
int err = bt_mesh_net_decrypt(&cred->enc, out, BT_MESH_NET_IVI_RX(rx), proxy);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move err definition to the beginning of the function.


if (!err) {
rx->cred = cred;
}

return err == 0;
}

/* Relaying from advertising to the advertising bearer should only happen
Expand Down Expand Up @@ -865,7 +875,7 @@ void bt_mesh_net_recv(struct net_buf_simple *data, int8_t rssi,
enum bt_mesh_net_if net_if)
{
NET_BUF_SIMPLE_DEFINE(buf, BT_MESH_NET_MAX_PDU_LEN);
struct bt_mesh_net_rx rx = { .ctx.recv_rssi = rssi };
struct bt_mesh_net_rx rx = { .cred = NULL, .ctx.recv_rssi = rssi };
struct net_buf_simple_state state;
int err;

Expand Down
1 change: 1 addition & 0 deletions subsys/bluetooth/mesh/net.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@ enum bt_mesh_net_if {
/* Decoding context for Network/Transport data */
struct bt_mesh_net_rx {
struct bt_mesh_subnet *sub;
const struct bt_mesh_net_cred *cred;
struct bt_mesh_msg_ctx ctx;
uint32_t seq; /* Sequence Number */
uint8_t old_iv:1, /* iv_index - 1 was used */
Expand Down