Re: [PATCH for-rc v2] IB/hfi1: Move cached value of mm into handler

[Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>]

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.


> NIT: I really think you should follow up with a spelling fix patch...  Sorry
> just got frustrated greping for 'handler' and not finding this!  ;-)
> 
>>   	INIT_WORK(&handlr->del_work, handle_remove);
>>   	INIT_LIST_HEAD(&handlr->del_list);
>>   	INIT_LIST_HEAD(&handlr->lru_list);
>>   	handlr->wq = wq;
>>   
>> -	ret = mmu_notifier_register(&handlr->mn, handlr->mm);
>> +	ret = mmu_notifier_register(&handlr->mn, current->mm);
>>   	if (ret) {
>>   		kfree(handlr);
>>   		return ret;
>>   	}
>>   
>> +	handlr->mm = current->mm;
> 
> Sorry I did not catch this before but do you need to store this pointer?  Is it
> not enough to check the ->mn.mm? ...
> 
> I think that would also make it clear you are relying on the mmget() within the
> mmu_notifier_register()  Because that is the reference you are using rather
> than having another reference here which could potentially be used wrongly in
> the future.

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.


-Denny