1. Patchew
  2. linux
  3. View series

[PATCH] Bluetooth: btintel_pcie: Fix array bounds check bugs

ZhaoJinming posted 1 patch 1 month, 2 weeks ago 
drivers/bluetooth/btintel_pcie.c | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
[PATCH] Bluetooth: btintel_pcie: Fix array bounds check bugs
Posted by ZhaoJinming 1 month, 2 weeks ago 
Fix four array bounds issues in the Intel BT PCIe driver:

1. btintel_pcie_send_sync(): bounds check for tfd_index uses '>' instead
   of '>='.  When tfd_index == txq->count (32), the check passes and
   btintel_pcie_prepare_tx() writes past the end of txq->tfds[] and
   txq->bufs[].

2. btintel_pcie_submit_rx(): same off-by-one on frbd_index.  When
   frbd_index == rxq->count (64), the check passes and
   btintel_pcie_prepare_rx() writes past the end of rxq->frbds[] and
   rxq->bufs[].

3. btintel_pcie_msix_tx_handle(): cr_tia (device-controlled, from
   shared DMA memory) is used to index txq->urbd0s[] before any bounds
   check, and the urbd0->tfd_index check uses '>' instead of '>='.

4. btintel_pcie_msix_rx_handle(): cr_tia (device-controlled) indexes
   rxq->urbd1s[] with no bounds check.  urbd1->frbd_tag is a 16-bit
   device-controlled field (0-65535) used directly as an index into
   rxq->bufs[] (64 elements).

Fix all four by correcting the comparison operators and adding explicit
bounds checks on device-controlled indices before array access.

Fixes: c2b636b3f788 ("Bluetooth: btintel_pcie: Add support for PCIe transport")
Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>
---
 drivers/bluetooth/btintel_pcie.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
index 2b7231be5973..32cfa0f5af1c 100644
--- a/drivers/bluetooth/btintel_pcie.c
+++ b/drivers/bluetooth/btintel_pcie.c
@@ -401,7 +401,7 @@ static int btintel_pcie_send_sync(struct btintel_pcie_data *data,
 
 	tfd_index = data->ia.tr_hia[BTINTEL_PCIE_TXQ_NUM];
 
-	if (tfd_index > txq->count)
+	if (tfd_index >= txq->count)
 		return -ERANGE;
 
 	/* Firmware raises alive interrupt on HCI_OP_RESET or
@@ -502,7 +502,7 @@ static int btintel_pcie_submit_rx(struct btintel_pcie_data *data)
 
 	frbd_index = data->ia.tr_hia[BTINTEL_PCIE_RXQ_NUM];
 
-	if (frbd_index > rxq->count)
+	if (frbd_index >= rxq->count)
 		return -ERANGE;
 
 	/* Prepare for RX submit. It updates the FRBD with the address of DMA
@@ -1094,12 +1094,15 @@ static void btintel_pcie_msix_tx_handle(struct btintel_pcie_data *data)
 	txq = &data->txq;
 
 	while (cr_tia != cr_hia) {
+		if (cr_tia >= txq->count)
+			return;
+
 		data->tx_wait_done = true;
 		wake_up(&data->tx_wait_q);
 
 		urbd0 = &txq->urbd0s[cr_tia];
 
-		if (urbd0->tfd_index > txq->count)
+		if (urbd0->tfd_index >= txq->count)
 			return;
 
 		cr_tia = (cr_tia + 1) % txq->count;
@@ -1584,9 +1587,21 @@ static void btintel_pcie_msix_rx_handle(struct btintel_pcie_data *data)
 	 * process all received CDs in this interrupt.
 	 */
 	while (cr_tia != cr_hia) {
+		if (cr_tia >= rxq->count) {
+			bt_dev_err(hdev, "RXQ: invalid cr_tia %u (count %u)",
+				   cr_tia, rxq->count);
+			return;
+		}
+
 		urbd1 = &rxq->urbd1s[cr_tia];
 		ipc_print_urbd1(data->hdev, urbd1, cr_tia);
 
+		if (urbd1->frbd_tag >= rxq->count) {
+			bt_dev_err(hdev, "RXQ: invalid frbd_tag %u (count %u)",
+				   urbd1->frbd_tag, rxq->count);
+			return;
+		}
+
 		buf = &rxq->bufs[urbd1->frbd_tag];
 		if (!buf) {
 			bt_dev_err(hdev, "RXQ: failed to get the DMA buffer for %d",

base-commit: 3d6d817622b0a9721e3cc404df3469171582be13
-- 
2.51.0
Re: [PATCH] Bluetooth: btintel_pcie: Fix array bounds check bugs
Posted by Paul Menzel 1 month, 2 weeks ago 
Dear Zhao,


Thank you for your patch. For the summary I’d use:

Bluetooth: btintel_pcie: Check array bounds of dev controlled indices

Am 13.08.26 um 12:37 schrieb ZhaoJinming:
> Fix four array bounds issues in the Intel BT PCIe driver:
> 
> 1. btintel_pcie_send_sync(): bounds check for tfd_index uses '>' instead
>     of '>='.  When tfd_index == txq->count (32), the check passes and
>     btintel_pcie_prepare_tx() writes past the end of txq->tfds[] and
>     txq->bufs[].
> 
> 2. btintel_pcie_submit_rx(): same off-by-one on frbd_index.  When
>     frbd_index == rxq->count (64), the check passes and
>     btintel_pcie_prepare_rx() writes past the end of rxq->frbds[] and
>     rxq->bufs[].
> 
> 3. btintel_pcie_msix_tx_handle(): cr_tia (device-controlled, from
>     shared DMA memory) is used to index txq->urbd0s[] before any bounds
>     check, and the urbd0->tfd_index check uses '>' instead of '>='.
> 
> 4. btintel_pcie_msix_rx_handle(): cr_tia (device-controlled) indexes
>     rxq->urbd1s[] with no bounds check.  urbd1->frbd_tag is a 16-bit
>     device-controlled field (0-65535) used directly as an index into
>     rxq->bufs[] (64 elements).

Enumerating things in the commit message, is a good indicator to split 
the commit into smaller ones.

> Fix all four by correcting the comparison operators and adding explicit
> bounds checks on device-controlled indices before array access.
> 
> Fixes: c2b636b3f788 ("Bluetooth: btintel_pcie: Add support for PCIe transport")
> Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>
> ---
>   drivers/bluetooth/btintel_pcie.c | 21 ++++++++++++++++++---
>   1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
> index 2b7231be5973..32cfa0f5af1c 100644
> --- a/drivers/bluetooth/btintel_pcie.c
> +++ b/drivers/bluetooth/btintel_pcie.c
> @@ -401,7 +401,7 @@ static int btintel_pcie_send_sync(struct btintel_pcie_data *data,
>   
>   	tfd_index = data->ia.tr_hia[BTINTEL_PCIE_TXQ_NUM];
>   
> -	if (tfd_index > txq->count)
> +	if (tfd_index >= txq->count)
>   		return -ERANGE;
>   
>   	/* Firmware raises alive interrupt on HCI_OP_RESET or
> @@ -502,7 +502,7 @@ static int btintel_pcie_submit_rx(struct btintel_pcie_data *data)
>   
>   	frbd_index = data->ia.tr_hia[BTINTEL_PCIE_RXQ_NUM];
>   
> -	if (frbd_index > rxq->count)
> +	if (frbd_index >= rxq->count)
>   		return -ERANGE;
>   
>   	/* Prepare for RX submit. It updates the FRBD with the address of DMA
> @@ -1094,12 +1094,15 @@ static void btintel_pcie_msix_tx_handle(struct btintel_pcie_data *data)
>   	txq = &data->txq;
>   
>   	while (cr_tia != cr_hia) {
> +		if (cr_tia >= txq->count)
> +			return;
> +
>   		data->tx_wait_done = true;
>   		wake_up(&data->tx_wait_q);
>   
>   		urbd0 = &txq->urbd0s[cr_tia];
>   
> -		if (urbd0->tfd_index > txq->count)
> +		if (urbd0->tfd_index >= txq->count)
>   			return;

gemini/gemini-3.1-pro-preview comments [1]:

> If urbd0->tfd_index is invalid and we return early here, doesn't this permanently
> stall the TX completion queue?
> By returning early, cr_tia is never advanced. The next interrupt will process
> the exact same corrupted descriptor and return early again.



>   
>   		cr_tia = (cr_tia + 1) % txq->count;
> @@ -1584,9 +1587,21 @@ static void btintel_pcie_msix_rx_handle(struct btintel_pcie_data *data)
>   	 * process all received CDs in this interrupt.
>   	 */
>   	while (cr_tia != cr_hia) {
> +		if (cr_tia >= rxq->count) {
> +			bt_dev_err(hdev, "RXQ: invalid cr_tia %u (count %u)",

Maybe: RXQ: invalid cr_tia %u >= %u, contact device vendor

> +				   cr_tia, rxq->count);
> +			return;
> +		}
> +
>   		urbd1 = &rxq->urbd1s[cr_tia];
>   		ipc_print_urbd1(data->hdev, urbd1, cr_tia);
>   
> +		if (urbd1->frbd_tag >= rxq->count) {
> +			bt_dev_err(hdev, "RXQ: invalid frbd_tag %u (count %u)",

Ditto regarding the log message.

> +				   urbd1->frbd_tag, rxq->count);
> +			return;
> +		}
> +

gemini/gemini-3.1-pro-preview comments [1]:

> By returning early when urbd1->frbd_tag is invalid, we fail to advance cr_tia.
> Will this permanently stall the RX completion queue since the next interrupt
> will process the same corrupted descriptor?

>   		buf = &rxq->bufs[urbd1->frbd_tag];

gemini/gemini-3.1-pro-preview comments [1]:

> Does this introduce a Time-of-Check to Time-of-Use (TOCTOU) vulnerability?
> urbd1->frbd_tag is a bitfield inside a DMA-coherent structure. The code accesses
> it once for the bounds check (above) and again here to index the array.
> Can the compiler emit two separate memory reads, allowing the device to alter
> the value after the check passes but before the array access?

>   		if (!buf) {
>   			bt_dev_err(hdev, "RXQ: failed to get the DMA buffer for %d",


Kind regards,

Paul


[1]: 
https://sashiko.dev/#/patchset/460316663D6D34ED%2B20260813103734.222955-1-zhaojinming%40uniontech.com
Re: [PATCH] Bluetooth: btintel_pcie: Fix array bounds check bugs
Posted by 赵金明 1 month, 1 week ago 
Thank you for your review.

I split the fixes into 3 patches in a new thread as suggested.
Changes in v1:
- Fixed off-by-one comparisons in the synchronous paths
- Added bounds checks for cr_tia and descriptor fields in TX/RX handlers
- Used READ_ONCE() for DMA-coherent bitfield reads to avoid TOCTOU
- Changed return to break to avoid repeated processing of corrupted descriptors

https://lore.kernel.org/all/20260820-btintel_pcie_bounds_fixes-v1-0-c9dcd1ac8bf6@uniontech.com/

Thanks,
Zhao Jinming



>Dear Zhao,



>



>



>Thank you for your patch. For the summary I’d use:



>



>Bluetooth: btintel_pcie: Check array bounds of dev controlled indices



>



>Am 13.08.26 um 12:37 schrieb ZhaoJinming:



>> Fix four array bounds issues in the Intel BT PCIe driver:



>> 



>> 1. btintel_pcie_send_sync(): bounds check for tfd_index uses '>' instead



>>???? of '>='.? When tfd_index == txq->count (32), the check passes and



>>???? btintel_pcie_prepare_tx() writes past the end of txq->tfds[] and



>>???? txq->bufs[].



>> 



>> 2. btintel_pcie_submit_rx(): same off-by-one on frbd_index.? When



>>???? frbd_index == rxq->count (64), the check passes and



>>???? btintel_pcie_prepare_rx() writes past the end of rxq->frbds[] and



>>???? rxq->bufs[].



>> 



>> 3. btintel_pcie_msix_tx_handle(): cr_tia (device-controlled, from



>>???? shared DMA memory) is used to index txq->urbd0s[] before any bounds



>>???? check, and the urbd0->tfd_index check uses '>' instead of '>='.



>> 



>> 4. btintel_pcie_msix_rx_handle(): cr_tia (device-controlled) indexes



>>???? rxq->urbd1s[] with no bounds check.? urbd1->frbd_tag is a 16-bit



>>???? device-controlled field (0-65535) used directly as an index into



>>???? rxq->bufs[] (64 elements).



>



>Enumerating things in the commit message, is a good indicator to split 



>the commit into smaller ones.



>



>> Fix all four by correcting the comparison operators and adding explicit



>> bounds checks on device-controlled indices before array access.



>> 



>> Fixes: c2b636b3f788 ("Bluetooth: btintel_pcie: Add support for PCIe transport")



>> Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>



>> ---



>>?? drivers/bluetooth/btintel_pcie.c | 21 ++++++++++++++++++---



>>?? 1 file changed, 18 insertions(+), 3 deletions(-)



>> 



>> diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c



>> index 2b7231be5973..32cfa0f5af1c 100644



>> --- a/drivers/bluetooth/btintel_pcie.c



>> +++ b/drivers/bluetooth/btintel_pcie.c



>> @@ -401,7 +401,7 @@ static int btintel_pcie_send_sync(struct btintel_pcie_data *data,



>>?? 



>>?? 	tfd_index = data->ia.tr_hia[BTINTEL_PCIE_TXQ_NUM];



>>?? 



>> -	if (tfd_index > txq->count)



>> +	if (tfd_index >= txq->count)



>>?? 		return -ERANGE;



>>?? 



>>?? 	/* Firmware raises alive interrupt on HCI_OP_RESET or



>> @@ -502,7 +502,7 @@ static int btintel_pcie_submit_rx(struct btintel_pcie_data *data)



>>?? 



>>?? 	frbd_index = data->ia.tr_hia[BTINTEL_PCIE_RXQ_NUM];



>>?? 



>> -	if (frbd_index > rxq->count)



>> +	if (frbd_index >= rxq->count)



>>?? 		return -ERANGE;



>>?? 



>>?? 	/* Prepare for RX submit. It updates the FRBD with the address of DMA



>> @@ -1094,12 +1094,15 @@ static void btintel_pcie_msix_tx_handle(struct btintel_pcie_data *data)



>>?? 	txq = &data->txq;



>>?? 



>>?? 	while (cr_tia != cr_hia) {



>> +		if (cr_tia >= txq->count)



>> +			return;



>> +



>>?? 		data->tx_wait_done = true;



>>?? 		wake_up(&data->tx_wait_q);



>>?? 



>>?? 		urbd0 = &txq->urbd0s[cr_tia];



>>?? 



>> -		if (urbd0->tfd_index > txq->count)



>> +		if (urbd0->tfd_index >= txq->count)



>>?? 			return;



>



>gemini/gemini-3.1-pro-preview comments [1]:



>



>> If urbd0->tfd_index is invalid and we return early here, doesn't this permanently



>> stall the TX completion queue?



>> By returning early, cr_tia is never advanced. The next interrupt will process



>> the exact same corrupted descriptor and return early again.



>



>



>



>>?? 



>>?? 		cr_tia = (cr_tia + 1) % txq->count;



>> @@ -1584,9 +1587,21 @@ static void btintel_pcie_msix_rx_handle(struct btintel_pcie_data *data)



>>?? 	 * process all received CDs in this interrupt.



>>?? 	 */



>>?? 	while (cr_tia != cr_hia) {



>> +		if (cr_tia >= rxq->count) {



>> +			bt_dev_err(hdev, "RXQ: invalid cr_tia %u (count %u)",



>



>Maybe: RXQ: invalid cr_tia %u >= %u, contact device vendor



>



>> +				?? cr_tia, rxq->count);



>> +			return;



>> +		}



>> +



>>?? 		urbd1 = &rxq->urbd1s[cr_tia];



>>?? 		ipc_print_urbd1(data->hdev, urbd1, cr_tia);



>>?? 



>> +		if (urbd1->frbd_tag >= rxq->count) {



>> +			bt_dev_err(hdev, "RXQ: invalid frbd_tag %u (count %u)",



>



>Ditto regarding the log message.



>



>> +				?? urbd1->frbd_tag, rxq->count);



>> +			return;



>> +		}



>> +



>



>gemini/gemini-3.1-pro-preview comments [1]:



>



>> By returning early when urbd1->frbd_tag is invalid, we fail to advance cr_tia.



>> Will this permanently stall the RX completion queue since the next interrupt



>> will process the same corrupted descriptor?



>



>>?? 		buf = &rxq->bufs[urbd1->frbd_tag];



>



>gemini/gemini-3.1-pro-preview comments [1]:



>



>> Does this introduce a Time-of-Check to Time-of-Use (TOCTOU) vulnerability?



>> urbd1->frbd_tag is a bitfield inside a DMA-coherent structure. The code accesses



>> it once for the bounds check (above) and again here to index the array.



>> Can the compiler emit two separate memory reads, allowing the device to alter



>> the value after the check passes but before the array access?



>



>>?? 		if (!buf) {



>>?? 			bt_dev_err(hdev, "RXQ: failed to get the DMA buffer for %d",



>



>



>Kind regards,



>



>Paul



>



>



>[1]: 



>https://sashiko.dev/#/patchset/460316663D6D34ED%2B20260813103734.222955-1-zhaojinming%40uniontech.com



>

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.