On Tue, Nov 17, 2020 at 06:32:13PM -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. > > To fix this correctly we move the cached value of the mm into the mmu > handler struct for the driver. Is this true for this version of the patch? It seems this version removes the mm member from the mmu_rb_handler and relies on the mmu notifier mm... > Now we can check in the insert, evict, > etc. routines that current->mm matched what the handler was registered > for. If not, then don't allow access. The register of the mmu notifier > will save the mm pointer. > > Note the check in the unregister is not needed in the event that > current->mm is empty. This means the tear down is happening due to a > SigKill or OOM Killer, something along those lines. If current->mm has a > value then it must be checked and only the task that did the register > can do the unregister. > > Since in do_exit() the exit_mm() is called before exit_files(), which > would call our close routine a reference is needed on the mm. We rely on > the mmgrab done by the registration of the notifier, whereas before it > was explicit. Since you need to clean up the commit message above I think another good idea would be to put this explanation in the code in hfi1_mmu_rb_unregister() so that people understand right away why that check is special. [snip] > @@ -92,7 +81,7 @@ static unsigned long mmu_node_last(struct mmu_rb_node *node) > return PAGE_ALIGN(node->addr + node->len) - 1; > } > > -int hfi1_mmu_rb_register(void *ops_arg, struct mm_struct *mm, > +int hfi1_mmu_rb_register(void *ops_arg, > struct mmu_rb_ops *ops, > struct workqueue_struct *wq, > struct mmu_rb_handler **handler) > @@ -110,13 +99,12 @@ int hfi1_mmu_rb_register(void *ops_arg, struct mm_struct *mm, > INIT_HLIST_NODE(&handlr->mn.hlist); > spin_lock_init(&handlr->lock); > handlr->mn.ops = &mn_opts; > - handlr->mm = mm; NIT: I still think you should fix the spelling of handler... ;-) Otherwise I think the logic and code looks good... With changes to the commit message and the comment... Reviewed-by: Ira Weiny <ira.weiny@xxxxxxxxx> Ira
