[PATCH v3] Bluetooth: mgmt: drop advertising instances the controller cannot carry

[PATCH v3] Bluetooth: mgmt: drop advertising instances the controller cannot carry

[Piyus Kedia]

An advertising instance's data is validated once, when the instance is
added, by tlv_data_is_valid() -> tlv_data_max_len() -> max_adv_len().
max_adv_len() branches on ext_adv_capable(hdev), which reads
hdev->le_features[1] - a controller capability re-read from the device on
every bring-up.

Nothing re-checks the stored instances when that capability changes.  A
controller that reported LE Extended Advertising when the instance was
added, and does not report it after the next bring-up, leaves behind an
instance of up to 251 bytes that is then rendered into the 31-byte cp.data
of hci_set_adv_data_sync() or __hci_set_scan_rsp_data_sync():

  Kernel panic - not syncing: stack-protector: Kernel stack is corrupted
  in: hci_update_adv_data_sync+0x1dc/0x1e0

  Kernel panic - not syncing: stack-protector: Kernel stack is corrupted
  in: hci_update_scan_rsp_data_sync+0x20f/0x210

Drop such instances once the capability has been re-read, and tell
userspace through MGMT_EV_ADVERTISING_REMOVED so it can add them again at a
size this controller will take.

Two details decide whether this is correct.

The test has to be tlv_data_max_len() itself, not a bare max_adv_len().
That function already encodes the per-flag adjustments the render applies -
4 bytes for MGMT_ADV_FLAG_APPEARANCE, calculate_name_len() for
MGMT_ADV_FLAG_LOCAL_NAME, and the flags and TX power fields on the
advertising data side - so re-using it keeps one source of truth and the
check cannot drift away from eir_create_adv_data() and
eir_create_scan_rsp().  Comparing the stored lengths against max_adv_len()
instead misses exactly those bytes: a 30-byte scan response carrying
MGMT_ADV_FLAG_APPEARANCE is within 31 and is still rendered as 34.
hci_adv_instance_flags() returns adv->flags for any non-zero instance, so
adv->flags is the flag set the render will use.

Using the add-time test means the rule is "would this instance still be
accepted today", which is marginally stricter than "would this instance
overflow".  eir_create_adv_data() drops the flags and TX power fields when
they do not fit rather than overflowing, so an instance can be removed
here that would in fact still have rendered.  That seems the right way
round: the instance is one userspace could not add now, and it is told
about the removal so it can add a version that fits.

The call goes before the HCI_LE_ENABLED test in
hci_powered_update_adv_sync(), not after it.  set_le() on an unpowered
device changes that flag without clearing hdev->adv_instances, so an
instance left behind would otherwise survive a bring-up untouched.  This
runs once per bring-up, which is also the only point at which the
capability can have changed: hdev->le_features is written only by the two
handlers for the controller's LE Read Local Supported Features reply, both
inside the init sequence.

Per Documentation/process/generated-content.rst: the out-of-bounds writes
were found by an AI-driven fuzzing harness developed jointly by Kapil
Vaswani and myself.  The approach taken here is Luiz's, from his review of
v1; the patch text, this changelog and the test programs were produced with
the assistance of a large language model and reviewed by Kapil Vaswani and
myself, who designed and ran the testing described below the ---.

Suggested-by: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Signed-off-by: Kapil Vaswani <kapil@presceralabs.ai>
Signed-off-by: Piyus Kedia <piyus@iiitd.ac.in>
---
Changes in v3:
  - take the approach you suggested on v1 and unregister the instances,
    rather than bounding the copies in eir.c
  - test with tlv_data_max_len() rather than max_adv_len(), which is what
    Sashiko's second finding is about: the per-flag adjustments cannot
    drift from the render if the check reuses the add path's own test
  - call it before the HCI_LE_ENABLED early return, which is Sashiko's
    first finding
  - runtime before/after on net/main for four scenarios

Changes in v2:
  - dropped oversized instances in hci_powered_update_adv_sync(), tested
    against max_adv_len()

Both Sashiko findings, and where they land:

  1. The early return skipping the loop.  Fixed by calling before it.  We
     could not reproduce the overflow the report draws from it, though: on
     re-enabling LE, set_le_sync() calls
     hci_update_adv_data_sync(hdev, 0x00) and
     hci_update_scan_rsp_data_sync(hdev, 0x00), and instance 0 has no
     adv_info, so neither copy runs.  The skip is real and worth fixing;
     the path from it to a write was not one we could trigger.

  2. MGMT_ADV_FLAG_APPEARANCE with scan_rsp_len in 28..31.  Reproduced at
     30, which v2 keeps and renders as 34.
     tlv_data_max_len(hdev, MGMT_ADV_FLAG_APPEARANCE, false) is 27, so all
     of 28..31 are now dropped, and 27 - which renders as exactly 31 -
     is kept.

Tested on net/main at a2fd526d6442 (7.4.0-rc1), x86_64 defconfig plus KASAN
with KASAN_STACK, booted by hand under qemu against the same rootfs each
time with the image opened read-only.  "len=" is the length field of the HCI
command the kernel emitted, read off the virtual controller - a witness
independent of KASAN, since anything above 31 means eir_create_*() has
already written past cp.data[31]:

  scenario    | unpatched           | v2                  | this patch
  ------------+---------------------+---------------------+----------------
  adv_data    | len=220, panic      | no command          | no cmd, pruned
  scan_rsp    | len=220, panic      | no command          | no cmd, pruned
  appearance  | len=34              | len=34              | no cmd, pruned
  le_toggle   | no command          | no command          | no cmd, pruned

adv_data / scan_rsp: a 220-byte instance added while the controller reported
LE Extended Advertising, then re-read without it.  Both panic on an
unpatched kernel:

  Kernel panic - not syncing: stack-protector: Kernel stack is corrupted
  in: hci_update_adv_data_sync+0x1dc/0x1e0
  in: hci_update_scan_rsp_data_sync+0x20f/0x210

appearance: KASAN does not report this one - three bytes past a 32-byte
stack object land in adjacent locals rather than in a redzone - which is
why the emitted length is the evidence.

No Fixes: tag - our clone is shallow and git blame lands on the shallow
boundary, so we cannot name the introducing commit.

The reproducer is one C program with the four modes above and needs no
Bluetooth hardware.  It is not included here: this was found with AI
assistance, which Documentation/process/security-bugs.rst says to treat as
public while not posting the reproducer.  Ask and we will send it.

 include/net/bluetooth/hci_core.h |  1 +
 net/bluetooth/hci_sync.c         |  8 ++++++
 net/bluetooth/mgmt.c             | 49 ++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 6fb713d5e4be..ab5aa80abe2b 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -2573,6 +2573,7 @@ bool mgmt_get_connectable(struct hci_dev *hdev);
 u8 mgmt_get_adv_discov_flags(struct hci_dev *hdev);
 void mgmt_advertising_added(struct sock *sk, struct hci_dev *hdev,
 			    u8 instance);
+void mgmt_remove_oversized_adv_instances(struct hci_dev *hdev);
 void mgmt_advertising_removed(struct sock *sk, struct hci_dev *hdev,
 			      u8 instance);
 int mgmt_phy_configuration_changed(struct hci_dev *hdev, struct sock *skip);
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 9213e3ef9f26..b9f06dad1c41 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -3605,6 +3605,14 @@ static int hci_powered_update_adv_sync(struct hci_dev *hdev)
 	struct adv_info *adv, *tmp;
 	int err;
 
+	/* The controller's advertising capability has just been re-read, so
+	 * this is the first point at which stored instances can be measured
+	 * against it.  Do it before the HCI_LE_ENABLED test below: set_le() on
+	 * an unpowered device clears that flag without clearing the instances,
+	 * and one left behind must not survive to be rendered later.
+	 */
+	mgmt_remove_oversized_adv_instances(hdev);
+
 	if (!hci_dev_test_flag(hdev, HCI_LE_ENABLED))
 		return 0;
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 260ef2a45719..d6a20a08b49d 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -8933,6 +8933,55 @@ static u8 tlv_data_max_len(struct hci_dev *hdev, u32 adv_flags,
 	return max_len;
 }
 
+/* Drop stored advertising instances that the controller can no longer carry.
+ *
+ * An instance's data was accepted against tlv_data_max_len() as it was when
+ * the instance was added, and that depends on max_adv_len(), which branches on
+ * ext_adv_capable() - a controller capability re-read from the device on every
+ * bring-up.  A controller that stops reporting LE Extended Advertising
+ * therefore leaves instances behind that can no longer be rendered:
+ * eir_create_adv_data() and eir_create_scan_rsp() would write past the 31-byte
+ * buffers of hci_set_adv_data_sync() and __hci_set_scan_rsp_data_sync().
+ *
+ * Re-using tlv_data_max_len() is deliberate.  It is the same test the add path
+ * applies, so the per-flag adjustments - 4 bytes for MGMT_ADV_FLAG_APPEARANCE,
+ * calculate_name_len() for MGMT_ADV_FLAG_LOCAL_NAME, and the flags and TX
+ * power fields on the advertising data side - cannot drift out of agreement
+ * with it.  Comparing the stored lengths against a bare max_adv_len() misses
+ * exactly those bytes.
+ *
+ * hci_adv_instance_flags() returns adv->flags for any non-zero instance, so
+ * adv->flags is the flag set the render will actually use.
+ */
+void mgmt_remove_oversized_adv_instances(struct hci_dev *hdev)
+{
+	struct adv_info *adv, *n;
+
+	/* This is safe as long as there is no command send while the lock is
+	 * held.
+	 */
+	hci_dev_lock(hdev);
+
+	list_for_each_entry_safe(adv, n, &hdev->adv_instances, list) {
+		u8 instance = adv->instance;
+
+		if (adv->adv_data_len <= tlv_data_max_len(hdev, adv->flags,
+							  true) &&
+		    adv->scan_rsp_len <= tlv_data_max_len(hdev, adv->flags,
+							  false))
+			continue;
+
+		bt_dev_warn(hdev,
+			    "advertising instance %u no longer fits, removing",
+			    instance);
+
+		if (!hci_remove_adv_instance(hdev, instance))
+			mgmt_advertising_removed(NULL, hdev, instance);
+	}
+
+	hci_dev_unlock(hdev);
+}
+
 static bool flags_managed(u32 adv_flags)
 {
 	return adv_flags & (MGMT_ADV_FLAG_DISCOV |

base-commit: a2fd526d644233d6a892c28173f07c61e21e4c93
-- 
2.34.1

RE: [v3] Bluetooth: mgmt: drop advertising instances the controller cannot carry

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1606 bytes --]

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/series/1209555/

---Test result---

Test Summary:
CheckPatch                    PASS      1.29 seconds
VerifyFixes                   PASS      0.13 seconds
VerifySignedoff               PASS      0.13 seconds
GitLint                       PASS      0.35 seconds
SubjectPrefix                 PASS      0.13 seconds
BuildKernel                   PASS      32.08 seconds
CheckAllWarning               PASS      35.75 seconds
CheckSparse                   PASS      38.44 seconds
BuildKernel32                 PASS      31.66 seconds
CheckKernelLLVM               PASS      34.27 seconds
TestRunnerSetup               PASS      753.64 seconds
TestRunner_l2cap-tester       PASS      14.96 seconds
TestRunner_iso-tester         PASS      13.30 seconds
TestRunner_bnep-tester        PASS      3.18 seconds
TestRunner_mgmt-tester        PASS      88.28 seconds
TestRunner_rfcomm-tester      PASS      4.54 seconds
TestRunner_sco-tester         PASS      5.66 seconds
TestRunner_ioctl-tester       PASS      4.72 seconds
TestRunner_mesh-tester        PASS      5.58 seconds
TestRunner_smp-tester         PASS      4.40 seconds
TestRunner_userchan-tester    PASS      3.26 seconds
TestRunner_6lowpan-tester     PASS      4.49 seconds
IncrementalBuild              PASS      29.58 seconds



https://github.com/bluez/bluetooth-next/pull/1122

---
Regards,
Linux Bluetooth