bluetooth: host: Fix stale RPA usage after invalidation

[PavelVPV]

Add !BT_ADV_RPA_VALID check to force RPA regeneration when re-enabling an advertising set after RPA rotation occurred while disabled.

The BT_ADV_RANDOM_ADDR_UPDATED flag was added to prevent unnecessary address regeneration (RPA/NRPA) between bt_le_ext_adv_param_set() and bt_le_ext_adv_start() calls. However, this revealed an issue:

When RPA rotation (le_force_rpa_timeout) occurs while an advertiser is disabled, BT_ADV_RPA_VALID is cleared but the RPA is not regenerated. On subsequent bt_le_ext_adv_start() without a new param_set() call:

• BT_ADV_RANDOM_ADDR_UPDATED is already cleared (from previous start)
• Without BT_PER_ADV_ENABLED, no regeneration occurs
• Stale RPA is used, violating privacy requirements

Add !BT_ADV_RPA_VALID check for both connectable and non-connectable advertisers to ensure fresh RPA generation when the previous RPA was invalidated while the advertiser was disabled.

Fixes regression introduced in #98117.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Thalley]

Is bt_le_adv_start covered as well?

[PavelVPV]

The BT_ADV_RANDOM_ADDR_UPDATED flag should not affect bt_le_adv_start as far as I see:

zephyr/subsys/bluetooth/host/adv.c

Lines 1279 to 1288 in d02cdc7

	err = adv_create_legacy();
	if (err) {
	return err;
	}
	adv = bt_le_adv_lookup_legacy();
	if (IS_ENABLED(CONFIG_BT_EXT_ADV) &&
	BT_DEV_FEAT_LE_EXT_ADV(bt_dev.le.features)) {
	err = bt_le_adv_start_ext(adv, param, ad, ad_len, sd, sd_len);

But, bt_le_adv_start is implemented differently from bt_le_ext_adv_start:

zephyr/subsys/bluetooth/host/adv.c

Lines 1513 to 1528 in d02cdc7

	if (atomic_test_bit(adv->flags, BT_ADV_CONNECTABLE)) {
	if (IS_ENABLED(CONFIG_BT_PRIVACY) &&
	!atomic_test_bit(adv->flags, BT_ADV_USE_IDENTITY) &&
	(!atomic_test_and_clear_bit(adv->flags, BT_ADV_RANDOM_ADDR_UPDATED) ||
	atomic_test_bit(adv->flags, BT_PER_ADV_ENABLED))) {
	bt_id_set_adv_private_addr(adv);
	}
	} else {
	if (!atomic_test_bit(adv->flags, BT_ADV_USE_IDENTITY) &&
	(!atomic_test_and_clear_bit(adv->flags, BT_ADV_RANDOM_ADDR_UPDATED) ||
	atomic_test_bit(adv->flags, BT_PER_ADV_ENABLED))) {
	bt_id_set_adv_private_addr(adv);
	}
	}
	err = bt_le_adv_set_enable_ext(adv, true, param);

zephyr/subsys/bluetooth/host/adv.c

Lines 1214 to 1242 in d02cdc7

	adv->id = param->id;
	err = le_ext_adv_param_set(adv, param, sd != NULL);
	if (err) {
	return err;
	}
	if (!dir_adv) {
	if (IS_ENABLED(CONFIG_BT_EXT_ADV)) {
	err = bt_le_ext_adv_set_data(adv, ad, ad_len, sd, sd_len);
	if (err) {
	return err;
	}
	}
	} else {
	if (!(param->options & BT_LE_ADV_OPT_DIR_MODE_LOW_DUTY)) {
	start_param.timeout =
	BT_GAP_ADV_HIGH_DUTY_CYCLE_MAX_TIMEOUT;
	atomic_set_bit(adv->flags, BT_ADV_LIMITED);
	}
	}
	if (IS_ENABLED(CONFIG_BT_PERIPHERAL) && (param->options & BT_LE_ADV_OPT_CONN)) {
	err = le_adv_start_add_conn(adv, &conn);
	if (err) {
	return err;
	}
	}
	err = bt_le_adv_set_enable_ext(adv, true, &start_param);

and can be vulnerable to this as well if e.g. bt_le_oob_get_local is called from another thread that gets execution time while bt_le_adv_start sending commands to Controller.

Though I don't have means to test this. If it is true, it should be fixed separately.

[Thalley]

Though I don't have means to test this. If it is true, it should be fixed separately.

That's fair.

Since bt_le_adv_start calls bt_le_adv_start_ext I think any fixes for bt_le_adv_start_ext should be applied automatically to bt_le_adv_start

[alwa-nordic]

Wow the logic was already a mess and it's getting more complex. It should work as a quick fix though.

I think there's a small error in the commit message, correct? @PavelVPV Too late to fix it now, but I just want to note it here for future git archeology.

- - BT_ADV_RANDOM_ADDR_UPDATED is already cleared (from previous start)
+ - BT_ADV_RANDOM_ADDR_UPDATED is already set (from previous start)

Do you have any thoughts about how to simplify this in future work, @PavelVPV? I think it's not sustainable to layer more and more logic for something that feels like it should be fairly straight forward.

I'm thinking an alternative would be to update the documentation of BT_ADV_RANDOM_ADDR_UPDATED...

-Advertising random address has been updated in the controller before enabling advertising
+Advertising random address is up to date in the controller

...and clear BT_ADV_RANDOM_ADDR_UPDATED when the RPA expires.

[PavelVPV]

Wow the logic was already a mess and it's getting more complex. It should work as a quick fix though.

I think there's a small error in the commit message, correct? @PavelVPV Too late to fix it now, but I just want to note it here for future git archeology.

- - BT_ADV_RANDOM_ADDR_UPDATED is already cleared (from previous start)
+ - BT_ADV_RANDOM_ADDR_UPDATED is already set (from previous start)

Yeah, correct.

Do you have any thoughts about how to simplify this in future work, @PavelVPV? I think it's not sustainable to layer more and more logic for something that feels like it should be fairly straight forward.

I'm thinking an alternative would be to update the documentation of BT_ADV_RANDOM_ADDR_UPDATED...

-Advertising random address has been updated in the controller before enabling advertising
+Advertising random address is up to date in the controller

...and clear BT_ADV_RANDOM_ADDR_UPDATED when the RPA expires.

This would probably work. The code is quite messy with that many flags changed and many different places by different modules. We should start refactoring from attacking each flag separately, modularizing each feature one at a time.