From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id A273FC38A02 for ; Sun, 30 Oct 2022 16:23:25 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229544AbiJ3QXY (ORCPT ); Sun, 30 Oct 2022 12:23:24 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38286 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229441AbiJ3QXV (ORCPT ); Sun, 30 Oct 2022 12:23:21 -0400 Received: from netrider.rowland.org (netrider.rowland.org [192.131.102.5]) by lindbergh.monkeyblade.net (Postfix) with SMTP id 57F8018B for ; Sun, 30 Oct 2022 09:23:20 -0700 (PDT) Received: (qmail 162170 invoked by uid 1000); 30 Oct 2022 12:23:19 -0400 Date: Sun, 30 Oct 2022 12:23:19 -0400 From: Alan Stern To: Eli Billauer Cc: gregkh@linuxfoundation.org, arnd@arndb.de, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org, imv4bel@gmail.com Subject: Re: [PATCH v2] char: xillybus: Prevent use-after-free due to race condition Message-ID: References: <20221030094209.65916-1-eli.billauer@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20221030094209.65916-1-eli.billauer@gmail.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Oct 30, 2022 at 11:42:09AM +0200, Eli Billauer wrote: > The driver for XillyUSB devices maintains a kref reference count on each > xillyusb_dev structure, which represents a physical device. This reference > count reaches zero when the device has been disconnected and there are no > open file descriptors that are related to the device. When this occurs, > kref_put() calls cleanup_dev(), which clears up the device's data, > including the structure itself. > > However, when xillyusb_open() is called, this reference count becomes > tricky: This function needs to obtain the xillyusb_dev structure that > relates to the inode's major and minor (as there can be several such). > xillybus_find_inode() (which is defined in xillybus_class.c) is called > for this purpose. xillybus_find_inode() holds a mutex that is global in > xillybus_class.c to protect the list of devices, and releases this > mutex before returning. As a result, nothing protects the xillyusb_dev's > reference counter from being decremented to zero before xillyusb_open() > increments it on its own behalf. Hence the structure can be freed > due to a rare race condition. > > To solve this, a mutex is added. It is locked by xillyusb_open() before > the call to xillybus_find_inode() and is released only after the kref > counter has been incremented on behalf of the newly opened inode. This > protects the kref reference counters of all xillyusb_dev structs from > being decremented by xillyusb_disconnect() during this time segment, as > the call to kref_put() in this function is done with the same lock held. > > There is no need to hold the lock on other calls to kref_put(), because > if xillybus_find_inode() finds a struct, xillyusb_disconnect() has not > made the call to remove it, and hence not made its call to kref_put(), > which takes place afterwards. Hence preventing xillyusb_disconnect's > call to kref_put() is enough to ensure that the reference doesn't reach > zero before it's incremented by xillyusb_open(). > > It would have been more natural to increment the reference count in > xillybus_find_inode() of course, however this function is also called by > Xillybus' driver for PCIe / OF, which registers a completely different > structure. Therefore, xillybus_find_inode() treats these structures as > void pointers, and accordingly can't make any changes. > > Reported-by: Hyunwoo Kim > Suggested-by: Alan Stern > Signed-off-by: Eli Billauer It looks like the xillybus driver already has a private mutex that would have been very well suited for this task: unit_mutex defined in xillybus_class.c. Of course, there's nothing wrong with using a new mutex instead -- just make sure there aren't any ABBA locking order problems. Alan Stern