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
