Re: buggy-looking mm_struct refcounting in HFI1 infiniband driver

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Sep 01, 2020 at 08:58:18AM -0400, Dennis Dalessandro wrote:
> On 8/31/2020 8:21 PM, Jason Gunthorpe wrote:
> > On Tue, Sep 01, 2020 at 01:45:06AM +0200, Jann Horn wrote:
> > 
> > > struct hfi1_filedata has a member ->mm that holds a ->mm_count reference:
> > > 
> > > static int hfi1_file_open(struct inode *inode, struct file *fp)
> > > {
> > >          struct hfi1_filedata *fd;
> > > [...]
> > >          fd->mm = current->mm;
> > >          mmgrab(fd->mm); // increments ->mm_count
> > > [...]
> > > }
> > 
> > Yikes, gross.
> > > However, e.g. the call chain hfi1_file_ioctl() -> user_exp_rcv_setup()
> > > -> hfi1_user_exp_rcv_setup() -> pin_rcv_pages() ->
> > > hfi1_acquire_user_pages() -> pin_user_pages_fast() can end up
> > > traversing VMAs without holding any ->mm_users reference, as far as I
> > > can tell. That will probably result in kernel memory corruption if
> > > that races the wrong way with an exiting task (with the ioctl() call
> > > coming from a task whose ->mm is different from fd->mm).
> > 
> > It looks like this path should be using current and storing the grab'd
> > mm in the tidbuf for later use by hfi1_release_user_pages()
> > 
> > The only other use of file->mm is to setup a notifier, but this is
> > also under hfi1_user_exp_rcv_setup() so it should just use tidbuf->mm
> > == current anyhow.
> > 
> > The pq->mm looks similar, looks like the pq should use current->mm,
> > and it sets up an old-style notifier, but I didn't check carefully if
> > all the call paths are linked back to an ioctl..
> > 
> > It doesn't make sense that a RDMA driver would do any page pinning
> > outside an ioctl, so it should always use current.
> 
> I sort of recall a bug where we were trusting current and it wasn't correct.
> I'll have to see if I can dig up the details and figure out what's going on
> here.

The trouble with not using current is it opens a security issue where
a process can access memory it shouldn't be allowed to access.

Jason