RE: [PATCH] Bluetooth: Fix atomicity violation in {conn,adv}_{min,max}_interval_set

[David Laight <David.Laight@ACULAB.COM>]

From: Gui-Dong Han
> Sent: 22 December 2023 10:55
> 
> In {conn,adv}_min_interval_set():
> 	if (val < ... || val > ... || val > hdev->le_{conn,adv}_max_interval)
> 		return -EINVAL;
> 	hci_dev_lock(hdev);
> 	hdev->le_{conn,adv}_min_interval = val;
> 	hci_dev_unlock(hdev);
> 
> In {conn,adv}_max_interval_set():
> 	if (val < ... || val > ... || val < hdev->le_{conn,adv}_min_interval)
> 		return -EINVAL;
> 	hci_dev_lock(hdev);
> 	hdev->le_{conn,adv}_max_interval
> 	hci_dev_unlock(hdev);
> 
> The atomicity violation occurs due to concurrent execution of set_min and
> set_max funcs which may lead to inconsistent reads and writes of the min
> value and the max value. The checks for value validity are ineffective as
> the min/max values could change immediately after being checked, raising
> the risk of the min value being greater than the max value and causing
> invalid settings.
> 
> This possible bug is found by an experimental static analysis tool
> developed by our team, BassCheck[1]. This tool analyzes the locking APIs
> to extract function pairs that can be concurrently executed, and then
> analyzes the instructions in the paired functions to identify possible
> concurrency bugs including data races and atomicity violations. The above
> possible bug is reported when our tool analyzes the source code of
> Linux 5.17.

Your static analysis tool is basically broken.

The only possible issues are if the accesses aren't atomic.
In practise they always will be but using READ_ONCE() and
WRITE_ONCE() would make that certain.

The lock sequence:
> 	hci_dev_lock(hdev);
>  	hdev->le_conn_min_interval = val;
>  	hci_dev_unlock(hdev);
is pretty pointless - is doesn't 'lock' two+ things together.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)