some bluetooth fixes

some bluetooth fixes

[Andi Kleen]

While reading bluetooth code I noticed some bugs and potential overflows.

Also I commented one ref-counting bug.

Patch against 2.6.2.

-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
@@ -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 > (4*PAGE_SIZE)/sizeof(*di))
+		return -EINVAL;
 
 	if (!(dl = kmalloc(size, GFP_KERNEL)))
 		return -ENOMEM;
@@ -389,9 +389,9 @@
 	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) ? -EFAULT : 0;
 	kfree(dl);
-	return 0;
+	return err;
 }
 
 static int rfcomm_get_dev_info(unsigned long arg)
@@ -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;
 
@@ -563,8 +565,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);
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) {
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) {
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);
@@ -375,6 +378,9 @@
 		register struct hci_conn *c;
 		c = list_entry(p, struct hci_conn, list);
 
+		if (n >= req.conn_num) 
+			break;
+
 		bacpy(&(ci + n)->bdaddr, &c->dst);
 		(ci + n)->handle = c->handle;
 		(ci + n)->type  = c->type;
@@ -391,10 +397,10 @@
 
 	hci_dev_put(hdev);
 
-	copy_to_user((void *) arg, cl, size);
+	err = copy_to_user((void *) arg, cl, size) ? -EFAULT : 0; 
 	kfree(cl);
 
-	return 0;
+	return err;
 }
 
 int hci_get_conn_info(struct hci_dev *hdev, unsigned long arg)
@@ -407,9 +413,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 +428,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 -u linux-2.6.2-work32/net/bluetooth/hci_core.c-o linux-2.6.2-work32/net/bluetooth/hci_core.c
--- linux-2.6.2-work32/net/bluetooth/hci_core.c-o	2004-02-05 08:09:54.000000000 +0100
+++ linux-2.6.2-work32/net/bluetooth/hci_core.c	2004-02-05 15:01:46.000000000 +0100
@@ -715,18 +715,16 @@
 	struct list_head *p;
 	int n = 0, size;
 	__u16 dev_num;
-
+	int err;
+	
 	if (get_user(dev_num, (__u16 *) arg))
 		return -EFAULT;
 
-	if (!dev_num)
+	if (!dev_num || dev_num >= (4*PAGE_SIZE)/sizeof(*dr))
 		return -EINVAL;
 	
 	size = dev_num * sizeof(*dr) + sizeof(*dl);
 
-	if (verify_area(VERIFY_WRITE, (void *) arg, size))
-		return -EFAULT;
-
 	if (!(dl = kmalloc(size, GFP_KERNEL)))
 		return -ENOMEM;
 	dr = dl->dev_req;
@@ -745,10 +743,10 @@
 	dl->dev_num = n;
 	size = n * sizeof(*dr) + sizeof(*dl);
 
-	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)

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

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

Re: some bluetooth fixes

[David S. Miller]

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

Thanks.

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

Re: some bluetooth fixes

[Andi Kleen]

On Fri, 06 Feb 2004 15:58:32 +0100
Marcel Holtmann <marcel@holtmann.org> wrote:

> 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?

Probably. I haven't looked at 2.4.

> > 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.

> > 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.

-Andi

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

Re: some bluetooth fixes

[Andi Kleen]

On Sat, 07 Feb 2004 12:13:31 +0100
Marcel Holtmann <marcel@holtmann.org> wrote:

> 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.

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

-Andi

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


-------------------------------------------------------
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

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;
 }

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


-------------------------------------------------------
SF.Net is sponsored by: Speed Start Your Linux Apps Now.
Build and deploy apps & Web services for Linux with
a free DVD software kit from IBM. Click Now!
http://ads.osdn.com/?ad_id=1356&alloc_id=3438&op=click

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

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;
 }