Re: use-after-free in usbnet

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Apr 21, 2012 at 9:49 AM, Ming Lei <tom.leiming@xxxxxxxxx> wrote:
> On Fri, Apr 20, 2012 at 10:56 PM, Huajun Li <huajun.li.lee@xxxxxxxxx> wrote:
>> On Fri, Apr 20, 2012 at 10:22 PM, Ming Lei <tom.leiming@xxxxxxxxx> wrote:
>>> On Fri, Apr 20, 2012 at 9:37 PM, Huajun Li <huajun.li.lee@xxxxxxxxx> wrote:
>>>>
>>>> Above patch has already been integrated to mainline. However, maybe
>>>> there still exists another potentail use-after-free issue, here is a
>>>> case:
>>>>      After release the lock in unlink_urbs(), defer_bh() may move
>>>> current skb from rxq/txq to dev->done queue, even cause the skb be
>>>> released. Then in next loop cycle, it can't refer to expected skb, and
>>>> may Oops again.
>>>
>>> Could you explain in a bit detail? Why can't the expected skb be refered
>>> to in next loop?
>>
>>
>>      unlink_urbs()                                           complete handler
>> --------------------------------------
>> -------------------------------------------------
>>     spin_unlock_irqrestore()
>>                                                                  rx_complete()
>>                                                                  derver_bh()
>>
>>  __skb_unlink()
>>
>>  __skb_queue_tail(&dev->done, skb)   =======> skb is moved to
>> dev->done, and can be freed by usbnet_bh()
>>      skb_queue_walk_safe()
>>                      tmp = skb->next   ===> refer to freed skb
>
> I see the problem, so looks skb_queue_walk_safe is not safe.
> I don' know why the 2nd ' tmp = skb->next' in  skb_queue_walk_safe
> is needed and it may become unsafe if skb is freed during current loop.
>
> But removing the 2nd 'tmp = skb->next' doesn't help the problem, because
> tmp still may become freed after releasing lock.
>
>> If its state is x_done/tx_done/rx_cleanup, that means the the skb will
>> be released soon, right? If so, it should avoid calling
>> usb_unlink_urb().
>
> Even though you can avoid calling unlink for completed URBs, the skbs
> still may be freed in unlink path because complete handler will be triggered
> by unlink and the referenced skb may be freed before next loop, so your
> patch can't fix the oops.
>

Hi Ming,
     That's why my patch uses skb_queue_walk() to traverse the queue,
it guarantees the skb available in each loop.  Is this what you
expected?

     The main idea of my patch(it is based on current mainline: 3.4.0-rc3) is:
     1. If the skb in txq/rxq, then it must be available,
unlink_urbs() can refer to it safely while it holds  q->lock;
     2. If the skb in txq/rxq and its state is
rx_done/tx_done/rx_cleanup, that means the skb's URB is complete, then
don't need to unlink it again;
     3. Before releasing  q->lock in unlink_urbs(), it will increase
the URB's refercount, so even the related skb is freed in future, the
URB is still available.

Thanks,
--Huajun

> As far as I can think of, we can hold lock of done queue to forbid skb free
> during unlinking. The below patch may fix the problem, are you OK
> with it?

Just skip trying this per your following email's comments.

>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index db99536..a9809d4 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -581,7 +581,8 @@ static int unlink_urbs (struct usbnet *dev, struct
> sk_buff_head *q)
>        struct sk_buff          *skb, *skbnext;
>        int                     count = 0;
>
> -       spin_lock_irqsave (&q->lock, flags);
> +       spin_lock_irqsave(&dev->done.lock, flags);
> +       spin_lock(&q->lock);
>        skb_queue_walk_safe(q, skb, skbnext) {
>                struct skb_data         *entry;
>                struct urb              *urb;
> @@ -598,7 +599,7 @@ static int unlink_urbs (struct usbnet *dev, struct
> sk_buff_head *q)
>                 * handler(include defer_bh).
>                 */
>                usb_get_urb(urb);
> -               spin_unlock_irqrestore(&q->lock, flags);
> +               spin_unlock(&q->lock);
>                // during some PM-driven resume scenarios,
>                // these (async) unlinks complete immediately
>                retval = usb_unlink_urb (urb);
> @@ -607,9 +608,10 @@ static int unlink_urbs (struct usbnet *dev,
> struct sk_buff_head *q)
>                else
>                        count++;
>                usb_put_urb(urb);
> -               spin_lock_irqsave(&q->lock, flags);
> +               spin_lock(&q->lock);
>        }
> -       spin_unlock_irqrestore (&q->lock, flags);
> +       spin_unlock(&q->lock);
> +       spin_unlock_irqrestore(&dev->done.lock, flags);
>        return count;
>  }
>
>
>
> Thanks,
> --
> Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux