Skip to contentSkip to conversation

Bluetooth: btmtk: Fix null pointer when processing coredump

PATCH Superseded Chris Lu 3 participants 5 messages Jul 12, 2023

Subsystems (2)
Bluetooth drivers Arm MediaTek

v3 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Chris Lu

author
#0 lore raw

PATCH v3 1/1 Bluetooth: btmtk: Fix null pointer when processing coredump

There may be a potential null pointer risk if offset value is less than 0 when doing memcmp in btmtk_process_coredump(). Checking offset is valid before doing memcmp.

Signed-off-by Chris Lu <[email protected]>
Co-developed-by Sean Wang <[email protected]>
Signed-off-by Sean Wang <[email protected]>

---
v2: fix typo
v3: fix bot checking error

---
 drivers/bluetooth/btmtk.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
drivers/bluetooth/btmtk.c +8 -8
index 786f775196ae..0f290430ae0e 100644
@@ -370,7 +370,7 @@ EXPORT_SYMBOL_GPL(btmtk_register_coredump);
 int btmtk_process_coredump(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct btmediatek_data *data = hci_get_priv(hdev);
-	int err;
+	int err, offset;
 
 	if (!IS_ENABLED(CONFIG_DEV_COREDUMP))
 		return 0;

@@ -392,15 +392,15 @@ int btmtk_process_coredump(struct hci_dev *hdev, struct sk_buff *skb)
 		if (err < 0)
 			break;
 		data->cd_info.cnt++;
+		offset = skb->len - sizeof(MTK_COREDUMP_END);
 
 		/* Mediatek coredump data would be more than MTK_COREDUMP_NUM */
-		if (data->cd_info.cnt > MTK_COREDUMP_NUM &&
-		    skb->len > sizeof(MTK_COREDUMP_END) &&
-		    !memcmp((char *)&skb->data[skb->len - sizeof(MTK_COREDUMP_END)],
-			    MTK_COREDUMP_END, sizeof(MTK_COREDUMP_END) - 1)) {
-			bt_dev_info(hdev, "Mediatek coredump end");
-			hci_devcd_complete(hdev);
-		}
+		if (data->cd_info.cnt > MTK_COREDUMP_NUM && offset > 0)
+			if (!memcmp((char *)&skb->data[offset], MTK_COREDUMP_END,
+				    sizeof(MTK_COREDUMP_END) - 1)) {
+				bt_dev_info(hdev, "Mediatek coredump end");
+				hci_devcd_complete(hdev);
+			}
 
 		break;
 	}
Signature · 1 line
2.18.0
#1 lore raw

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=764645

---Test result---

Test Summary:

CheckPatch                    PASS      0.63 seconds
GitLint                       PASS      0.28 seconds
SubjectPrefix                 PASS      0.09 seconds
BuildKernel                   PASS      35.31 seconds
CheckAllWarning               PASS      38.53 seconds
CheckSparse                   PASS      43.91 seconds
CheckSmatch                   PASS      117.28 seconds
BuildKernel32                 PASS      33.93 seconds
TestRunnerSetup               PASS      498.54 seconds
TestRunner_l2cap-tester       PASS      23.75 seconds
TestRunner_iso-tester         PASS      42.87 seconds
TestRunner_bnep-tester        PASS      10.73 seconds
TestRunner_mgmt-tester        PASS      219.56 seconds
TestRunner_rfcomm-tester      PASS      16.09 seconds
TestRunner_sco-tester         PASS      17.10 seconds
TestRunner_ioctl-tester       PASS      18.26 seconds
TestRunner_mesh-tester        PASS      13.55 seconds
TestRunner_smp-tester         PASS      14.53 seconds
TestRunner_userchan-tester    PASS      11.33 seconds
IncrementalBuild              PASS      30.74 seconds

---
Regards,
Linux Bluetooth

#2 lore raw
In reply to Chris Lu

Dear Chris,

Am 12.07.23 um 07:18 schrieb Chris Lu:

Quoted 3 lines

There may be a potential null pointer risk if offset value is less than 0 when doing memcmp in btmtk_process_coredump(). Checking offset is valid before doing memcmp.

Use imperative mood: Check offset …

Quoted 24 lines
Signed-off-by Chris Lu <[email protected]>
Co-developed-by Sean Wang <[email protected]>
Signed-off-by Sean Wang <[email protected]>

---
v2: fix typo
v3: fix bot checking error

---
  drivers/bluetooth/btmtk.c | 16 ++++++++--------
  1 file changed, 8 insertions(+), 8 deletions(-)
drivers/bluetooth/btmtk.c +1 -1
index 786f775196ae..0f290430ae0e 100644
@@ -370,7 +370,7 @@ EXPORT_SYMBOL_GPL(btmtk_register_coredump);
  int btmtk_process_coredump(struct hci_dev *hdev, struct sk_buff *skb)
  {
  	struct btmediatek_data *data = hci_get_priv(hdev);
-	int err;
+	int err, offset;
 
  	if (!IS_ENABLED(CONFIG_DEV_COREDUMP))
  		return 0;

drivers/bluetooth/btmtk.c int btmtk_process_coredump(struct hci_dev *hdev, struct sk_buff *skb) · line 395

Quoted 4 lines
drivers/bluetooth/btmtk.c +1 -0
@@ -392,15 +392,15 @@ int btmtk_process_coredump(struct hci_dev *hdev, struct sk_buff *skb)
  		if (err < 0)
  			break;
  		data->cd_info.cnt++;
+		offset = skb->len - sizeof(MTK_COREDUMP_END);

For sizeof() shouldn’t you use size_t? But that is unsigned of course. Maybe ssize_t then?

drivers/bluetooth/btmtk.c int btmtk_process_coredump(struct hci_dev *hdev, struct sk_buff *skb) · line 398

Quoted 10 lines
drivers/bluetooth/btmtk.c +1 -7
  		/* Mediatek coredump data would be more than MTK_COREDUMP_NUM */
-		if (data->cd_info.cnt > MTK_COREDUMP_NUM &&
-		    skb->len > sizeof(MTK_COREDUMP_END) &&
-		    !memcmp((char *)&skb->data[skb->len - sizeof(MTK_COREDUMP_END)],
-			    MTK_COREDUMP_END, sizeof(MTK_COREDUMP_END) - 1)) {
-			bt_dev_info(hdev, "Mediatek coredump end");
-			hci_devcd_complete(hdev);
-		}
+		if (data->cd_info.cnt > MTK_COREDUMP_NUM && offset > 0)

Why not keep it like before, and just add the condition skb->len < sizeof(MTK_COREDUMP_END)? The compiler is probably going to optimize so the value is not calculated twice.

Kind regards,

Paul

Quoted 8 lines
drivers/bluetooth/btmtk.c +5 -0
+			if (!memcmp((char *)&skb->data[offset], MTK_COREDUMP_END,
+				    sizeof(MTK_COREDUMP_END) - 1)) {
+				bt_dev_info(hdev, "Mediatek coredump end");
+				hci_devcd_complete(hdev);
+			}
 
  		break;
  	}
author
#3 lore raw
Quoted 5 lines · Paul Menzel

External email : Please do not click links or open attachments until you have verified the sender or the content.
Dear Chris,

Dear Paul,

Thanks for your review and feedback to Mediatek's Bluetooth driver code.

Quoted 34 lines

Am 12.07.23 um 07:18 schrieb Chris Lu:

Quoted 3 lines

There may be a potential null pointer risk if offset value is less than 0 when doing memcmp in btmtk_process_coredump(). Checking offset is valid before doing memcmp.

Use imperative mood: Check offset …

Quoted 15 lines
Signed-off-by Chris Lu <[email protected]>
Co-developed-by Sean Wang <[email protected]>
Signed-off-by Sean Wang <[email protected]>

---
v2: fix typo
v3: fix bot checking error

---
  drivers/bluetooth/btmtk.c | 16 ++++++++--------
  1 file changed, 8 insertions(+), 8 deletions(-)
drivers/bluetooth/btmtk.c +0 -0
index 786f775196ae..0f290430ae0e 100644

drivers/bluetooth/btmtk.c EXPORT_SYMBOL_GPL(btmtk_register_coredump); · line 370

Quoted 1 line
drivers/bluetooth/btmtk.c +0 -0
@@ -370,7 +370,7 @@ EXPORT_SYMBOL_GPL(btmtk_register_coredump);
  int btmtk_process_coredump(struct hci_dev *hdev, struct sk_buff

*skb)

drivers/bluetooth/btmtk.c int btmtk_process_coredump(struct hci_dev

Quoted 8 lines
drivers/bluetooth/btmtk.c +1 -1
  {
  struct btmediatek_data *data = hci_get_priv(hdev);
-int err;
+int err, offset;
 
  if (!IS_ENABLED(CONFIG_DEV_COREDUMP))
  return 0;

@@ -392,15 +392,15 @@ int btmtk_process_coredump(struct hci_dev

*hdev, struct sk_buff *skb)

Quoted 8 lines

drivers/bluetooth/btmtk.c int btmtk_process_coredump(struct hci_dev *hdev, struct sk_buff *skb) · line 395

Quoted 4 lines
drivers/bluetooth/btmtk.c +1 -0
  if (err < 0)
  break;
  data->cd_info.cnt++;
+offset = skb->len - sizeof(MTK_COREDUMP_END);

For sizeof() shouldn’t you use size_t? But that is unsigned of course. Maybe ssize_t then?

yes, it's better to use ssize_t or size_t, I'll change declaratins of offset from int to ssize_t.

Quoted 18 lines

drivers/bluetooth/btmtk.c int btmtk_process_coredump(struct hci_dev *hdev, struct sk_buff *skb)

Quoted 5 lines
drivers/bluetooth/btmtk.c +0 -3
  /* Mediatek coredump data would be more than MTK_COREDUMP_NUM */
-if (data->cd_info.cnt > MTK_COREDUMP_NUM &&
-    skb->len > sizeof(MTK_COREDUMP_END) &&
-    !memcmp((char *)&skb->data[skb->len -

sizeof(MTK_COREDUMP_END)],

drivers/bluetooth/btmtk.c int btmtk_process_coredump(struct hci_dev *hdev, struct sk_buff *skb) · line 398

Quoted 5 lines
drivers/bluetooth/btmtk.c +1 -4
-    MTK_COREDUMP_END, sizeof(MTK_COREDUMP_END) - 1)) {
-bt_dev_info(hdev, "Mediatek coredump end");
-hci_devcd_complete(hdev);
-}
+if (data->cd_info.cnt > MTK_COREDUMP_NUM && offset > 0)

Why not keep it like before, and just add the condition skb->len < sizeof(MTK_COREDUMP_END)? The compiler is probably going to optimize so
the value is not calculated twice.

Quoted 5 lines

Kind regards,

Paul

The reason why I send this patch is when I backport devcoredump feature to specific project with older kernel version, the compiler might not so optimized that it would cause kernel panic when run into memcmp. As a result, make sure skb->len > sizeof(MTK_COREDUMP_END) before doing memcmp part can avoid null pointer issue.
Besides, only in condition 'data->cd_info.cnt > MTK_COREDUMP_NUM && offset > 0' need to do memcmp to check the end of coredump. Driver do noting with condition skb->len < sizeof(MTK_COREDUMP_END) that additional condiction is not really necessary.

Quoted 8 lines
Quoted 8 lines
drivers/bluetooth/btmtk.c +5 -0
+if (!memcmp((char *)&skb->data[offset], MTK_COREDUMP_END,
+    sizeof(MTK_COREDUMP_END) - 1)) {
+bt_dev_info(hdev, "Mediatek coredump end");
+hci_devcd_complete(hdev);
+}
 
  break;
  }
#4 lore raw

Dear Chris,

Am 12.07.23 um 10:53 schrieb Chris Lu (陸稚泓):

Quoted 4 lines · Paul Menzel
Quoted 3 lines

External email : Please do not click links or open attachments until you have verified the sender or the content.

(It’d be nice if you removed such (automatically added) phrases from your reply.)

Quoted 2 lines

Thanks for your review and feedback to Mediatek's Bluetooth driver code.

Thank you for your reply.

Quoted 58 lines
Quoted 32 lines

Am 12.07.23 um 07:18 schrieb Chris Lu:

Quoted 3 lines

There may be a potential null pointer risk if offset value is less than 0 when doing memcmp in btmtk_process_coredump(). Checking offset is valid before doing memcmp.

Use imperative mood: Check offset …

drivers/bluetooth/btmtk.c int btmtk_process_coredump(struct hci_dev

Quoted 24 lines
Signed-off-by Chris Lu <[email protected]>
Co-developed-by Sean Wang <[email protected]>
Signed-off-by Sean Wang <[email protected]>

---
v2: fix typo
v3: fix bot checking error

---
   drivers/bluetooth/btmtk.c | 16 ++++++++--------
   1 file changed, 8 insertions(+), 8 deletions(-)
drivers/bluetooth/btmtk.c +1 -1
index 786f775196ae..0f290430ae0e 100644
@@ -370,7 +370,7 @@ EXPORT_SYMBOL_GPL(btmtk_register_coredump);
   int btmtk_process_coredump(struct hci_dev *hdev, struct sk_buff *skb)
   {
   struct btmediatek_data *data = hci_get_priv(hdev);
-int err;
+int err, offset;
 
   if (!IS_ENABLED(CONFIG_DEV_COREDUMP))
   return 0;

@@ -392,15 +392,15 @@ int btmtk_process_coredump(struct hci_dev

*hdev, struct sk_buff *skb)

Quoted 7 lines

drivers/bluetooth/btmtk.c int btmtk_process_coredump(struct hci_dev *hdev, struct sk_buff *skb) · line 395

Quoted 4 lines
drivers/bluetooth/btmtk.c +1 -0
   if (err < 0)
   break;
   data->cd_info.cnt++;
+offset = skb->len - sizeof(MTK_COREDUMP_END);

For sizeof() shouldn’t you use size_t? But that is unsigned of course. Maybe ssize_t then?

yes, it's better to use ssize_t or size_t, I'll change declaratins of offset from int to ssize_t.

Quoted 14 lines

drivers/bluetooth/btmtk.c int btmtk_process_coredump(struct hci_dev *hdev, struct sk_buff *skb) · line 398

Quoted 10 lines
drivers/bluetooth/btmtk.c +1 -7
   /* Mediatek coredump data would be more than MTK_COREDUMP_NUM */
-if (data->cd_info.cnt > MTK_COREDUMP_NUM &&
-    skb->len > sizeof(MTK_COREDUMP_END) &&
-    !memcmp((char *)&skb->data[skb->len - sizeof(MTK_COREDUMP_END)],
-    MTK_COREDUMP_END, sizeof(MTK_COREDUMP_END) - 1)) {
-bt_dev_info(hdev, "Mediatek coredump end");
-hci_devcd_complete(hdev);
-}
+if (data->cd_info.cnt > MTK_COREDUMP_NUM && offset > 0)

Why not keep it like before, and just add the condition skb->len < sizeof(MTK_COREDUMP_END)? The compiler is probably going to optimize so the value is not calculated twice.

Quoted 9 lines

The reason why I send this patch is when I backport devcoredump feature to specific project with older kernel version, the compiler might not so optimized that it would cause kernel panic when run into memcmp. As a result, make sure skb->len > sizeof(MTK_COREDUMP_END) before doing memcmp part can avoid null pointer issue.
Besides, only in condition 'data->cd_info.cnt > MTK_COREDUMP_NUM && offset > 0' need to do memcmp to check the end of coredump. Driver do noting with condition skb->len < sizeof(MTK_COREDUMP_END) that additional condiction is not really necessary.

Just to avoid misunderstandings, my point, to add the comparison and get rid of the variable offset.

Kind regards,

Paul

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/bluetooth/btmtk.c +5 -0
+if (!memcmp((char *)&skb->data[offset], MTK_COREDUMP_END,
+    sizeof(MTK_COREDUMP_END) - 1)) {
+bt_dev_info(hdev, "Mediatek coredump end");
+hci_devcd_complete(hdev);
+}
 
   break;
   }