Re: buggy-looking mm_struct refcounting in HFI1 infiniband driver

[Dennis Dalessandro <dennis.dalessandro@intel.com>]

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.

>> 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?

I'll look at it closer and either send a patch or an explanation. Thanks 
for bringing it to our attention Jann!

-Denny