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: <ba308dcf-ed07-e0c6-4992-9b609e0d7626@gmail.com>
Date:   Thu, 27 Oct 2022 11:05:21 +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:     Alan Stern <stern@rowland.harvard.edu>
Cc:     gregkh@linuxfoundation.org, arnd@arndb.de,
        linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
        imv4bel@gmail.com
References: <2e5cbdfe-f6cd-d24f-9785-55176af6c975@gmail.com>
 <Y1lL+dVsJo2zu3Gy@rowland.harvard.edu>
Content-Language: en-US
In-Reply-To: <Y1lL+dVsJo2zu3Gy@rowland.harvard.edu>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hello Alan,

On 26/10/2022 18:02, Alan Stern wrote:
> On Wed, Oct 26, 2022 at 11:52:40AM +0300, Eli Billauer wrote:
>> To fix this, xillybus_find_inode() supplies the pointer to the mutex that
>> it has locked (when returning success), so xillyusb_open() releases this
>> mutex only after obtaining the mutex that is specific to a device file.
>> This ensures that xillyusb_disconnect() won't release anything that is in
>> use.
> 
> The standard way of handling this problem is different from this.  The
> driver defines a private mutex, and it ensures that any routine calling
> *_find_inode() holds the mutex.  It also ensures that the mutex is held
> while a new device is being registered and while a device is being
> removed.

Thanks, I'm going to follow that advice in my v2 patch.

> 
> Even that won't fix all the synchronization problems.  A process can
> open a device, and then after the device has been removed the process
> can still try to access the device.  The driver needs to ensure that
> such accesses are not allowed.

Indeed. For that purpose, the relevant struct has a kref_counter, and an 
error flag that indicates a removal among others, along with mutexes. 
The problem is the time gap from the moment that the struct has been 
found by xillybus_find_inode() until it has been secured with the kref.

A new mutex is going to solve that.

Regards,
   Eli