From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966800AbdCXRLu (ORCPT ); Fri, 24 Mar 2017 13:11:50 -0400 Received: from mail-vk0-f42.google.com ([209.85.213.42]:36792 "EHLO mail-vk0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752400AbdCXRLl (ORCPT ); Fri, 24 Mar 2017 13:11:41 -0400 MIME-Version: 1.0 In-Reply-To: References: From: Dmitry Vyukov Date: Fri, 24 Mar 2017 18:11:14 +0100 Message-ID: Subject: Re: usb: use-after-free write in usb_hcd_link_urb_to_ep To: Alan Stern Cc: Greg Kroah-Hartman , mathias.nyman@linux.intel.com, baoyou.xie@linaro.org, peter.chen@nxp.com, william wu , wsa-dev@sang-engineering.com, javier@osg.samsung.com, Chris Bainbridge , USB list , LKML , syzkaller Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Mar 24, 2017 at 3:27 PM, Alan Stern wrote: > On Fri, 24 Mar 2017, Dmitry Vyukov wrote: > >> On Thu, Mar 23, 2017 at 4:22 PM, Dmitry Vyukov 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 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); >