buggy-looking mm_struct refcounting in HFI1 infiniband driver

[Jann Horn <jannh@google.com>]

Hi!

mm_struct has two refcounts: ->mm_users for references that pin
everything (including page tables, VMAs, ...) and ->mm_count (just
protects the mm_struct itself and the pgd, but not the page tables or
VMAs).

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
[...]
}

It looks like that's from commit
e0cf75deab8155334c8228eb7f097b15127d0a49 ("IB/hfi1: Fix mm_struct use
after free").

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).

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.


AFAICS the three options to fix this, if it indeed is a real bug, would be:

1) use mmget_not_zero() around any operations that need the VMAs
and/or pagetables to be stable (if the mm_struct's virtual memory
should disappear once all tasks that were using it have gone away)
2) take a long-term ->mm_users reference (generally frowned upon, but
may be reasonable if you explicitly want to preserve mm contents even
if all tasks that were using the mm have gone away)
3) bail out early on any operation where `current->mm != fd->mm` and
declare that using the hfi1 fd from another process is not supported
(but I haven't checked whether you might still have some code paths
that will have to remotely operate on the mm_struct)