[PATCH 3/3] Bluetooth: Make hci_send_to_sock usable for management control sockets

[PATCH 3/3] Bluetooth: Make hci_send_to_sock usable for management control sockets

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@nokia.com>

In order to send data to management control sockets the function should:

  - skip checks intended for raw HCI data and stack internal events
  - make sure RAW HCI data or stack internal events don't go to
    management control sockets

In order to accomplish this the patch adds a new member to the bluetooth
skb private data to flag skb's that are destined for management control
sockets.

Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
---
 include/net/bluetooth/bluetooth.h |    1 +
 net/bluetooth/hci_sock.c          |   10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index d81ea79..0c5e725 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -144,6 +144,7 @@ struct bt_skb_cb {
 	__u8 tx_seq;
 	__u8 retries;
 	__u8 sar;
+	unsigned short channel;
 };
 #define bt_cb(skb) ((struct bt_skb_cb *)((skb)->cb))
 
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 730a4e8..ab449b2 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -104,6 +104,12 @@ void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb)
 		if (skb->sk == sk)
 			continue;
 
+		if (bt_cb(skb)->channel != hci_pi(sk)->channel)
+			continue;
+
+		if (bt_cb(skb)->channel == HCI_CHANNEL_CONTROL)
+			goto clone;
+
 		/* Apply filter */
 		flt = &hci_pi(sk)->filter;
 
@@ -127,11 +133,13 @@ void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb)
 				continue;
 		}
 
+clone:
 		if (!(nskb = skb_clone(skb, GFP_ATOMIC)))
 			continue;
 
 		/* Put type byte before the data */
-		memcpy(skb_push(nskb, 1), &bt_cb(nskb)->pkt_type, 1);
+		if (bt_cb(skb)->channel == HCI_CHANNEL_RAW)
+			memcpy(skb_push(nskb, 1), &bt_cb(nskb)->pkt_type, 1);
 
 		if (sock_queue_rcv_skb(sk, nskb))
 			kfree_skb(nskb);
-- 
1.7.2.3

Re: [PATCH 3/3] Bluetooth: Make hci_send_to_sock usable for management control sockets

[Marcel Holtmann]

Hi Johan,

> In order to send data to management control sockets the function should:
> 
>   - skip checks intended for raw HCI data and stack internal events
>   - make sure RAW HCI data or stack internal events don't go to
>     management control sockets
> 
> In order to accomplish this the patch adds a new member to the bluetooth
> skb private data to flag skb's that are destined for management control
> sockets.

looks fine to me.

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

Regards

Marcel

Initial Management interface patches (rebased)

[johan.hedberg]

Hi,

I'm resending these since the original set no longer applies cleanly to
bluetooth-next-2.6.git (due to some coding style cleanup patches that
were pushed there recently). I've kept the already received acks since
essentally there aren't other changes besides those meant to make these
apply cleanly.

Johan

[PATCH 1/3] Bluetooth: Add Bluetooth Management interface definitions

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@nokia.com>

Add initial definitions for the new Bluetooth Management interface to
the bluetooth headers.

Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
 include/net/bluetooth/hci.h      |    4 +++
 include/net/bluetooth/hci_core.h |    1 +
 include/net/bluetooth/mgmt.h     |   46 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+), 0 deletions(-)
 create mode 100644 include/net/bluetooth/mgmt.h

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index f3c5ed6..29a7a8c 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -934,9 +934,13 @@ static inline struct hci_sco_hdr *hci_sco_hdr(const struct sk_buff *skb)
 struct sockaddr_hci {
 	sa_family_t    hci_family;
 	unsigned short hci_dev;
+	unsigned short hci_channel;
 };
 #define HCI_DEV_NONE	0xffff
 
+#define HCI_CHANNEL_RAW		0
+#define HCI_CHANNEL_CONTROL	1
+
 struct hci_filter {
 	unsigned long type_mask;
 	unsigned long event_mask[2];
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 9c08625..3e34359 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -668,6 +668,7 @@ struct hci_pinfo {
 	struct hci_dev    *hdev;
 	struct hci_filter filter;
 	__u32             cmsg_mask;
+	unsigned short   channel;
 };
 
 /* HCI security filter */
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
new file mode 100644
index 0000000..95974da
--- /dev/null
+++ b/include/net/bluetooth/mgmt.h
@@ -0,0 +1,46 @@
+/*
+   BlueZ - Bluetooth protocol stack for Linux
+
+   Copyright (C) 2010  Nokia Corporation
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License version 2 as
+   published by the Free Software Foundation;
+
+   THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+   OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+   FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT OF THIRD PARTY RIGHTS.
+   IN NO EVENT SHALL THE COPYRIGHT HOLDER(S) AND AUTHOR(S) BE LIABLE FOR ANY
+   CLAIM, OR ANY SPECIAL INDIRECT OR CONSEQUENTIAL DAMAGES, OR ANY DAMAGES
+   WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+   ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+   OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+
+   ALL LIABILITY, INCLUDING LIABILITY FOR INFRINGEMENT OF ANY PATENTS,
+   COPYRIGHTS, TRADEMARKS OR OTHER RIGHTS, RELATING TO USE OF THIS
+   SOFTWARE IS DISCLAIMED.
+*/
+
+struct mgmt_hdr {
+	__le16 opcode;
+	__le16 len;
+} __packed;
+#define MGMT_HDR_SIZE			4
+
+#define MGMT_EV_CMD_COMPLETE		0x0001
+struct mgmt_ev_cmd_complete {
+	__le16 opcode;
+	__u8 data[0];
+} __packed;
+
+#define MGMT_EV_CMD_STATUS		0x0002
+struct mgmt_ev_cmd_status {
+	__u8 status;
+	__le16 opcode;
+} __packed;
+
+#define MGMT_EV_CONTROLLER_ERROR	0x0003
+struct mgmt_ev_controller_error {
+	__le16 index;
+	__u8 error_code;
+} __packed;
-- 
1.7.2.3

[PATCH 2/3] Bluetooth: Add initial Bluetooth Management interface callbacks

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@nokia.com>

Add initial code for handling Bluetooth Management interface messages.

Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Acked-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
---
 include/net/bluetooth/hci_core.h |    3 +
 net/bluetooth/Makefile           |    2 +-
 net/bluetooth/hci_sock.c         |   39 +++++++++++++--
 net/bluetooth/mgmt.c             |   99 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 136 insertions(+), 7 deletions(-)
 create mode 100644 net/bluetooth/mgmt.c

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 3e34359..1992fac 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -660,6 +660,9 @@ void hci_si_event(struct hci_dev *hdev, int type, int dlen, void *data);
 /* ----- HCI Sockets ----- */
 void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb);
 
+/* Management interface */
+int mgmt_control(struct sock *sk, struct msghdr *msg, size_t len);
+
 /* HCI info for socket */
 #define hci_pi(sk) ((struct hci_pinfo *) sk)
 
diff --git a/net/bluetooth/Makefile b/net/bluetooth/Makefile
index d1e433f..8b411d9 100644
--- a/net/bluetooth/Makefile
+++ b/net/bluetooth/Makefile
@@ -10,4 +10,4 @@ obj-$(CONFIG_BT_BNEP)	+= bnep/
 obj-$(CONFIG_BT_CMTP)	+= cmtp/
 obj-$(CONFIG_BT_HIDP)	+= hidp/
 
-bluetooth-objs := af_bluetooth.o hci_core.o hci_conn.o hci_event.o hci_sock.o hci_sysfs.o lib.o
+bluetooth-objs := af_bluetooth.o hci_core.o hci_conn.o hci_event.o mgmt.o hci_sock.o hci_sysfs.o lib.o
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index b3753ba..207be7a 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -49,6 +49,8 @@
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
 
+static int enable_mgmt;
+
 /* ----- HCI socket interface ----- */
 
 static inline int hci_test_bit(int nr, void *addr)
@@ -353,25 +355,35 @@ static int hci_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long a
 
 static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
 {
-	struct sockaddr_hci *haddr = (struct sockaddr_hci *) addr;
+	struct sockaddr_hci haddr;
 	struct sock *sk = sock->sk;
 	struct hci_dev *hdev = NULL;
-	int err = 0;
+	int len, err = 0;
 
 	BT_DBG("sock %p sk %p", sock, sk);
 
-	if (!haddr || haddr->hci_family != AF_BLUETOOTH)
+	if (!addr)
+		return -EINVAL;
+
+	memset(&haddr, 0, sizeof(haddr));
+	len = min_t(unsigned int, sizeof(haddr), addr_len);
+	memcpy(&haddr, addr, len);
+
+	if (haddr.hci_family != AF_BLUETOOTH)
+		return -EINVAL;
+
+	if (haddr.hci_channel != HCI_CHANNEL_RAW && !enable_mgmt)
 		return -EINVAL;
 
 	lock_sock(sk);
 
-	if (hci_pi(sk)->hdev) {
+	if (sk->sk_state == BT_BOUND || hci_pi(sk)->hdev) {
 		err = -EALREADY;
 		goto done;
 	}
 
-	if (haddr->hci_dev != HCI_DEV_NONE) {
-		hdev = hci_dev_get(haddr->hci_dev);
+	if (haddr.hci_dev != HCI_DEV_NONE) {
+		hdev = hci_dev_get(haddr.hci_dev);
 		if (!hdev) {
 			err = -ENODEV;
 			goto done;
@@ -380,6 +392,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, int addr_le
 		atomic_inc(&hdev->promisc);
 	}
 
+	hci_pi(sk)->channel = haddr.hci_channel;
 	hci_pi(sk)->hdev = hdev;
 	sk->sk_state = BT_BOUND;
 
@@ -502,6 +515,17 @@ static int hci_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
 
 	lock_sock(sk);
 
+	switch (hci_pi(sk)->channel) {
+	case HCI_CHANNEL_RAW:
+		break;
+	case HCI_CHANNEL_CONTROL:
+		err = mgmt_control(sk, msg, len);
+		goto done;
+	default:
+		err = -EINVAL;
+		goto done;
+	}
+
 	hdev = hci_pi(sk)->hdev;
 	if (!hdev) {
 		err = -EBADFD;
@@ -831,3 +855,6 @@ void __exit hci_sock_cleanup(void)
 
 	proto_unregister(&hci_sk_proto);
 }
+
+module_param(enable_mgmt, bool, 0644);
+MODULE_PARM_DESC(enable_mgmt, "Enable Management interface");
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
new file mode 100644
index 0000000..78255f1
--- /dev/null
+++ b/net/bluetooth/mgmt.c
@@ -0,0 +1,99 @@
+/*
+   BlueZ - Bluetooth protocol stack for Linux
+   Copyright (C) 2010  Nokia Corporation
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License version 2 as
+   published by the Free Software Foundation;
+
+   THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+   OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+   FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT OF THIRD PARTY RIGHTS.
+   IN NO EVENT SHALL THE COPYRIGHT HOLDER(S) AND AUTHOR(S) BE LIABLE FOR ANY
+   CLAIM, OR ANY SPECIAL INDIRECT OR CONSEQUENTIAL DAMAGES, OR ANY DAMAGES
+   WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+   ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+   OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+
+   ALL LIABILITY, INCLUDING LIABILITY FOR INFRINGEMENT OF ANY PATENTS,
+   COPYRIGHTS, TRADEMARKS OR OTHER RIGHTS, RELATING TO USE OF THIS
+   SOFTWARE IS DISCLAIMED.
+*/
+
+/* Bluetooth HCI Management interface */
+
+#include <asm/uaccess.h>
+#include <asm/unaligned.h>
+
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
+#include <net/bluetooth/mgmt.h>
+
+static void cmd_status(struct sock *sk, u16 cmd, u8 status)
+{
+	struct sk_buff *skb;
+	struct mgmt_hdr *hdr;
+	struct mgmt_ev_cmd_status *ev;
+
+	BT_DBG("sock %p", sk);
+
+	skb = alloc_skb(sizeof(*hdr) + sizeof(*ev), GFP_ATOMIC);
+	if (!skb)
+		return;
+
+	hdr = (void *) skb_put(skb, sizeof(struct mgmt_hdr));
+
+	hdr->opcode = cpu_to_le16(MGMT_EV_CMD_STATUS);
+	hdr->len = cpu_to_le16(3);
+
+	ev = (void *) skb_put(skb, sizeof(*ev));
+	ev->status = status;
+	put_unaligned_le16(cmd, &ev->opcode);
+
+	if (sock_queue_rcv_skb(sk, skb) < 0)
+		kfree_skb(skb);
+}
+
+int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
+{
+	unsigned char *buf;
+	struct mgmt_hdr *hdr;
+	u16 opcode, len;
+	int err;
+
+	BT_DBG("got %zu bytes", msglen);
+
+	if (msglen < sizeof(*hdr))
+		return -EINVAL;
+
+	buf = kmalloc(msglen, GFP_ATOMIC);
+	if (!buf)
+		return -ENOMEM;
+
+	if (memcpy_fromiovec(buf, msg->msg_iov, msglen)) {
+		err = -EFAULT;
+		goto done;
+	}
+
+	hdr = (struct mgmt_hdr *) buf;
+	opcode = get_unaligned_le16(&hdr->opcode);
+	len = get_unaligned_le16(&hdr->len);
+
+	if (len != msglen - sizeof(struct mgmt_hdr)) {
+		err = -EINVAL;
+		goto done;
+	}
+
+	switch (opcode) {
+	default:
+		BT_DBG("Unknown op %u", opcode);
+		cmd_status(sk, opcode, 0x01);
+		break;
+	}
+
+	err = msglen;
+
+done:
+	kfree(buf);
+	return err;
+}
-- 
1.7.2.3

[PATCH 3/3] Bluetooth: Make hci_send_to_sock usable for management control sockets

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@nokia.com>

In order to send data to management control sockets the function should:

  - skip checks intended for raw HCI data and stack internal events
  - make sure RAW HCI data or stack internal events don't go to
    management control sockets

In order to accomplish this the patch adds a new member to the bluetooth
skb private data to flag skb's that are destined for management control
sockets.

Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
 include/net/bluetooth/bluetooth.h |    1 +
 net/bluetooth/hci_sock.c          |   10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index d81ea79..0c5e725 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -144,6 +144,7 @@ struct bt_skb_cb {
 	__u8 tx_seq;
 	__u8 retries;
 	__u8 sar;
+	unsigned short channel;
 };
 #define bt_cb(skb) ((struct bt_skb_cb *)((skb)->cb))
 
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 207be7a..f6c18ab 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -104,6 +104,12 @@ void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb)
 		if (skb->sk == sk)
 			continue;
 
+		if (bt_cb(skb)->channel != hci_pi(sk)->channel)
+			continue;
+
+		if (bt_cb(skb)->channel == HCI_CHANNEL_CONTROL)
+			goto clone;
+
 		/* Apply filter */
 		flt = &hci_pi(sk)->filter;
 
@@ -127,12 +133,14 @@ void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb)
 				continue;
 		}
 
+clone:
 		nskb = skb_clone(skb, GFP_ATOMIC);
 		if (!nskb)
 			continue;
 
 		/* Put type byte before the data */
-		memcpy(skb_push(nskb, 1), &bt_cb(nskb)->pkt_type, 1);
+		if (bt_cb(skb)->channel == HCI_CHANNEL_RAW)
+			memcpy(skb_push(nskb, 1), &bt_cb(nskb)->pkt_type, 1);
 
 		if (sock_queue_rcv_skb(sk, nskb))
 			kfree_skb(nskb);
-- 
1.7.2.3

Re: [PATCH 2/3] Bluetooth: Add initial Bluetooth Management interface callbacks

[Anderson Lizardo]

Hi Johan,

On Sun, Dec 5, 2010 at 2:19 PM,  <johan.hedberg@gmail.com> wrote:
> +static void cmd_status(struct sock *sk, u16 cmd, u8 status)
> +{

I see some inconsistence on how you calculate struct sizes on this
function. See below...

> +       struct sk_buff *skb;
> +       struct mgmt_hdr *hdr;
> +       struct mgmt_ev_cmd_status *ev;
> +
> +       BT_DBG("sock %p", sk);
> +
> +       skb = alloc_skb(sizeof(*hdr) + sizeof(*ev), GFP_ATOMIC);

Here you use sizeof(<var>)

> +       if (!skb)
> +               return;
> +
> +       hdr = (void *) skb_put(skb, sizeof(struct mgmt_hdr));

But here you use sizeof(<struct>). Could be sizeof(*hdr) ? Note there
is also the (unused) MGMT_HDR_SIZE.

> +
> +       hdr->opcode = cpu_to_le16(MGMT_EV_CMD_STATUS);
> +       hdr->len = cpu_to_le16(3);

and here a hard-coded size. Could be sizeof(struct mgmt_ev_cmd_status)
? or maybe create a #define for it?

> +
> +       ev = (void *) skb_put(skb, sizeof(*ev));
> +       ev->status = status;
> +       put_unaligned_le16(cmd, &ev->opcode);
> +
> +       if (sock_queue_rcv_skb(sk, skb) < 0)
> +               kfree_skb(skb);
> +}
> +
> +int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
> +{
> +       unsigned char *buf;
> +       struct mgmt_hdr *hdr;
> +       u16 opcode, len;
> +       int err;
> +
> +       BT_DBG("got %zu bytes", msglen);
> +
> +       if (msglen < sizeof(*hdr))
> +               return -EINVAL;
> +
> +       buf = kmalloc(msglen, GFP_ATOMIC);
> +       if (!buf)
> +               return -ENOMEM;
> +
> +       if (memcpy_fromiovec(buf, msg->msg_iov, msglen)) {
> +               err = -EFAULT;
> +               goto done;
> +       }
> +
> +       hdr = (struct mgmt_hdr *) buf;
> +       opcode = get_unaligned_le16(&hdr->opcode);
> +       len = get_unaligned_le16(&hdr->len);
> +
> +       if (len != msglen - sizeof(struct mgmt_hdr)) {

You could use sizeof(*hdr) here.

> +               err = -EINVAL;
> +               goto done;
> +       }
> +
> +       switch (opcode) {
> +       default:
> +               BT_DBG("Unknown op %u", opcode);
> +               cmd_status(sk, opcode, 0x01);
> +               break;
> +       }
> +
> +       err = msglen;

Would there be a chance of integer overflow here? The function returns
(signed) int, but msglen is (unsigned) size_t.

> +
> +done:
> +       kfree(buf);
> +       return err;
> +}

Regards,
-- 
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil

Re: [PATCH 2/3] Bluetooth: Add initial Bluetooth Management interface callbacks

[Johan Hedberg]

Hi Anderson,

On Mon, Dec 06, 2010, Anderson Lizardo wrote:
> On Sun, Dec 5, 2010 at 2:19 PM,  <johan.hedberg@gmail.com> wrote:
> > +static void cmd_status(struct sock *sk, u16 cmd, u8 status)
> > +{
> 
> I see some inconsistence on how you calculate struct sizes on this
> function. See below...
> 
> > +       struct sk_buff *skb;
> > +       struct mgmt_hdr *hdr;
> > +       struct mgmt_ev_cmd_status *ev;
> > +
> > +       BT_DBG("sock %p", sk);
> > +
> > +       skb = alloc_skb(sizeof(*hdr) + sizeof(*ev), GFP_ATOMIC);
> 
> Here you use sizeof(<var>)

Yep, in Chapter 14 of Documentation/CodingStyle this seems to be the
preferred form.

> > +       if (!skb)
> > +               return;
> > +
> > +       hdr = (void *) skb_put(skb, sizeof(struct mgmt_hdr));
> 
> But here you use sizeof(<struct>). Could be sizeof(*hdr)?

Yes, could be.

> > +
> > +       hdr->opcode = cpu_to_le16(MGMT_EV_CMD_STATUS);
> > +       hdr->len = cpu_to_le16(3);
> 
> and here a hard-coded size. Could be sizeof(struct mgmt_ev_cmd_status)?

Yes, could be.

> > +       if (len != msglen - sizeof(struct mgmt_hdr)) {
> 
> You could use sizeof(*hdr) here.

Indeed.

I suppose these style fixes should be as a separate patch since the
original one already got acks from the relevant people? (if not, someone
please enlighten me how the kernel patch process deals with comments
received after acks :)

> > +               err = -EINVAL;
> > +               goto done;
> > +       }
> > +
> > +       switch (opcode) {
> > +       default:
> > +               BT_DBG("Unknown op %u", opcode);
> > +               cmd_status(sk, opcode, 0x01);
> > +               break;
> > +       }
> > +
> > +       err = msglen;
> 
> Would there be a chance of integer overflow here? The function returns
> (signed) int, but msglen is (unsigned) size_t.

The core of the issue is in the way that the sendmsg callback for
sockets is defined. At least L2CAP and RFCOMM sockets do similar
assignments in their sendmsg callbacks so I've assumed it to be ok.

Johan

Re: [PATCH 2/3] Bluetooth: Add initial Bluetooth Management interface callbacks

[Gustavo F. Padovan]

Hi Johan,

* Johan Hedberg <johan.hedberg@gmail.com> [2010-12-06 16:21:16 +0200]:

> Hi Anderson,
> 
> On Mon, Dec 06, 2010, Anderson Lizardo wrote:
> > On Sun, Dec 5, 2010 at 2:19 PM,  <johan.hedberg@gmail.com> wrote:
> > > +static void cmd_status(struct sock *sk, u16 cmd, u8 status)
> > > +{
> > 
> > I see some inconsistence on how you calculate struct sizes on this
> > function. See below...
> > 
> > > +       struct sk_buff *skb;
> > > +       struct mgmt_hdr *hdr;
> > > +       struct mgmt_ev_cmd_status *ev;
> > > +
> > > +       BT_DBG("sock %p", sk);
> > > +
> > > +       skb = alloc_skb(sizeof(*hdr) + sizeof(*ev), GFP_ATOMIC);
> > 
> > Here you use sizeof(<var>)
> 
> Yep, in Chapter 14 of Documentation/CodingStyle this seems to be the
> preferred form.
> 
> > > +       if (!skb)
> > > +               return;
> > > +
> > > +       hdr = (void *) skb_put(skb, sizeof(struct mgmt_hdr));
> > 
> > But here you use sizeof(<struct>). Could be sizeof(*hdr)?
> 
> Yes, could be.
> 
> > > +
> > > +       hdr->opcode = cpu_to_le16(MGMT_EV_CMD_STATUS);
> > > +       hdr->len = cpu_to_le16(3);
> > 
> > and here a hard-coded size. Could be sizeof(struct mgmt_ev_cmd_status)?
> 
> Yes, could be.
> 
> > > +       if (len != msglen - sizeof(struct mgmt_hdr)) {
> > 
> > You could use sizeof(*hdr) here.
> 
> Indeed.
> 
> I suppose these style fixes should be as a separate patch since the
> original one already got acks from the relevant people? (if not, someone
> please enlighten me how the kernel patch process deals with comments
> received after acks :)

>From what I understand the Acked-by is more about the whole idea of the patch,
so to me it is fine to do such styles fixes and keep the ack in the patch. You
won't change essentials parts of the patch.

-- 
Gustavo F. Padovan
http://profusion.mobi

[PATCH 3/3] Bluetooth: Make hci_send_to_sock usable for management control sockets

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@nokia.com>

In order to send data to management control sockets the function should:

  - skip checks intended for raw HCI data and stack internal events
  - make sure RAW HCI data or stack internal events don't go to
    management control sockets

In order to accomplish this the patch adds a new member to the bluetooth
skb private data to flag skb's that are destined for management control
sockets.

Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
 include/net/bluetooth/bluetooth.h |    1 +
 net/bluetooth/hci_sock.c          |   10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index d81ea79..0c5e725 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -144,6 +144,7 @@ struct bt_skb_cb {
 	__u8 tx_seq;
 	__u8 retries;
 	__u8 sar;
+	unsigned short channel;
 };
 #define bt_cb(skb) ((struct bt_skb_cb *)((skb)->cb))
 
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 207be7a..f6c18ab 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -104,6 +104,12 @@ void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb)
 		if (skb->sk == sk)
 			continue;
 
+		if (bt_cb(skb)->channel != hci_pi(sk)->channel)
+			continue;
+
+		if (bt_cb(skb)->channel == HCI_CHANNEL_CONTROL)
+			goto clone;
+
 		/* Apply filter */
 		flt = &hci_pi(sk)->filter;
 
@@ -127,12 +133,14 @@ void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb)
 				continue;
 		}
 
+clone:
 		nskb = skb_clone(skb, GFP_ATOMIC);
 		if (!nskb)
 			continue;
 
 		/* Put type byte before the data */
-		memcpy(skb_push(nskb, 1), &bt_cb(nskb)->pkt_type, 1);
+		if (bt_cb(skb)->channel == HCI_CHANNEL_RAW)
+			memcpy(skb_push(nskb, 1), &bt_cb(nskb)->pkt_type, 1);
 
 		if (sock_queue_rcv_skb(sk, nskb))
 			kfree_skb(nskb);
-- 
1.7.2.3

Re: [PATCH 3/3] Bluetooth: Make hci_send_to_sock usable for management control sockets

[Gustavo F. Padovan]

Hi Johan,

* johan.hedberg@gmail.com <johan.hedberg@gmail.com> [2010-12-08 00:21:07 +0200]:

> From: Johan Hedberg <johan.hedberg@nokia.com>
> 
> In order to send data to management control sockets the function should:
> 
>   - skip checks intended for raw HCI data and stack internal events
>   - make sure RAW HCI data or stack internal events don't go to
>     management control sockets
> 
> In order to accomplish this the patch adds a new member to the bluetooth
> skb private data to flag skb's that are destined for management control
> sockets.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
> Acked-by: Marcel Holtmann <marcel@holtmann.org>

All 3 patches have been applied. Thanks.

-- 
Gustavo F. Padovan
http://profusion.mobi