Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-713-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: David Laight <David.Laight@ACULAB.COM>
To: 'Gui-Dong Han' <2045gemini@gmail.com>, "marcel@holtmann.org"
	<marcel@holtmann.org>, "johan.hedberg@gmail.com" <johan.hedberg@gmail.com>,
	"luiz.dentz@gmail.com" <luiz.dentz@gmail.com>
CC: "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"baijiaju1990@outlook.com" <baijiaju1990@outlook.com>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>, BassCheck
	<bass@buaa.edu.cn>
Subject: RE: [PATCH] Bluetooth: Fix atomicity violation in
 {conn,adv}_{min,max}_interval_set
Thread-Topic: [PATCH] Bluetooth: Fix atomicity violation in
 {conn,adv}_{min,max}_interval_set
Thread-Index: AQHaNMVjcf/fSWdBLky9VyA+zJFtpLC1LJZg
Date: Fri, 22 Dec 2023 11:41:09 +0000
Message-ID: <0565eabbd25141fab9f3206db4e86196@AcuMS.aculab.com>
References: <20231222105526.9208-1-2045gemini@gmail.com>
In-Reply-To: <20231222105526.9208-1-2045gemini@gmail.com>
Accept-Language: en-GB, en-US
Precedence: bulk
MIME-Version: 1.0
Content-Language: en-US
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

From: Gui-Dong Han
> Sent: 22 December 2023 10:55
>=20
> In {conn,adv}_min_interval_set():
> =09if (val < ... || val > ... || val > hdev->le_{conn,adv}_max_interval)
> =09=09return -EINVAL;
> =09hci_dev_lock(hdev);
> =09hdev->le_{conn,adv}_min_interval =3D val;
> =09hci_dev_unlock(hdev);
>=20
> In {conn,adv}_max_interval_set():
> =09if (val < ... || val > ... || val < hdev->le_{conn,adv}_min_interval)
> =09=09return -EINVAL;
> =09hci_dev_lock(hdev);
> =09hdev->le_{conn,adv}_max_interval
> =09hci_dev_unlock(hdev);
>=20
> 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.
>=20
> 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:
> =09hci_dev_lock(hdev);
>  =09hdev->le_conn_min_interval =3D val;
>  =09hci_dev_unlock(hdev);
is pretty pointless - is doesn't 'lock' two+ things together.

=09David

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