Re: [PATCH] char: xillybus: Prevent use-after-free due to race condition

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux