Bluetooth: MGMT: reject HCI_CMD_SYNC params_len above 255

[PATCH] Bluetooth: MGMT: reject HCI_CMD_SYNC params_len above 255

Posted by Ali Ahmet Memis 1 month, 3 weeks ago

mgmt_hci_cmd_sync() checks that the message length agrees with params_len
but puts no upper bound on it. params_len is __le16 while the parameter
length in the HCI command header is a u8:

	struct hci_command_hdr {
		__le16	opcode;
		__u8	plen;
	} __packed;

hci_cmd_sync_alloc() assigns one to the other:

	hdr->plen = plen;

	if (plen)
		skb_put_data(skb, param, plen);

so a params_len of 256 leaves plen at 0 while all 256 bytes are still
appended. The frame handed to the driver then declares no parameters and
carries 256 of them. On a length framed transport such as H:4 the
controller takes the trailing bytes as the start of the next packet.

The mgmt socket MTU is HCI_MAX_FRAME_SIZE, so params_len can reach about
1KB this way. Commit 03f1700b9b4d ("Bluetooth: MGMT: reject malformed
HCI_CMD_SYNC commands") only made params_len agree with the message
length, a value that fits the message but not the header field is still
accepted.

Reject params_len that does not fit the header field.

Fixes: 827af4787e74 ("Bluetooth: MGMT: Add initial implementation of MGMT_OP_HCI_CMD_SYNC")
Cc: stable@vger.kernel.org
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
Checked on a vhci controller, with the emulated controller printing what
it receives on the transport.

Before:

  sending HCI_CMD_SYNC with params_len=256
  controller saw: read()=260 bytes, hdr.plen=0, params carried=256
  first params bytes: aa aa aa, last: 5a
  HCI_CMD_SYNC: opcode 0x005b status 0x00

After:

  sending HCI_CMD_SYNC with params_len=4
  controller saw: read()=8 bytes, hdr.plen=4, params carried=4
  valid: opcode 0x005b status 0x00
  sending HCI_CMD_SYNC with params_len=256
  HCI_CMD_SYNC: opcode 0x005b cmd status 0x0d

One thing I left alone: hci_cmd_sync_alloc() performs the same truncation
for every caller, so a guard there would cover more than this one entry
point. All in kernel callers I looked at pass a fixed sizeof(), except
msft_add_monitor_sync() which computes total_size from the pattern list a
user supplies through MGMT_OP_ADD_ADV_PATTERNS_MONITOR. That one looks
like it can exceed 255 as well, but it needs a controller with MSFT
extension support and I have not verified it, so I am not claiming it
here. Happy to send a follow up for either if you want it.

 net/bluetooth/mgmt.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 167d75e34526..c16b0b80c193 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2668,6 +2668,14 @@ static int mgmt_hci_cmd_sync(struct sock *sk, struct hci_dev *hdev,
 		return mgmt_cmd_status(sk, hdev->id, MGMT_OP_HCI_CMD_SYNC,
 				       MGMT_STATUS_INVALID_PARAMS);
 
+	/* The HCI command header carries the parameter length in a u8, a
+	 * larger value would be truncated there while the parameters are
+	 * still appended to the frame in full.
+	 */
+	if (le16_to_cpu(cp->params_len) > U8_MAX)
+		return mgmt_cmd_status(sk, hdev->id, MGMT_OP_HCI_CMD_SYNC,
+				       MGMT_STATUS_INVALID_PARAMS);
+
 	hci_dev_lock(hdev);
 	cmd = mgmt_pending_new(sk, MGMT_OP_HCI_CMD_SYNC, hdev, data, len);
 	if (!cmd)

base-commit: 0d839570765118029aa8bf4a95444c6a11aacf85
-- 
2.55.0

Re: [PATCH] Bluetooth: MGMT: reject HCI_CMD_SYNC params_len above 255

Posted by Luiz Augusto von Dentz 1 month, 3 weeks ago

Hi Ali,

On Thu, Aug 6, 2026 at 1:40 PM Ali Ahmet Memis <ali@iusegentoo.com> wrote:
>
> mgmt_hci_cmd_sync() checks that the message length agrees with params_len
> but puts no upper bound on it. params_len is __le16 while the parameter
> length in the HCI command header is a u8:
>
>         struct hci_command_hdr {
>                 __le16  opcode;
>                 __u8    plen;
>         } __packed;
>
> hci_cmd_sync_alloc() assigns one to the other:
>
>         hdr->plen = plen;
>
>         if (plen)
>                 skb_put_data(skb, param, plen);
>
> so a params_len of 256 leaves plen at 0 while all 256 bytes are still
> appended. The frame handed to the driver then declares no parameters and
> carries 256 of them. On a length framed transport such as H:4 the
> controller takes the trailing bytes as the start of the next packet.
>
> The mgmt socket MTU is HCI_MAX_FRAME_SIZE, so params_len can reach about
> 1KB this way. Commit 03f1700b9b4d ("Bluetooth: MGMT: reject malformed
> HCI_CMD_SYNC commands") only made params_len agree with the message
> length, a value that fits the message but not the header field is still
> accepted.
>
> Reject params_len that does not fit the header field.
>
> Fixes: 827af4787e74 ("Bluetooth: MGMT: Add initial implementation of MGMT_OP_HCI_CMD_SYNC")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
> ---
> Checked on a vhci controller, with the emulated controller printing what
> it receives on the transport.
>
> Before:
>
>   sending HCI_CMD_SYNC with params_len=256
>   controller saw: read()=260 bytes, hdr.plen=0, params carried=256
>   first params bytes: aa aa aa, last: 5a
>   HCI_CMD_SYNC: opcode 0x005b status 0x00
>
> After:
>
>   sending HCI_CMD_SYNC with params_len=4
>   controller saw: read()=8 bytes, hdr.plen=4, params carried=4
>   valid: opcode 0x005b status 0x00
>   sending HCI_CMD_SYNC with params_len=256
>   HCI_CMD_SYNC: opcode 0x005b cmd status 0x0d
>
> One thing I left alone: hci_cmd_sync_alloc() performs the same truncation
> for every caller, so a guard there would cover more than this one entry
> point. All in kernel callers I looked at pass a fixed sizeof(), except
> msft_add_monitor_sync() which computes total_size from the pattern list a
> user supplies through MGMT_OP_ADD_ADV_PATTERNS_MONITOR. That one looks
> like it can exceed 255 as well, but it needs a controller with MSFT
> extension support and I have not verified it, so I am not claiming it
> here. Happy to send a follow up for either if you want it.
>
>  net/bluetooth/mgmt.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 167d75e34526..c16b0b80c193 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -2668,6 +2668,14 @@ static int mgmt_hci_cmd_sync(struct sock *sk, struct hci_dev *hdev,
>                 return mgmt_cmd_status(sk, hdev->id, MGMT_OP_HCI_CMD_SYNC,
>                                        MGMT_STATUS_INVALID_PARAMS);
>
> +       /* The HCI command header carries the parameter length in a u8, a
> +        * larger value would be truncated there while the parameters are
> +        * still appended to the frame in full.
> +        */
> +       if (le16_to_cpu(cp->params_len) > U8_MAX)
> +               return mgmt_cmd_status(sk, hdev->id, MGMT_OP_HCI_CMD_SYNC,
> +                                      MGMT_STATUS_INVALID_PARAMS);
> +

Hmm, the command should really use u8 rather than le16 here, but that
is probably too late to change it.

>         hci_dev_lock(hdev);
>         cmd = mgmt_pending_new(sk, MGMT_OP_HCI_CMD_SYNC, hdev, data, len);
>         if (!cmd)
>
> base-commit: 0d839570765118029aa8bf4a95444c6a11aacf85
> --
> 2.55.0
>


-- 
Luiz Augusto von Dentz