[RFCv2 0/6] Implement basic HCI block-based flow control

[RFCv2 0/6] Implement basic HCI block-based flow control

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Changes:
	RFCv2: taken Marcel's comments: added check for flow control,
	removed magic pointers and numbers, added SCO type check.

Simplify current handling of number of completed packets.
Adds basic HCI commands and event handlers related to data block
flow control.

Andrei Emeltchenko (6):
  Bluetooth: Add HCI Read Data Block Size function
  Bluetooth: Simplify num_comp_pkts_evt function
  Bluetooth: Check for flow control mode
  Bluetooth: Clean up magic pointers
  Bluetooth: Correct packet len calculation
  Bluetooth: Process num completed data blocks event

 include/net/bluetooth/hci.h      |   28 +++++++-
 include/net/bluetooth/hci_core.h |    5 ++
 net/bluetooth/hci_event.c        |  149 +++++++++++++++++++++++++++++++-------
 3 files changed, 156 insertions(+), 26 deletions(-)

-- 
1.7.4.1

[RFCv2 1/6] Bluetooth: Add HCI Read Data Block Size function

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Implement block size read function. Use different variables for
packet-based and block-based flow control.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
 include/net/bluetooth/hci.h      |    8 ++++++++
 include/net/bluetooth/hci_core.h |    5 +++++
 net/bluetooth/hci_event.c        |   26 ++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 12649d0..9490c2c 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -749,6 +749,14 @@ struct hci_rp_read_bd_addr {
 	bdaddr_t bdaddr;
 } __packed;
 
+#define HCI_OP_READ_DATA_BLOCK_SIZE	0x100a
+struct hci_rp_read_data_block_size {
+	__u8     status;
+	__le16   max_acl_len;
+	__le16   block_len;
+	__le16   num_blocks;
+} __packed;
+
 #define HCI_OP_WRITE_PAGE_SCAN_ACTIVITY	0x0c1c
 struct hci_cp_write_page_scan_activity {
 	__le16   interval;
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index e34cd71..4c61161 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -188,6 +188,11 @@ struct hci_dev {
 	unsigned int	sco_pkts;
 	unsigned int	le_pkts;
 
+	__u16		block_len;
+	__u16		block_mtu;
+	__u16		num_blocks;
+	__u16		block_cnt;
+
 	unsigned long	acl_last_tx;
 	unsigned long	sco_last_tx;
 	unsigned long	le_last_tx;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index afd4731..273e1cb 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -776,6 +776,28 @@ static void hci_cc_read_bd_addr(struct hci_dev *hdev, struct sk_buff *skb)
 	hci_req_complete(hdev, HCI_OP_READ_BD_ADDR, rp->status);
 }
 
+static void hci_cc_read_data_block_size(struct hci_dev *hdev,
+							struct sk_buff *skb)
+{
+	struct hci_rp_read_data_block_size *rp = (void *) skb->data;
+
+	BT_DBG("%s status 0x%x", hdev->name, rp->status);
+
+	if (rp->status)
+		return;
+
+	hdev->block_mtu = __le16_to_cpu(rp->max_acl_len);
+	hdev->block_len = __le16_to_cpu(rp->block_len);
+	hdev->num_blocks = __le16_to_cpu(rp->num_blocks);
+
+	hdev->block_cnt = hdev->num_blocks;
+
+	BT_DBG("%s blk mtu %d cnt %d len %d", hdev->name, hdev->block_mtu,
+					hdev->block_cnt, hdev->block_len);
+
+	hci_req_complete(hdev, HCI_OP_READ_DATA_BLOCK_SIZE, rp->status);
+}
+
 static void hci_cc_write_ca_timeout(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	__u8 status = *((__u8 *) skb->data);
@@ -2031,6 +2053,10 @@ static inline void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *sk
 		hci_cc_read_bd_addr(hdev, skb);
 		break;
 
+	case HCI_OP_READ_DATA_BLOCK_SIZE:
+		hci_cc_read_data_block_size(hdev, skb);
+		break;
+
 	case HCI_OP_WRITE_CA_TIMEOUT:
 		hci_cc_write_ca_timeout(hdev, skb);
 		break;
-- 
1.7.4.1

[RFCv2 2/6] Bluetooth: Simplify num_comp_pkts_evt function

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Simplify function and remove fourth level of indentation.
---
 net/bluetooth/hci_event.c |   47 +++++++++++++++++++++++++++-----------------
 1 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 273e1cb..493e08b 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2288,28 +2288,39 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
 		count  = get_unaligned_le16(ptr++);
 
 		conn = hci_conn_hash_lookup_handle(hdev, handle);
-		if (conn) {
-			conn->sent -= count;
-
-			if (conn->type == ACL_LINK) {
+		if (!conn)
+			continue;
+
+		conn->sent -= count;
+
+		switch (conn->type) {
+		case ACL_LINK:
+			hdev->acl_cnt += count;
+			if (hdev->acl_cnt > hdev->acl_pkts)
+				hdev->acl_cnt = hdev->acl_pkts;
+			break;
+
+		case LE_LINK:
+			if (hdev->le_pkts) {
+				hdev->le_cnt += count;
+				if (hdev->le_cnt > hdev->le_pkts)
+					hdev->le_cnt = hdev->le_pkts;
+			} else {
 				hdev->acl_cnt += count;
 				if (hdev->acl_cnt > hdev->acl_pkts)
 					hdev->acl_cnt = hdev->acl_pkts;
-			} else if (conn->type == LE_LINK) {
-				if (hdev->le_pkts) {
-					hdev->le_cnt += count;
-					if (hdev->le_cnt > hdev->le_pkts)
-						hdev->le_cnt = hdev->le_pkts;
-				} else {
-					hdev->acl_cnt += count;
-					if (hdev->acl_cnt > hdev->acl_pkts)
-						hdev->acl_cnt = hdev->acl_pkts;
-				}
-			} else {
-				hdev->sco_cnt += count;
-				if (hdev->sco_cnt > hdev->sco_pkts)
-					hdev->sco_cnt = hdev->sco_pkts;
 			}
+			break;
+
+		case SCO_LINK:
+			hdev->sco_cnt += count;
+			if (hdev->sco_cnt > hdev->sco_pkts)
+				hdev->sco_cnt = hdev->sco_pkts;
+			break;
+
+		default:
+			BT_ERR("Unknown type %d conn %p", conn->type, conn);
+			break;
 		}
 	}
 
-- 
1.7.4.1

[RFCv2 3/6] Bluetooth: Check for flow control mode

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

---
 net/bluetooth/hci_event.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 493e08b..a06cc60 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2273,6 +2273,11 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
 
 	BT_DBG("%s num_hndl %d", hdev->name, ev->num_hndl);
 
+	if (hdev->flow_ctl_mode != HCI_FLOW_CTL_MODE_PACKET_BASED) {
+		BT_ERR("Wrong event for mode %d", hdev->flow_ctl_mode);
+		return;
+	}
+
 	if (skb->len < ev->num_hndl * 4) {
 		BT_DBG("%s bad parameters", hdev->name);
 		return;
-- 
1.7.4.1

[RFCv2 4/6] Bluetooth: Clean up magic pointers

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

---
 include/net/bluetooth/hci.h |    7 ++++++-
 net/bluetooth/hci_event.c   |    8 ++++----
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 9490c2c..b9c0087 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -981,9 +981,14 @@ struct hci_ev_role_change {
 } __packed;
 
 #define HCI_EV_NUM_COMP_PKTS		0x13
+struct hci_comp_pkts_info {
+	__le16   handle;
+	__le16   count;
+} __packed;
+
 struct hci_ev_num_comp_pkts {
 	__u8     num_hndl;
-	/* variable length part */
+	struct hci_comp_pkts_info handles[0];
 } __packed;
 
 #define HCI_EV_MODE_CHANGE		0x14
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index a06cc60..cfccf7e 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2266,7 +2266,6 @@ static inline void hci_role_change_evt(struct hci_dev *hdev, struct sk_buff *skb
 static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct hci_ev_num_comp_pkts *ev = (void *) skb->data;
-	__le16 *ptr;
 	int i;
 
 	skb_pull(skb, sizeof(*ev));
@@ -2285,12 +2284,13 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
 
 	tasklet_disable(&hdev->tx_task);
 
-	for (i = 0, ptr = (__le16 *) skb->data; i < ev->num_hndl; i++) {
+	for (i = 0; i < ev->num_hndl; i++) {
 		struct hci_conn *conn;
+		struct hci_comp_pkts_info *info = &ev->handles[i];
 		__u16  handle, count;
 
-		handle = get_unaligned_le16(ptr++);
-		count  = get_unaligned_le16(ptr++);
+		handle = __le16_to_cpu(info->handle);
+		count  = __le16_to_cpu(info->count);
 
 		conn = hci_conn_hash_lookup_handle(hdev, handle);
 		if (!conn)
-- 
1.7.4.1

[RFCv2 5/6] Bluetooth: Correct packet len calculation

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Remove unneeded skb_pull and correct packet length calculation
removing magic number.
---
 net/bluetooth/hci_event.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index cfccf7e..21c1bf0 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2268,8 +2268,6 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
 	struct hci_ev_num_comp_pkts *ev = (void *) skb->data;
 	int i;
 
-	skb_pull(skb, sizeof(*ev));
-
 	BT_DBG("%s num_hndl %d", hdev->name, ev->num_hndl);
 
 	if (hdev->flow_ctl_mode != HCI_FLOW_CTL_MODE_PACKET_BASED) {
@@ -2277,7 +2275,8 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
 		return;
 	}
 
-	if (skb->len < ev->num_hndl * 4) {
+	if (skb->len < ev->num_hndl * sizeof(struct hci_comp_pkts_info) +
+			sizeof(*ev)) {
 		BT_DBG("%s bad parameters", hdev->name);
 		return;
 	}
-- 
1.7.4.1

[RFCv2 6/6] Bluetooth: Process num completed data blocks event

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Adds support for Number Of Completed Data Blocks Event. Highly
modified version of Code Aurora code.
---
 include/net/bluetooth/hci.h |   13 +++++++++
 net/bluetooth/hci_event.c   |   58 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index b9c0087..6846f3e 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1154,6 +1154,19 @@ struct hci_ev_le_meta {
 	__u8     subevent;
 } __packed;
 
+#define HCI_EV_NUM_COMP_BLOCKS		0x48
+struct hci_comp_blocks_info {
+	__le16   handle;
+	__le16   pkts;
+	__le16   blocks;
+} __packed;
+
+struct hci_ev_num_comp_blocks {
+	__le16   num_blocks;
+	__u8     num_hndl;
+	struct hci_comp_blocks_info handles[0];
+} __packed;
+
 /* Low energy meta events */
 #define HCI_EV_LE_CONN_COMPLETE		0x01
 struct hci_ev_le_conn_complete {
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 21c1bf0..6a43bdb 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2333,6 +2333,60 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
 	tasklet_enable(&hdev->tx_task);
 }
 
+static inline void hci_num_comp_blocks_evt(struct hci_dev *hdev,
+							struct sk_buff *skb)
+{
+	struct hci_ev_num_comp_blocks *ev = (void *) skb->data;
+	int i;
+
+	BT_DBG("%s num_blocks %d num_hndl %d", hdev->name, ev->num_blocks,
+								ev->num_hndl);
+
+	if (hdev->flow_ctl_mode != HCI_FLOW_CTL_MODE_BLOCK_BASED) {
+		BT_ERR("Wrong event for mode %d", hdev->flow_ctl_mode);
+		return;
+	}
+
+	if (skb->len < ev->num_hndl * sizeof(struct hci_comp_blocks_info) +
+								sizeof(*ev)) {
+		BT_DBG("%s bad parameters", hdev->name);
+		return;
+	}
+
+	tasklet_disable(&hdev->tx_task);
+
+	for (i = 0; i < ev->num_hndl; i++) {
+		struct hci_conn *conn;
+		struct hci_comp_blocks_info *info = &ev->handles[i];
+		__u16  handle, block_count;
+
+		handle = __le16_to_cpu(info->handle);
+		block_count = __le16_to_cpu(info->blocks);
+
+		conn = hci_conn_hash_lookup_handle(hdev, handle);
+		if (!conn)
+			continue;
+
+		conn->sent -= block_count;
+
+		switch (conn->type) {
+		case ACL_LINK:
+			hdev->block_cnt += block_count;
+			if (hdev->block_cnt > hdev->num_blocks)
+				hdev->block_cnt = hdev->num_blocks;
+			break;
+
+		default:
+			BT_ERR("Unknown type %d conn %p", conn->type, conn);
+			break;
+		}
+	}
+
+	tasklet_schedule(&hdev->tx_task);
+
+	tasklet_enable(&hdev->tx_task);
+}
+
 static inline void hci_mode_change_evt(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct hci_ev_mode_change *ev = (void *) skb->data;
@@ -3272,6 +3326,10 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
 		hci_remote_oob_data_request_evt(hdev, skb);
 		break;
 
+	case HCI_EV_NUM_COMP_BLOCKS:
+		hci_num_comp_blocks_evt(hdev, skb);
+		break;
+
 	default:
 		BT_DBG("%s event 0x%x", hdev->name, event);
 		break;
-- 
1.7.4.1

Re: [RFCv2 2/6] Bluetooth: Simplify num_comp_pkts_evt function

[Marcel Holtmann]

Hi Andrei,

> Simplify function and remove fourth level of indentation.
> ---
>  net/bluetooth/hci_event.c |   47 +++++++++++++++++++++++++++-----------------
>  1 files changed, 29 insertions(+), 18 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [RFCv2 3/6] Bluetooth: Check for flow control mode

[Marcel Holtmann]

Hi Andrei,

> ---
>  net/bluetooth/hci_event.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [RFCv2 4/6] Bluetooth: Clean up magic pointers

[Marcel Holtmann]

Hi Andrei,

>  include/net/bluetooth/hci.h |    7 ++++++-
>  net/bluetooth/hci_event.c   |    8 ++++----
>  2 files changed, 10 insertions(+), 5 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [RFCv2 5/6] Bluetooth: Correct packet len calculation

[Marcel Holtmann]

Hi Andrei,

> Remove unneeded skb_pull and correct packet length calculation
> removing magic number.
> ---
>  net/bluetooth/hci_event.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index cfccf7e..21c1bf0 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2268,8 +2268,6 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
>  	struct hci_ev_num_comp_pkts *ev = (void *) skb->data;
>  	int i;
>  
> -	skb_pull(skb, sizeof(*ev));
> -
>  	BT_DBG("%s num_hndl %d", hdev->name, ev->num_hndl);
>  
>  	if (hdev->flow_ctl_mode != HCI_FLOW_CTL_MODE_PACKET_BASED) {
> @@ -2277,7 +2275,8 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
>  		return;
>  	}
>  
> -	if (skb->len < ev->num_hndl * 4) {
> +	if (skb->len < ev->num_hndl * sizeof(struct hci_comp_pkts_info) +
> +			sizeof(*ev)) {
>  		BT_DBG("%s bad parameters", hdev->name);
>  		return;
>  	}

actually this check is not enough. You need to make this two-fold.

	if (skb->len < sizeof(*ev)) {
		...
		return;
	}

This needs to be checked first. Otherwise you are accessing ev->num_hndl
before even knowing that it is valid. A malicious device could otherwise
sneak in some weird behavior.

Regards

Marcel

Re: [RFCv2 6/6] Bluetooth: Process num completed data blocks event

[Marcel Holtmann]

Hi Andrei,

> Adds support for Number Of Completed Data Blocks Event. Highly
> modified version of Code Aurora code.
> ---
>  include/net/bluetooth/hci.h |   13 +++++++++
>  net/bluetooth/hci_event.c   |   58 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 71 insertions(+), 0 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index b9c0087..6846f3e 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -1154,6 +1154,19 @@ struct hci_ev_le_meta {
>  	__u8     subevent;
>  } __packed;
>  
> +#define HCI_EV_NUM_COMP_BLOCKS		0x48
> +struct hci_comp_blocks_info {
> +	__le16   handle;
> +	__le16   pkts;
> +	__le16   blocks;
> +} __packed;
> +
> +struct hci_ev_num_comp_blocks {
> +	__le16   num_blocks;
> +	__u8     num_hndl;
> +	struct hci_comp_blocks_info handles[0];
> +} __packed;
> +
>  /* Low energy meta events */
>  #define HCI_EV_LE_CONN_COMPLETE		0x01
>  struct hci_ev_le_conn_complete {
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 21c1bf0..6a43bdb 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2333,6 +2333,60 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
>  	tasklet_enable(&hdev->tx_task);
>  }
>  
> +static inline void hci_num_comp_blocks_evt(struct hci_dev *hdev,
> +							struct sk_buff *skb)
> +{
> +	struct hci_ev_num_comp_blocks *ev = (void *) skb->data;
> +	int i;
> +
> +	BT_DBG("%s num_blocks %d num_hndl %d", hdev->name, ev->num_blocks,
> +								ev->num_hndl);
> +
> +	if (hdev->flow_ctl_mode != HCI_FLOW_CTL_MODE_BLOCK_BASED) {
> +		BT_ERR("Wrong event for mode %d", hdev->flow_ctl_mode);
> +		return;
> +	}
> +
> +	if (skb->len < ev->num_hndl * sizeof(struct hci_comp_blocks_info) +
> +								sizeof(*ev)) {
> +		BT_DBG("%s bad parameters", hdev->name);
> +		return;
> +	}

same comment applies here as from my other email.

> +	tasklet_disable(&hdev->tx_task);
> +
> +	for (i = 0; i < ev->num_hndl; i++) {
> +		struct hci_conn *conn;
> +		struct hci_comp_blocks_info *info = &ev->handles[i];

Put hci_conn declaration after hci_comp_blocks_info. I prefer to have
variables that get a value assigned before others.

> +		__u16  handle, block_count;
> +
> +		handle = __le16_to_cpu(info->handle);
> +		block_count = __le16_to_cpu(info->blocks);
> +
> +		conn = hci_conn_hash_lookup_handle(hdev, handle);
> +		if (!conn)
> +			continue;
> +
> +		conn->sent -= block_count;
> +
> +		switch (conn->type) {
> +		case ACL_LINK:
> +			hdev->block_cnt += block_count;
> +			if (hdev->block_cnt > hdev->num_blocks)
> +				hdev->block_cnt = hdev->num_blocks;
> +			break;
> +
> +		default:
> +			BT_ERR("Unknown type %d conn %p", conn->type, conn);
> +			break;
> +		}
> +	}
> +
> +	tasklet_schedule(&hdev->tx_task);
> +
> +	tasklet_enable(&hdev->tx_task);
> +}
> +
>  static inline void hci_mode_change_evt(struct hci_dev *hdev, struct sk_buff *skb)
>  {
>  	struct hci_ev_mode_change *ev = (void *) skb->data;
> @@ -3272,6 +3326,10 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
>  		hci_remote_oob_data_request_evt(hdev, skb);
>  		break;
>  
> +	case HCI_EV_NUM_COMP_BLOCKS:
> +		hci_num_comp_blocks_evt(hdev, skb);
> +		break;
> +
>  	default:
>  		BT_DBG("%s event 0x%x", hdev->name, event);
>  		break;

Rest looks fine to me.

Regards

Marcel

Re: [RFCv2 5/6] Bluetooth: Correct packet len calculation

[Emeltchenko Andrei]

Hi Marcel,

On Thu, Dec 08, 2011 at 10:18:03AM +0200, Marcel Holtmann wrote:
> Hi Andrei,
> 
> > Remove unneeded skb_pull and correct packet length calculation
> > removing magic number.
> > ---
> >  net/bluetooth/hci_event.c |    5 ++---
> >  1 files changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index cfccf7e..21c1bf0 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -2268,8 +2268,6 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
> >  	struct hci_ev_num_comp_pkts *ev = (void *) skb->data;
> >  	int i;
> >  
> > -	skb_pull(skb, sizeof(*ev));
> > -
> >  	BT_DBG("%s num_hndl %d", hdev->name, ev->num_hndl);
> >  
> >  	if (hdev->flow_ctl_mode != HCI_FLOW_CTL_MODE_PACKET_BASED) {
> > @@ -2277,7 +2275,8 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
> >  		return;
> >  	}
> >  
> > -	if (skb->len < ev->num_hndl * 4) {
> > +	if (skb->len < ev->num_hndl * sizeof(struct hci_comp_pkts_info) +
> > +			sizeof(*ev)) {
> >  		BT_DBG("%s bad parameters", hdev->name);
> >  		return;
> >  	}
> 
> actually this check is not enough. You need to make this two-fold.
> 
> 	if (skb->len < sizeof(*ev)) {

maybe:

if (skb->len < sizeof(*ev) || (...)) 
	return;

> This needs to be checked first. Otherwise you are accessing ev->num_hndl
> before even knowing that it is valid. A malicious device could otherwise
> sneak in some weird behavior.

Agree.
Best regards 
Andrei Emeltchenko 

Re: [RFCv2 5/6] Bluetooth: Correct packet len calculation

[Marcel Holtmann]

Hi Andrei,

> > > Remove unneeded skb_pull and correct packet length calculation
> > > removing magic number.
> > > ---
> > >  net/bluetooth/hci_event.c |    5 ++---
> > >  1 files changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > > index cfccf7e..21c1bf0 100644
> > > --- a/net/bluetooth/hci_event.c
> > > +++ b/net/bluetooth/hci_event.c
> > > @@ -2268,8 +2268,6 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
> > >  	struct hci_ev_num_comp_pkts *ev = (void *) skb->data;
> > >  	int i;
> > >  
> > > -	skb_pull(skb, sizeof(*ev));
> > > -
> > >  	BT_DBG("%s num_hndl %d", hdev->name, ev->num_hndl);
> > >  
> > >  	if (hdev->flow_ctl_mode != HCI_FLOW_CTL_MODE_PACKET_BASED) {
> > > @@ -2277,7 +2275,8 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
> > >  		return;
> > >  	}
> > >  
> > > -	if (skb->len < ev->num_hndl * 4) {
> > > +	if (skb->len < ev->num_hndl * sizeof(struct hci_comp_pkts_info) +
> > > +			sizeof(*ev)) {
> > >  		BT_DBG("%s bad parameters", hdev->name);
> > >  		return;
> > >  	}
> > 
> > actually this check is not enough. You need to make this two-fold.
> > 
> > 	if (skb->len < sizeof(*ev)) {
> 
> maybe:
> 
> if (skb->len < sizeof(*ev) || (...)) 
> 	return;

that will work as well.

Regards

Marcel

Re: [RFCv2 1/6] Bluetooth: Add HCI Read Data Block Size function

[Gustavo Padovan]

* Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2011-12-07 15:56:51 +0200]:

> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> 
> Implement block size read function. Use different variables for
> packet-based and block-based flow control.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> Acked-by: Marcel Holtmann <marcel@holtmann.org>
> ---
>  include/net/bluetooth/hci.h      |    8 ++++++++
>  include/net/bluetooth/hci_core.h |    5 +++++
>  net/bluetooth/hci_event.c        |   26 ++++++++++++++++++++++++++
>  3 files changed, 39 insertions(+), 0 deletions(-)

Applied, thanks.

	Gustavo

Re: [RFCv2 2/6] Bluetooth: Simplify num_comp_pkts_evt function

[Gustavo Padovan]

* Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2011-12-07 15:56:52 +0200]:

> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> 
> Simplify function and remove fourth level of indentation.
> ---
>  net/bluetooth/hci_event.c |   47 +++++++++++++++++++++++++++-----------------
>  1 files changed, 29 insertions(+), 18 deletions(-)

Applied, thanks. And you forgot the Signed-off-by line here, I added it for
you.

	Gustavo

Re: [RFCv2 3/6] Bluetooth: Check for flow control mode

[Gustavo Padovan]

* Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2011-12-07 15:56:53 +0200]:

> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> 
> ---
>  net/bluetooth/hci_event.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)

So there is more patches without a Signed-off-by line, please add them and
resend.

	Gustavo