[PATCH v1 1/2] Bluetooth: btintel_pcie: validate packet_len before skb_put_data

[PATCH v1 1/2] Bluetooth: btintel_pcie: validate packet_len before skb_put_data

[Kiran K]

btintel_pcie_submit_rx_work() reads packet_len from rfh_hdr without
checking if it exceeds the RX buffer size. An oversized packet_len
can lead to an out-of-bounds read in skb_put_data().

Validate packet_len to ensure it is non-zero and does not exceed
BTINTEL_PCIE_BUFFER_SIZE - sizeof(*rfh_hdr), logging an error when
invalid.

Fixes: c2b636b3f788 ("Bluetooth: btintel_pcie: Add support for PCIe transport")
Signed-off-by: Kiran K <kiran.k@intel.com>
---
 drivers/bluetooth/btintel_pcie.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
index 30923eaabed7..1eabb0c8326f 100644
--- a/drivers/bluetooth/btintel_pcie.c
+++ b/drivers/bluetooth/btintel_pcie.c
@@ -1954,7 +1954,8 @@ static int btintel_pcie_submit_rx_work(struct btintel_pcie_data *data, u8 status
 	rfh_hdr = buf;
 
 	len = rfh_hdr->packet_len;
-	if (len <= 0) {
+	if (len == 0 || len > BTINTEL_PCIE_BUFFER_SIZE - sizeof(*rfh_hdr)) {
+		bt_dev_err(data->hdev, "Invalid packet_len %d", len);
 		ret = -EINVAL;
 		goto resubmit;
 	}
-- 
2.54.0

[PATCH v1 2/2] Bluetooth: btintel_pcie: fix tx_handle bounds off-by-one

[Kiran K]

Valid indices into txq->urbd0s/tfds/bufs are 0..txq->count-1, so
tfd_index == txq->count is already out of range. Change the guard in
btintel_pcie_msix_tx_handle() from '> txq->count' to '>= txq->count'.

Fixes: c2b636b3f788 ("Bluetooth: btintel_pcie: Add support for PCIe transport")
Signed-off-by: Kiran K <kiran.k@intel.com>
---
 drivers/bluetooth/btintel_pcie.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
index 1eabb0c8326f..e2cd27fef0c4 100644
--- a/drivers/bluetooth/btintel_pcie.c
+++ b/drivers/bluetooth/btintel_pcie.c
@@ -1423,7 +1423,7 @@ static void btintel_pcie_msix_tx_handle(struct btintel_pcie_data *data)
 
 		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;
-- 
2.54.0

RE: [v1,1/2] Bluetooth: btintel_pcie: validate packet_len before skb_put_data

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1181 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=1156173

---Test result---

Test Summary:
CheckPatch                    PASS      1.47 seconds
VerifyFixes                   PASS      0.14 seconds
VerifySignedoff               PASS      0.14 seconds
GitLint                       PASS      0.68 seconds
SubjectPrefix                 PASS      0.26 seconds
BuildKernel                   PASS      26.76 seconds
CheckAllWarning               PASS      28.91 seconds
CheckSparse                   PASS      27.68 seconds
BuildKernel32                 PASS      25.35 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      472.42 seconds
IncrementalBuild              PASS      26.78 seconds

Details
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found


https://github.com/bluez/bluetooth-next/pull/687

---
Regards,
Linux Bluetooth

Re: [PATCH v1 1/2] Bluetooth: btintel_pcie: validate packet_len before skb_put_data

[Paul Menzel]

Dear Kiran,


Thank you for your patch.

Am 02.09.26 um 15:18 schrieb Kiran K:
> btintel_pcie_submit_rx_work() reads packet_len from rfh_hdr without
> checking if it exceeds the RX buffer size. An oversized packet_len
> can lead to an out-of-bounds read in skb_put_data().
> 
> Validate packet_len to ensure it is non-zero and does not exceed
> BTINTEL_PCIE_BUFFER_SIZE - sizeof(*rfh_hdr), logging an error when
> invalid.

Can this case be forced somehow? Please document how.

Did a tool find this case or did you experience it during 
testing/review? If a tool, please document it.

> Fixes: c2b636b3f788 ("Bluetooth: btintel_pcie: Add support for PCIe transport")
> Signed-off-by: Kiran K <kiran.k@intel.com>
> ---
>   drivers/bluetooth/btintel_pcie.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
> index 30923eaabed7..1eabb0c8326f 100644
> --- a/drivers/bluetooth/btintel_pcie.c
> +++ b/drivers/bluetooth/btintel_pcie.c
> @@ -1954,7 +1954,8 @@ static int btintel_pcie_submit_rx_work(struct btintel_pcie_data *data, u8 status
>   	rfh_hdr = buf;
>   
>   	len = rfh_hdr->packet_len;
> -	if (len <= 0) {
> +	if (len == 0 || len > BTINTEL_PCIE_BUFFER_SIZE - sizeof(*rfh_hdr)) {
> +		bt_dev_err(data->hdev, "Invalid packet_len %d", len);

What about negative values for `len` as was checked before?

To have a more useful log, I’d also log the value of 
`BTINTEL_PCIE_BUFFER_SIZE` and `sizeof(*rfh_hdr)`.

>   		ret = -EINVAL;
>   		goto resubmit;
>   	}


Kind regards,

Paul

RE: [PATCH v1 1/2] Bluetooth: btintel_pcie: validate packet_len before skb_put_data

[K, Kiran]

Hi Paul,

Thanks for your comments.

>Subject: Re: [PATCH v1 1/2] Bluetooth: btintel_pcie: validate packet_len before
>skb_put_data
>
>Dear Kiran,
>
>
>Thank you for your patch.
>
>Am 02.09.26 um 15:18 schrieb Kiran K:
>> btintel_pcie_submit_rx_work() reads packet_len from rfh_hdr without
>> checking if it exceeds the RX buffer size. An oversized packet_len can
>> lead to an out-of-bounds read in skb_put_data().
>>
>> Validate packet_len to ensure it is non-zero and does not exceed
>> BTINTEL_PCIE_BUFFER_SIZE - sizeof(*rfh_hdr), logging an error when
>> invalid.
>
>Can this case be forced somehow? Please document how.

Yes, this condition can be simulated in a test environment by either:
1. Using a customized firmware image configured to send a frame header with an invalid  packet_len  (e.g.,  0  or  > 4088 ).
2. Temporarily modifying  rfh_hdr->packet_len  in the driver code (or using a debug hook) right before  btintel_pcie_submit_rx_work()  is invoked.

>
>Did a tool find this case or did you experience it during testing/review? If a
>tool, please document it.
Yes. This issue was reported by Mythos.  I will document in commit message.

>
>> Fixes: c2b636b3f788 ("Bluetooth: btintel_pcie: Add support for PCIe
>> transport")
>> Signed-off-by: Kiran K <kiran.k@intel.com>
>> ---
>>   drivers/bluetooth/btintel_pcie.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bluetooth/btintel_pcie.c
>> b/drivers/bluetooth/btintel_pcie.c
>> index 30923eaabed7..1eabb0c8326f 100644
>> --- a/drivers/bluetooth/btintel_pcie.c
>> +++ b/drivers/bluetooth/btintel_pcie.c
>> @@ -1954,7 +1954,8 @@ static int btintel_pcie_submit_rx_work(struct
>btintel_pcie_data *data, u8 status
>>   	rfh_hdr = buf;
>>
>>   	len = rfh_hdr->packet_len;
>> -	if (len <= 0) {
>> +	if (len == 0 || len > BTINTEL_PCIE_BUFFER_SIZE - sizeof(*rfh_hdr)) {
>> +		bt_dev_err(data->hdev, "Invalid packet_len %d", len);
>
>What about negative values for `len` as was checked before?
rfh_hdr->packet_len  is a 16-bit unsigned bitfield ( u64 packet_len:16 ), which takes values in the range  0  to  65535. When assigned to  int len , it is zero-extended and cannot produce a negative value ( len >= 0  is always true).

>
>To have a more useful log, I’d also log the value of
>`BTINTEL_PCIE_BUFFER_SIZE` and `sizeof(*rfh_hdr)`.
Ack.
>
>>   		ret = -EINVAL;
>>   		goto resubmit;
>>   	}
>
>
>Kind regards,
>
>Paul

Thanks,
Kiran