use-after-free in usbnet

Hi Huajun,

On Sat, Apr 21, 2012 at 4:23 PM, Huajun Li <huajun.li.lee@gmail.com> wrote:
> On Sat, Apr 21, 2012 at 3:56 PM, Ming Lei <tom.leiming@gmail.com> wrote:
>> Hi Huajun,
>>
>> On Sat, Apr 21, 2012 at 3:50 PM, Huajun Li <huajun.li.lee@gmail.com> wrote:
>>
>>> Did we on the same page, could you please review my patch again?
>>>
>>> My draft patch was based on current mainline( 3.4.0-rc3)  which had
>>> already integrated your previous patch. And in my patch, it replaced
>>> skb_queue_walk_safe() with skb_queue_walk(), so you will not see  'tmp
>>> = skb->next'  any more.
>>
>> Replace skb_queue_walk_safe with skb_queue_walk doesn't improve
>> the problem, since 'skb = skb->next' in skb_queue_walk still may trigger
>> the oops, does it?
>>
>
> No.
> In each loop, my patch traverse the queue from its head, and it always
> holds  q->lock when it need refer "skb->next", this can make sure the
> right skb is not moved out of rxq/txq.

OK, your patch can avoid the oops, sorry for miss the point.

>
> Can this fix what you concern? If so, IMO, there is no need to revert
> your previous patch.

But your patch may introduce another problem, in fact, what your patch does
is basically same with the below change[1]:

So we can find easily that one same URB may be unlinked more than one
time with your patch because usb_unlink_urb is asynchronous even though
it behaves synchronously sometimes.

I remembered that is not allowed, at least usb_unlink_urb's comment says so:

          URBs complete only once per submission, and may be canceled only
          once per submission.


[1], against 3.4.0-rc3

Thanks,

On Sat, Apr 21, 2012 at 5:40 PM, Ming Lei <tom.leiming@gmail.com> wrote:
> Hi Huajun,
>
> On Sat, Apr 21, 2012 at 4:23 PM, Huajun Li <huajun.li.lee@gmail.com> wrote:
>> On Sat, Apr 21, 2012 at 3:56 PM, Ming Lei <tom.leiming@gmail.com> wrote:
>>> Hi Huajun,
>>>
>>> On Sat, Apr 21, 2012 at 3:50 PM, Huajun Li <huajun.li.lee@gmail.com> wrote:
>>>
>>>> Did we on the same page, could you please review my patch again?
>>>>
>>>> My draft patch was based on current mainline( 3.4.0-rc3)  which had
>>>> already integrated your previous patch. And in my patch, it replaced
>>>> skb_queue_walk_safe() with skb_queue_walk(), so you will not see  'tmp
>>>> = skb->next'  any more.
>>>
>>> Replace skb_queue_walk_safe with skb_queue_walk doesn't improve
>>> the problem, since 'skb = skb->next' in skb_queue_walk still may trigger
>>> the oops, does it?
>>>
>>
>> No.
>> In each loop, my patch traverse the queue from its head, and it always
>> holds  q->lock when it need refer "skb->next", this can make sure the
>> right skb is not moved out of rxq/txq.
>
> OK, your patch can avoid the oops, sorry for miss the point.
>
>>
>> Can this fix what you concern? If so, IMO, there is no need to revert
>> your previous patch.
>
> But your patch may introduce another problem, in fact, what your patch does
> is basically same with the below change[1]:
>
> So we can find easily that one same URB may be unlinked more than one
> time with your patch because usb_unlink_urb is asynchronous even though
> it behaves synchronously sometimes.
>
> I remembered that is not allowed, at least usb_unlink_urb's comment says so:
>
>          URBs complete only once per submission, and may be canceled only
>          once per submission.
>

Yes, this is a problem should be avoided.

> [1], against 3.4.0-rc3
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index db99536..aadf009 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -578,15 +578,19 @@ EXPORT_SYMBOL_GPL(usbnet_purge_paused_rxq);
>  static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q)
>  {
>        unsigned long           flags;
> -       struct sk_buff          *skb, *skbnext;
> +       struct sk_buff          *skb;
>        int                     count = 0;
>
>        spin_lock_irqsave (&q->lock, flags);
> -       skb_queue_walk_safe(q, skb, skbnext) {
> +       while (1) {
>                struct skb_data         *entry;
>                struct urb              *urb;
>                int                     retval;
>
> +               skb = q->next;
> +               if (skb == (struct sk_buff *)q)
> +                       break;
> +
>                entry = (struct skb_data *) skb->cb;
>                urb = entry->urb;
>
>
> Thanks,
> --
> Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html