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
|