Re: [PATCH v11 12/26] gunyah: vm_mgr: Add/remove user memory regions

[Will Deacon]

On Tue, Apr 11, 2023 at 01:34:34PM -0700, Elliot Berman wrote:
> On 3/24/2023 11:37 AM, Will Deacon wrote:
> > On Fri, Mar 03, 2023 at 05:06:18PM -0800, Elliot Berman wrote:
> > > +
> > > +	pinned = pin_user_pages_fast(region->userspace_addr, mapping->npages,
> > > +					FOLL_WRITE | FOLL_LONGTERM, mapping->pages);
> > > +	if (pinned < 0) {
> > > +		ret = pinned;
> > > +		mapping->npages = 0; /* update npages for reclaim */
> > > +		goto reclaim;
> > > +	} else if (pinned != mapping->npages) {
> > > +		ret = -EFAULT;
> > > +		mapping->npages = pinned; /* update npages for reclaim */
> > > +		goto reclaim;
> > > +	}
> > 
> > I think Fuad mentioned this on an older version of these patches, but it
> > looks like you're failing to account for the pinned memory here which is
> > a security issue depending on who is able to issue the ioctl() calling
> > into here.
> > 
> > Specifically, I'm thinking that your kXalloc() calls should be using
> > GFP_KERNEL_ACCOUNT in this function and also that you should be calling
> > account_locked_vm() for the pages being pinned.
> > 
> 
> Added the accounting for the v12.
> 
> > Finally, what happens if userspace passes in a file mapping?
> 
> Userspace will get EBADADDR (-14) back when trying to launch the VM
> (pin_user_pages_fast returns this as you might have been expecting). We
> haven't yet had any need to support file-backed mappings.

Hmm, no, that's actually surprising to me. I'd have thought GUP would
happily pin page-cache pages for file mappings, so I'm intrigued as to
which FOLL_ flag is causing you to get an error code back. Can you
enlighten me on where the failure originates, please?

Will