Dear, On Sun, Nov 13, 2022 at 10:30:16AM +0200, Eli Billauer wrote: > On 13/11/2022 10:05, Hyunwoo Kim wrote: > > Dear, > > > > Sorry for the late review. > > > > This patch cannot prevent the UAF scenario I presented: > > ``` > > cpu0 cpu1 > > 1. xillyusb_open() > > mutex_lock(&kref_mutex); // unaffected lock > > xillybus_find_inode() > > mutex_lock(&unit_mutex); > > unit = iter; > > mutex_unlock(&unit_mutex); > > 2. xillyusb_disconnect() > > xillybus_cleanup_chrdev() > > mutex_lock(&unit_mutex); > > kfree(unit); > > mutex_unlock(&unit_mutex); > > 3. *private_data = unit->private_data; // UAF > > > > ``` > > > > This is a UAF for 'unit', not a UAF for 'xdev'. > > So, the added 'kref_mutex' has no effect. > > > > You're correct. This submitted patch solves only one problem out of two. It > prevents the content of @private_data to be freed, but you're right that > @unit itself isn't protected well enough. > > The problem you're pointing at is that @unit can be freed before its > attempted use, because the mutex is released too early. > > This is easily solved by moving down the mutex_unlock() call to after @unit > has been used. Do you want the pleasure to submit this patch, or should I? I hope you work as before. And, even if the mutex_unlock(&unit_mutex); of xillybus_find_inode() is finally moved, xdev may be released before kref_get() is executed if xillyusb_disconnect() ends just before the function returns. (Of course, this is an extremely rare case.) So, in xillyusb_open() we need to move kref_get() above xillybus_find_inode(). Regards, Hyunwoo Kim
