| Message ID | 20230712051857.13812-1-chris.lu@mediatek.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] Bluetooth: btmtk: Fix null pointer when processing coredump | expand |
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
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; > > }
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 --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; }