[PATCH BlueZ v1 1/5] avrcp: Fix out-of-bounds parsing of ListPlayerAttributes response

[PATCH BlueZ v1 1/5] avrcp: Fix out-of-bounds parsing of ListPlayerAttributes response

[Luiz Augusto von Dentz]

From: Bastien Nocera <hadess@hadess.net>

In profiles/audio/avrcp.c, avrcp_list_player_attributes_rsp() parsed the
response using hand-computed offsets into the operands buffer, without
accounting for the fact that operand_count spans the 7 byte AVRCP header
as well as the parameters:

- attrs is a 4 byte array which could be written out-of-bounds if a
  length greater than 4 was declared in the first parameter byte.

- The attribute bytes were read with a bound derived from operand_count,
  so a truncated response could be read past its end. As the receive
  buffer is reused across packets, those stale bytes could be echoed
  back to the peer in the following GetCurrentPlayerValue request.

- params_len was compared against count, which was only ever 0 at that
  point, so the length of the PDU was in practice never validated.

Parse the response through a struct iovec using the util_iov_pull_*
helpers instead, so that the header and each subsequent field are bounds
checked as they are consumed and the remaining length is tracked for us.
This lets params_len be validated against the actual number of parameter
bytes received. The attribute count is still clamped to
AVRCP_ATTRIBUTE_LAST, which is what bounds the write into attrs.

Reported-by: @ax-nnlabs
Closes: https://github.com/bluez/bluez/security/advisories/GHSA-m2vx-pw5f-rc8v
---
 profiles/audio/avrcp.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 3271b84782e8..906f93424872 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -2395,31 +2395,49 @@ static gboolean avrcp_list_player_attributes_rsp(struct avctp *conn,
 					uint8_t transaction, uint8_t *operands,
 					size_t operand_count, void *user_data)
 {
+	struct iovec iov = { operands, operand_count };
 	uint8_t attrs[AVRCP_ATTRIBUTE_LAST];
 	struct avrcp *session = user_data;
-	struct avrcp_header *pdu = (void *) operands;
+	struct avrcp_header *pdu;
 	uint8_t len, count = 0;
 	int i;
 
 	if (code == AVC_CTYPE_REJECTED || code == AVC_CTYPE_NOT_IMPLEMENTED)
 		return FALSE;
 
-	len = pdu->params[0];
+	pdu = util_iov_pull_mem(&iov, sizeof(*pdu));
+	if (!pdu) {
+		error("Invalid AVRCP header");
+		return FALSE;
+	}
 
-	if (be16_to_cpu(pdu->params_len) < count) {
+	if (be16_to_cpu(pdu->params_len) != iov.iov_len) {
 		error("Invalid parameters");
 		return FALSE;
 	}
 
-	for (i = 0; len > 0; len--, i++) {
+	if (!util_iov_pull_u8(&iov, &len))
+		return FALSE;
+
+	len = MIN(len, AVRCP_ATTRIBUTE_LAST);
+
+	for (i = 0; i < len; i++) {
+		uint8_t attr;
+
+		if (!util_iov_pull_u8(&iov, &attr))
+			break;
+
 		/* Don't query invalid attributes */
-		if (pdu->params[i + 1] == AVRCP_ATTRIBUTE_ILLEGAL ||
-				pdu->params[i + 1] > AVRCP_ATTRIBUTE_LAST)
+		if (attr == AVRCP_ATTRIBUTE_ILLEGAL ||
+					attr > AVRCP_ATTRIBUTE_LAST)
 			continue;
 
-		attrs[count++] = pdu->params[i + 1];
+		attrs[count++] = attr;
 	}
 
+	if (!count)
+		return FALSE;
+
 	avrcp_get_current_player_value(session, attrs, count);
 
 	return FALSE;
-- 
2.54.0

[PATCH BlueZ v1 2/5] avrcp: Fix out-of-bounds read parsing attribute lists

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

avrcp_parse_attribute_list() received only a pointer and an attribute
count, with no indication of how many bytes were actually available. For
each attribute it read an 8 byte header followed by a 16 bit length and
that many bytes of value, none of which was bounds checked.

The callers only validated the fixed portion of each entry:

    if (be16_to_cpu(pdu->params_len) - 1 < count * 8)

which says nothing about the variable length values that follow, so a
response declaring a single attribute with a value length of 0xFFFF
would read far past the end of the receive buffer and pass the result to
media_player_set_metadata().

These are response callbacks, so they do not go through
handle_vendordep_pdu() and params_len had itself never been checked
against the number of bytes received. avrcp_get_element_attributes_rsp()
also cast the operands to an AVRCP header without checking that a full
header was present.

parse_media_element() had a related off-by-one, reading the attribute
count at operands[13 + namesize] when parse_media_name() only
guaranteed that 13 + namesize bytes were present.

Parse all of this through a struct iovec using the util_iov_pull_*
helpers so the remaining length is tracked as each field is consumed,
and validate params_len against the bytes actually received.
---
 profiles/audio/avrcp.c | 134 +++++++++++++++++++++++------------------
 1 file changed, 77 insertions(+), 57 deletions(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 906f93424872..f9d0841ef2d5 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -2462,34 +2462,31 @@ static void avrcp_list_player_attributes(struct avrcp *session)
 
 static void avrcp_parse_attribute_list(struct avrcp_player *player,
 					struct media_item *item,
-					uint8_t *operands, uint8_t count)
+					struct iovec *iov, uint8_t count)
 {
 	struct media_player *mp = player->user_data;
-	int i;
 
-	for (i = 0; count > 0; count--) {
+	for (; count > 0; count--) {
 		uint32_t id;
 		uint16_t charset, len;
+		uint8_t *value;
 
-		id = get_be32(&operands[i]);
-		i += sizeof(uint32_t);
+		if (!util_iov_pull_be32(iov, &id) ||
+				!util_iov_pull_be16(iov, &charset) ||
+				!util_iov_pull_be16(iov, &len))
+			return;
 
-		charset = get_be16(&operands[i]);
-		i += sizeof(uint16_t);
-
-		len = get_be16(&operands[i]);
-		i += sizeof(uint16_t);
+		value = util_iov_pull_mem(iov, len);
+		if (!value)
+			return;
 
 		if (charset == 106) {
 			const char *key = metadata_to_str(id);
 
 			if (key != NULL)
-				media_player_set_metadata(mp, item,
-							metadata_to_str(id),
-							&operands[i], len);
+				media_player_set_metadata(mp, item, key,
+								value, len);
 		}
-
-		i += len;
 	}
 }
 
@@ -2520,7 +2517,8 @@ static gboolean avrcp_get_element_attributes_rsp(struct avctp *conn,
 {
 	struct avrcp *session = user_data;
 	struct avrcp_player *player = session->controller->player;
-	struct avrcp_header *pdu = (void *) operands;
+	struct iovec iov = { operands, operand_count };
+	struct avrcp_header *pdu;
 	struct media_player *mp = player->user_data;
 	struct media_item *item;
 	uint8_t count;
@@ -2528,6 +2526,12 @@ static gboolean avrcp_get_element_attributes_rsp(struct avctp *conn,
 	if (code == AVC_CTYPE_REJECTED)
 		return FALSE;
 
+	pdu = util_iov_pull_mem(&iov, sizeof(*pdu));
+	if (!pdu) {
+		error("Invalid AVRCP header");
+		return FALSE;
+	}
+
 	/* Abort fragmented responses as reassembly is not supported */
 	if (pdu->packet_type == AVRCP_PACKET_TYPE_START ||
 			pdu->packet_type == AVRCP_PACKET_TYPE_CONTINUING) {
@@ -2535,18 +2539,19 @@ static gboolean avrcp_get_element_attributes_rsp(struct avctp *conn,
 		return FALSE;
 	}
 
-	count = pdu->params[0];
-
-	if (be16_to_cpu(pdu->params_len) - 1 < count * 8) {
+	if (be16_to_cpu(pdu->params_len) != iov.iov_len) {
 		error("Invalid parameters");
 		return FALSE;
 	}
 
+	if (!util_iov_pull_u8(&iov, &count))
+		return FALSE;
+
 	media_player_clear_metadata(mp);
 
 	item = media_player_set_playlist_item(mp, player->uid);
 
-	avrcp_parse_attribute_list(player, item, &pdu->params[1], count);
+	avrcp_parse_attribute_list(player, item, &iov, count);
 
 	media_player_metadata_changed(mp);
 
@@ -2628,46 +2633,46 @@ static const char *subtype_to_string(uint32_t subtype)
 	return "None";
 }
 
-static gboolean parse_media_name(uint8_t *operands, uint16_t len,
-				size_t name_len_offset,
-				char *name, uint16_t *namelen)
+static gboolean parse_media_name(struct iovec *iov, char *name)
 {
-	uint16_t namesize;
+	uint16_t namelen;
+	uint8_t *namebuf;
 
-	if (len < name_len_offset + 2)
+	if (!util_iov_pull_be16(iov, &namelen))
 		return FALSE;
 
+	namebuf = util_iov_pull_mem(iov, namelen);
+	if (!namebuf)
+		return FALSE;
+
+	namelen = MIN(namelen, NAME_MAX_LEN - 1);
+
 	memset(name, 0, NAME_MAX_LEN);
-	namesize = MIN(get_be16(&operands[name_len_offset]),
-			len - name_len_offset - 2);
-	namesize = MIN(namesize, NAME_MAX_LEN - 1);
-	if (namesize > 0) {
-		if (len < name_len_offset + 2 + namesize)
-			return FALSE;
-		memcpy(name, &operands[name_len_offset + 2], namesize);
-		strtoutf8(name, namesize);
-	}
-	if (namelen)
-		*namelen = namesize;
+	memcpy(name, namebuf, namelen);
+	strtoutf8(name, namelen);
+
 	return TRUE;
 }
 
 static struct media_item *parse_media_element(struct avrcp *session,
-					uint8_t *operands, uint16_t len)
+							struct iovec *iov)
 {
 	struct avrcp_player *player;
 	struct media_player *mp;
 	struct media_item *item;
-	uint16_t namesize;
 	char name[NAME_MAX_LEN];
 	uint64_t uid;
 	uint8_t count;
 
-	if (!parse_media_name(operands, len, 11, name, &namesize))
+	/* Skip the media type and character set */
+	if (!util_iov_pull_be64(iov, &uid) || !util_iov_pull(iov, 3))
 		return NULL;
 
-	uid = get_be64(&operands[0]);
-	count = operands[13 + namesize];
+	if (!parse_media_name(iov, name))
+		return NULL;
+
+	if (!util_iov_pull_u8(iov, &count))
+		return NULL;
 
 	player = session->controller->player;
 	mp = player->user_data;
@@ -2678,14 +2683,13 @@ static struct media_item *parse_media_element(struct avrcp *session,
 
 	media_item_set_playable(item, true);
 
-	avrcp_parse_attribute_list(player, item, &operands[14 + namesize],
-					count);
+	avrcp_parse_attribute_list(player, item, iov, count);
 
 	return item;
 }
 
 static struct media_item *parse_media_folder(struct avrcp *session,
-					uint8_t *operands, uint16_t len)
+							struct iovec *iov)
 {
 	struct avrcp_player *player = session->controller->player;
 	struct media_player *mp = player->user_data;
@@ -2695,12 +2699,15 @@ static struct media_item *parse_media_folder(struct avrcp *session,
 	uint8_t type;
 	uint8_t playable;
 
-	if (!parse_media_name(operands, len, 12, name, NULL))
+	/* Skip the character set */
+	if (!util_iov_pull_be64(iov, &uid) ||
+			!util_iov_pull_u8(iov, &type) ||
+			!util_iov_pull_u8(iov, &playable) ||
+			!util_iov_pull(iov, 2))
 		return NULL;
 
-	uid = get_be64(&operands[0]);
-	type = operands[8];
-	playable = operands[9];
+	if (!parse_media_name(iov, name))
+		return NULL;
 
 	item = media_player_create_folder(mp, name, type, uid);
 	if (!item)
@@ -2755,6 +2762,7 @@ static gboolean avrcp_list_items_rsp(struct avctp *conn, uint8_t *operands,
 
 	for (i = 8; count && i + 3 < operand_count; count--) {
 		struct media_item *item;
+		struct iovec iov;
 		uint8_t type;
 		uint16_t len;
 
@@ -2772,10 +2780,13 @@ static gboolean avrcp_list_items_rsp(struct avctp *conn, uint8_t *operands,
 			break;
 		}
 
+		iov.iov_base = &operands[i];
+		iov.iov_len = len;
+
 		if (type == 0x03)
-			item = parse_media_element(session, &operands[i], len);
+			item = parse_media_element(session, &iov);
 		else
-			item = parse_media_folder(session, &operands[i], len);
+			item = parse_media_folder(session, &iov);
 
 		if (item) {
 			p->items = g_slist_append(p->items, item);
@@ -2959,33 +2970,42 @@ static gboolean avrcp_get_item_attributes_rsp(struct avctp *conn,
 {
 	struct avrcp *session = user_data;
 	struct avrcp_player *player = session->controller->player;
-	struct avrcp_browsing_header *pdu = (void *) operands;
+	struct iovec iov = { operands, operand_count };
+	struct avrcp_browsing_header *pdu;
 	struct media_player *mp = player->user_data;
 	struct media_item *item;
-	uint8_t count;
+	uint8_t status, count;
 
-	if (pdu == NULL) {
+	if (operands == NULL) {
 		avrcp_get_element_attributes(session);
 		return FALSE;
 	}
 
-	if (pdu->params[0] != AVRCP_STATUS_SUCCESS || operand_count < 4) {
+	pdu = util_iov_pull_mem(&iov, sizeof(*pdu));
+	if (!pdu) {
 		avrcp_get_element_attributes(session);
 		return FALSE;
 	}
 
-	count = pdu->params[1];
+	if (!util_iov_pull_u8(&iov, &status) ||
+					status != AVRCP_STATUS_SUCCESS) {
+		avrcp_get_element_attributes(session);
+		return FALSE;
+	}
 
-	if (be16_to_cpu(pdu->param_len) - 1 < count * 8) {
+	if (be16_to_cpu(pdu->param_len) != operand_count - sizeof(*pdu)) {
 		error("Invalid parameters");
 		return FALSE;
 	}
 
+	if (!util_iov_pull_u8(&iov, &count))
+		return FALSE;
+
 	media_player_clear_metadata(mp);
 
 	item = media_player_set_playlist_item(mp, player->uid);
 
-	avrcp_parse_attribute_list(player, item, &pdu->params[2], count);
+	avrcp_parse_attribute_list(player, item, &iov, count);
 
 	media_player_metadata_changed(mp);
 
-- 
2.54.0

[PATCH BlueZ v1 3/5] avrcp: Use util_iov helpers to parse responses

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Every controller side response callback parsed the PDU with hand
computed offsets into the operands buffer. None of them validated the
declared parameters length against the number of bytes actually
received: unlike commands, responses do not go through
handle_vendordep_pdu(), so nothing did it on their behalf. Several read
past the end of the receive buffer as a result, for example:

- avrcp_get_capabilities_resp() read pdu->params[1 + count] for a count
  taken from the response itself, with no length check at all, and then
  shifted by the resulting event id without bounding it.

- avrcp_player_value_rsp() bounded its loop with

      if (pdu->params_len < count * 2)

  comparing a big endian field without byte swapping it, so on little
  endian the check passes for practically any value.

- avrcp_get_play_status_rsp() only checked params_len, which is supplied
  by the peer, and read nine bytes on the strength of it.

- avrcp_set_browsed_player_rsp() indexed folder names relative to
  pdu->params but bounded them against operand_count, which also spans
  the browsing header.

Add avrcp_pull_header() and avrcp_pull_browsing_header(), which pull the
respective header out of a struct iovec and check that the length it
declares matches what was received, and convert the response callbacks
to pull their fields with the util_iov helpers so the remaining length
is tracked as it is consumed.

Since the receive buffer is reused between packets, the bytes read past
the end of a short response were the contents of an earlier PDU, some of
which were then reported over D-Bus or echoed back to the peer.
---
 profiles/audio/avrcp.c | 329 ++++++++++++++++++++++++++---------------
 1 file changed, 207 insertions(+), 122 deletions(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index f9d0841ef2d5..aff3b5ce21a2 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -2260,6 +2260,52 @@ static const char *status_to_string(uint8_t status)
 	}
 }
 
+/*
+ * Pull the AVRCP header out of iov and validate that the parameters length
+ * it declares matches the number of bytes actually received, leaving iov
+ * pointing at the parameters.
+ */
+static struct avrcp_header *avrcp_pull_header(struct iovec *iov)
+{
+	struct avrcp_header *pdu;
+
+	pdu = util_iov_pull_mem(iov, sizeof(*pdu));
+	if (!pdu) {
+		error("Invalid AVRCP header");
+		return NULL;
+	}
+
+	if (be16_to_cpu(pdu->params_len) != iov->iov_len) {
+		error("Invalid parameters");
+		return NULL;
+	}
+
+	return pdu;
+}
+
+/*
+ * Same as avrcp_pull_header() but for the browsing channel, which uses a
+ * different header layout.
+ */
+static struct avrcp_browsing_header *avrcp_pull_browsing_header(
+							struct iovec *iov)
+{
+	struct avrcp_browsing_header *pdu;
+
+	pdu = util_iov_pull_mem(iov, sizeof(*pdu));
+	if (!pdu) {
+		error("Invalid AVRCP browsing header");
+		return NULL;
+	}
+
+	if (be16_to_cpu(pdu->param_len) != iov->iov_len) {
+		error("Invalid parameters");
+		return NULL;
+	}
+
+	return pdu;
+}
+
 static gboolean avrcp_get_play_status_rsp(struct avctp *conn, uint8_t code,
 					uint8_t subunit, uint8_t transaction,
 					uint8_t *operands, size_t operand_count,
@@ -2268,22 +2314,24 @@ static gboolean avrcp_get_play_status_rsp(struct avctp *conn, uint8_t code,
 	struct avrcp *session = user_data;
 	struct avrcp_player *player = session->controller->player;
 	struct media_player *mp = player->user_data;
-	struct avrcp_header *pdu = (void *) operands;
+	struct iovec iov = { operands, operand_count };
 	uint32_t duration;
 	uint32_t position;
 	uint8_t status;
 
-	if (pdu == NULL || code == AVC_CTYPE_REJECTED ||
-					be16_to_cpu(pdu->params_len) != 9)
+	if (operands == NULL || code == AVC_CTYPE_REJECTED)
+		return FALSE;
+
+	if (!avrcp_pull_header(&iov))
+		return FALSE;
+
+	if (!util_iov_pull_be32(&iov, &duration) ||
+			!util_iov_pull_be32(&iov, &position) ||
+			!util_iov_pull_u8(&iov, &status))
 		return FALSE;
 
-	duration = get_be32(pdu->params);
 	media_player_set_duration(mp, duration);
-
-	position = get_be32(pdu->params + 4);
 	media_player_set_position(mp, position);
-
-	status = get_u8(pdu->params + 8);
 	media_player_set_status(mp, status_to_string(status));
 
 	return FALSE;
@@ -2330,35 +2378,41 @@ static gboolean avrcp_player_value_rsp(struct avctp *conn, uint8_t code,
 	struct avrcp *session = user_data;
 	struct avrcp_player *player = session->controller->player;
 	struct media_player *mp = player->user_data;
-	struct avrcp_header *pdu = (void *) operands;
-	uint8_t count;
-	int i;
+	struct iovec iov = { operands, operand_count };
+	uint8_t count, status;
 
-	if (pdu == NULL) {
+	if (operands == NULL) {
 		media_player_set_setting(mp, "Error", "Timeout");
 		return FALSE;
 	}
 
+	if (!avrcp_pull_header(&iov))
+		return FALSE;
+
 	if (code == AVC_CTYPE_REJECTED) {
-		media_player_set_setting(mp, "Error",
-					status_to_str(pdu->params[0]));
+		if (util_iov_pull_u8(&iov, &status))
+			media_player_set_setting(mp, "Error",
+						status_to_str(status));
 		return FALSE;
 	}
 
-	count = pdu->params[0];
-
-	if (pdu->params_len < count * 2)
+	if (!util_iov_pull_u8(&iov, &count))
 		return FALSE;
 
-	for (i = 1; count > 0; count--, i += 2) {
+	for (; count > 0; count--) {
 		const char *key;
 		const char *value;
+		uint8_t attr, val;
 
-		key = attr_to_str(pdu->params[i]);
+		if (!util_iov_pull_u8(&iov, &attr) ||
+				!util_iov_pull_u8(&iov, &val))
+			break;
+
+		key = attr_to_str(attr);
 		if (key == NULL)
 			continue;
 
-		value = attrval_to_str(pdu->params[i], pdu->params[i + 1]);
+		value = attrval_to_str(attr, val);
 		if (value == NULL)
 			continue;
 
@@ -2398,23 +2452,14 @@ static gboolean avrcp_list_player_attributes_rsp(struct avctp *conn,
 	struct iovec iov = { operands, operand_count };
 	uint8_t attrs[AVRCP_ATTRIBUTE_LAST];
 	struct avrcp *session = user_data;
-	struct avrcp_header *pdu;
 	uint8_t len, count = 0;
 	int i;
 
 	if (code == AVC_CTYPE_REJECTED || code == AVC_CTYPE_NOT_IMPLEMENTED)
 		return FALSE;
 
-	pdu = util_iov_pull_mem(&iov, sizeof(*pdu));
-	if (!pdu) {
-		error("Invalid AVRCP header");
+	if (!avrcp_pull_header(&iov))
 		return FALSE;
-	}
-
-	if (be16_to_cpu(pdu->params_len) != iov.iov_len) {
-		error("Invalid parameters");
-		return FALSE;
-	}
 
 	if (!util_iov_pull_u8(&iov, &len))
 		return FALSE;
@@ -2526,11 +2571,9 @@ static gboolean avrcp_get_element_attributes_rsp(struct avctp *conn,
 	if (code == AVC_CTYPE_REJECTED)
 		return FALSE;
 
-	pdu = util_iov_pull_mem(&iov, sizeof(*pdu));
-	if (!pdu) {
-		error("Invalid AVRCP header");
+	pdu = avrcp_pull_header(&iov);
+	if (!pdu)
 		return FALSE;
-	}
 
 	/* Abort fragmented responses as reassembly is not supported */
 	if (pdu->packet_type == AVRCP_PACKET_TYPE_START ||
@@ -2539,11 +2582,6 @@ static gboolean avrcp_get_element_attributes_rsp(struct avctp *conn,
 		return FALSE;
 	}
 
-	if (be16_to_cpu(pdu->params_len) != iov.iov_len) {
-		error("Invalid parameters");
-		return FALSE;
-	}
-
 	if (!util_iov_pull_u8(&iov, &count))
 		return FALSE;
 
@@ -2856,23 +2894,28 @@ static gboolean avrcp_change_path_rsp(struct avctp *conn,
 					uint8_t *operands, size_t operand_count,
 					void *user_data)
 {
-	struct avrcp_browsing_header *pdu = (void *) operands;
+	struct iovec iov = { operands, operand_count };
 	struct avrcp *session = user_data;
 	struct avrcp_player *player = session->controller->player;
 	struct media_player *mp = player->user_data;
+	uint32_t items;
+	uint8_t status;
 	int ret;
 
-	if (pdu == NULL) {
+	if (operands == NULL) {
 		ret = -ETIMEDOUT;
 		goto done;
 	}
 
-	if (pdu->params[0] != AVRCP_STATUS_SUCCESS) {
+	if (!avrcp_pull_browsing_header(&iov) ||
+			!util_iov_pull_u8(&iov, &status) ||
+			status != AVRCP_STATUS_SUCCESS ||
+			!util_iov_pull_be32(&iov, &items)) {
 		ret = -EINVAL;
 		goto done;
 	}
 
-	ret = get_be32(&pdu->params[1]);
+	ret = items;
 
 done:
 	if (ret < 0) {
@@ -2900,41 +2943,47 @@ static gboolean avrcp_set_browsed_player_rsp(struct avctp *conn,
 	struct avrcp *session = user_data;
 	struct avrcp_player *player = session->controller->player;
 	struct media_player *mp = player->user_data;
-	struct avrcp_browsing_header *pdu = (void *) operands;
+	struct iovec iov = { operands, operand_count };
 	uint32_t items;
 	char **folders;
-	uint8_t depth, count;
-	size_t i;
+	uint16_t uid_counter, charset;
+	uint8_t status, depth, count;
 
-	if (pdu == NULL || pdu->params[0] != AVRCP_STATUS_SUCCESS ||
-							operand_count < 13)
+	if (operands == NULL)
 		return FALSE;
 
-	player->uid_counter = get_be16(&pdu->params[1]);
+	if (!avrcp_pull_browsing_header(&iov) ||
+			!util_iov_pull_u8(&iov, &status) ||
+			status != AVRCP_STATUS_SUCCESS ||
+			!util_iov_pull_be16(&iov, &uid_counter) ||
+			!util_iov_pull_be32(&iov, &items) ||
+			!util_iov_pull_be16(&iov, &charset) ||
+			!util_iov_pull_u8(&iov, &depth))
+		return FALSE;
+
+	player->uid_counter = uid_counter;
 	player->browsed = true;
 
-	items = get_be32(&pdu->params[3]);
-
-	depth = pdu->params[9];
-
 	folders = g_new0(char *, depth + 2);
 	folders[0] = g_strdup("/Filesystem");
 
-	for (i = 10, count = 1; count - 1 < depth && i < operand_count;
-								count++) {
+	for (count = 1; count - 1 < depth; count++) {
 		uint8_t len;
+		void *name;
+
+		if (!util_iov_pull_u8(&iov, &len))
+			break;
 
-		len = pdu->params[i++];
 		if (!len)
 			continue;
 
-		if (i + len > operand_count) {
+		name = util_iov_pull_mem(&iov, len);
+		if (!name) {
 			error("Invalid folder length");
 			break;
 		}
 
-		folders[count] = util_memdup(&pdu->params[i], len);
-		i += len;
+		folders[count] = util_memdup(name, len);
 	}
 
 	player->path = g_build_pathv("/", folders);
@@ -2971,7 +3020,6 @@ static gboolean avrcp_get_item_attributes_rsp(struct avctp *conn,
 	struct avrcp *session = user_data;
 	struct avrcp_player *player = session->controller->player;
 	struct iovec iov = { operands, operand_count };
-	struct avrcp_browsing_header *pdu;
 	struct media_player *mp = player->user_data;
 	struct media_item *item;
 	uint8_t status, count;
@@ -2981,23 +3029,13 @@ static gboolean avrcp_get_item_attributes_rsp(struct avctp *conn,
 		return FALSE;
 	}
 
-	pdu = util_iov_pull_mem(&iov, sizeof(*pdu));
-	if (!pdu) {
+	if (!avrcp_pull_browsing_header(&iov) ||
+			!util_iov_pull_u8(&iov, &status) ||
+			status != AVRCP_STATUS_SUCCESS) {
 		avrcp_get_element_attributes(session);
 		return FALSE;
 	}
 
-	if (!util_iov_pull_u8(&iov, &status) ||
-					status != AVRCP_STATUS_SUCCESS) {
-		avrcp_get_element_attributes(session);
-		return FALSE;
-	}
-
-	if (be16_to_cpu(pdu->param_len) != operand_count - sizeof(*pdu)) {
-		error("Invalid parameters");
-		return FALSE;
-	}
-
 	if (!util_iov_pull_u8(&iov, &count))
 		return FALSE;
 
@@ -3086,9 +3124,12 @@ static gboolean avrcp_set_addressed_player_rsp(struct avctp *conn, uint8_t code,
 {
 	struct avrcp *session = user_data;
 	struct avrcp_player *player = session->controller->player;
-	struct avrcp_header *pdu = (void *) operands;
+	struct iovec iov = { operands, operand_count };
 
-	if (!pdu || code != AVC_CTYPE_ACCEPTED)
+	if (!operands || code != AVC_CTYPE_ACCEPTED)
+		return FALSE;
+
+	if (!avrcp_pull_header(&iov))
 		return FALSE;
 
 	player->addressed = true;
@@ -3372,24 +3413,31 @@ static int ct_change_folder(struct media_player *mp, const char *path,
 static gboolean avrcp_search_rsp(struct avctp *conn, uint8_t *operands,
 					size_t operand_count, void *user_data)
 {
-	struct avrcp_browsing_header *pdu = (void *) operands;
+	struct iovec iov = { operands, operand_count };
 	struct avrcp *session = (void *) user_data;
 	struct avrcp_player *player = session->controller->player;
 	struct media_player *mp = player->user_data;
+	uint32_t items;
+	uint16_t uid_counter;
+	uint8_t status;
 	int ret;
 
-	if (pdu == NULL) {
+	if (operands == NULL) {
 		ret = -ETIMEDOUT;
 		goto done;
 	}
 
-	if (pdu->params[0] != AVRCP_STATUS_SUCCESS || operand_count < 7) {
+	if (!avrcp_pull_browsing_header(&iov) ||
+			!util_iov_pull_u8(&iov, &status) ||
+			status != AVRCP_STATUS_SUCCESS ||
+			!util_iov_pull_be16(&iov, &uid_counter) ||
+			!util_iov_pull_be32(&iov, &items)) {
 		ret = -EINVAL;
 		goto done;
 	}
 
-	player->uid_counter = get_be16(&pdu->params[1]);
-	ret = get_be32(&pdu->params[3]);
+	player->uid_counter = uid_counter;
+	ret = items;
 
 done:
 	media_player_search_complete(mp, ret);
@@ -3437,19 +3485,25 @@ static gboolean avrcp_play_item_rsp(struct avctp *conn, uint8_t code,
 					uint8_t *operands, size_t operand_count,
 					void *user_data)
 {
-	struct avrcp_header *pdu = (void *) operands;
+	struct iovec iov = { operands, operand_count };
 	struct avrcp *session = (void *) user_data;
 	struct avrcp_player *player = session->controller->player;
 	struct media_player *mp = player->user_data;
+	uint8_t status;
 	int ret = 0;
 
-	if (pdu == NULL) {
+	if (operands == NULL) {
 		ret = -ETIMEDOUT;
 		goto done;
 	}
 
-	if (pdu->params[0] != AVRCP_STATUS_SUCCESS) {
-		switch (pdu->params[0]) {
+	if (!avrcp_pull_header(&iov) || !util_iov_pull_u8(&iov, &status)) {
+		ret = -EINVAL;
+		goto done;
+	}
+
+	if (status != AVRCP_STATUS_SUCCESS) {
+		switch (status) {
 		case AVRCP_STATUS_UID_CHANGED:
 		case AVRCP_STATUS_DOES_NOT_EXIST:
 			ret = -ENOENT;
@@ -3567,23 +3621,32 @@ static gboolean avrcp_get_total_numberofitems_rsp(struct avctp *conn,
 					uint8_t *operands, size_t operand_count,
 					void *user_data)
 {
-	struct avrcp_browsing_header *pdu = (void *) operands;
+	struct iovec iov = { operands, operand_count };
 	struct avrcp *session = user_data;
 	struct avrcp_player *player = session->controller->player;
 	struct media_player *mp = player->user_data;
 	uint32_t num_of_items = 0;
+	uint16_t uid_counter;
+	uint8_t status;
 
-	if (pdu == NULL)
+	if (operands == NULL)
 		return -ETIMEDOUT;
 
-	if (pdu->params[0] != AVRCP_STATUS_SUCCESS || operand_count < 7)
+	if (!avrcp_pull_browsing_header(&iov) ||
+			!util_iov_pull_u8(&iov, &status))
 		return -EINVAL;
 
-	if (pdu->params[0] == AVRCP_STATUS_OUT_OF_BOUNDS)
+	if (status == AVRCP_STATUS_OUT_OF_BOUNDS)
 		goto done;
 
-	player->uid_counter = get_be16(&pdu->params[1]);
-	num_of_items = get_be32(&pdu->params[3]);
+	if (status != AVRCP_STATUS_SUCCESS)
+		return -EINVAL;
+
+	if (!util_iov_pull_be16(&iov, &uid_counter) ||
+			!util_iov_pull_be32(&iov, &num_of_items))
+		return -EINVAL;
+
+	player->uid_counter = uid_counter;
 
 	if (!num_of_items)
 		return -EINVAL;
@@ -3812,44 +3875,46 @@ static gboolean avrcp_get_media_player_list_rsp(struct avctp *conn,
 						size_t operand_count,
 						void *user_data)
 {
-	struct avrcp_browsing_header *pdu = (void *) operands;
+	struct iovec iov = { operands, operand_count };
 	struct avrcp *session = user_data;
-	uint16_t count;
-	size_t i;
+	uint16_t uid_counter, count;
+	uint8_t status;
 	GSList *removed;
 
-	if (pdu == NULL || pdu->params[0] != AVRCP_STATUS_SUCCESS ||
-							operand_count < 5)
+	if (operands == NULL)
+		return FALSE;
+
+	if (!avrcp_pull_browsing_header(&iov) ||
+			!util_iov_pull_u8(&iov, &status) ||
+			status != AVRCP_STATUS_SUCCESS ||
+			!util_iov_pull_be16(&iov, &uid_counter) ||
+			!util_iov_pull_be16(&iov, &count))
 		return FALSE;
 
 	removed = g_slist_copy(session->controller->players);
-	count = get_be16(&operands[6]);
 
-	for (i = 8; count && i < operand_count; count--) {
+	for (; count > 0; count--) {
 		struct avrcp_player *player;
 		uint8_t type;
 		uint16_t len;
+		void *data;
 
-		type = operands[i++];
-		len = get_be16(&operands[i]);
-		i += 2;
+		if (!util_iov_pull_u8(&iov, &type) ||
+				!util_iov_pull_be16(&iov, &len))
+			break;
 
-		if (type != 0x01) {
-			i += len;
-			continue;
-		}
-
-		if (i + len > operand_count) {
+		data = util_iov_pull_mem(&iov, len);
+		if (!data) {
 			error("Invalid player item length");
-			return FALSE;
+			break;
 		}
 
-		player = avrcp_parse_media_player_item(session, &operands[i],
-									len);
+		if (type != 0x01)
+			continue;
+
+		player = avrcp_parse_media_player_item(session, data, len);
 		if (player)
 			removed = g_slist_remove(removed, player);
-
-		i += len;
 	}
 
 	g_slist_free_full(removed, player_remove);
@@ -4025,15 +4090,20 @@ static gboolean avrcp_handle_event(struct avctp *conn, uint8_t code,
 {
 	struct avrcp *session = user_data;
 	struct avrcp_data *controller = session->controller;
-	struct avrcp_header *pdu = (void *) operands;
+	struct iovec iov = { operands, operand_count };
+	struct avrcp_header *pdu;
 	uint8_t event;
 
-	if (!pdu)
+	if (!operands)
 		return FALSE;
 
 	if (!controller)
 		return FALSE;
 
+	pdu = avrcp_pull_header(&iov);
+	if (!pdu || !iov.iov_len)
+		return FALSE;
+
 	if ((code != AVC_CTYPE_INTERIM && code != AVC_CTYPE_CHANGED)) {
 		if (pdu->params[0] == AVRCP_STATUS_ADDRESSED_PLAYER_CHANGED &&
 				code == AVC_CTYPE_REJECTED) {
@@ -4140,12 +4210,18 @@ static gboolean avrcp_get_capabilities_resp(struct avctp *conn, uint8_t code,
 					void *user_data)
 {
 	struct avrcp *session = user_data;
-	struct avrcp_header *pdu = (void *) operands;
+	struct iovec iov = { operands, operand_count };
 	uint16_t events = 0;
-	uint8_t count;
+	uint8_t count, cap;
 
 	if (code == AVC_CTYPE_REJECTED || code == AVC_CTYPE_NOT_IMPLEMENTED ||
-			pdu == NULL || pdu->params[0] != CAP_EVENTS_SUPPORTED)
+							operands == NULL)
+		return FALSE;
+
+	if (!avrcp_pull_header(&iov))
+		return FALSE;
+
+	if (!util_iov_pull_u8(&iov, &cap) || cap != CAP_EVENTS_SUPPORTED)
 		return FALSE;
 
 	/* Connect browsing if pending */
@@ -4155,12 +4231,17 @@ static gboolean avrcp_get_capabilities_resp(struct avctp *conn, uint8_t code,
 		avctp_connect_browsing(session->conn);
 	}
 
-	count = pdu->params[1];
+	if (!util_iov_pull_u8(&iov, &count))
+		return FALSE;
 
 	for (; count > 0; count--) {
-		uint8_t event = pdu->params[1 + count];
+		uint8_t event;
 
-		events |= (1 << event);
+		if (!util_iov_pull_u8(&iov, &event))
+			break;
+
+		if (event < sizeof(events) * 8)
+			events |= (1 << event);
 
 		switch (event) {
 		case AVRCP_EVENT_STATUS_CHANGED:
@@ -4728,14 +4809,18 @@ static gboolean avrcp_handle_set_volume(struct avctp *conn, uint8_t code,
 					void *user_data)
 {
 	struct avrcp *session = user_data;
-	struct avrcp_header *pdu = (void *) operands;
+	struct iovec iov = { operands, operand_count };
+	uint8_t value;
 	int8_t volume;
 
 	if (code == AVC_CTYPE_REJECTED || code == AVC_CTYPE_NOT_IMPLEMENTED ||
-								pdu == NULL)
+							operands == NULL)
 		return FALSE;
 
-	volume = pdu->params[0] & 0x7F;
+	if (!avrcp_pull_header(&iov) || !util_iov_pull_u8(&iov, &value))
+		return FALSE;
+
+	volume = value & 0x7F;
 
 	/* Always attempt to update the transport volume */
 	media_transport_set_a2dp_volume(session->dev, volume);
-- 
2.54.0

[PATCH BlueZ v1 4/5] avrcp: Move response parsers to avrcp-parse.c

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

The parsing of controller side responses lives in the middle of
avrcp.c, interleaved with the media_player and D-Bus glue that consumes
its results. That makes it unreachable from the unit tests: none of it
can be called without an adapter, a device, a service and a session.

The three preceding fixes were all in this code, and none of them could
be covered by a regression test as a result.

Move the parsing proper to a new avrcp-parse.c, which depends on
nothing but util_iov and log.h:

- avrcp_pull_header() and avrcp_pull_browsing_header()
- avrcp_parse_player_attributes(), which now takes the bound on the
  attribute array from its caller
- avrcp_parse_attribute_list(), which reports each attribute through a
  callback rather than calling media_player_set_metadata() itself
- avrcp_parse_media_name(), avrcp_parse_media_element() and
  avrcp_parse_media_folder(), which fill a plain struct rather than
  creating media items

struct avrcp_header, struct avrcp_browsing_header, NAME_MAX_LEN and the
player attribute ids move to the new header, which avrcp.h now includes
so that its users are unaffected.

The logic is unchanged; avrcp.c keeps the glue as thin wrappers.

Assisted-by: Claude:claude-opus-5 valgrind
---
 Makefile.plugins             |   2 +
 profiles/audio/avrcp-parse.c | 174 +++++++++++++++++++++++++++
 profiles/audio/avrcp-parse.h | 102 ++++++++++++++++
 profiles/audio/avrcp.c       | 221 +++++++----------------------------
 profiles/audio/avrcp.h       |   8 +-
 5 files changed, 322 insertions(+), 185 deletions(-)
 create mode 100644 profiles/audio/avrcp-parse.c
 create mode 100644 profiles/audio/avrcp-parse.h

diff --git a/Makefile.plugins b/Makefile.plugins
index ac667beda847..a73569e555fe 100644
--- a/Makefile.plugins
+++ b/Makefile.plugins
@@ -37,6 +37,8 @@ builtin_modules += avrcp
 builtin_sources += profiles/audio/control.h profiles/audio/control.c \
 			profiles/audio/avctp.h profiles/audio/avctp.c \
 			profiles/audio/avrcp.h profiles/audio/avrcp.c \
+			profiles/audio/avrcp-parse.h \
+			profiles/audio/avrcp-parse.c \
 			profiles/audio/avrcp-player.c
 endif
 
diff --git a/profiles/audio/avrcp-parse.c b/profiles/audio/avrcp-parse.c
new file mode 100644
index 000000000000..e0c73ec9b03a
--- /dev/null
+++ b/profiles/audio/avrcp-parse.c
@@ -0,0 +1,174 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2026  Intel Corporation
+ *
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <stdbool.h>
+#include <stdint.h>
+#include <string.h>
+#include <sys/uio.h>
+
+#include "src/log.h"
+#include "src/shared/util.h"
+
+#include "avrcp-parse.h"
+
+/*
+ * Pull the AVRCP header out of iov and validate that the parameters length
+ * it declares matches the number of bytes actually received, leaving iov
+ * pointing at the parameters.
+ */
+struct avrcp_header *avrcp_pull_header(struct iovec *iov)
+{
+	struct avrcp_header *pdu;
+
+	pdu = util_iov_pull_mem(iov, sizeof(*pdu));
+	if (!pdu) {
+		error("Invalid AVRCP header");
+		return NULL;
+	}
+
+	if (be16_to_cpu(pdu->params_len) != iov->iov_len) {
+		error("Invalid parameters");
+		return NULL;
+	}
+
+	return pdu;
+}
+
+/*
+ * Same as avrcp_pull_header() but for the browsing channel, which uses a
+ * different header layout.
+ */
+struct avrcp_browsing_header *avrcp_pull_browsing_header(struct iovec *iov)
+{
+	struct avrcp_browsing_header *pdu;
+
+	pdu = util_iov_pull_mem(iov, sizeof(*pdu));
+	if (!pdu) {
+		error("Invalid AVRCP browsing header");
+		return NULL;
+	}
+
+	if (be16_to_cpu(pdu->param_len) != iov->iov_len) {
+		error("Invalid parameters");
+		return NULL;
+	}
+
+	return pdu;
+}
+
+/*
+ * Pull a ListPlayerApplicationSettingAttributes response body out of iov,
+ * skipping the attributes that cannot be queried. At most max attributes are
+ * written to attrs, which is what bounds the write.
+ */
+uint8_t avrcp_parse_player_attributes(struct iovec *iov, uint8_t *attrs,
+							uint8_t max)
+{
+	uint8_t len, count = 0;
+	int i;
+
+	if (!util_iov_pull_u8(iov, &len))
+		return 0;
+
+	len = MIN(len, max);
+
+	for (i = 0; i < len; i++) {
+		uint8_t attr;
+
+		if (!util_iov_pull_u8(iov, &attr))
+			break;
+
+		/* Don't query invalid attributes */
+		if (attr == AVRCP_ATTRIBUTE_ILLEGAL ||
+					attr > AVRCP_ATTRIBUTE_LAST)
+			continue;
+
+		attrs[count++] = attr;
+	}
+
+	return count;
+}
+
+void avrcp_parse_attribute_list(struct iovec *iov, uint8_t count,
+					avrcp_attribute_func_t func,
+					void *user_data)
+{
+	for (; count > 0; count--) {
+		struct avrcp_attribute attr;
+
+		if (!util_iov_pull_be32(iov, &attr.id) ||
+				!util_iov_pull_be16(iov, &attr.charset) ||
+				!util_iov_pull_be16(iov, &attr.len))
+			return;
+
+		attr.value = util_iov_pull_mem(iov, attr.len);
+		if (!attr.value)
+			return;
+
+		func(&attr, user_data);
+	}
+}
+
+bool avrcp_parse_media_name(struct iovec *iov, char *name)
+{
+	uint16_t namelen;
+	uint8_t *namebuf;
+
+	if (!util_iov_pull_be16(iov, &namelen))
+		return false;
+
+	namebuf = util_iov_pull_mem(iov, namelen);
+	if (!namebuf)
+		return false;
+
+	namelen = MIN(namelen, NAME_MAX_LEN - 1);
+
+	memset(name, 0, NAME_MAX_LEN);
+	memcpy(name, namebuf, namelen);
+	strtoutf8(name, namelen);
+
+	return true;
+}
+
+bool avrcp_parse_media_element(struct iovec *iov,
+					struct avrcp_media_element *element)
+{
+	/* Skip the media type and character set */
+	if (!util_iov_pull_be64(iov, &element->uid) || !util_iov_pull(iov, 3))
+		return false;
+
+	if (!avrcp_parse_media_name(iov, element->name))
+		return false;
+
+	if (!util_iov_pull_u8(iov, &element->count))
+		return false;
+
+	return true;
+}
+
+bool avrcp_parse_media_folder(struct iovec *iov,
+					struct avrcp_media_folder *folder)
+{
+	/* Skip the character set */
+	if (!util_iov_pull_be64(iov, &folder->uid) ||
+			!util_iov_pull_u8(iov, &folder->type) ||
+			!util_iov_pull_u8(iov, &folder->playable) ||
+			!util_iov_pull(iov, 2))
+		return false;
+
+	if (!avrcp_parse_media_name(iov, folder->name))
+		return false;
+
+	return true;
+}
diff --git a/profiles/audio/avrcp-parse.h b/profiles/audio/avrcp-parse.h
new file mode 100644
index 000000000000..1a27c0684b32
--- /dev/null
+++ b/profiles/audio/avrcp-parse.h
@@ -0,0 +1,102 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2026  Intel Corporation
+ *
+ *
+ */
+
+#ifndef __AVRCP_PARSE_H
+#define __AVRCP_PARSE_H
+
+#include <stdbool.h>
+#include <stdint.h>
+#include <sys/uio.h>
+
+#define NAME_MAX_LEN 255
+
+/* player attributes */
+#define AVRCP_ATTRIBUTE_ILLEGAL		0x00
+#define AVRCP_ATTRIBUTE_EQUALIZER	0x01
+#define AVRCP_ATTRIBUTE_REPEAT_MODE	0x02
+#define AVRCP_ATTRIBUTE_SHUFFLE		0x03
+#define AVRCP_ATTRIBUTE_SCAN		0x04
+#define AVRCP_ATTRIBUTE_LAST		AVRCP_ATTRIBUTE_SCAN
+
+#if __BYTE_ORDER == __LITTLE_ENDIAN
+
+struct avrcp_header {
+	uint8_t company_id[3];
+	uint8_t pdu_id;
+	uint8_t packet_type:2;
+	uint8_t rsvd:6;
+	uint16_t params_len;
+	uint8_t params[0];
+} __attribute__ ((packed));
+
+#elif __BYTE_ORDER == __BIG_ENDIAN
+
+struct avrcp_header {
+	uint8_t company_id[3];
+	uint8_t pdu_id;
+	uint8_t rsvd:6;
+	uint8_t packet_type:2;
+	uint16_t params_len;
+	uint8_t params[0];
+} __attribute__ ((packed));
+
+#else
+#error "Unknown byte order"
+#endif
+
+#define AVRCP_HEADER_LENGTH 7
+
+struct avrcp_browsing_header {
+	uint8_t pdu_id;
+	uint16_t param_len;
+	uint8_t params[0];
+} __attribute__ ((packed));
+#define AVRCP_BROWSING_HEADER_LENGTH 3
+
+struct avrcp_attribute {
+	uint32_t id;
+	uint16_t charset;
+	uint16_t len;
+	uint8_t *value;
+};
+
+struct avrcp_media_element {
+	uint64_t uid;
+	char name[NAME_MAX_LEN];
+	uint8_t count;
+};
+
+struct avrcp_media_folder {
+	uint64_t uid;
+	uint8_t type;
+	uint8_t playable;
+	char name[NAME_MAX_LEN];
+};
+
+typedef void (*avrcp_attribute_func_t)(const struct avrcp_attribute *attr,
+							void *user_data);
+
+struct avrcp_header *avrcp_pull_header(struct iovec *iov);
+struct avrcp_browsing_header *avrcp_pull_browsing_header(struct iovec *iov);
+
+uint8_t avrcp_parse_player_attributes(struct iovec *iov, uint8_t *attrs,
+							uint8_t max);
+
+void avrcp_parse_attribute_list(struct iovec *iov, uint8_t count,
+					avrcp_attribute_func_t func,
+					void *user_data);
+
+bool avrcp_parse_media_name(struct iovec *iov, char *name);
+bool avrcp_parse_media_element(struct iovec *iov,
+					struct avrcp_media_element *element);
+bool avrcp_parse_media_folder(struct iovec *iov,
+					struct avrcp_media_folder *folder);
+
+#endif /* __AVRCP_PARSE_H */
diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index aff3b5ce21a2..df5f97eafc88 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -52,6 +52,7 @@
 
 #include "avctp.h"
 #include "avrcp.h"
+#include "avrcp-parse.h"
 #include "control.h"
 #include "media.h"
 #include "player.h"
@@ -145,46 +146,9 @@
 #define AVRCP_SCOPE_SEARCH				0x02
 #define AVRCP_SCOPE_NOW_PLAYING			0x03
 
-#define NAME_MAX_LEN 255
-
-#if __BYTE_ORDER == __LITTLE_ENDIAN
-
-struct avrcp_header {
-	uint8_t company_id[3];
-	uint8_t pdu_id;
-	uint8_t packet_type:2;
-	uint8_t rsvd:6;
-	uint16_t params_len;
-	uint8_t params[0];
-} __attribute__ ((packed));
-#define AVRCP_HEADER_LENGTH 7
-
-#elif __BYTE_ORDER == __BIG_ENDIAN
-
-struct avrcp_header {
-	uint8_t company_id[3];
-	uint8_t pdu_id;
-	uint8_t rsvd:6;
-	uint8_t packet_type:2;
-	uint16_t params_len;
-	uint8_t params[0];
-} __attribute__ ((packed));
-#define AVRCP_HEADER_LENGTH 7
-
-#else
-#error "Unknown byte order"
-#endif
-
 #define AVRCP_MTU	(AVC_MTU - AVC_HEADER_LENGTH)
 #define AVRCP_PDU_MTU	(AVRCP_MTU - AVRCP_HEADER_LENGTH)
 
-struct avrcp_browsing_header {
-	uint8_t pdu_id;
-	uint16_t param_len;
-	uint8_t params[0];
-} __attribute__ ((packed));
-#define AVRCP_BROWSING_HEADER_LENGTH 3
-
 struct get_folder_items_rsp {
 	uint8_t status;
 	uint16_t uid_counter;
@@ -2260,52 +2224,6 @@ static const char *status_to_string(uint8_t status)
 	}
 }
 
-/*
- * Pull the AVRCP header out of iov and validate that the parameters length
- * it declares matches the number of bytes actually received, leaving iov
- * pointing at the parameters.
- */
-static struct avrcp_header *avrcp_pull_header(struct iovec *iov)
-{
-	struct avrcp_header *pdu;
-
-	pdu = util_iov_pull_mem(iov, sizeof(*pdu));
-	if (!pdu) {
-		error("Invalid AVRCP header");
-		return NULL;
-	}
-
-	if (be16_to_cpu(pdu->params_len) != iov->iov_len) {
-		error("Invalid parameters");
-		return NULL;
-	}
-
-	return pdu;
-}
-
-/*
- * Same as avrcp_pull_header() but for the browsing channel, which uses a
- * different header layout.
- */
-static struct avrcp_browsing_header *avrcp_pull_browsing_header(
-							struct iovec *iov)
-{
-	struct avrcp_browsing_header *pdu;
-
-	pdu = util_iov_pull_mem(iov, sizeof(*pdu));
-	if (!pdu) {
-		error("Invalid AVRCP browsing header");
-		return NULL;
-	}
-
-	if (be16_to_cpu(pdu->param_len) != iov->iov_len) {
-		error("Invalid parameters");
-		return NULL;
-	}
-
-	return pdu;
-}
-
 static gboolean avrcp_get_play_status_rsp(struct avctp *conn, uint8_t code,
 					uint8_t subunit, uint8_t transaction,
 					uint8_t *operands, size_t operand_count,
@@ -2452,8 +2370,7 @@ static gboolean avrcp_list_player_attributes_rsp(struct avctp *conn,
 	struct iovec iov = { operands, operand_count };
 	uint8_t attrs[AVRCP_ATTRIBUTE_LAST];
 	struct avrcp *session = user_data;
-	uint8_t len, count = 0;
-	int i;
+	uint8_t count;
 
 	if (code == AVC_CTYPE_REJECTED || code == AVC_CTYPE_NOT_IMPLEMENTED)
 		return FALSE;
@@ -2461,25 +2378,7 @@ static gboolean avrcp_list_player_attributes_rsp(struct avctp *conn,
 	if (!avrcp_pull_header(&iov))
 		return FALSE;
 
-	if (!util_iov_pull_u8(&iov, &len))
-		return FALSE;
-
-	len = MIN(len, AVRCP_ATTRIBUTE_LAST);
-
-	for (i = 0; i < len; i++) {
-		uint8_t attr;
-
-		if (!util_iov_pull_u8(&iov, &attr))
-			break;
-
-		/* Don't query invalid attributes */
-		if (attr == AVRCP_ATTRIBUTE_ILLEGAL ||
-					attr > AVRCP_ATTRIBUTE_LAST)
-			continue;
-
-		attrs[count++] = attr;
-	}
-
+	count = avrcp_parse_player_attributes(&iov, attrs, sizeof(attrs));
 	if (!count)
 		return FALSE;
 
@@ -2505,34 +2404,38 @@ static void avrcp_list_player_attributes(struct avrcp *session)
 					session);
 }
 
-static void avrcp_parse_attribute_list(struct avrcp_player *player,
+struct parse_attribute_data {
+	struct media_player *mp;
+	struct media_item *item;
+};
+
+static void parse_attribute(const struct avrcp_attribute *attr,
+							void *user_data)
+{
+	struct parse_attribute_data *data = user_data;
+	const char *key;
+
+	if (attr->charset != 106)
+		return;
+
+	key = metadata_to_str(attr->id);
+	if (key == NULL)
+		return;
+
+	media_player_set_metadata(data->mp, data->item, key, attr->value,
+								attr->len);
+}
+
+static void avrcp_player_parse_attributes(struct avrcp_player *player,
 					struct media_item *item,
 					struct iovec *iov, uint8_t count)
 {
-	struct media_player *mp = player->user_data;
+	struct parse_attribute_data data = {
+		.mp = player->user_data,
+		.item = item,
+	};
 
-	for (; count > 0; count--) {
-		uint32_t id;
-		uint16_t charset, len;
-		uint8_t *value;
-
-		if (!util_iov_pull_be32(iov, &id) ||
-				!util_iov_pull_be16(iov, &charset) ||
-				!util_iov_pull_be16(iov, &len))
-			return;
-
-		value = util_iov_pull_mem(iov, len);
-		if (!value)
-			return;
-
-		if (charset == 106) {
-			const char *key = metadata_to_str(id);
-
-			if (key != NULL)
-				media_player_set_metadata(mp, item, key,
-								value, len);
-		}
-	}
+	avrcp_parse_attribute_list(iov, count, parse_attribute, &data);
 }
 
 static void avrcp_abort_continuing(struct avrcp *session, uint8_t pdu_id)
@@ -2589,7 +2492,7 @@ static gboolean avrcp_get_element_attributes_rsp(struct avctp *conn,
 
 	item = media_player_set_playlist_item(mp, player->uid);
 
-	avrcp_parse_attribute_list(player, item, &iov, count);
+	avrcp_player_parse_attributes(player, item, &iov, count);
 
 	media_player_metadata_changed(mp);
 
@@ -2671,57 +2574,28 @@ static const char *subtype_to_string(uint32_t subtype)
 	return "None";
 }
 
-static gboolean parse_media_name(struct iovec *iov, char *name)
-{
-	uint16_t namelen;
-	uint8_t *namebuf;
-
-	if (!util_iov_pull_be16(iov, &namelen))
-		return FALSE;
-
-	namebuf = util_iov_pull_mem(iov, namelen);
-	if (!namebuf)
-		return FALSE;
-
-	namelen = MIN(namelen, NAME_MAX_LEN - 1);
-
-	memset(name, 0, NAME_MAX_LEN);
-	memcpy(name, namebuf, namelen);
-	strtoutf8(name, namelen);
-
-	return TRUE;
-}
-
 static struct media_item *parse_media_element(struct avrcp *session,
 							struct iovec *iov)
 {
 	struct avrcp_player *player;
 	struct media_player *mp;
 	struct media_item *item;
-	char name[NAME_MAX_LEN];
-	uint64_t uid;
-	uint8_t count;
+	struct avrcp_media_element element;
 
-	/* Skip the media type and character set */
-	if (!util_iov_pull_be64(iov, &uid) || !util_iov_pull(iov, 3))
-		return NULL;
-
-	if (!parse_media_name(iov, name))
-		return NULL;
-
-	if (!util_iov_pull_u8(iov, &count))
+	if (!avrcp_parse_media_element(iov, &element))
 		return NULL;
 
 	player = session->controller->player;
 	mp = player->user_data;
 
-	item = media_player_create_item(mp, name, PLAYER_ITEM_TYPE_AUDIO, uid);
+	item = media_player_create_item(mp, element.name,
+					PLAYER_ITEM_TYPE_AUDIO, element.uid);
 	if (item == NULL)
 		return NULL;
 
 	media_item_set_playable(item, true);
 
-	avrcp_parse_attribute_list(player, item, iov, count);
+	avrcp_player_parse_attributes(player, item, iov, element.count);
 
 	return item;
 }
@@ -2732,26 +2606,17 @@ static struct media_item *parse_media_folder(struct avrcp *session,
 	struct avrcp_player *player = session->controller->player;
 	struct media_player *mp = player->user_data;
 	struct media_item *item;
-	char name[NAME_MAX_LEN];
-	uint64_t uid;
-	uint8_t type;
-	uint8_t playable;
+	struct avrcp_media_folder folder;
 
-	/* Skip the character set */
-	if (!util_iov_pull_be64(iov, &uid) ||
-			!util_iov_pull_u8(iov, &type) ||
-			!util_iov_pull_u8(iov, &playable) ||
-			!util_iov_pull(iov, 2))
+	if (!avrcp_parse_media_folder(iov, &folder))
 		return NULL;
 
-	if (!parse_media_name(iov, name))
-		return NULL;
-
-	item = media_player_create_folder(mp, name, type, uid);
+	item = media_player_create_folder(mp, folder.name, folder.type,
+								folder.uid);
 	if (!item)
 		return NULL;
 
-	media_item_set_playable(item, playable & 0x01);
+	media_item_set_playable(item, folder.playable & 0x01);
 
 	return item;
 }
@@ -3043,7 +2908,7 @@ static gboolean avrcp_get_item_attributes_rsp(struct avctp *conn,
 
 	item = media_player_set_playlist_item(mp, player->uid);
 
-	avrcp_parse_attribute_list(player, item, &iov, count);
+	avrcp_player_parse_attributes(player, item, &iov, count);
 
 	media_player_metadata_changed(mp);
 
diff --git a/profiles/audio/avrcp.h b/profiles/audio/avrcp.h
index 21351a4bc422..9f72c2ed4413 100644
--- a/profiles/audio/avrcp.h
+++ b/profiles/audio/avrcp.h
@@ -9,13 +9,7 @@
  *
  */
 
-/* player attributes */
-#define AVRCP_ATTRIBUTE_ILLEGAL		0x00
-#define AVRCP_ATTRIBUTE_EQUALIZER	0x01
-#define AVRCP_ATTRIBUTE_REPEAT_MODE	0x02
-#define AVRCP_ATTRIBUTE_SHUFFLE		0x03
-#define AVRCP_ATTRIBUTE_SCAN		0x04
-#define AVRCP_ATTRIBUTE_LAST		AVRCP_ATTRIBUTE_SCAN
+#include "avrcp-parse.h"
 
 /* equalizer values */
 #define AVRCP_EQUALIZER_OFF		0x01
-- 
2.54.0

[PATCH BlueZ v1 5/5] unit/test-avrcp: Add robustness tests for response parsing

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

The existing tests drive avrcp-lib.c through the AVCTP harness and all
feed it well formed PDUs. Nothing covered what happens when a peer
sends a response that lies about its own length, which is what the
three preceding fixes were about.

Add tests under /robustness that call the parsers in avrcp-parse.c
directly, since a response is entirely peer controlled and the parser
is what has to survive it:

- headers that are short, that declare more parameter bytes than were
  received, and that declare fewer

- a ListPlayerApplicationSettingAttributes response declaring 255
  attributes, which used to be written into a four byte array, and one
  declaring more attributes than it carries

- attribute lists declaring a 0xFFFF byte value with none of it
  present, a truncated value, a truncated attribute header, and more
  attributes than were received

- media elements missing the attribute count that follows the name,
  carrying a name longer than NAME_MAX_LEN, or declaring a name that is
  not there, and the equivalent for media folders

Each PDU is copied into a buffer of exactly its size, so that reading
past the end of it is an out-of-bounds access rather than a read of
whatever the receive buffer happened to hold beforehand, and the
attribute array is surrounded by a guard so that a write past its end
is caught without a sanitizer.

Reverting the three fixes fails ten of these outright and trips
valgrind on six more.

Assisted-by: Claude:claude-opus-5 valgrind
---
 Makefile.am       |   4 +-
 unit/test-avrcp.c | 395 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 398 insertions(+), 1 deletion(-)

diff --git a/Makefile.am b/Makefile.am
index 475a344c713d..f0028cfb39f8 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -674,7 +674,9 @@ unit_tests += unit/test-avrcp
 unit_test_avrcp_SOURCES = unit/test-avrcp.c \
 				src/log.h src/log.c \
 				unit/avctp.c unit/avctp.h \
-				unit/avrcp-lib.c unit/avrcp-lib.h
+				unit/avrcp-lib.c unit/avrcp-lib.h \
+				profiles/audio/avrcp-parse.h \
+				profiles/audio/avrcp-parse.c
 unit_test_avrcp_LDADD = lib/libbluetooth-internal.la \
 				src/libshared-glib.la $(GLIB_LIBS)
 
diff --git a/unit/test-avrcp.c b/unit/test-avrcp.c
index 7bed8fbaf74a..e0c8971514ba 100644
--- a/unit/test-avrcp.c
+++ b/unit/test-avrcp.c
@@ -30,6 +30,7 @@
 
 #include "unit/avctp.h"
 #include "unit/avrcp-lib.h"
+#include "profiles/audio/avrcp-parse.h"
 
 struct test_pdu {
 	bool valid;
@@ -986,6 +987,398 @@ static void test_client(gconstpointer data)
 		avrcp_send_passthrough(context->session, 0, AVC_FAST_FORWARD);
 }
 
+/*
+ * Robustness tests for the controller side response parsers.
+ *
+ * These call the parsers directly rather than going through the AVCTP
+ * harness, since responses are what the peer controls and the parsers are
+ * what has to survive them. Each PDU is copied into a buffer of exactly its
+ * size, so that reading past the end of it is an out-of-bounds access rather
+ * than a read of whatever the receive buffer happened to hold before.
+ */
+
+struct robustness_test {
+	char *test_name;
+	uint8_t *data;
+	size_t size;
+	/* Expected result, or -1 if the PDU must be rejected */
+	int expected;
+};
+
+#define define_robustness_test(name, function, exp, args...)		\
+	do {								\
+		static struct robustness_test rt;			\
+		rt.test_name = g_strdup(name);				\
+		rt.data = util_memdup(data(args), sizeof(data(args)));	\
+		rt.size = sizeof(data(args));				\
+		rt.expected = exp;					\
+		tester_add(name, &rt, NULL, function, NULL);		\
+	} while (0)
+
+/* AVRCP header: BT SIG company id, pdu id, packet type, parameters length */
+#define AVRCP_HDR(pdu_id, len)						\
+	0x00, 0x19, 0x58, pdu_id, 0x00, ((len) >> 8) & 0xff, (len) & 0xff
+
+#define X4	'x', 'x', 'x', 'x'
+#define X16	X4, X4, X4, X4
+#define X64	X16, X16, X16, X16
+#define X256	X64, X64, X64, X64
+#define LONG_NAME_260	X256, X4
+
+static void robustness_result(struct robustness_test *rt, void *buf,
+								int result)
+{
+	free(buf);
+
+	if (result != rt->expected) {
+		tester_warn("%s: expected %d, got %d", rt->test_name,
+							rt->expected, result);
+		tester_test_failed();
+		return;
+	}
+
+	tester_test_passed();
+}
+
+static void *robustness_iov(const struct robustness_test *rt,
+							struct iovec *iov)
+{
+	iov->iov_base = util_memdup(rt->data, rt->size);
+	iov->iov_len = rt->size;
+
+	return iov->iov_base;
+}
+
+/* Expected is the number of parameter bytes left, or -1 if rejected */
+static void test_pull_header(gconstpointer data)
+{
+	struct robustness_test *rt = (void *) data;
+	struct iovec iov;
+	void *buf = robustness_iov(rt, &iov);
+
+	if (!avrcp_pull_header(&iov)) {
+		robustness_result(rt, buf, -1);
+		return;
+	}
+
+	robustness_result(rt, buf, iov.iov_len);
+}
+
+static void test_pull_browsing_header(gconstpointer data)
+{
+	struct robustness_test *rt = (void *) data;
+	struct iovec iov;
+	void *buf = robustness_iov(rt, &iov);
+
+	if (!avrcp_pull_browsing_header(&iov)) {
+		robustness_result(rt, buf, -1);
+		return;
+	}
+
+	robustness_result(rt, buf, iov.iov_len);
+}
+
+/*
+ * The attribute count declared by the peer is what bounds the write into
+ * attrs, so surround it with a guard and check that nothing was written
+ * past its end. Expected is the number of attributes accepted.
+ */
+#define ATTRS_GUARD 8
+
+static void test_player_attributes(gconstpointer data)
+{
+	struct robustness_test *rt = (void *) data;
+	uint8_t attrs[AVRCP_ATTRIBUTE_LAST + ATTRS_GUARD];
+	struct iovec iov;
+	void *buf = robustness_iov(rt, &iov);
+	uint8_t count;
+	int i;
+
+	memset(attrs, 0xaa, sizeof(attrs));
+
+	if (!avrcp_pull_header(&iov)) {
+		robustness_result(rt, buf, -1);
+		return;
+	}
+
+	count = avrcp_parse_player_attributes(&iov, attrs,
+						AVRCP_ATTRIBUTE_LAST);
+
+	for (i = 0; i < ATTRS_GUARD; i++) {
+		if (attrs[AVRCP_ATTRIBUTE_LAST + i] == 0xaa)
+			continue;
+
+		tester_warn("%s: wrote %u bytes past the end of attrs",
+					rt->test_name, ATTRS_GUARD - i);
+		free(buf);
+		tester_test_failed();
+		return;
+	}
+
+	robustness_result(rt, buf, count);
+}
+
+static void count_attribute(const struct avrcp_attribute *attr,
+							void *user_data)
+{
+	unsigned int *count = user_data;
+	unsigned int sum = 0;
+	uint16_t i;
+
+	/* Read the whole value so that a bogus length is caught */
+	for (i = 0; i < attr->len; i++)
+		sum += attr->value[i];
+
+	(void) sum;
+
+	(*count)++;
+}
+
+/* Expected is the number of attributes reported */
+static void test_attribute_list(gconstpointer data)
+{
+	struct robustness_test *rt = (void *) data;
+	struct iovec iov;
+	void *buf = robustness_iov(rt, &iov);
+	unsigned int count = 0;
+	uint8_t number;
+
+	if (!avrcp_pull_header(&iov) || !util_iov_pull_u8(&iov, &number)) {
+		robustness_result(rt, buf, -1);
+		return;
+	}
+
+	avrcp_parse_attribute_list(&iov, number, count_attribute, &count);
+
+	robustness_result(rt, buf, count);
+}
+
+/* Expected is the declared attribute count, or -1 if rejected */
+static void test_media_element(gconstpointer data)
+{
+	struct robustness_test *rt = (void *) data;
+	struct iovec iov;
+	void *buf = robustness_iov(rt, &iov);
+	struct avrcp_media_element element;
+
+	if (!avrcp_parse_media_element(&iov, &element)) {
+		robustness_result(rt, buf, -1);
+		return;
+	}
+
+	/* The name must always be truncated to fit */
+	if (strlen(element.name) >= NAME_MAX_LEN) {
+		tester_warn("%s: name not truncated", rt->test_name);
+		free(buf);
+		tester_test_failed();
+		return;
+	}
+
+	robustness_result(rt, buf, element.count);
+}
+
+/* Expected is the playable flag, or -1 if rejected */
+static void test_media_folder(gconstpointer data)
+{
+	struct robustness_test *rt = (void *) data;
+	struct iovec iov;
+	void *buf = robustness_iov(rt, &iov);
+	struct avrcp_media_folder folder;
+
+	if (!avrcp_parse_media_folder(&iov, &folder)) {
+		robustness_result(rt, buf, -1);
+		return;
+	}
+
+	if (strlen(folder.name) >= NAME_MAX_LEN) {
+		tester_warn("%s: name not truncated", rt->test_name);
+		free(buf);
+		tester_test_failed();
+		return;
+	}
+
+	robustness_result(rt, buf, folder.playable);
+}
+
+static void define_robustness_tests(void)
+{
+	/*
+	 * Responses do not go through handle_vendordep_pdu(), so nothing
+	 * validated the declared parameters length against the number of
+	 * bytes actually received.
+	 */
+
+	/* One byte short of a complete header */
+	define_robustness_test("/robustness/header/short",
+			test_pull_header, -1,
+			0x00, 0x19, 0x58, 0x10, 0x00, 0x00);
+
+	/* Declares 16 parameter bytes but carries one */
+	define_robustness_test("/robustness/header/truncated",
+			test_pull_header, -1,
+			AVRCP_HDR(0x10, 16), 0x04);
+
+	/* Declares fewer parameter bytes than were received */
+	define_robustness_test("/robustness/header/overlong",
+			test_pull_header, -1,
+			AVRCP_HDR(0x10, 1), 0x04, 0x01, 0x02);
+
+	define_robustness_test("/robustness/header/valid",
+			test_pull_header, 2,
+			AVRCP_HDR(0x10, 2), 0x01, 0x04);
+
+	define_robustness_test("/robustness/browsing-header/short",
+			test_pull_browsing_header, -1,
+			0x71, 0x00);
+
+	/* Declares 32 parameter bytes but carries one */
+	define_robustness_test("/robustness/browsing-header/truncated",
+			test_pull_browsing_header, -1,
+			0x71, 0x00, 0x20, 0x04);
+
+	define_robustness_test("/robustness/browsing-header/valid",
+			test_pull_browsing_header, 2,
+			0x71, 0x00, 0x02, 0x04, 0x01);
+
+	/*
+	 * ListPlayerApplicationSettingAttributes response, see
+	 * GHSA-m2vx-pw5f-rc8v. The declared count is what bounds the write
+	 * into a four byte array.
+	 */
+
+	/* Declares and carries 255 valid attributes */
+	define_robustness_test("/robustness/player-attributes/overflow",
+			test_player_attributes, AVRCP_ATTRIBUTE_LAST,
+			AVRCP_HDR(0x11, 21), 0xff,
+			0x01, 0x02, 0x03, 0x04, 0x01, 0x02, 0x03, 0x04,
+			0x01, 0x02, 0x03, 0x04, 0x01, 0x02, 0x03, 0x04,
+			0x01, 0x02, 0x03, 0x04);
+
+	/* Declares four attributes but carries two */
+	define_robustness_test("/robustness/player-attributes/truncated",
+			test_player_attributes, 2,
+			AVRCP_HDR(0x11, 3), 0x04, 0x01, 0x02);
+
+	/* Declares one attribute but carries none */
+	define_robustness_test("/robustness/player-attributes/empty",
+			test_player_attributes, 0,
+			AVRCP_HDR(0x11, 1), 0x01);
+
+	/* Illegal and out of range attributes must be skipped */
+	define_robustness_test("/robustness/player-attributes/illegal",
+			test_player_attributes, 1,
+			AVRCP_HDR(0x11, 5), 0x04,
+			AVRCP_ATTRIBUTE_ILLEGAL, 0x7f,
+			AVRCP_ATTRIBUTE_SHUFFLE, 0xff);
+
+	/*
+	 * GetElementAttributes and GetItemAttributes carry variable length
+	 * attribute values which were never bounds checked.
+	 */
+
+	/* Declares a 0xFFFF byte value with none of it present */
+	define_robustness_test("/robustness/attribute-list/huge-len",
+			test_attribute_list, 0,
+			AVRCP_HDR(0x20, 9), 0x01,
+			0x00, 0x00, 0x00, 0x01,		/* Title */
+			0x00, 0x6a,			/* UTF-8 */
+			0xff, 0xff);			/* value length */
+
+	/* Declares a four byte value but carries two */
+	define_robustness_test("/robustness/attribute-list/truncated-value",
+			test_attribute_list, 0,
+			AVRCP_HDR(0x20, 11), 0x01,
+			0x00, 0x00, 0x00, 0x01,
+			0x00, 0x6a,
+			0x00, 0x04,
+			'a', 'b');
+
+	/* Declares one attribute but carries a partial header for it */
+	define_robustness_test("/robustness/attribute-list/truncated-header",
+			test_attribute_list, 0,
+			AVRCP_HDR(0x20, 4), 0x01,
+			0x00, 0x00, 0x00);
+
+	/* Declares 255 attributes but carries one */
+	define_robustness_test("/robustness/attribute-list/count-overrun",
+			test_attribute_list, 1,
+			AVRCP_HDR(0x20, 12), 0xff,
+			0x00, 0x00, 0x00, 0x01,
+			0x00, 0x6a,
+			0x00, 0x03,
+			'a', 'b', 'c');
+
+	define_robustness_test("/robustness/attribute-list/valid",
+			test_attribute_list, 2,
+			AVRCP_HDR(0x20, 18), 0x02,
+			0x00, 0x00, 0x00, 0x01,
+			0x00, 0x6a,
+			0x00, 0x01, 'a',
+			0x00, 0x00, 0x00, 0x02,
+			0x00, 0x6a,
+			0x00, 0x00);
+
+	/* Media element and folder entries of a GetFolderItems response */
+
+	/* UID, media type and character set only, no name length */
+	define_robustness_test("/robustness/media-element/truncated",
+			test_media_element, -1,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01,
+			0x02, 0x00, 0x6a);
+
+	/* Declares a 0xFFFF byte name with none of it present */
+	define_robustness_test("/robustness/media-element/huge-name",
+			test_media_element, -1,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01,
+			0x02, 0x00, 0x6a,
+			0xff, 0xff);
+
+	/*
+	 * The name is complete but the attribute count byte that follows it
+	 * is not present. This is the off-by-one that used to read
+	 * operands[13 + namesize].
+	 */
+	define_robustness_test("/robustness/media-element/no-count",
+			test_media_element, -1,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01,
+			0x02, 0x00, 0x6a,
+			0x00, 0x03, 'a', 'b', 'c');
+
+	/* A name longer than NAME_MAX_LEN must be truncated, not overflow */
+	define_robustness_test("/robustness/media-element/long-name",
+			test_media_element, 0,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01,
+			0x02, 0x00, 0x6a,
+			0x01, 0x04,	/* 260 byte name */
+			LONG_NAME_260,
+			0x00);
+
+	define_robustness_test("/robustness/media-element/valid",
+			test_media_element, 1,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x2a,
+			0x02, 0x00, 0x6a,
+			0x00, 0x03, 'a', 'b', 'c',
+			0x01);
+
+	/* UID, folder type and playable flag only */
+	define_robustness_test("/robustness/media-folder/truncated",
+			test_media_folder, -1,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01,
+			0x01, 0x01);
+
+	define_robustness_test("/robustness/media-folder/huge-name",
+			test_media_folder, -1,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01,
+			0x01, 0x01, 0x00, 0x6a,
+			0xff, 0xff, 'a');
+
+	define_robustness_test("/robustness/media-folder/valid",
+			test_media_folder, 1,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x07,
+			0x01, 0x01, 0x00, 0x6a,
+			0x00, 0x03, 'a', 'b', 'c');
+}
+
 int main(int argc, char *argv[])
 {
 	tester_init(&argc, &argv);
@@ -2080,5 +2473,7 @@ int main(int argc, char *argv[])
 				0x00, 0x19, 0x58, AVRCP_ABORT_CONTINUING,
 				0x00, 0x00, 0x00));
 
+	define_robustness_tests();
+
 	return tester_run();
 }
-- 
2.54.0

RE: [BlueZ,v1,1/5] avrcp: Fix out-of-bounds parsing of ListPlayerAttributes response

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 4673 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=1155564

---Test result---

Test Summary:
CheckPatch                    FAIL      2.45 seconds
GitLint                       PASS      1.20 seconds
BuildEll                      PASS      18.31 seconds
BluezMake                     PASS      650.51 seconds
MakeCheck                     PASS      18.63 seconds
MakeDistcheck                 PASS      146.15 seconds
CheckValgrind                 PASS      220.71 seconds
CheckSmatch                   WARNING   273.13 seconds
bluezmakeextell               PASS      95.39 seconds
IncrementalBuild              PASS      819.11 seconds
ScanBuild                     PASS      882.40 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ,v1,1/5] avrcp: Fix out-of-bounds parsing of ListPlayerAttributes response
ERROR:BAD_SIGN_OFF: Unrecognized email address: '@ax-nnlabs'
#121: 
Reported-by: @ax-nnlabs

WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#122: 
Closes: https://github.com/bluez/bluez/security/advisories/GHSA-m2vx-pw5f-rc8v

/github/workspace/src/patch/14781994.patch total: 1 errors, 1 warnings, 56 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/patch/14781994.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


[BlueZ,v1,4/5] avrcp: Move response parsers to avrcp-parse.c
WARNING:BAD_SIGN_OFF: Non-standard signature: Assisted-by:
#127: 
Assisted-by: Claude:claude-opus-5 valgrind

ERROR:BAD_SIGN_OFF: Unrecognized email address: 'Claude:claude-opus-5 valgrind'
#127: 
Assisted-by: Claude:claude-opus-5 valgrind

WARNING:PREFER_DEFINED_ATTRIBUTE_MACRO: Prefer __packed over __attribute__((packed))
#373: FILE: profiles/audio/avrcp-parse.h:37:
+} __attribute__ ((packed));

WARNING:PREFER_DEFINED_ATTRIBUTE_MACRO: Prefer __packed over __attribute__((packed))
#384: FILE: profiles/audio/avrcp-parse.h:48:
+} __attribute__ ((packed));

WARNING:PREFER_DEFINED_ATTRIBUTE_MACRO: Prefer __packed over __attribute__((packed))
#396: FILE: profiles/audio/avrcp-parse.h:60:
+} __attribute__ ((packed));

/github/workspace/src/patch/14781997.patch total: 1 errors, 4 warnings, 609 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/patch/14781997.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


[BlueZ,v1,5/5] unit/test-avrcp: Add robustness tests for response parsing
WARNING:BAD_SIGN_OFF: Non-standard signature: Assisted-by:
#135: 
Assisted-by: Claude:claude-opus-5 valgrind

ERROR:BAD_SIGN_OFF: Unrecognized email address: 'Claude:claude-opus-5 valgrind'
#135: 
Assisted-by: Claude:claude-opus-5 valgrind

/github/workspace/src/patch/14781998.patch total: 1 errors, 1 warnings, 422 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/patch/14781998.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
unit/test-avrcp.c:374:26: warning: Variable length array is used.unit/test-avrcp.c:399:26: warning: Variable length array is used.unit/test-avrcp.c:415:24: warning: Variable length array is used.


https://github.com/bluez/bluez/pull/2467

---
Regards,
Linux Bluetooth

Re: [PATCH BlueZ v1 1/5] avrcp: Fix out-of-bounds parsing of ListPlayerAttributes response

[Bastien Nocera]

On Tue, 2026-09-01 at 13:53 -0400, Luiz Augusto von Dentz wrote:
> From: Bastien Nocera <hadess@hadess.net>

The patch has substantially changed from the version I posted
privately, so you can remove my authorship.

> In profiles/audio/avrcp.c, avrcp_list_player_attributes_rsp() parsed
> the
> response using hand-computed offsets into the operands buffer,
> without
> accounting for the fact that operand_count spans the 7 byte AVRCP
> header
> as well as the parameters:
> 
> - attrs is a 4 byte array which could be written out-of-bounds if a
>   length greater than 4 was declared in the first parameter byte.
> 
> - The attribute bytes were read with a bound derived from
> operand_count,
>   so a truncated response could be read past its end. As the receive
>   buffer is reused across packets, those stale bytes could be echoed
>   back to the peer in the following GetCurrentPlayerValue request.
> 
> - params_len was compared against count, which was only ever 0 at
> that
>   point, so the length of the PDU was in practice never validated.
> 
> Parse the response through a struct iovec using the util_iov_pull_*
> helpers instead, so that the header and each subsequent field are
> bounds
> checked as they are consumed and the remaining length is tracked for
> us.
> This lets params_len be validated against the actual number of
> parameter
> bytes received. The attribute count is still clamped to
> AVRCP_ATTRIBUTE_LAST, which is what bounds the write into attrs.
> 
> Reported-by: @ax-nnlabs
> Closes:
> https://github.com/bluez/bluez/security/advisories/GHSA-m2vx-pw5f-rc8v

It was embargoed, and it's now been made public.

> ---
>  profiles/audio/avrcp.c | 32 +++++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> index 3271b84782e8..906f93424872 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -2395,31 +2395,49 @@ static gboolean
> avrcp_list_player_attributes_rsp(struct avctp *conn,
>  					uint8_t transaction, uint8_t
> *operands,
>  					size_t operand_count, void
> *user_data)
>  {
> +	struct iovec iov = { operands, operand_count };
>  	uint8_t attrs[AVRCP_ATTRIBUTE_LAST];
>  	struct avrcp *session = user_data;
> -	struct avrcp_header *pdu = (void *) operands;
> +	struct avrcp_header *pdu;
>  	uint8_t len, count = 0;
>  	int i;
>  
>  	if (code == AVC_CTYPE_REJECTED || code ==
> AVC_CTYPE_NOT_IMPLEMENTED)
>  		return FALSE;
>  
> -	len = pdu->params[0];
> +	pdu = util_iov_pull_mem(&iov, sizeof(*pdu));
> +	if (!pdu) {
> +		error("Invalid AVRCP header");
> +		return FALSE;
> +	}
>  
> -	if (be16_to_cpu(pdu->params_len) < count) {
> +	if (be16_to_cpu(pdu->params_len) != iov.iov_len) {
>  		error("Invalid parameters");
>  		return FALSE;
>  	}
>  
> -	for (i = 0; len > 0; len--, i++) {
> +	if (!util_iov_pull_u8(&iov, &len))
> +		return FALSE;
> +
> +	len = MIN(len, AVRCP_ATTRIBUTE_LAST);
> +
> +	for (i = 0; i < len; i++) {
> +		uint8_t attr;
> +
> +		if (!util_iov_pull_u8(&iov, &attr))
> +			break;
> +
>  		/* Don't query invalid attributes */
> -		if (pdu->params[i + 1] == AVRCP_ATTRIBUTE_ILLEGAL ||
> -				pdu->params[i + 1] >
> AVRCP_ATTRIBUTE_LAST)
> +		if (attr == AVRCP_ATTRIBUTE_ILLEGAL ||
> +					attr > AVRCP_ATTRIBUTE_LAST)
>  			continue;
>  
> -		attrs[count++] = pdu->params[i + 1];
> +		attrs[count++] = attr;
>  	}
>  
> +	if (!count)
> +		return FALSE;
> +
>  	avrcp_get_current_player_value(session, attrs, count);
>  
>  	return FALSE;

Re: [PATCH BlueZ v1 1/5] avrcp: Fix out-of-bounds parsing of ListPlayerAttributes response

[Bastien Nocera]

On Thu, 2026-09-03 at 15:02 +0200, Bastien Nocera wrote:
> On Tue, 2026-09-01 at 13:53 -0400, Luiz Augusto von Dentz wrote:
> > From: Bastien Nocera <hadess@hadess.net>
> 
> The patch has substantially changed from the version I posted
> privately, so you can remove my authorship.
> 
> > In profiles/audio/avrcp.c, avrcp_list_player_attributes_rsp()
> > parsed
> > the
> > response using hand-computed offsets into the operands buffer,
> > without
> > accounting for the fact that operand_count spans the 7 byte AVRCP
> > header
> > as well as the parameters:
> > 
> > - attrs is a 4 byte array which could be written out-of-bounds if a
> >   length greater than 4 was declared in the first parameter byte.
> > 
> > - The attribute bytes were read with a bound derived from
> > operand_count,
> >   so a truncated response could be read past its end. As the
> > receive
> >   buffer is reused across packets, those stale bytes could be
> > echoed
> >   back to the peer in the following GetCurrentPlayerValue request.
> > 
> > - params_len was compared against count, which was only ever 0 at
> > that
> >   point, so the length of the PDU was in practice never validated.
> > 
> > Parse the response through a struct iovec using the util_iov_pull_*
> > helpers instead, so that the header and each subsequent field are
> > bounds
> > checked as they are consumed and the remaining length is tracked
> > for
> > us.
> > This lets params_len be validated against the actual number of
> > parameter
> > bytes received. The attribute count is still clamped to
> > AVRCP_ATTRIBUTE_LAST, which is what bounds the write into attrs.
> > 
> > Reported-by: @ax-nnlabs
> > Closes:
> > https://github.com/bluez/bluez/security/advisories/GHSA-m2vx-pw5f-rc8v
> 
> It was embargoed, and it's now been made public.

You can reference CVE-2026-85218 for this one, thanks!

> 
> > ---
> >  profiles/audio/avrcp.c | 32 +++++++++++++++++++++++++-------
> >  1 file changed, 25 insertions(+), 7 deletions(-)
> > 
> > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > index 3271b84782e8..906f93424872 100644
> > --- a/profiles/audio/avrcp.c
> > +++ b/profiles/audio/avrcp.c
> > @@ -2395,31 +2395,49 @@ static gboolean
> > avrcp_list_player_attributes_rsp(struct avctp *conn,
> >  					uint8_t transaction,
> > uint8_t
> > *operands,
> >  					size_t operand_count, void
> > *user_data)
> >  {
> > +	struct iovec iov = { operands, operand_count };
> >  	uint8_t attrs[AVRCP_ATTRIBUTE_LAST];
> >  	struct avrcp *session = user_data;
> > -	struct avrcp_header *pdu = (void *) operands;
> > +	struct avrcp_header *pdu;
> >  	uint8_t len, count = 0;
> >  	int i;
> >  
> >  	if (code == AVC_CTYPE_REJECTED || code ==
> > AVC_CTYPE_NOT_IMPLEMENTED)
> >  		return FALSE;
> >  
> > -	len = pdu->params[0];
> > +	pdu = util_iov_pull_mem(&iov, sizeof(*pdu));
> > +	if (!pdu) {
> > +		error("Invalid AVRCP header");
> > +		return FALSE;
> > +	}
> >  
> > -	if (be16_to_cpu(pdu->params_len) < count) {
> > +	if (be16_to_cpu(pdu->params_len) != iov.iov_len) {
> >  		error("Invalid parameters");
> >  		return FALSE;
> >  	}
> >  
> > -	for (i = 0; len > 0; len--, i++) {
> > +	if (!util_iov_pull_u8(&iov, &len))
> > +		return FALSE;
> > +
> > +	len = MIN(len, AVRCP_ATTRIBUTE_LAST);
> > +
> > +	for (i = 0; i < len; i++) {
> > +		uint8_t attr;
> > +
> > +		if (!util_iov_pull_u8(&iov, &attr))
> > +			break;
> > +
> >  		/* Don't query invalid attributes */
> > -		if (pdu->params[i + 1] == AVRCP_ATTRIBUTE_ILLEGAL
> > ||
> > -				pdu->params[i + 1] >
> > AVRCP_ATTRIBUTE_LAST)
> > +		if (attr == AVRCP_ATTRIBUTE_ILLEGAL ||
> > +					attr >
> > AVRCP_ATTRIBUTE_LAST)
> >  			continue;
> >  
> > -		attrs[count++] = pdu->params[i + 1];
> > +		attrs[count++] = attr;
> >  	}
> >  
> > +	if (!count)
> > +		return FALSE;
> > +
> >  	avrcp_get_current_player_value(session, attrs, count);
> >  
> >  	return FALSE;