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?
Thanks,
Eli
