netdev
[Top] [All Lists]

Re: some bluetooth fixes

To: Andi Kleen <ak@xxxxxxx>
Subject: Re: some bluetooth fixes
From: Marcel Holtmann <marcel@xxxxxxxxxxxx>
Date: Sat, 07 Feb 2004 12:13:31 +0100
Cc: bluez-devel@xxxxxxxxxxxxxxxxxxxxx, netdev@xxxxxxxxxxx, viro@xxxxxxxxxxxxxxxxxx
In-reply-to: <20040207032428.56ffbebc.ak@xxxxxxx>
References: <20040206050042.20a2b3b0.ak@xxxxxxx> <1076079512.2806.40.camel@pegasus> <20040207032428.56ffbebc.ak@xxxxxxx>
Sender: netdev-bounce@xxxxxxxxxxx
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



<Prev in Thread] Current Thread [Next in Thread>