[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5d1c6160-6bc4-5246-2a0b-de5ddcbbc2c4@quicinc.com>
Date: Tue, 11 Apr 2023 13:34:34 -0700
From: Elliot Berman <quic_eberman@...cinc.com>
To: Will Deacon <will@...nel.org>
CC: Alex Elder <elder@...aro.org>,
Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
Prakruthi Deepak Heragu <quic_pheragu@...cinc.com>,
Murali Nalajala <quic_mnalajal@...cinc.com>,
Trilok Soni <quic_tsoni@...cinc.com>,
Srivatsa Vaddagiri <quic_svaddagi@...cinc.com>,
Carl van Schaik <quic_cvanscha@...cinc.com>,
Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konrad.dybcio@...aro.org>,
"Arnd Bergmann" <arnd@...db.de>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Jonathan Corbet <corbet@....net>,
"Bagas Sanjaya" <bagasdotme@...il.com>,
Andy Gross <agross@...nel.org>,
"Catalin Marinas" <catalin.marinas@....com>,
Jassi Brar <jassisinghbrar@...il.com>,
<linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-doc@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v11 12/26] gunyah: vm_mgr: Add/remove user memory regions
On 3/24/2023 11:37 AM, Will Deacon wrote:
> On Fri, Mar 03, 2023 at 05:06:18PM -0800, Elliot Berman wrote:
>> When launching a virtual machine, Gunyah userspace allocates memory for
>> the guest and informs Gunyah about these memory regions through
>> SET_USER_MEMORY_REGION ioctl.
>>
>> Co-developed-by: Prakruthi Deepak Heragu <quic_pheragu@...cinc.com>
>> Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@...cinc.com>
>> Signed-off-by: Elliot Berman <quic_eberman@...cinc.com>
>> ---
>> drivers/virt/gunyah/Makefile | 2 +-
>> drivers/virt/gunyah/vm_mgr.c | 44 ++++++
>> drivers/virt/gunyah/vm_mgr.h | 25 ++++
>> drivers/virt/gunyah/vm_mgr_mm.c | 229 ++++++++++++++++++++++++++++++++
>> include/uapi/linux/gunyah.h | 29 ++++
>> 5 files changed, 328 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/virt/gunyah/vm_mgr_mm.c
>
> [...]
>
>> +int gh_vm_mem_alloc(struct gh_vm *ghvm, struct gh_userspace_memory_region *region)
>> +{
>> + struct gh_vm_mem *mapping, *tmp_mapping;
>> + struct gh_rm_mem_entry *mem_entries;
>> + phys_addr_t curr_page, prev_page;
>> + struct gh_rm_mem_parcel *parcel;
>> + int i, j, pinned, ret = 0;
>> + size_t entry_size;
>> + u16 vmid;
>> +
>> + if (!region->memory_size || !PAGE_ALIGNED(region->memory_size) ||
>> + !PAGE_ALIGNED(region->userspace_addr) || !PAGE_ALIGNED(region->guest_phys_addr))
>> + return -EINVAL;
>> +
>> + if (region->guest_phys_addr + region->memory_size < region->guest_phys_addr)
>> + return -EOVERFLOW;
>> +
>> + ret = mutex_lock_interruptible(&ghvm->mm_lock);
>> + if (ret)
>> + return ret;
>> +
>> + mapping = __gh_vm_mem_find_by_label(ghvm, region->label);
>> + if (mapping) {
>> + mutex_unlock(&ghvm->mm_lock);
>> + return -EEXIST;
>> + }
>> +
>> + mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
>> + if (!mapping) {
>> + mutex_unlock(&ghvm->mm_lock);
>> + return -ENOMEM;
>> + }
>> +
>> + mapping->parcel.label = region->label;
>> + mapping->guest_phys_addr = region->guest_phys_addr;
>> + mapping->npages = region->memory_size >> PAGE_SHIFT;
>> + parcel = &mapping->parcel;
>> + parcel->mem_handle = GH_MEM_HANDLE_INVAL; /* to be filled later by mem_share/mem_lend */
>> + parcel->mem_type = GH_RM_MEM_TYPE_NORMAL;
>> +
>> + /* Check for overlap */
>> + list_for_each_entry(tmp_mapping, &ghvm->memory_mappings, list) {
>> + if (!((mapping->guest_phys_addr + (mapping->npages << PAGE_SHIFT) <=
>> + tmp_mapping->guest_phys_addr) ||
>> + (mapping->guest_phys_addr >=
>> + tmp_mapping->guest_phys_addr + (tmp_mapping->npages << PAGE_SHIFT)))) {
>> + ret = -EEXIST;
>> + goto free_mapping;
>> + }
>> + }
>> +
>> + list_add(&mapping->list, &ghvm->memory_mappings);
>> +
>> + mapping->pages = kcalloc(mapping->npages, sizeof(*mapping->pages), GFP_KERNEL);
>> + if (!mapping->pages) {
>> + ret = -ENOMEM;
>> + mapping->npages = 0; /* update npages for reclaim */
>> + goto reclaim;
>> + }
>> +
>> + 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.
Thanks,
Elliot
Powered by blists - more mailing lists