linux-kernel - RE: [PATCH] Bluetooth: Fix atomicity violation in {conn,adv}_{min,max}_interval

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0565eabbd25141fab9f3206db4e86196@AcuMS.aculab.com>
Date: Fri, 22 Dec 2023 11:41:09 +0000
From: David Laight <David.Laight@...LAB.COM>
To: 'Gui-Dong Han' <2045gemini@...il.com>, "marcel@...tmann.org"
	<marcel@...tmann.org>, "johan.hedberg@...il.com" <johan.hedberg@...il.com>,
	"luiz.dentz@...il.com" <luiz.dentz@...il.com>
CC: "linux-bluetooth@...r.kernel.org" <linux-bluetooth@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"baijiaju1990@...look.com" <baijiaju1990@...look.com>,
	"stable@...r.kernel.org" <stable@...r.kernel.org>, BassCheck
	<bass@...a.edu.cn>
Subject: RE: [PATCH] Bluetooth: Fix atomicity violation in
 {conn,adv}_{min,max}_interval_set

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)