Re: buggy-looking mm_struct refcounting in HFI1 infiniband driver

[Jason Gunthorpe <jgg@nvidia.com>]

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.

> Disclaimer: I haven't actually tested this - I just stumbled over it
> while working on some other stuff, and I don't have any infiniband
> hardware to test with. So it might well be that I just missed an
> mmget_not_zero() somewhere, or something like that.

It looks wrong to me too.

Dennis?

Jason