Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <818c678f-8a51-a087-f8c0-09553ca1304d@gmail.com>
Date:   Thu, 27 Oct 2022 11:01:25 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
From:   Eli Billauer <eli.billauer@gmail.com>
Subject: Re: [PATCH] char: xillybus: Prevent use-after-free due to race
 condition
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     arnd@arndb.de, linux-kernel@vger.kernel.org,
        linux-usb@vger.kernel.org, imv4bel@gmail.com
References: <2e5cbdfe-f6cd-d24f-9785-55176af6c975@gmail.com>
 <Y1kjCqUlOFJUgLqZ@kroah.com>
Content-Language: en-US
In-Reply-To: <Y1kjCqUlOFJUgLqZ@kroah.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hello Greg,

I'm going to follow Alan's advice, and add a mutex instead of juggling 
with the existing one. So I'll prepare a new patch with a completely 
different solution. I've answered your questions below, even though I'm 
not sure any of that is still relevant.

On 26/10/2022 15:07, Greg KH wrote:

>> int xillybus_find_inode(struct inode *inode,
>> +			struct mutex **to_unlock,
> 
> To unlock when?  Who unlocks it?  What is the lifespan here?

xillybus_find_inode() finds the structure that represents a Xillybus 
(PCIe / OF) or XillyUSB (USB) device. The idea was to delay the unlock 
of the mutex until other means have been taken to prevent that structure 
from being freed. Which was virtually immediately after 
xillybus_find_inode() returns, but gave XillyUSB's driver a chance to 
acquire another mutex before releasing this one.

XillyUSB's driver can't prevent the release of this structure before it 
knows which structure it is (that's xillybus_find_inode()'s job to find 
out). But because xillybus_find_inode() unlocks its mutex before 
returning, there is a short period of time where the structure is 
unprotected. So I thought, let's extend the life of the first mutex just 
a little bit to keep the whole thing watertight.

> 
> Why can't it just be part of the structure?

The problem is that this structure is either a struct xilly_endpoint 
(for PCIe / OF) or a struct xillyusb_dev (for USB), and these have 
virtually nothing in common. xillybus_find_inode() is used by two 
completely different drivers that are grouped into a single class.

>>
>> 	*private_data = unit->private_data;
>> 	*index = minor - unit->lowest_minor;
>> +	*to_unlock = &unit_mutex;
> 
> Why are you wanting the caller to unlock this?  It's a global mutex (for
> the whole file), this feels really odd.

As mentioned above, this gives the caller (i.e. XillyUSB's driver) a 
chance to ensure that the structure isn't freed as a result of the 
device's disconnection.
> 
> What is this function supposed to be doing?  You only return an int, and
> you have some odd opaque void pointer being set.  That feels wrong and
> is not a normal design at all.

xillybus_find_inode() finds the device's structure, based upon the 
inode's major and minor.

The opaque void pointer is the structure that represents the device, 
either a PCIe / OF device or a XillyUSB device. Because of the 
difference between a driver for PCIe / OF and a USB driver, these are 
different structs, and hence represented by a void pointer. The int just 
says which of the device files, that are covered by the struct, has been 
opened.

Once again, all this is history, as I'm preparing a new patch, which is 
going to add a separate mutex.

Regards,
    Eli