Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933167AbdHVPpc (ORCPT ); Tue, 22 Aug 2017 11:45:32 -0400 Received: from mail.ispras.ru ([83.149.199.45]:49170 "EHLO mail.ispras.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933040AbdHVPpa (ORCPT ); Tue, 22 Aug 2017 11:45:30 -0400 Subject: Re: [PATCH] udc: Memory leak on error path and use after free To: Alan Stern Cc: Felipe Balbi , Greg Kroah-Hartman , Jussi Kivilinna , Peter Senna Tschudin , Raz Manor , Romain Perier , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, ldv-project@linuxtesting.org References: From: Anton Vasilyev Message-ID: <78731daa-98a5-3a28-97b2-f70cdd93ca72@ispras.ru> Date: Tue, 22 Aug 2017 18:45:29 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2244 Lines: 69 Sorry for delayed reply. On 16.08.2017 19:35, Alan Stern wrote: > On Wed, 16 Aug 2017, Anton Vasilyev wrote: > >> On 16.08.2017 18:29, Alan Stern wrote: >>> On Wed, 16 Aug 2017, Anton Vasilyev wrote: >>> >>>> gadget_release() is responsible for cleanup dev memory. >>>> But if net2280_probe() fails after dev allocation, then >>>> gadget_release() become unregistered and dev memory leaks. >>> >>> This isn't needed if usb_add_gadget_udc_release() is fixed, right? >>> >> >> No, this situation could appear before call >> usb_add_gadget_udc_release(). >> >>>> Also net2280_remove() calls usb_del_gadget_udc() which >>>> perform schedule_delayed_work() with gadget_release(), so >>>> it is possible that dev will be deallocated exactly after >>>> this call and leads to use after free. >>> >>> Where is there a possible use after free? >>> >> >> net2280_remove() continue work with struct net2280 *dev after call >> usb_del_gadget_udc(&dev->gadget), but this net2280 *dev could be >> deallocated by gadget_release() >> >>>> The patch moves deallocation from gadget_release() to >>>> net2280_remove(). >>> >>> Alan Stern > > Okay, now I understand what you were saying. Yes, I agree, the > existing code isn't right. > > But a better solution would be to move the usb_del_gadget_udc() call > from the beginning of net2280_remove() to the end. And make the call > conditional, depending on whether usb_add_gadget_udc_release() has > already been called successfully. If allow gadget_release() to deallocate net2280 *dev then it will be called on fail of usb_add_gadget_udc_release() and it will be unsafe to perform clean-up. My point is that gadget shouldn't deallocate its parent memory at all. > > The point is that the device core does not allow drivers to deallocate > memory containing a struct device before the ->release callback has > been invoked. Your patch might do that, if the release was delayed for > some reason. I don't see possibility for parent device to be removed before its child was released. Please point if I'm wrong. Alternative way to move allocation under devm interface. > > Alan Stern > -- Anton Vasilyev Linux Verification Center, ISPRAS web: http://linuxtesting.org e-mail: vasilyev@ispras.ru