diff mbox series

[v3] Bluetooth: btmtk: Fix null pointer when processing coredump

Message ID 20230712051857.13812-1-chris.lu@mediatek.com (mailing list archive)
State Superseded
Headers show
Series [v3] Bluetooth: btmtk: Fix null pointer when processing coredump | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch success CheckPatch PASS
tedd_an/GitLint success Gitlint PASS
tedd_an/SubjectPrefix success Gitlint PASS
tedd_an/BuildKernel success BuildKernel PASS
tedd_an/CheckAllWarning success CheckAllWarning PASS
tedd_an/CheckSparse success CheckSparse PASS
tedd_an/CheckSmatch success CheckSparse PASS
tedd_an/BuildKernel32 success BuildKernel32 PASS
tedd_an/TestRunnerSetup success TestRunnerSetup PASS
tedd_an/TestRunner_l2cap-tester success TestRunner PASS
tedd_an/TestRunner_iso-tester success TestRunner PASS
tedd_an/TestRunner_bnep-tester success TestRunner PASS
tedd_an/TestRunner_mgmt-tester success TestRunner PASS
tedd_an/TestRunner_rfcomm-tester success TestRunner PASS
tedd_an/TestRunner_sco-tester success TestRunner PASS
tedd_an/TestRunner_ioctl-tester success TestRunner PASS
tedd_an/TestRunner_mesh-tester success TestRunner PASS
tedd_an/TestRunner_smp-tester success TestRunner PASS
tedd_an/TestRunner_userchan-tester success TestRunner PASS
tedd_an/IncrementalBuild success Incremental Build PASS

Commit Message

Chris Lu July 12, 2023, 5:18 a.m. UTC 
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 <chris.lu@mediatek.com>
Co-developed-by: Sean Wang <sean.wang@mediatek.com>
Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
v2: fix typo
v3: fix bot checking error
---
 drivers/bluetooth/btmtk.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

bluez.test.bot@gmail.com July 12, 2023, 6:03 a.m. UTC | #1 
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
Paul Menzel July 12, 2023, 6:11 a.m. UTC | #2 
Dear Chris,


Am 12.07.23 um 07:18 schrieb Chris Lu:
> 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 …

> Signed-off-by: Chris Lu <chris.lu@mediatek.com>
> Co-developed-by: Sean Wang <sean.wang@mediatek.com>
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
> v2: fix typo
> v3: fix bot checking error
> ---
>   drivers/bluetooth/btmtk.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/bluetooth/btmtk.c b/drivers/bluetooth/btmtk.c
> index 786f775196ae..0f290430ae0e 100644
> --- a/drivers/bluetooth/btmtk.c
> +++ b/drivers/bluetooth/btmtk.c
> @@ -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);

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

>   
>   		/* 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


> +			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;
>   	}
Chris Lu July 12, 2023, 8:53 a.m. UTC | #3 
On Wed, 2023-07-12 at 08:11 +0200, Paul Menzel wrote:
>  	 
> 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.

> 
> Am 12.07.23 um 07:18 schrieb Chris Lu:
> > 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 …
> 
> > Signed-off-by: Chris Lu <chris.lu@mediatek.com>
> > Co-developed-by: Sean Wang <sean.wang@mediatek.com>
> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > ---
> > v2: fix typo
> > v3: fix bot checking error
> > ---
> >   drivers/bluetooth/btmtk.c | 16 ++++++++--------
> >   1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/bluetooth/btmtk.c b/drivers/bluetooth/btmtk.c
> > index 786f775196ae..0f290430ae0e 100644
> > --- a/drivers/bluetooth/btmtk.c
> > +++ b/drivers/bluetooth/btmtk.c
> > @@ -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);
> 
> 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.

> >   
> >   /* 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
> 
> 
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.

> > +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;
> >   }
Paul Menzel July 12, 2023, 9:25 a.m. UTC | #4 
Dear Chris,


Am 12.07.23 um 10:53 schrieb Chris Lu (陸稚泓):
> On Wed, 2023-07-12 at 08:11 +0200, Paul Menzel wrote:
>>   	
>> 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.)

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

Thank you for your reply.

>> Am 12.07.23 um 07:18 schrieb Chris Lu:
>>> 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 …
>>
>>> Signed-off-by: Chris Lu <chris.lu@mediatek.com>
>>> Co-developed-by: Sean Wang <sean.wang@mediatek.com>
>>> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
>>> ---
>>> v2: fix typo
>>> v3: fix bot checking error
>>> ---
>>>    drivers/bluetooth/btmtk.c | 16 ++++++++--------
>>>    1 file changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/bluetooth/btmtk.c b/drivers/bluetooth/btmtk.c
>>> index 786f775196ae..0f290430ae0e 100644
>>> --- a/drivers/bluetooth/btmtk.c
>>> +++ b/drivers/bluetooth/btmtk.c
>>> @@ -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);
>>
>> 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.
> 
>>>    
>>>    /* 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.

> 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


>>> +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;
>>>    }
diff mbox series

Patch

diff --git a/drivers/bluetooth/btmtk.c b/drivers/bluetooth/btmtk.c
index 786f775196ae..0f290430ae0e 100644
--- a/drivers/bluetooth/btmtk.c
+++ b/drivers/bluetooth/btmtk.c
@@ -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;
 	}