On 4/11/2023 2:19 PM, Will Deacon wrote:
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?
Ah this ended up being an error on my part. Userspace was opening the
file as RO and Gunyah driver will unconditionally add FOLL_WRITE as part
of the gup flags. I got the flags aligned and seemed to be able to boot
the VM ok and it works as expected.
Thanks,
Elliot
