[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CACT4Y+azrMsFv3_AV736EYRxs2=bRs668NfJgJyMLQvbN-kVsA@mail.gmail.com>
Date: Fri, 24 Mar 2017 18:11:14 +0100
From: Dmitry Vyukov <dvyukov@...gle.com>
To: Alan Stern <stern@...land.harvard.edu>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
mathias.nyman@...ux.intel.com, baoyou.xie@...aro.org,
peter.chen@....com, william wu <wulf@...k-chips.com>,
wsa-dev@...g-engineering.com, javier@....samsung.com,
Chris Bainbridge <chris.bainbridge@...il.com>,
USB list <linux-usb@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
syzkaller <syzkaller@...glegroups.com>
Subject: Re: usb: use-after-free write in usb_hcd_link_urb_to_ep
On Fri, Mar 24, 2017 at 3:27 PM, Alan Stern <stern@...land.harvard.edu> wrote:
> On Fri, 24 Mar 2017, Dmitry Vyukov wrote:
>
>> On Thu, Mar 23, 2017 at 4:22 PM, Dmitry Vyukov <dvyukov@...gle.com> wrote:
>> >> On Thu, 23 Mar 2017, Dmitry Vyukov wrote:
>> >>
>> >>> > Putting these together:
>> >>> >
>> >>> > The memory was allocated in usb_internal_control_msg() line 93.
>> >>> > The later events occurred within the call in line 100 to
>> >>> > usb_start_wait_urb().
>> >>> >
>> >>> > The invalid access occurred within usb_start_wait_urb() line 56.
>> >>> >
>> >>> > The memory was deallocated within usb_start_wait_urb() line 78.
>> >>> >
>> >>> > Since these routines don't involve any loops or backward jumps, this
>> >>> > says that the invalid access occurred before the memory was
>> >>> > deallocated! So why is it reported as a problem?
>> >>>
>> >>>
>> >>> My first guess would be that pid 3348 did 2 calls to open and the urb
>> >>> was somehow referenced across these calls. Is it possible?
>> >>
>> >> I don't think so. The URB gets allocated and deallocated separately
>> >> for each call. You can see this very plainly by reading the source
>> >> code for usb_internal_control_msg() and usb_start_wait_urb().
>> >>
>> >> It's possible that the same memory location was allocated and
>> >> deallocated for two different calls at different times. That wouldn't
>> >> fool syzkaller, would it?
>> >
>> >
>> > Generally it does not fool KASAN because of heap memory quarantine.
>> > I will take a closer look tomorrow.
>> > Thanks for looking into this.
>>
>>
>> The bug looks real to me and it can be easily reproduced by executing:
>>
>> mmap(&(0x7f0000000000/0xfff000)=nil, (0xfff000), 0x3, 0x32,
>> 0xffffffffffffffff, 0x0)
>> syz_open_dev$usb(&(0x7f0000001000-0x15)="2f6465762f6275732f7573622f3030232f30302300",
>> 0x1ff, 0x200)
>> syz_open_dev$usb(&(0x7f0000002000-0x15)="2f6465762f6275732f7573622f3030232f30302300",
>> 0x3f, 0x0)
>
> (Incidentally, those ASCII strings say "/dev/bus/usb/00#/00#", which is
> a rather unusual name for a USB device node. Where did it come from?)
We use "/dev/bus/usb/00#/00#" to enumerate all devices in the
syzkaller (# are later replaced by some numbers).
>> and failing 7-th malloc in the first one, this one:
>>
>> kzalloc include/linux/slab.h:663 [inline]
>> rh_call_control drivers/usb/core/hcd.c:522 [inline]
>>
>> From the report:
>>
>> (from the failure stack)
>> kmalloc in rh_call_control fails, and it straight returns -ENOMEM
>> without calling usb_hcd_unlink_urb_from_ep(hcd, urb) leaving urb
>> linked into hcd (and that's probably the root problem)
>
> Ah, very good detective work! I agree.
>
>> consequently:
>> rh_urb_enqueue return -ENOMEM
>> usb_hcd_submit_urb does INIT_LIST_HEAD(&urb->urb_list), but the urb is
>> still linked, so we got corrupted list
>> (from the free stack)
>> eventually usb_start_wait_urb frees the still linked urb by calling usb_free_urb
>> (form the use-after-free stack)
>> the subsequent open links a new urb into the corrupted list in
>> usb_hcd_link_urb_to_ep
>> bang!
>
> Here's a patch to fix the problem; let me know how it works.
Yes, this works.
Tested-by: Dmitry Vyukov <dvyukov@...gle.com>
Thanks!
> Alan
>
>
> Index: usb-4.x/drivers/usb/core/hcd.c
> ===================================================================
> --- usb-4.x.orig/drivers/usb/core/hcd.c
> +++ usb-4.x/drivers/usb/core/hcd.c
> @@ -520,8 +520,10 @@ static int rh_call_control (struct usb_h
> */
> tbuf_size = max_t(u16, sizeof(struct usb_hub_descriptor), wLength);
> tbuf = kzalloc(tbuf_size, GFP_KERNEL);
> - if (!tbuf)
> - return -ENOMEM;
> + if (!tbuf) {
> + status = -ENOMEM;
> + goto err_alloc;
> + }
>
> bufp = tbuf;
>
> @@ -734,6 +736,7 @@ error:
> }
>
> kfree(tbuf);
> + err_alloc:
>
> /* any errors get returned through the urb completion */
> spin_lock_irq(&hcd_root_hub_lock);
>
Powered by blists - more mailing lists