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
