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)
