From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965787AbdCXO1x (ORCPT ); Fri, 24 Mar 2017 10:27:53 -0400 Received: from iolanthe.rowland.org ([192.131.102.54]:58212 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1753072AbdCXO1p (ORCPT ); Fri, 24 Mar 2017 10:27:45 -0400 Date: Fri, 24 Mar 2017 10:27:43 -0400 (EDT) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: Dmitry Vyukov cc: Greg Kroah-Hartman , , , , william wu , , , Chris Bainbridge , USB list , LKML , syzkaller Subject: Re: usb: use-after-free write in usb_hcd_link_urb_to_ep In-Reply-To: Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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?) > > 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. 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);