[PATCH v1] Bluetooth: btintel: validate DDC record lengths

[PATCH v1] Bluetooth: btintel: validate DDC record lengths

[Sai Teja Aluvala]

Parse DDC record lengths in unsigned storage so 0xFF does not wrap.
Reject records smaller than the mandatory three-byte header, records
exceeding U8_MAX, and records exceeding remaining firmware bytes before
issuing Intel_Write_DDC.

This issue was reported by Claude Mythos.

Fixes: 145f2368c5fd ("Bluetooth: btintel: Add Device Configuration support")
Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
---
 drivers/bluetooth/btintel.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index 964d2de30e65..aecfab25edff 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -409,8 +409,16 @@ int btintel_load_ddc_config(struct hci_dev *hdev, const char *ddc_name)
 	/* DDC file contains one or more DDC structure which has
 	 * Length (1 byte), DDC ID (2 bytes), and DDC value (Length - 2).
 	 */
-	while (fw->size > fw_ptr - fw->data) {
-		u8 cmd_plen = fw_ptr[0] + sizeof(u8);
+	while (fw->size > (size_t)(fw_ptr - fw->data)) {
+		size_t remaining = fw->size - (fw_ptr - fw->data);
+		u8 cmd_plen = fw_ptr[0] + 1U;
+
+		if (cmd_plen < 3 || cmd_plen > U8_MAX || cmd_plen > remaining) {
+			bt_dev_err(hdev, "Malformed DDC record (plen=%u, remaining=%zu)",
+				   cmd_plen, remaining);
+			release_firmware(fw);
+			return -EINVAL;
+		}
 
 		skb = __hci_cmd_sync(hdev, 0xfc8b, cmd_plen, fw_ptr,
 				     HCI_INIT_TIMEOUT);
-- 
2.53.0

RE: [v1] Bluetooth: btintel: validate DDC record lengths

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1019 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/series/1168432/

---Test result---

Test Summary:
CheckPatch                    PASS      0.50 seconds
VerifyFixes                   PASS      0.09 seconds
VerifySignedoff               PASS      0.09 seconds
GitLint                       PASS      0.24 seconds
SubjectPrefix                 PASS      0.09 seconds
BuildKernel                   PASS      21.65 seconds
CheckAllWarning               PASS      26.48 seconds
CheckSparse                   PASS      26.42 seconds
BuildKernel32                 PASS      23.70 seconds
CheckKernelLLVM               PASS      20.97 seconds
TestRunnerSetup               PASS      499.02 seconds
IncrementalBuild              PASS      19.68 seconds



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

---
Regards,
Linux Bluetooth

Re: [PATCH v1] Bluetooth: btintel: validate DDC record lengths

[Jeremy Dean]

Sai,

A couple of things could be worth a second look here.

cmd_plen is still declared as u8:

    u8 cmd_plen = fw_ptr[0] + 1U;

The addition itself happens in a wider type due to integer promotion,
but the result is truncated back down the moment it's stored in
cmd_plen. 

So if fw_ptr[0] is 0xFF, cmd_plen still wraps to 0, the same
as before this patch. The commit message says this parses the length
"in unsigned storage so 0xFF does not wrap". It will wrap as soon as it 
is placed back into the u8 cmd_plen variable.

It happens to get caught afterward by the cmd_plen < 3 check, 
since a wrapped value of 0 is less than 3. Is that the intended fix?
The commit message seems to mismatch what is diffed.

Also, the cmd_plen > U8_MAX check can never be true. Since
cmd_plen is a u8, its value is bounded by U8_MAX by definition.
There's no input that will make this branch fire.

Regards,
Jeremy Dean
~             

RE: [PATCH v1] Bluetooth: btintel: validate DDC record lengths

[Aluvala Sai Teja]

Sai,

A couple of things could be worth a second look here.

cmd_plen is still declared as u8:

   u8 cmd_plen = fw_ptr[0] + 1U;

The addition itself happens in a wider type due to integer promotion, but the result is truncated back down the moment it's stored in cmd_plen. 

So if fw_ptr[0] is 0xFF, cmd_plen still wraps to 0, the same as before this patch. The commit message says this parses the length "in unsigned storage so 0xFF does not wrap". It will wrap as soon as it is placed back into the u8 cmd_plen variable.
[Sai] : Agreed .will change cmd_plen declaration to unsigned int in v2 patch

It happens to get caught afterward by the cmd_plen < 3 check, since a wrapped value of 0 is less than 3. Is that the intended fix?
[Sai] : Yes. DDC format is Length (1 byte), DDC ID (2 bytes), and DDC value . length and ddc id of 2 bytes are minimum expected bytes.
The commit message seems to mismatch what is diffed.

Also, the cmd_plen > U8_MAX check can never be true. Since cmd_plen is a u8, its value is bounded by U8_MAX by definition.
There's no input that will make this branch fire.
[Sai] : Agreed. This check will become valid once cmd_plen is declared as unsigned int

Regards,
Jeremy Dean
~             

Re: [PATCH v1] Bluetooth: btintel: validate DDC record lengths

[Jeremy Clifton]

Sai,

If you are busy, I am willing to
send a follow-up patch myself with cmd_plen widened to unsigned int,
referencing your original patch.

Thoughts? Is that proper etiquette?

Jeremy Dean

On 9/22/26 8:43 AM, Aluvala Sai Teja wrote:
> Sai,
> 
> A couple of things could be worth a second look here.
> 
> cmd_plen is still declared as u8:
> 
>     u8 cmd_plen = fw_ptr[0] + 1U;
> 
> The addition itself happens in a wider type due to integer promotion, but the result is truncated back down the moment it's stored in cmd_plen.
> 
> So if fw_ptr[0] is 0xFF, cmd_plen still wraps to 0, the same as before this patch. The commit message says this parses the length "in unsigned storage so 0xFF does not wrap". It will wrap as soon as it is placed back into the u8 cmd_plen variable.
> [Sai] : Agreed .will change cmd_plen declaration to unsigned int in v2 patch
> 
> It happens to get caught afterward by the cmd_plen < 3 check, since a wrapped value of 0 is less than 3. Is that the intended fix?
> [Sai] : Yes. DDC format is Length (1 byte), DDC ID (2 bytes), and DDC value . length and ddc id of 2 bytes are minimum expected bytes.
> The commit message seems to mismatch what is diffed.
> 
> Also, the cmd_plen > U8_MAX check can never be true. Since cmd_plen is a u8, its value is bounded by U8_MAX by definition.
> There's no input that will make this branch fire.
> [Sai] : Agreed. This check will become valid once cmd_plen is declared as unsigned int
> 
> Regards,
> Jeremy Dean
> ~