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

[Ira Weiny <ira.weiny@intel.com>]

On Thu, Nov 05, 2020 at 12:13:23PM -0500, Dennis Dalessandro wrote:
> On 11/4/2020 7:12 PM, Ira Weiny wrote:
> > On Wed, Oct 28, 2020 at 09:22:43PM -0400, 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.
> > 
> > I'm not clear on the issue in this last sentence.  If process A is accessing
> > the FD via ioctl then process B closes the fd release should not be called
> > until process A calls close as well?
> 
> Is that the case. If proc A opened the FD then forked. Won't a call by B to
> close the fd end up doing the release?

Only if A closes the FD as well.  In which case A can't be in the middle of an
ioctl.

> 
> > I don't think it really matters much the code is wrong for other reasons.  I'm
> > just trying to understand what you are saying here.
> > 
> > > 
> > > To fix this correctly we move the cached value of the mm into the mmu
> > > handler struct for the driver. 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.
> > 
> > I'm not seeing this bit of logic below...  Sorry.
> 
> Hmm. Maybe I goofed on a merge or something. Will fix up for a v2.
> 
> > > 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. This is
> > > taken when the notifier is registered and dropped in the file close
> > > routine.
> > 
> > This should be moved below as a comment for why you need the mmget/put()
> 
> I can do that.
> 
> > > Also of note is we do not do any explicit work to protect the interval
> > > tree notifier. It doesn't seem that this is going to be needed since we
> > > aren't actually doing anything with current->mm. The interval tree
> > > notifier stuff still has a FIXME noted from a previous commit that will
> > > be addressed in a follow on patch.
> > > 
> > > Fixes: e0cf75deab81 ("IB/hfi1: Fix mm_struct use after free")
> > > Fixes: 3faa3d9a308e ("IB/hfi1: Make use of mm consistent")
> > > Reported-by: Jann Horn <jannh@google.com>
> > > Reported-by: Jason Gunthorpe <jgg@nvidia.com>
> > > Cc: Ira Weiny <ira.weiny@intel.com>
> > > Reviewed-by: Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>
> > > Signed-off-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
> > > 
> > > ---
> > > 
> > > Will add a Cc for stable once the patch is finalized. I'd like to get
> > > some more feedback on this patch especially in the mmu_interval_notifier
> > > stuff. I expect to be able to get a Reviewed-by from Ira as well but
> > > I've made changes he hasn't yet seen so going with a Cc for this round.
> > > 
> > > This is sort of a v1 as the patch was sent to security folks at first
> > > which included a few others as well. This is the first public posting.
> > 
> > Is this really a security issue or a correctness issue?  Is there really any
> > way that a user can corrupt another users data?  Or is this just going to
> > scribble all over the wrong process or send the wrong data on the wire?
> 
> I guess it depends on your viewpoint. It's a security issue in the sense
> that we are currently letting one process operate while using a pointer to
> another's mm pointer. Could someone exploit it to do somethign bad, I don't
> know. I'd rather not take the chance. Due to the nature of the bug we
> thought it prudent to not take chances here.

I agree this should be fixed.  But I think it is low on the security issue.  I
have not really thought about what it might mean in an HPC sense of MPI jobs
but I still think it would be contained to a single users data.  So still seems
like a bug which would only affect a single user and I'm not sure how it would
be exploited to other users data.

That said we should get it fixed...

[snip]

> > > @@ -92,7 +80,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,18 +98,20 @@ 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;
> > >   	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;
> > >   	}
> > > +	mmget(current->mm);
> > 
> > I flagged this initially but then reviewed the commit message for why you need
> > this reference.  I think it is worth a comment here as well as below.
> > Specifically mentioning the order of calls in do_exit().
> 
> Sure.

I'm somewhat with Jason on this regarding the mmget().  It does say it should not
be held for a long time.  This is why I flagged the code above initially.

But then you mentioned the whole do_exit() thing which I did not take the time
to really dig into.  So perhaps it should be revisited if you need to use the
mmget_not_zero() call to catch this do_exit() senario?

Ira