michaelh/zephyr
1
0
Fork 0
Code Releases Activity

Bluetooth: Host: Forbid holding on to buf given to stack

Browse source 
These are safety checks to guard against silent data corruption. The
implementation currently does not clobber bufs, but soon it will. The
bufs will be zero-copy segmented and fragmented, which involves
overwriting already-sent contents with headers for the next fragment.

Signed-off-by: Aleksander Wasaznik <aleksander.wasaznik@nordicsemi.no>
This commit is contained in:
Aleksander Wasaznik 2024-05-10 13:40:43 +02:00 • committed by Fabio Baltieri
commit 93d0eac834
2 changed files with 36 additions and 9 deletions
Download patch file Download diff file Expand all files Collapse all files

10
subsys/bluetooth/host/conn.c
View file

@ -478,6 +478,16 @@ int bt_conn_send_cb(struct bt_conn *conn, struct net_buf *buf,
LOG_DBG("conn handle %u buf len %u cb %p user_data %p", conn->handle, buf->len, cb,
user_data);
if (buf->ref != 1) {
/* The host may alter the buf contents when fragmenting. Higher
* layers cannot expect the buf contents to stay intact. Extra
* refs suggests a silent data corruption would occur if not for
* this error.
*/
LOG_ERR("buf given to conn has other refs");
return -EINVAL;
}
if (buf->user_data_size < CONFIG_BT_CONN_TX_USER_DATA_SIZE) {
LOG_ERR("not enough room in user_data %d < %d pool %u",
buf->user_data_size,

35
subsys/bluetooth/host/l2cap.c
View file

@ -1945,7 +1945,8 @@ static int l2cap_chan_le_send(struct bt_l2cap_le_chan *ch,
if ((buf->len <= ch->tx.mps) &&
(net_buf_headroom(buf) >= BT_L2CAP_BUF_SIZE(0))) {
LOG_DBG("len <= MPS, not allocating seg for %p", buf);
seg = net_buf_ref(buf);
/* move `buf` to `seg`. `buf` now borrows `seg`. */
seg = buf;
len = seg->len;
} else {
@ -2002,14 +2003,21 @@ static int l2cap_chan_le_send(struct bt_l2cap_le_chan *ch,
*/
LOG_DBG("unref %p (%s)", seg,
buf == seg ? "orig" : "seg");
net_buf_unref(seg);
if (seg == buf) {
/* move `seg` to `buf` */
} else {
net_buf_unref(seg);
}
if (err == -ENOBUFS) {
/* Restore state since segment could not be sent */
net_buf_simple_restore(&buf->b, &state);
return -EAGAIN;
err = -EAGAIN;
}
/* move `buf` back to caller */
return err;
}
@ -2026,7 +2034,9 @@ static int l2cap_chan_le_send(struct bt_l2cap_le_chan *ch,
return len;
}
/* return next netbuf fragment if present, also assign metadata */
/**
* @param buf [move on success]
*/
static int l2cap_chan_le_send_sdu(struct bt_l2cap_le_chan *ch,
struct net_buf *buf)
{
@ -2048,15 +2058,12 @@ static int l2cap_chan_le_send_sdu(struct bt_l2cap_le_chan *ch,
}
sent += ret;
/* If the current buffer has been fully consumed, destroy it */
if (sent == rem_len) {
net_buf_unref(buf);
}
}
LOG_DBG("ch %p cid 0x%04x sent %u", ch, ch->tx.cid, sent);
/* `l2cap_chan_le_send` moved `buf` for final seg */
return sent;
}
@ -3065,6 +3072,16 @@ static int bt_l2cap_dyn_chan_send(struct bt_l2cap_le_chan *le_chan, struct net_b
return -EMSGSIZE;
}
if (buf->ref != 1) {
/* The host may alter the buf contents when segmenting. Higher
* layers cannot expect the buf contents to stay intact. Extra
* refs suggests a silent data corruption would occur if not for
* this error.
*/
LOG_ERR("buf given to l2cap has other refs");
return -EINVAL;
}
if (net_buf_headroom(buf) < BT_L2CAP_SDU_CHAN_SEND_RESERVE) {
/* Call `net_buf_reserve(buf, BT_L2CAP_SDU_CHAN_SEND_RESERVE)`
* when allocating buffers intended for bt_l2cap_chan_send().