[PATCH] Bluetooth: add timeout sanity check to hci_inquiry

[PATCH] Bluetooth: add timeout sanity check to hci_inquiry

[Pavel Skripkin]

Syzbot hit "task hung" bug in hci_req_sync(). The problem was in
unreasonable huge inquiry timeout passed from userspace.
Fix it by adding sanity check for timeout value and add constant to
hsi_sock.h to inform userspace, that hci_inquiry_req::length field has
maximum possible value.

Since hci_inquiry() is the only user of hci_req_sync() with user
controlled timeout value, it makes sense to check timeout value in
hci_inquiry() and don't touch hci_req_sync().

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-and-tested-by: syzbot+be2baed593ea56c6a84c@syzkaller.appspotmail.com
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---

Hi, Bluetooth maintainers/reviewers!

I believe, 60 seconds will be more than enough for inquiry request. I've
searched for examples on the internet and maximum ir.length I found was 
8. Maybe, we have users, which need more than 60 seconds... I look forward
to receiving your views on this value.

---
 include/net/bluetooth/hci_sock.h | 1 +
 net/bluetooth/hci_core.c         | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/include/net/bluetooth/hci_sock.h b/include/net/bluetooth/hci_sock.h
index 9949870f7d78..1cd63d4da00b 100644
--- a/include/net/bluetooth/hci_sock.h
+++ b/include/net/bluetooth/hci_sock.h
@@ -168,6 +168,7 @@ struct hci_inquiry_req {
 	__u16 dev_id;
 	__u16 flags;
 	__u8  lap[3];
+#define HCI_INQUIRY_MAX_TIMEOUT		30
 	__u8  length;
 	__u8  num_rsp;
 };
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index e1a545c8a69f..104babf67351 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1343,6 +1343,11 @@ int hci_inquiry(void __user *arg)
 		goto done;
 	}
 
+	if (ir.length > HCI_INQUIRY_MAX_TIMEOUT) {
+		err = -EINVAL;
+		goto done;
+	}
+
 	hci_dev_lock(hdev);
 	if (inquiry_cache_age(hdev) > INQUIRY_CACHE_AGE_MAX ||
 	    inquiry_cache_empty(hdev) || ir.flags & IREQ_CACHE_FLUSH) {
-- 
2.32.0

Re: [PATCH] Bluetooth: add timeout sanity check to hci_inquiry

[Marcel Holtmann]

Hi Pavel,

> Syzbot hit "task hung" bug in hci_req_sync(). The problem was in
> unreasonable huge inquiry timeout passed from userspace.
> Fix it by adding sanity check for timeout value and add constant to
> hsi_sock.h to inform userspace, that hci_inquiry_req::length field has
> maximum possible value.
> 
> Since hci_inquiry() is the only user of hci_req_sync() with user
> controlled timeout value, it makes sense to check timeout value in
> hci_inquiry() and don't touch hci_req_sync().
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-and-tested-by: syzbot+be2baed593ea56c6a84c@syzkaller.appspotmail.com
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> ---
> 
> Hi, Bluetooth maintainers/reviewers!
> 
> I believe, 60 seconds will be more than enough for inquiry request. I've
> searched for examples on the internet and maximum ir.length I found was 
> 8. Maybe, we have users, which need more than 60 seconds... I look forward
> to receiving your views on this value.
> 
> ---
> include/net/bluetooth/hci_sock.h | 1 +
> net/bluetooth/hci_core.c         | 5 +++++
> 2 files changed, 6 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci_sock.h b/include/net/bluetooth/hci_sock.h
> index 9949870f7d78..1cd63d4da00b 100644
> --- a/include/net/bluetooth/hci_sock.h
> +++ b/include/net/bluetooth/hci_sock.h
> @@ -168,6 +168,7 @@ struct hci_inquiry_req {
> 	__u16 dev_id;
> 	__u16 flags;
> 	__u8  lap[3];
> +#define HCI_INQUIRY_MAX_TIMEOUT		30
> 	__u8  length;
> 	__u8  num_rsp;
> };
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index e1a545c8a69f..104babf67351 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1343,6 +1343,11 @@ int hci_inquiry(void __user *arg)
> 		goto done;
> 	}
> 

	/* Restrict maximum inquiry length to 60 seconds */
	if (ir.length > 60) {
		..
	}

> +	if (ir.length > HCI_INQUIRY_MAX_TIMEOUT) {
> +		err = -EINVAL;
> +		goto done;
> +	}
> +

I found this easier to read than adding anything define somewhere else. And since this is a legacy interface that is no longer used by bluetoothd, this should be fine. We will start to deprecate this eventually.

And I prefer 1 minute max time here. Just to be safe.

Regards

Marcel

Re: [PATCH] Bluetooth: add timeout sanity check to hci_inquiry

[Pavel Skripkin]

On 8/19/21 6:05 PM, Marcel Holtmann wrote:
> Hi Pavel,
> 
>> 	}
>> 
> 
> 	/* Restrict maximum inquiry length to 60 seconds */
> 	if (ir.length > 60) {
> 		..
> 	}
> 
>> +	if (ir.length > HCI_INQUIRY_MAX_TIMEOUT) {
>> +		err = -EINVAL;
>> +		goto done;
>> +	}
>> +
> 
> I found this easier to read than adding anything define somewhere else. And since this is a legacy interface that is no longer used by bluetoothd, this should be fine. We will start to deprecate this eventually.
> 
> And I prefer 1 minute max time here. Just to be safe.
> 

I thought, that user-space should be aware of maximum value, that's why 
I decided to add this define :) I didn't know, that this interface is 
legacy.

Will fix in v2, thank you!


With regards,
Pavel Skripkin

[PATCH v2] Bluetooth: add timeout sanity check to hci_inquiry

[Pavel Skripkin]

Syzbot hit "task hung" bug in hci_req_sync(). The problem was in
unreasonable huge inquiry timeout passed from userspace.
Fix it by adding sanity check for timeout value to hci_inquiry().

Since hci_inquiry() is the only user of hci_req_sync() with user
controlled timeout value, it makes sense to check timeout value in
hci_inquiry() and don't touch hci_req_sync().

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-and-tested-by: syzbot+be2baed593ea56c6a84c@syzkaller.appspotmail.com
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---

Changes in v2:
	Removed define + added comment suggested by Marcel

---
 net/bluetooth/hci_core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index e1a545c8a69f..170f513efa86 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1343,6 +1343,12 @@ int hci_inquiry(void __user *arg)
 		goto done;
 	}
 
+	/* Restrict maximum inquiry length to 60 seconds */
+	if (ir.length > 60) {
+		err = -EINVAL;
+		goto done;
+	}
+
 	hci_dev_lock(hdev);
 	if (inquiry_cache_age(hdev) > INQUIRY_CACHE_AGE_MAX ||
 	    inquiry_cache_empty(hdev) || ir.flags & IREQ_CACHE_FLUSH) {
-- 
2.32.0

Re: [PATCH v2] Bluetooth: add timeout sanity check to hci_inquiry

[Marcel Holtmann]

Hi Pavel,

> Syzbot hit "task hung" bug in hci_req_sync(). The problem was in
> unreasonable huge inquiry timeout passed from userspace.
> Fix it by adding sanity check for timeout value to hci_inquiry().
> 
> Since hci_inquiry() is the only user of hci_req_sync() with user
> controlled timeout value, it makes sense to check timeout value in
> hci_inquiry() and don't touch hci_req_sync().
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-and-tested-by: syzbot+be2baed593ea56c6a84c@syzkaller.appspotmail.com
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> ---
> 
> Changes in v2:
> 	Removed define + added comment suggested by Marcel
> 
> ---
> net/bluetooth/hci_core.c | 6 ++++++
> 1 file changed, 6 insertions(+)

patch has been applied to bluetooth-next tree.

Regards

Marcel

Re: [PATCH] Bluetooth: add timeout sanity check to hci_inquiry

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4760 bytes --]

Hi Pavel,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on bluetooth/master]
[also build test ERROR on bluetooth-next/master net-next/master net/master sparc-next/master v5.14-rc6 next-20210816]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Pavel-Skripkin/Bluetooth-add-timeout-sanity-check-to-hci_inquiry/20210817-040113
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth.git master
config: hexagon-randconfig-r022-20210816 (attached as .config)
compiler: clang version 12.0.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/cb175bf2ea0de6152c66ce30cd1d3d665fda338b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Pavel-Skripkin/Bluetooth-add-timeout-sanity-check-to-hci_inquiry/20210817-040113
        git checkout cb175bf2ea0de6152c66ce30cd1d3d665fda338b
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross O=build_dir ARCH=hexagon SHELL=/bin/bash net/bluetooth/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> net/bluetooth/hci_core.c:1346:18: error: use of undeclared identifier 'HCI_MAX_TIMEOUT'
           if (ir.length > HCI_MAX_TIMEOUT) {
                           ^
   1 error generated.


vim +/HCI_MAX_TIMEOUT +1346 net/bluetooth/hci_core.c

  1309	
  1310	int hci_inquiry(void __user *arg)
  1311	{
  1312		__u8 __user *ptr = arg;
  1313		struct hci_inquiry_req ir;
  1314		struct hci_dev *hdev;
  1315		int err = 0, do_inquiry = 0, max_rsp;
  1316		long timeo;
  1317		__u8 *buf;
  1318	
  1319		if (copy_from_user(&ir, ptr, sizeof(ir)))
  1320			return -EFAULT;
  1321	
  1322		hdev = hci_dev_get(ir.dev_id);
  1323		if (!hdev)
  1324			return -ENODEV;
  1325	
  1326		if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
  1327			err = -EBUSY;
  1328			goto done;
  1329		}
  1330	
  1331		if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED)) {
  1332			err = -EOPNOTSUPP;
  1333			goto done;
  1334		}
  1335	
  1336		if (hdev->dev_type != HCI_PRIMARY) {
  1337			err = -EOPNOTSUPP;
  1338			goto done;
  1339		}
  1340	
  1341		if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED)) {
  1342			err = -EOPNOTSUPP;
  1343			goto done;
  1344		}
  1345	
> 1346		if (ir.length > HCI_MAX_TIMEOUT) {
  1347			err = -EINVAL;
  1348			goto done;
  1349		}
  1350	
  1351		hci_dev_lock(hdev);
  1352		if (inquiry_cache_age(hdev) > INQUIRY_CACHE_AGE_MAX ||
  1353		    inquiry_cache_empty(hdev) || ir.flags & IREQ_CACHE_FLUSH) {
  1354			hci_inquiry_cache_flush(hdev);
  1355			do_inquiry = 1;
  1356		}
  1357		hci_dev_unlock(hdev);
  1358	
  1359		timeo = ir.length * msecs_to_jiffies(2000);
  1360	
  1361		if (do_inquiry) {
  1362			err = hci_req_sync(hdev, hci_inq_req, (unsigned long) &ir,
  1363					   timeo, NULL);
  1364			if (err < 0)
  1365				goto done;
  1366	
  1367			/* Wait until Inquiry procedure finishes (HCI_INQUIRY flag is
  1368			 * cleared). If it is interrupted by a signal, return -EINTR.
  1369			 */
  1370			if (wait_on_bit(&hdev->flags, HCI_INQUIRY,
  1371					TASK_INTERRUPTIBLE)) {
  1372				err = -EINTR;
  1373				goto done;
  1374			}
  1375		}
  1376	
  1377		/* for unlimited number of responses we will use buffer with
  1378		 * 255 entries
  1379		 */
  1380		max_rsp = (ir.num_rsp == 0) ? 255 : ir.num_rsp;
  1381	
  1382		/* cache_dump can't sleep. Therefore we allocate temp buffer and then
  1383		 * copy it to the user space.
  1384		 */
  1385		buf = kmalloc_array(max_rsp, sizeof(struct inquiry_info), GFP_KERNEL);
  1386		if (!buf) {
  1387			err = -ENOMEM;
  1388			goto done;
  1389		}
  1390	
  1391		hci_dev_lock(hdev);
  1392		ir.num_rsp = inquiry_cache_dump(hdev, max_rsp, buf);
  1393		hci_dev_unlock(hdev);
  1394	
  1395		BT_DBG("num_rsp %d", ir.num_rsp);
  1396	
  1397		if (!copy_to_user(ptr, &ir, sizeof(ir))) {
  1398			ptr += sizeof(ir);
  1399			if (copy_to_user(ptr, buf, sizeof(struct inquiry_info) *
  1400					 ir.num_rsp))
  1401				err = -EFAULT;
  1402		} else
  1403			err = -EFAULT;
  1404	
  1405		kfree(buf);
  1406	
  1407	done:
  1408		hci_dev_put(hdev);
  1409		return err;
  1410	}
  1411	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31583 bytes --]

Re: [PATCH] Bluetooth: add timeout sanity check to hci_inquiry

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5026 bytes --]

Hi Pavel,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on bluetooth/master]
[also build test ERROR on bluetooth-next/master net-next/master net/master sparc-next/master v5.14-rc6 next-20210816]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Pavel-Skripkin/Bluetooth-add-timeout-sanity-check-to-hci_inquiry/20210817-040113
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth.git master
config: arc-randconfig-r043-20210816 (attached as .config)
compiler: arc-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/cb175bf2ea0de6152c66ce30cd1d3d665fda338b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Pavel-Skripkin/Bluetooth-add-timeout-sanity-check-to-hci_inquiry/20210817-040113
        git checkout cb175bf2ea0de6152c66ce30cd1d3d665fda338b
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash net/bluetooth/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   net/bluetooth/hci_core.c: In function 'hci_inquiry':
>> net/bluetooth/hci_core.c:1346:25: error: 'HCI_MAX_TIMEOUT' undeclared (first use in this function); did you mean 'HCI_CMD_TIMEOUT'?
    1346 |         if (ir.length > HCI_MAX_TIMEOUT) {
         |                         ^~~~~~~~~~~~~~~
         |                         HCI_CMD_TIMEOUT
   net/bluetooth/hci_core.c:1346:25: note: each undeclared identifier is reported only once for each function it appears in


vim +1346 net/bluetooth/hci_core.c

  1309	
  1310	int hci_inquiry(void __user *arg)
  1311	{
  1312		__u8 __user *ptr = arg;
  1313		struct hci_inquiry_req ir;
  1314		struct hci_dev *hdev;
  1315		int err = 0, do_inquiry = 0, max_rsp;
  1316		long timeo;
  1317		__u8 *buf;
  1318	
  1319		if (copy_from_user(&ir, ptr, sizeof(ir)))
  1320			return -EFAULT;
  1321	
  1322		hdev = hci_dev_get(ir.dev_id);
  1323		if (!hdev)
  1324			return -ENODEV;
  1325	
  1326		if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
  1327			err = -EBUSY;
  1328			goto done;
  1329		}
  1330	
  1331		if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED)) {
  1332			err = -EOPNOTSUPP;
  1333			goto done;
  1334		}
  1335	
  1336		if (hdev->dev_type != HCI_PRIMARY) {
  1337			err = -EOPNOTSUPP;
  1338			goto done;
  1339		}
  1340	
  1341		if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED)) {
  1342			err = -EOPNOTSUPP;
  1343			goto done;
  1344		}
  1345	
> 1346		if (ir.length > HCI_MAX_TIMEOUT) {
  1347			err = -EINVAL;
  1348			goto done;
  1349		}
  1350	
  1351		hci_dev_lock(hdev);
  1352		if (inquiry_cache_age(hdev) > INQUIRY_CACHE_AGE_MAX ||
  1353		    inquiry_cache_empty(hdev) || ir.flags & IREQ_CACHE_FLUSH) {
  1354			hci_inquiry_cache_flush(hdev);
  1355			do_inquiry = 1;
  1356		}
  1357		hci_dev_unlock(hdev);
  1358	
  1359		timeo = ir.length * msecs_to_jiffies(2000);
  1360	
  1361		if (do_inquiry) {
  1362			err = hci_req_sync(hdev, hci_inq_req, (unsigned long) &ir,
  1363					   timeo, NULL);
  1364			if (err < 0)
  1365				goto done;
  1366	
  1367			/* Wait until Inquiry procedure finishes (HCI_INQUIRY flag is
  1368			 * cleared). If it is interrupted by a signal, return -EINTR.
  1369			 */
  1370			if (wait_on_bit(&hdev->flags, HCI_INQUIRY,
  1371					TASK_INTERRUPTIBLE)) {
  1372				err = -EINTR;
  1373				goto done;
  1374			}
  1375		}
  1376	
  1377		/* for unlimited number of responses we will use buffer with
  1378		 * 255 entries
  1379		 */
  1380		max_rsp = (ir.num_rsp == 0) ? 255 : ir.num_rsp;
  1381	
  1382		/* cache_dump can't sleep. Therefore we allocate temp buffer and then
  1383		 * copy it to the user space.
  1384		 */
  1385		buf = kmalloc_array(max_rsp, sizeof(struct inquiry_info), GFP_KERNEL);
  1386		if (!buf) {
  1387			err = -ENOMEM;
  1388			goto done;
  1389		}
  1390	
  1391		hci_dev_lock(hdev);
  1392		ir.num_rsp = inquiry_cache_dump(hdev, max_rsp, buf);
  1393		hci_dev_unlock(hdev);
  1394	
  1395		BT_DBG("num_rsp %d", ir.num_rsp);
  1396	
  1397		if (!copy_to_user(ptr, &ir, sizeof(ir))) {
  1398			ptr += sizeof(ir);
  1399			if (copy_to_user(ptr, buf, sizeof(struct inquiry_info) *
  1400					 ir.num_rsp))
  1401				err = -EFAULT;
  1402		} else
  1403			err = -EFAULT;
  1404	
  1405		kfree(buf);
  1406	
  1407	done:
  1408		hci_dev_put(hdev);
  1409		return err;
  1410	}
  1411	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32665 bytes --]