| Message ID | tencent_70F452CB86430990EEC56EEB4CBB27D40606@qq.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bluetooth: fix double free in hci_req_sync_complete | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | warning | WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?) #99: Reported-and-tested-by: syzbot+35ebc808442df6420eae@syzkaller.appspotmail.com total: 0 errors, 1 warnings, 0 checks, 9 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /github/workspace/src/src/13708545.patch has style problems, please review. NOTE: Ignored message types: UNKNOWN_COMMIT_ID NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. |
| 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 |
Hi, su, 2024-06-23 kello 17:06 +0800, Edward Adam Davis kirjoitti: > Look at the following situation: > > cpu1 cpu2 > ==== ==== > sock_ioctl > sock_do_ioctl > hci_sock_ioctl > hci_rx_work hci_dev_cmd > hci_event_packet hci_req_sync > req_complete_skb __hci_req_sync > hci_req_sync_complete > > If hci_rx_work executes before __hci_req_sync releases req_skb, everything > is normal, otherwise it will result in double free of req_skb. > > Adding NULL check of req_skb before releasing it can avoid double free. Do you understand why? kfree_skb(NULL) is allowed, so this is logically a no-op. Probably it perturbs the timings so syzkaller repro no longer hits the race window, ie doesn't fix the issue. > Fixes: 45d355a926ab ("Bluetooth: Fix memory leak in hci_req_sync_complete()") > Reported-and-tested-by: syzbot+35ebc808442df6420eae@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=35ebc808442df6420eae > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > --- > net/bluetooth/hci_request.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c > index efea25eb56ce..3862fa6bb288 100644 > --- a/net/bluetooth/hci_request.c > +++ b/net/bluetooth/hci_request.c > @@ -106,7 +106,8 @@ void hci_req_sync_complete(struct hci_dev *hdev, u8 result, u16 opcode, > hdev->req_result = result; > hdev->req_status = HCI_REQ_DONE; > if (skb) { > - kfree_skb(hdev->req_skb); > + if (hdev->req_skb) > + kfree_skb(hdev->req_skb); > hdev->req_skb = skb_get(skb); > } > wake_up_interruptible(&hdev->req_wait_q);
On Sun, 23 Jun 2024 13:30:50 +0300, Pauli Virtanen wrote: > > cpu1 cpu2 > > ==== ==== > > sock_ioctl > > sock_do_ioctl > > hci_sock_ioctl > > hci_rx_work hci_dev_cmd > > hci_event_packet hci_req_sync > > req_complete_skb __hci_req_sync > > hci_req_sync_complete > > > > If hci_rx_work executes before __hci_req_sync releases req_skb, everything > > is normal, otherwise it will result in double free of req_skb. > > > > Adding NULL check of req_skb before releasing it can avoid double free. > > Do you understand why? > > kfree_skb(NULL) is allowed, so this is logically a no-op. > > Probably it perturbs the timings so syzkaller repro no longer hits the > race window, ie doesn't fix the issue. Good, even if you already know race, let me ask you a question: how to reduce race window? `` Edward
Hi Edward, On Sun, Jun 23, 2024 at 8:02 AM Edward Adam Davis <eadavis@qq.com> wrote: > > On Sun, 23 Jun 2024 13:30:50 +0300, Pauli Virtanen wrote: > > > cpu1 cpu2 > > > ==== ==== > > > sock_ioctl > > > sock_do_ioctl > > > hci_sock_ioctl > > > hci_rx_work hci_dev_cmd > > > hci_event_packet hci_req_sync > > > req_complete_skb __hci_req_sync > > > hci_req_sync_complete > > > > > > If hci_rx_work executes before __hci_req_sync releases req_skb, everything > > > is normal, otherwise it will result in double free of req_skb. > > > > > > Adding NULL check of req_skb before releasing it can avoid double free. > > > > Do you understand why? > > > > kfree_skb(NULL) is allowed, so this is logically a no-op. > > > > Probably it perturbs the timings so syzkaller repro no longer hits the > > race window, ie doesn't fix the issue. > Good, even if you already know race, let me ask you a question: how to reduce race window? We actually need to stop using hci_req_sync since that has been deprecated, instead it shall use hci_cmd_sync_submit, we could for the time being just make hci_req_sync use hci_cmd_sync_submit to avoid such races. > `` > Edward >
diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c index efea25eb56ce..3862fa6bb288 100644 --- a/net/bluetooth/hci_request.c +++ b/net/bluetooth/hci_request.c @@ -106,7 +106,8 @@ void hci_req_sync_complete(struct hci_dev *hdev, u8 result, u16 opcode, hdev->req_result = result; hdev->req_status = HCI_REQ_DONE; if (skb) { - kfree_skb(hdev->req_skb); + if (hdev->req_skb) + kfree_skb(hdev->req_skb); hdev->req_skb = skb_get(skb); } wake_up_interruptible(&hdev->req_wait_q);
Look at the following situation: cpu1 cpu2 ==== ==== sock_ioctl sock_do_ioctl hci_sock_ioctl hci_rx_work hci_dev_cmd hci_event_packet hci_req_sync req_complete_skb __hci_req_sync hci_req_sync_complete If hci_rx_work executes before __hci_req_sync releases req_skb, everything is normal, otherwise it will result in double free of req_skb. Adding NULL check of req_skb before releasing it can avoid double free. Fixes: 45d355a926ab ("Bluetooth: Fix memory leak in hci_req_sync_complete()") Reported-and-tested-by: syzbot+35ebc808442df6420eae@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=35ebc808442df6420eae Signed-off-by: Edward Adam Davis <eadavis@qq.com> --- net/bluetooth/hci_request.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)