KASAN reports that there's a use-after-free in
hci_remove_adv_monitor(). Trawling through the disassembly, you can
see that the complaint is from the access in bt_dev_dbg() under the
HCI_ADV_MONITOR_EXT_MSFT case. The problem case happens because
msft_remove_monitor() can end up freeing the monitor
structure. Specifically:
hci_remove_adv_monitor() ->
msft_remove_monitor() ->
msft_remove_monitor_sync() ->
msft_le_cancel_monitor_advertisement_cb() ->
hci_free_adv_monitor()
Let's fix the problem by just stashing the relevant data when it's
still valid.
Fixes: 7cf5c2978f23 ("Bluetooth: hci_sync: Refactor remove Adv Monitor")
Signed-off-by: Douglas Anderson <[email protected]>
---
Changes in v3:
- Back to v1 where we stash the handle in a local.
Changes in v2:
- Move the print, don't stash handle in a local.
net/bluetooth/hci_core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 48917c68358d..dbb2043a9112 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1972,6 +1972,7 @@ static int hci_remove_adv_monitor(struct hci_dev *hdev,
struct adv_monitor *monitor)
{
int status = 0;
+ int handle;
switch (hci_get_adv_monitor_offload_ext(hdev)) {
case HCI_ADV_MONITOR_EXT_NONE: /* also goes here when powered off */
@@ -1980,9 +1981,10 @@ static int hci_remove_adv_monitor(struct hci_dev *hdev,
goto free_monitor;
case HCI_ADV_MONITOR_EXT_MSFT:
+ handle = monitor->handle;
status = msft_remove_monitor(hdev, monitor);
bt_dev_dbg(hdev, "%s remove monitor %d msft status %d",
- hdev->name, monitor->handle, status);
+ hdev->name, handle, status);
break;
}
--
2.41.0.255.g8b1d071c50-goog
The hci_add_adv_monitor() hci_remove_adv_monitor() functions call
bt_dev_dbg() to print some debug statements. The bt_dev_dbg() macro
automatically adds in the device's name. That means that we shouldn't
include the name in the bt_dev_dbg() calls.
Suggested-by: Luiz Augusto von Dentz <[email protected]>
Signed-off-by: Douglas Anderson <[email protected]>
---
(no changes since v2)
Changes in v2:
- ("Don't double print name...") new for v2.
net/bluetooth/hci_core.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index dbb2043a9112..de15a2c77e9f 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1949,14 +1949,14 @@ int hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor)
switch (hci_get_adv_monitor_offload_ext(hdev)) {
case HCI_ADV_MONITOR_EXT_NONE:
- bt_dev_dbg(hdev, "%s add monitor %d status %d", hdev->name,
+ bt_dev_dbg(hdev, "add monitor %d status %d",
monitor->handle, status);
/* Message was not forwarded to controller - not an error */
break;
case HCI_ADV_MONITOR_EXT_MSFT:
status = msft_add_monitor_pattern(hdev, monitor);
- bt_dev_dbg(hdev, "%s add monitor %d msft status %d", hdev->name,
+ bt_dev_dbg(hdev, "add monitor %d msft status %d",
monitor->handle, status);
break;
}
@@ -1976,15 +1976,15 @@ static int hci_remove_adv_monitor(struct hci_dev *hdev,
switch (hci_get_adv_monitor_offload_ext(hdev)) {
case HCI_ADV_MONITOR_EXT_NONE: /* also goes here when powered off */
- bt_dev_dbg(hdev, "%s remove monitor %d status %d", hdev->name,
+ bt_dev_dbg(hdev, "remove monitor %d status %d",
monitor->handle, status);
goto free_monitor;
case HCI_ADV_MONITOR_EXT_MSFT:
handle = monitor->handle;
status = msft_remove_monitor(hdev, monitor);
- bt_dev_dbg(hdev, "%s remove monitor %d msft status %d",
- hdev->name, handle, status);
+ bt_dev_dbg(hdev, "remove monitor %d msft status %d",
+ handle, status);
break;
}
--
2.41.0.255.g8b1d071c50-goog
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=761691
---Test result---
Test Summary:
CheckPatch PASS 1.10 seconds
GitLint FAIL 0.77 seconds
SubjectPrefix PASS 0.16 seconds
BuildKernel PASS 32.78 seconds
CheckAllWarning PASS 35.74 seconds
CheckSparse PASS 40.37 seconds
CheckSmatch PASS 112.19 seconds
BuildKernel32 PASS 31.17 seconds
TestRunnerSetup PASS 447.30 seconds
TestRunner_l2cap-tester PASS 13.11 seconds
TestRunner_iso-tester PASS 23.69 seconds
TestRunner_bnep-tester PASS 5.58 seconds
TestRunner_mgmt-tester PASS 132.56 seconds
TestRunner_rfcomm-tester PASS 9.05 seconds
TestRunner_sco-tester PASS 10.42 seconds
TestRunner_ioctl-tester PASS 9.73 seconds
TestRunner_mesh-tester PASS 7.02 seconds
TestRunner_smp-tester PASS 8.21 seconds
TestRunner_userchan-tester PASS 5.80 seconds
IncrementalBuild PASS 35.20 seconds
Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[v3,1/2] Bluetooth: hci_sync: Avoid use-after-free in dbg for hci_remove_adv_monitor()
WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
1: T1 Title exceeds max length (86>80): "[v3,1/2] Bluetooth: hci_sync: Avoid use-after-free in dbg for hci_remove_adv_monitor()"
---
Regards,
Linux Bluetooth
Hi,
On Fri, Jun 30, 2023 at 3:59 PM <[email protected]> wrote:
>
> 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=761691
>
> ---Test result---
>
> Test Summary:
> CheckPatch PASS 1.10 seconds
> GitLint FAIL 0.77 seconds
> SubjectPrefix PASS 0.16 seconds
> BuildKernel PASS 32.78 seconds
> CheckAllWarning PASS 35.74 seconds
> CheckSparse PASS 40.37 seconds
> CheckSmatch PASS 112.19 seconds
> BuildKernel32 PASS 31.17 seconds
> TestRunnerSetup PASS 447.30 seconds
> TestRunner_l2cap-tester PASS 13.11 seconds
> TestRunner_iso-tester PASS 23.69 seconds
> TestRunner_bnep-tester PASS 5.58 seconds
> TestRunner_mgmt-tester PASS 132.56 seconds
> TestRunner_rfcomm-tester PASS 9.05 seconds
> TestRunner_sco-tester PASS 10.42 seconds
> TestRunner_ioctl-tester PASS 9.73 seconds
> TestRunner_mesh-tester PASS 7.02 seconds
> TestRunner_smp-tester PASS 8.21 seconds
> TestRunner_userchan-tester PASS 5.80 seconds
> IncrementalBuild PASS 35.20 seconds
>
> Details
> ##############################
> Test: GitLint - FAIL
> Desc: Run gitlint
> Output:
> [v3,1/2] Bluetooth: hci_sync: Avoid use-after-free in dbg for hci_remove_adv_monitor()
>
> WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
> 1: T1 Title exceeds max length (86>80): "[v3,1/2] Bluetooth: hci_sync: Avoid use-after-free in dbg for hci_remove_adv_monitor()"
I'll plan to ignore this one. It looks like it only exceeds 80
characters because of the version prefix "[v3, 1/2] ". If someone
really wants me to re-post with a shorter subject, please yell and I
will do that.
Thanks!
-Doug
Hello:
This series was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <[email protected]>:
On Fri, 30 Jun 2023 15:33:14 -0700 you wrote:
> KASAN reports that there's a use-after-free in
> hci_remove_adv_monitor(). Trawling through the disassembly, you can
> see that the complaint is from the access in bt_dev_dbg() under the
> HCI_ADV_MONITOR_EXT_MSFT case. The problem case happens because
> msft_remove_monitor() can end up freeing the monitor
> structure. Specifically:
> hci_remove_adv_monitor() ->
> msft_remove_monitor() ->
> msft_remove_monitor_sync() ->
> msft_le_cancel_monitor_advertisement_cb() ->
> hci_free_adv_monitor()
>
> [...]
Here is the summary with links:
- [v3,1/2] Bluetooth: hci_sync: Avoid use-after-free in dbg for hci_remove_adv_monitor()
https://git.kernel.org/bluetooth/bluetooth-next/c/2648c5eb33ca
- [v3,2/2] Bluetooth: hci_sync: Don't double print name in add/remove adv_monitor
https://git.kernel.org/bluetooth/bluetooth-next/c/22d2055a576d
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html