On Thu, Nov 12, 2020 at 05:06:30PM -0500, Dennis Dalessandro wrote: > On 11/12/2020 12:14 PM, Ira Weiny wrote: > > On Wed, Nov 11, 2020 at 09:58:37PM -0500, Dennis Dalessandro wrote: > > > Two earlier bug fixes have created a security problem in the hfi1 > > > driver. One fix aimed to solve an issue where current->mm was not valid > > > when closing the hfi1 cdev. It attempted to do this by saving a cached > > > value of the current->mm pointer at file open time. This is a problem if > > > another process with access to the FD calls in via write() or ioctl() to > > > pin pages via the hfi driver. The other fix tried to solve a use after > > > free by taking a reference on the mm. This was just wrong because its > > > possible for a race condition between one process with an mm that opened > > > the cdev if it was accessing via an IOCTL, and another process > > > attempting to close the cdev with a different current->mm. > > > > Again I'm still not seeing the race here. It is entirely possible that the fix > > I was trying to do way back was mistaken too... ;-) I would just delete the > > last 2 sentences... and/or reference the commit of those fixes and help > > explain this more. > > I was attempting to refer to [1], the email that started all of this. > > > > > > > To fix this correctly we move the cached value of the mm into the mmu > > > handler struct for the driver. > > > > Looking at this closer I don't think you need the mm member of mmu_rb_handler > > any longer. See below. > > We went back and forth on this as well. We thought it better to rely on our > own pointer vs looking into the notifier to get the mm. Same reasoning for > doing our own referecne counting. Question is what is the preferred way > here. Functionally it makes no difference and I'm fine going either way. Use the mm pointer in the notifier if you have a notifier registered, it is clearer as to the lifetime and matches what other places do > That's the question. It does make sense to do that if we are sticking iwth > the notifier's reference vs our own explicit one. I'm not 100% sold that we > should not be doing the ref counting and keeping our own pointer. To me we > shoudln't be looking inside the notifer struct and instead honestly there > should probably be an API/helper call to get the mm from it. I'm open to > either approach. The notifier is there to support users of the notifier, and nearly all notifier users require the mm at various points. Adding get accessors is a bit of a kernel anti-pattern, this isn't Java.. Jason
