Commit 93d0eac8 authored by Aleksander Wasaznik's avatar Aleksander Wasaznik Committed by Fabio Baltieri
Browse files

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

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: default avatarAleksander Wasaznik <aleksander.wasaznik@nordicsemi.no>
parent 261b358d
Loading
Loading
Loading
Loading
+10 −0
Changes for subsys/bluetooth/host/conn.c: 10 added lines, 0 removed lines.
Original line number Diff line number Diff line
@@ -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,
+25 −8
Changes for subsys/bluetooth/host/l2cap.c: 25 added lines, 8 removed lines.
Original line number Diff line number Diff line
@@ -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");

		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().