[Bluez-devel] Re: some bluetooth fixes

[Bluez-devel] Re: some bluetooth fixes

[Marcel Holtmann]

Hi Andi,

> While reading bluetooth code I noticed some bugs and potential overflows.
> 
> Also I commented one ref-counting bug.
> 
> Patch against 2.6.2.

thanks for looking at it. Same applies to 2.4, right?

> diff -u linux-2.6.2-work32/net/bluetooth/cmtp/sock.c-o linux-2.6.2-work32/net/bluetooth/cmtp/sock.c
> --- linux-2.6.2-work32/net/bluetooth/cmtp/sock.c-o	2004-02-05 08:09:54.000000000 +0100
> +++ linux-2.6.2-work32/net/bluetooth/cmtp/sock.c	2004-02-05 14:57:57.000000000 +0100
> @@ -87,8 +87,10 @@
>  		if (!nsock)
>  			return err;
>  
> -		if (nsock->sk->sk_state != BT_CONNECTED)
> +		if (nsock->sk->sk_state != BT_CONNECTED) { 
> +			fput(nsock->file);
>  			return -EBADFD;
> +		}
>  
>  		err = cmtp_add_connection(&ca, nsock);
>  		if (!err) {

The same should apply to net/bluetooth/bnep/sock.c

> diff -u linux-2.6.2-work32/net/bluetooth/hci_sock.c-o linux-2.6.2-work32/net/bluetooth/hci_sock.c
> --- linux-2.6.2-work32/net/bluetooth/hci_sock.c-o	2004-02-05 08:09:54.000000000 +0100
> +++ linux-2.6.2-work32/net/bluetooth/hci_sock.c	2004-02-05 14:57:59.000000000 +0100
> @@ -392,6 +392,8 @@
>  
>  	skb->pkt_type = *((unsigned char *) skb->data);
>  	skb_pull(skb, 1);
> +	/* AK: looks broken. Who guarantees that hdev doesn't go away while
> +	   the skb is queued ? */
>  	skb->dev = (void *) hdev;
>  
>  	if (skb->pkt_type == HCI_COMMAND_PKT) {

Why should hdev go away?

> diff -u linux-2.6.2-work32/net/bluetooth/hci_conn.c-o linux-2.6.2-work32/net/bluetooth/hci_conn.c
> --- linux-2.6.2-work32/net/bluetooth/hci_conn.c-o	2004-02-05 08:09:54.000000000 +0100
> +++ linux-2.6.2-work32/net/bluetooth/hci_conn.c	2004-02-05 15:06:10.000000000 +0100
> @@ -353,21 +353,24 @@
>  	struct hci_conn_info *ci;
>  	struct hci_dev *hdev;
>  	struct list_head *p;
> -	int n = 0, size;
> +	int n = 0, size, err;
>  
>  	if (copy_from_user(&req, (void *) arg, sizeof(req)))
>  		return -EFAULT;
>  
> -	if (!(hdev = hci_dev_get(req.dev_id)))
> -		return -ENODEV;
> +	if (req.conn_num >= (2*PAGE_SIZE)/sizeof(struct hci_conn_info))
> +		return -EINVAL; 
>  
>  	size = req.conn_num * sizeof(struct hci_conn_info) + sizeof(req);
>  
> -	if (verify_area(VERIFY_WRITE, (void *)arg, size))
> -		return -EFAULT;
> -
>  	if (!(cl = (void *) kmalloc(size, GFP_KERNEL)))
>  		return -ENOMEM;
> +
> +	if (!(hdev = hci_dev_get(req.dev_id))) { 
> +		kfree(cl);
> +		return -ENODEV;
> +	}
> +
>  	ci = cl->conn_info;
>  
>  	hci_dev_lock_bh(hdev);

Why 2*PAGE_SIZE in this case? What is different?

Regards

Marcel




-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

[Bluez-devel] Re: some bluetooth fixes

[Marcel Holtmann]

Hi Andi,

> diff -u linux-2.6.2-work32/net/bluetooth/rfcomm/tty.c-o linux-2.6.2-work32/net/bluetooth/rfcomm/tty.c
> --- linux-2.6.2-work32/net/bluetooth/rfcomm/tty.c-o	2003-09-28 10:53:25.000000000 +0200
> +++ linux-2.6.2-work32/net/bluetooth/rfcomm/tty.c	2004-02-06 04:58:28.000000000 +0100
> @@ -549,8 +549,10 @@
>  
>  	BT_DBG("dev %p dst %s channel %d opened %d", dev, batostr(&dev->dst), dev->channel, dev->opened);
>  
> -	if (dev->opened++ != 0)
> +	if (dev->opened++ != 0) {
> +		rfcomm_dev_put(dev);
>  		return 0;
> +	}
>  
>  	dlc = dev->dlc;

this part is wrong, because it is not an error case. It is success and
the refcount must stay increased.

Regards

Marcel




-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: some bluetooth fixes

[David S. Miller]

Marcelo/Andi, I assume you guys will work things out and send
me a final patch?

Thanks.

[Bluez-devel] Re: some bluetooth fixes

[Marcel Holtmann]

Hi Dave,

> Marcelo/Andi, I assume you guys will work things out and send
> me a final patch?

I already splitted Andi's patch into smaller parts and I wait for the
answers to my questions. I also have some other fixes in the queue, so
you will get a number of patches ;)

Regards

Marcel




-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

[Bluez-devel] Re: some bluetooth fixes

[Marcel Holtmann]

Hi Andi,

> > > diff -u linux-2.6.2-work32/net/bluetooth/hci_sock.c-o linux-2.6.2-work32/net/bluetooth/hci_sock.c
> > > --- linux-2.6.2-work32/net/bluetooth/hci_sock.c-o	2004-02-05 08:09:54.000000000 +0100
> > > +++ linux-2.6.2-work32/net/bluetooth/hci_sock.c	2004-02-05 14:57:59.000000000 +0100
> > > @@ -392,6 +392,8 @@
> > >  
> > >  	skb->pkt_type = *((unsigned char *) skb->data);
> > >  	skb_pull(skb, 1);
> > > +	/* AK: looks broken. Who guarantees that hdev doesn't go away while
> > > +	   the skb is queued ? */
> > >  	skb->dev = (void *) hdev;
> > >  
> > >  	if (skb->pkt_type == HCI_COMMAND_PKT) {
> > 
> > Why should hdev go away?
> 
> Because the skbuff is queued, but there is no reference count to keep the device around.
> I wasn't 100% sure on that, so I just commented it. Feel free to remove if you think
> it's correct.

The queue itself is part of the hdev structure and the only call that
let hdev go away is hci_unregister_dev and this clears the queue. So I
don't see a problem here.

> > > diff -u linux-2.6.2-work32/net/bluetooth/hci_conn.c-o linux-2.6.2-work32/net/bluetooth/hci_conn.c
> > > --- linux-2.6.2-work32/net/bluetooth/hci_conn.c-o	2004-02-05 08:09:54.000000000 +0100
> > > +++ linux-2.6.2-work32/net/bluetooth/hci_conn.c	2004-02-05 15:06:10.000000000 +0100
> > > @@ -353,21 +353,24 @@
> > >  	struct hci_conn_info *ci;
> > >  	struct hci_dev *hdev;
> > >  	struct list_head *p;
> > > -	int n = 0, size;
> > > +	int n = 0, size, err;
> > >  
> > >  	if (copy_from_user(&req, (void *) arg, sizeof(req)))
> > >  		return -EFAULT;
> > >  
> > > -	if (!(hdev = hci_dev_get(req.dev_id)))
> > > -		return -ENODEV;
> > > +	if (req.conn_num >= (2*PAGE_SIZE)/sizeof(struct hci_conn_info))
> > > +		return -EINVAL; 
> > >  
> > >  	size = req.conn_num * sizeof(struct hci_conn_info) + sizeof(req);
> > >  
> > > -	if (verify_area(VERIFY_WRITE, (void *)arg, size))
> > > -		return -EFAULT;
> > > -
> > >  	if (!(cl = (void *) kmalloc(size, GFP_KERNEL)))
> > >  		return -ENOMEM;
> > > +
> > > +	if (!(hdev = hci_dev_get(req.dev_id))) { 
> > > +		kfree(cl);
> > > +		return -ENODEV;
> > > +	}
> > > +
> > >  	ci = cl->conn_info;
> > >  
> > >  	hci_dev_lock_bh(hdev);
> > 
> > Why 2*PAGE_SIZE in this case? What is different?
> 
> It's just an arbitary number. Mainly to stop overflow attacks on 
> user controlled values. e.g. user can pass UINT_MAX for conn_num.
> The kmalloc would overflow and succeed. But a later loop running
> through the values could do wrong things on the too small buffer.
> The code seems to not be vunerable to this, but only by luck.
> 
> Also in general it's good practice to stop user controlled kmalloc
> at a reasonable size.

I check this. Maybe we have more of them. What do you propose as max
size value for kmalloc? 2*PAGE_SIZE or 4*PAGE_SIZE?

Regards

Marcel




-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

[Bluez-devel] Re: some bluetooth fixes

[Marcel Holtmann]

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

Hi Andi,

> > I check this. Maybe we have more of them. What do you propose as max
> > size value for kmalloc? 2*PAGE_SIZE or 4*PAGE_SIZE?
> 
> What better fits the intended use case. I don't know how many objects are expected
> here. Smaller is better probably.

I now looked carefully through your patch and changed and added some
parts to better fit into. I also fixed another RFCOMM refcount bug.
Please review it, before I send it to Dave.

> If you want to handle more objects this way you should use seq_file instead.

The general plan is to move over to sysfs.

Regards

Marcel


[-- Attachment #2: patch --]
[-- Type: text/x-patch, Size: 4372 bytes --]

diff -Nru a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
--- a/net/bluetooth/hci_conn.c	Sat Feb  7 17:52:09 2004
+++ b/net/bluetooth/hci_conn.c	Sat Feb  7 17:52:09 2004
@@ -353,21 +353,24 @@
 	struct hci_conn_info *ci;
 	struct hci_dev *hdev;
 	struct list_head *p;
-	int n = 0, size;
+	int n = 0, size, err;
 
 	if (copy_from_user(&req, (void *) arg, sizeof(req)))
 		return -EFAULT;
 
-	if (!(hdev = hci_dev_get(req.dev_id)))
-		return -ENODEV;
-
-	size = req.conn_num * sizeof(struct hci_conn_info) + sizeof(req);
+	size = sizeof(req) + req.conn_num * sizeof(*ci);
 
-	if (verify_area(VERIFY_WRITE, (void *)arg, size))
-		return -EFAULT;
+	if (size > PAGE_SIZE * 2)
+		return -EINVAL;
 
 	if (!(cl = (void *) kmalloc(size, GFP_KERNEL)))
 		return -ENOMEM;
+
+	if (!(hdev = hci_dev_get(req.dev_id))) {
+		kfree(cl);
+		return -ENODEV;
+	}
+
 	ci = cl->conn_info;
 
 	hci_dev_lock_bh(hdev);
@@ -381,20 +384,21 @@
 		(ci + n)->out   = c->out;
 		(ci + n)->state = c->state;
 		(ci + n)->link_mode = c->link_mode;
-		n++;
+		if (++n >= req.conn_num)
+			break;
 	}
 	hci_dev_unlock_bh(hdev);
 
 	cl->dev_id = hdev->id;
 	cl->conn_num = n;
-	size = n * sizeof(struct hci_conn_info) + sizeof(req);
+	size = sizeof(req) + n * sizeof(*ci);
 
 	hci_dev_put(hdev);
 
-	copy_to_user((void *) arg, cl, size);
+	err = copy_to_user((void *) arg, cl, size);
 	kfree(cl);
 
-	return 0;
+	return err ? -EFAULT : 0;
 }
 
 int hci_get_conn_info(struct hci_dev *hdev, unsigned long arg)
@@ -407,9 +411,6 @@
 	if (copy_from_user(&req, (void *) arg, sizeof(req)))
 		return -EFAULT;
 
-	if (verify_area(VERIFY_WRITE, ptr, sizeof(ci)))
-		return -EFAULT;
-
 	hci_dev_lock_bh(hdev);
 	conn = hci_conn_hash_lookup_ba(hdev, req.type, &req.bdaddr);
 	if (conn) {
@@ -425,6 +426,5 @@
 	if (!conn)
 		return -ENOENT;
 
-	copy_to_user(ptr, &ci, sizeof(ci));
-	return 0;
+	return copy_to_user(ptr, &ci, sizeof(ci)) ? -EFAULT : 0;
 }
diff -Nru a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
--- a/net/bluetooth/hci_core.c	Sat Feb  7 17:52:09 2004
+++ b/net/bluetooth/hci_core.c	Sat Feb  7 17:52:09 2004
@@ -716,7 +716,7 @@
 	struct hci_dev_list_req *dl;
 	struct hci_dev_req *dr;
 	struct list_head *p;
-	int n = 0, size;
+	int n = 0, size, err;
 	__u16 dev_num;
 
 	if (get_user(dev_num, (__u16 *) arg))
@@ -724,14 +724,15 @@
 
 	if (!dev_num)
 		return -EINVAL;
-	
-	size = dev_num * sizeof(*dr) + sizeof(*dl);
 
-	if (verify_area(VERIFY_WRITE, (void *) arg, size))
-		return -EFAULT;
+	size = sizeof(*dl) + dev_num * sizeof(*dr);
+
+	if (size > PAGE_SIZE * 2)
+		return -EINVAL;
 
 	if (!(dl = kmalloc(size, GFP_KERNEL)))
 		return -ENOMEM;
+
 	dr = dl->dev_req;
 
 	read_lock_bh(&hci_dev_list_lock);
@@ -746,12 +747,12 @@
 	read_unlock_bh(&hci_dev_list_lock);
 
 	dl->dev_num = n;
-	size = n * sizeof(*dr) + sizeof(*dl);
+	size = sizeof(*dl) + n * sizeof(*dr);
 
-	copy_to_user((void *) arg, dl, size);
+	err = copy_to_user((void *) arg, dl, size);
 	kfree(dl);
 
-	return 0;
+	return err ? -EFAULT : 0;
 }
 
 int hci_get_dev_info(unsigned long arg)
diff -Nru a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
--- a/net/bluetooth/rfcomm/tty.c	Sat Feb  7 17:52:09 2004
+++ b/net/bluetooth/rfcomm/tty.c	Sat Feb  7 17:52:09 2004
@@ -349,7 +349,7 @@
 	struct rfcomm_dev_list_req *dl;
 	struct rfcomm_dev_info *di;
 	struct list_head *p;
-	int n = 0, size;
+	int n = 0, size, err;
 	u16 dev_num;
 
 	BT_DBG("");
@@ -362,8 +362,8 @@
 
 	size = sizeof(*dl) + dev_num * sizeof(*di);
 
-	if (verify_area(VERIFY_WRITE, (void *)arg, size))
-		return -EFAULT;
+	if (size > PAGE_SIZE * 4)
+		return -EINVAL;
 
 	if (!(dl = kmalloc(size, GFP_KERNEL)))
 		return -ENOMEM;
@@ -389,9 +389,10 @@
 	dl->dev_num = n;
 	size = sizeof(*dl) + n * sizeof(*di);
 
-	copy_to_user((void *) arg, dl, size);
+	err = copy_to_user((void *) arg, dl, size);
 	kfree(dl);
-	return 0;
+
+	return err ? -EFAULT : 0;
 }
 
 static int rfcomm_get_dev_info(unsigned long arg)
@@ -563,8 +564,10 @@
 	set_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
 
 	err = rfcomm_dlc_open(dlc, &dev->src, &dev->dst, dev->channel);
-	if (err < 0)
+	if (err < 0) {
+		rfcomm_dev_put(dev);
 		return err;
+	}
 
 	/* Wait for DLC to connect */
 	add_wait_queue(&dev->wait, &wait);
@@ -588,6 +591,9 @@
 	}
 	set_current_state(TASK_RUNNING);
 	remove_wait_queue(&dev->wait, &wait);
+
+	if (err < 0)
+		rfcomm_dev_put(dev);
 
 	return err;
 }

Re: some bluetooth fixes

[Andi Kleen]

On Sat, Feb 07, 2004 at 05:57:48PM +0100, Marcel Holtmann wrote:
> I now looked carefully through your patch and changed and added some
> parts to better fit into. I also fixed another RFCOMM refcount bug.
> Please review it, before I send it to Dave.

Doing size checks after the multiply is too late - they could
have already overflowed. You have to check the raw value from the user.

-Andi

[Bluez-devel] Re: some bluetooth fixes

[Marcel Holtmann]

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

Hi Andi,

> Doing size checks after the multiply is too late - they could
> have already overflowed. You have to check the raw value from the user.

new patch is attached.

Regards

Marcel


[-- Attachment #2: patch --]
[-- Type: text/x-patch, Size: 4511 bytes --]

diff -Nru a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
--- a/net/bluetooth/hci_conn.c	Wed Feb 11 19:39:20 2004
+++ b/net/bluetooth/hci_conn.c	Wed Feb 11 19:39:20 2004
@@ -353,21 +353,24 @@
 	struct hci_conn_info *ci;
 	struct hci_dev *hdev;
 	struct list_head *p;
-	int n = 0, size;
+	int n = 0, size, err;
 
 	if (copy_from_user(&req, (void *) arg, sizeof(req)))
 		return -EFAULT;
 
-	if (!(hdev = hci_dev_get(req.dev_id)))
-		return -ENODEV;
-
-	size = req.conn_num * sizeof(struct hci_conn_info) + sizeof(req);
+	if (req.conn_num * sizeof(*ci) > PAGE_SIZE * 2)
+		return -EINVAL;
 
-	if (verify_area(VERIFY_WRITE, (void *)arg, size))
-		return -EFAULT;
+	size = sizeof(req) + req.conn_num * sizeof(*ci);
 
 	if (!(cl = (void *) kmalloc(size, GFP_KERNEL)))
 		return -ENOMEM;
+
+	if (!(hdev = hci_dev_get(req.dev_id))) {
+		kfree(cl);
+		return -ENODEV;
+	}
+
 	ci = cl->conn_info;
 
 	hci_dev_lock_bh(hdev);
@@ -381,20 +384,21 @@
 		(ci + n)->out   = c->out;
 		(ci + n)->state = c->state;
 		(ci + n)->link_mode = c->link_mode;
-		n++;
+		if (++n >= req.conn_num)
+			break;
 	}
 	hci_dev_unlock_bh(hdev);
 
 	cl->dev_id = hdev->id;
 	cl->conn_num = n;
-	size = n * sizeof(struct hci_conn_info) + sizeof(req);
+	size = sizeof(req) + n * sizeof(*ci);
 
 	hci_dev_put(hdev);
 
-	copy_to_user((void *) arg, cl, size);
+	err = copy_to_user((void *) arg, cl, size);
 	kfree(cl);
 
-	return 0;
+	return err ? -EFAULT : 0;
 }
 
 int hci_get_conn_info(struct hci_dev *hdev, unsigned long arg)
@@ -407,9 +411,6 @@
 	if (copy_from_user(&req, (void *) arg, sizeof(req)))
 		return -EFAULT;
 
-	if (verify_area(VERIFY_WRITE, ptr, sizeof(ci)))
-		return -EFAULT;
-
 	hci_dev_lock_bh(hdev);
 	conn = hci_conn_hash_lookup_ba(hdev, req.type, &req.bdaddr);
 	if (conn) {
@@ -425,6 +426,5 @@
 	if (!conn)
 		return -ENOENT;
 
-	copy_to_user(ptr, &ci, sizeof(ci));
-	return 0;
+	return copy_to_user(ptr, &ci, sizeof(ci)) ? -EFAULT : 0;
 }
diff -Nru a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
--- a/net/bluetooth/hci_core.c	Wed Feb 11 19:39:20 2004
+++ b/net/bluetooth/hci_core.c	Wed Feb 11 19:39:20 2004
@@ -716,7 +716,7 @@
 	struct hci_dev_list_req *dl;
 	struct hci_dev_req *dr;
 	struct list_head *p;
-	int n = 0, size;
+	int n = 0, size, err;
 	__u16 dev_num;
 
 	if (get_user(dev_num, (__u16 *) arg))
@@ -724,14 +724,15 @@
 
 	if (!dev_num)
 		return -EINVAL;
-	
-	size = dev_num * sizeof(*dr) + sizeof(*dl);
 
-	if (verify_area(VERIFY_WRITE, (void *) arg, size))
-		return -EFAULT;
+	if (dev_num * sizeof(*dr) > PAGE_SIZE * 2)
+		return -EINVAL;
+
+	size = sizeof(*dl) + dev_num * sizeof(*dr);
 
 	if (!(dl = kmalloc(size, GFP_KERNEL)))
 		return -ENOMEM;
+
 	dr = dl->dev_req;
 
 	read_lock_bh(&hci_dev_list_lock);
@@ -746,12 +747,12 @@
 	read_unlock_bh(&hci_dev_list_lock);
 
 	dl->dev_num = n;
-	size = n * sizeof(*dr) + sizeof(*dl);
+	size = sizeof(*dl) + n * sizeof(*dr);
 
-	copy_to_user((void *) arg, dl, size);
+	err = copy_to_user((void *) arg, dl, size);
 	kfree(dl);
 
-	return 0;
+	return err ? -EFAULT : 0;
 }
 
 int hci_get_dev_info(unsigned long arg)
diff -Nru a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
--- a/net/bluetooth/rfcomm/tty.c	Wed Feb 11 19:39:20 2004
+++ b/net/bluetooth/rfcomm/tty.c	Wed Feb 11 19:39:20 2004
@@ -349,7 +349,7 @@
 	struct rfcomm_dev_list_req *dl;
 	struct rfcomm_dev_info *di;
 	struct list_head *p;
-	int n = 0, size;
+	int n = 0, size, err;
 	u16 dev_num;
 
 	BT_DBG("");
@@ -360,10 +360,10 @@
 	if (!dev_num)
 		return -EINVAL;
 
-	size = sizeof(*dl) + dev_num * sizeof(*di);
+	if (dev_num * sizeof(*di) > PAGE_SIZE * 4)
+		return -EINVAL;
 
-	if (verify_area(VERIFY_WRITE, (void *)arg, size))
-		return -EFAULT;
+	size = sizeof(*dl) + dev_num * sizeof(*di);
 
 	if (!(dl = kmalloc(size, GFP_KERNEL)))
 		return -ENOMEM;
@@ -389,9 +389,10 @@
 	dl->dev_num = n;
 	size = sizeof(*dl) + n * sizeof(*di);
 
-	copy_to_user((void *) arg, dl, size);
+	err = copy_to_user((void *) arg, dl, size);
 	kfree(dl);
-	return 0;
+
+	return err ? -EFAULT : 0;
 }
 
 static int rfcomm_get_dev_info(unsigned long arg)
@@ -563,8 +564,10 @@
 	set_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
 
 	err = rfcomm_dlc_open(dlc, &dev->src, &dev->dst, dev->channel);
-	if (err < 0)
+	if (err < 0) {
+		rfcomm_dev_put(dev);
 		return err;
+	}
 
 	/* Wait for DLC to connect */
 	add_wait_queue(&dev->wait, &wait);
@@ -588,6 +591,9 @@
 	}
 	set_current_state(TASK_RUNNING);
 	remove_wait_queue(&dev->wait, &wait);
+
+	if (err < 0)
+		rfcomm_dev_put(dev);
 
 	return err;
 }

[Bluez-devel] Re: some bluetooth fixes

[Marcel Holtmann]

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

Hi Andi,

> +	if (req.conn_num * sizeof(*ci) > PAGE_SIZE * 2)
> +		return -EINVAL;
> 
> This can still overflow. It should be 
> 
> 	if (req.conn_num > (PAGE_SIZE * 2)/sizeof(*ci))
> 		return -EINVAL

thanks for reviewing the patch again. The fixed version is only attached
for control. It goes out to Dave in the next minutes.

Regards

Marcel


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 4516 bytes --]

===== net/bluetooth/hci_conn.c 1.9 vs edited =====
--- 1.9/net/bluetooth/hci_conn.c	Tue Jan 13 03:50:09 2004
+++ edited/net/bluetooth/hci_conn.c	Wed Feb 11 21:00:52 2004
@@ -353,21 +353,24 @@
 	struct hci_conn_info *ci;
 	struct hci_dev *hdev;
 	struct list_head *p;
-	int n = 0, size;
+	int n = 0, size, err;
 
 	if (copy_from_user(&req, (void *) arg, sizeof(req)))
 		return -EFAULT;
 
-	if (!(hdev = hci_dev_get(req.dev_id)))
-		return -ENODEV;
-
-	size = req.conn_num * sizeof(struct hci_conn_info) + sizeof(req);
+	if (!req.conn_num || req.conn_num > (PAGE_SIZE * 2) / sizeof(*ci))
+		return -EINVAL;
 
-	if (verify_area(VERIFY_WRITE, (void *)arg, size))
-		return -EFAULT;
+	size = sizeof(req) + req.conn_num * sizeof(*ci);
 
 	if (!(cl = (void *) kmalloc(size, GFP_KERNEL)))
 		return -ENOMEM;
+
+	if (!(hdev = hci_dev_get(req.dev_id))) {
+		kfree(cl);
+		return -ENODEV;
+	}
+
 	ci = cl->conn_info;
 
 	hci_dev_lock_bh(hdev);
@@ -381,20 +384,21 @@
 		(ci + n)->out   = c->out;
 		(ci + n)->state = c->state;
 		(ci + n)->link_mode = c->link_mode;
-		n++;
+		if (++n >= req.conn_num)
+			break;
 	}
 	hci_dev_unlock_bh(hdev);
 
 	cl->dev_id = hdev->id;
 	cl->conn_num = n;
-	size = n * sizeof(struct hci_conn_info) + sizeof(req);
+	size = sizeof(req) + n * sizeof(*ci);
 
 	hci_dev_put(hdev);
 
-	copy_to_user((void *) arg, cl, size);
+	err = copy_to_user((void *) arg, cl, size);
 	kfree(cl);
 
-	return 0;
+	return err ? -EFAULT : 0;
 }
 
 int hci_get_conn_info(struct hci_dev *hdev, unsigned long arg)
@@ -407,9 +411,6 @@
 	if (copy_from_user(&req, (void *) arg, sizeof(req)))
 		return -EFAULT;
 
-	if (verify_area(VERIFY_WRITE, ptr, sizeof(ci)))
-		return -EFAULT;
-
 	hci_dev_lock_bh(hdev);
 	conn = hci_conn_hash_lookup_ba(hdev, req.type, &req.bdaddr);
 	if (conn) {
@@ -425,6 +426,5 @@
 	if (!conn)
 		return -ENOENT;
 
-	copy_to_user(ptr, &ci, sizeof(ci));
-	return 0;
+	return copy_to_user(ptr, &ci, sizeof(ci)) ? -EFAULT : 0;
 }
===== net/bluetooth/hci_core.c 1.22 vs edited =====
--- 1.22/net/bluetooth/hci_core.c	Thu Feb  5 13:07:36 2004
+++ edited/net/bluetooth/hci_core.c	Wed Feb 11 20:59:03 2004
@@ -716,22 +716,20 @@
 	struct hci_dev_list_req *dl;
 	struct hci_dev_req *dr;
 	struct list_head *p;
-	int n = 0, size;
+	int n = 0, size, err;
 	__u16 dev_num;
 
 	if (get_user(dev_num, (__u16 *) arg))
 		return -EFAULT;
 
-	if (!dev_num)
+	if (!dev_num || dev_num > (PAGE_SIZE * 2) / sizeof(*dr))
 		return -EINVAL;
-	
-	size = dev_num * sizeof(*dr) + sizeof(*dl);
 
-	if (verify_area(VERIFY_WRITE, (void *) arg, size))
-		return -EFAULT;
+	size = sizeof(*dl) + dev_num * sizeof(*dr);
 
 	if (!(dl = kmalloc(size, GFP_KERNEL)))
 		return -ENOMEM;
+
 	dr = dl->dev_req;
 
 	read_lock_bh(&hci_dev_list_lock);
@@ -746,12 +744,12 @@
 	read_unlock_bh(&hci_dev_list_lock);
 
 	dl->dev_num = n;
-	size = n * sizeof(*dr) + sizeof(*dl);
+	size = sizeof(*dl) + n * sizeof(*dr);
 
-	copy_to_user((void *) arg, dl, size);
+	err = copy_to_user((void *) arg, dl, size);
 	kfree(dl);
 
-	return 0;
+	return err ? -EFAULT : 0;
 }
 
 int hci_get_dev_info(unsigned long arg)
===== net/bluetooth/rfcomm/tty.c 1.28 vs edited =====
--- 1.28/net/bluetooth/rfcomm/tty.c	Thu Aug  7 02:24:51 2003
+++ edited/net/bluetooth/rfcomm/tty.c	Wed Feb 11 21:01:25 2004
@@ -349,7 +349,7 @@
 	struct rfcomm_dev_list_req *dl;
 	struct rfcomm_dev_info *di;
 	struct list_head *p;
-	int n = 0, size;
+	int n = 0, size, err;
 	u16 dev_num;
 
 	BT_DBG("");
@@ -357,14 +357,11 @@
 	if (get_user(dev_num, (u16 *) arg))
 		return -EFAULT;
 
-	if (!dev_num)
+	if (!dev_num || dev_num > (PAGE_SIZE * 4) / sizeof(*di))
 		return -EINVAL;
 
 	size = sizeof(*dl) + dev_num * sizeof(*di);
 
-	if (verify_area(VERIFY_WRITE, (void *)arg, size))
-		return -EFAULT;
-
 	if (!(dl = kmalloc(size, GFP_KERNEL)))
 		return -ENOMEM;
 
@@ -389,9 +386,10 @@
 	dl->dev_num = n;
 	size = sizeof(*dl) + n * sizeof(*di);
 
-	copy_to_user((void *) arg, dl, size);
+	err = copy_to_user((void *) arg, dl, size);
 	kfree(dl);
-	return 0;
+
+	return err ? -EFAULT : 0;
 }
 
 static int rfcomm_get_dev_info(unsigned long arg)
@@ -563,8 +561,10 @@
 	set_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
 
 	err = rfcomm_dlc_open(dlc, &dev->src, &dev->dst, dev->channel);
-	if (err < 0)
+	if (err < 0) {
+		rfcomm_dev_put(dev);
 		return err;
+	}
 
 	/* Wait for DLC to connect */
 	add_wait_queue(&dev->wait, &wait);
@@ -588,6 +588,9 @@
 	}
 	set_current_state(TASK_RUNNING);
 	remove_wait_queue(&dev->wait, &wait);
+
+	if (err < 0)
+		rfcomm_dev_put(dev);
 
 	return err;
 }

Re: some bluetooth fixes

[Andi Kleen]

On Wed, 11 Feb 2004 19:55:43 +0100
Marcel Holtmann <marcel@holtmann.org> wrote:

> Hi Andi,
> 
> > Doing size checks after the multiply is too late - they could
> > have already overflowed. You have to check the raw value from the user.
> 
> new patch is attached.


+	if (req.conn_num * sizeof(*ci) > PAGE_SIZE * 2)
+		return -EINVAL;

This can still overflow. It should be 

	if (req.conn_num > (PAGE_SIZE * 2)/sizeof(*ci))
		return -EINVAL

-Andi