linux-kernel - Re: [PATCH v13 10/24] gunyah: vm_mgr: Add/remove user memory regions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230519115948.GB2637@willie-the-truck>
Date:   Fri, 19 May 2023 12:59:49 +0100
From:   Will Deacon <will@...nel.org>
To:     Elliot Berman <quic_eberman@...cinc.com>
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 v13 10/24] gunyah: vm_mgr: Add/remove user memory regions

Hi Elliot,

On Tue, May 09, 2023 at 01:47:47PM -0700, 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    |  59 +++++++-
>  drivers/virt/gunyah/vm_mgr.h    |  26 ++++
>  drivers/virt/gunyah/vm_mgr_mm.c | 236 ++++++++++++++++++++++++++++++++
>  include/uapi/linux/gunyah.h     |  37 +++++
>  5 files changed, 356 insertions(+), 4 deletions(-)
>  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 page *curr_page, *prev_page;
> +	struct gh_rm_mem_parcel *parcel;
> +	int i, j, pinned, ret = 0;
> +	unsigned int gup_flags;
> +	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 (overflows_type(region->guest_phys_addr + region->memory_size, u64))
> +		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) {
> +		ret = -EEXIST;
> +		goto unlock;
> +	}
> +
> +	list_for_each_entry(tmp_mapping, &ghvm->memory_mappings, list) {
> +		if (gh_vm_mem_overlap(tmp_mapping, region->guest_phys_addr,
> +					region->memory_size)) {
> +			ret = -EEXIST;
> +			goto unlock;
> +		}
> +	}
> +
> +	mapping = kzalloc(sizeof(*mapping), GFP_KERNEL_ACCOUNT);
> +	if (!mapping) {
> +		ret = -ENOMEM;
> +		goto unlock;
> +	}
> +
> +	mapping->guest_phys_addr = region->guest_phys_addr;
> +	mapping->npages = region->memory_size >> PAGE_SHIFT;
> +	parcel = &mapping->parcel;
> +	parcel->label = region->label;
> +	parcel->mem_handle = GH_MEM_HANDLE_INVAL; /* to be filled later by mem_share/mem_lend */
> +	parcel->mem_type = GH_RM_MEM_TYPE_NORMAL;
> +
> +	ret = account_locked_vm(ghvm->mm, mapping->npages, true);
> +	if (ret)
> +		goto free_mapping;
> +
> +	mapping->pages = kcalloc(mapping->npages, sizeof(*mapping->pages), GFP_KERNEL_ACCOUNT);
> +	if (!mapping->pages) {
> +		ret = -ENOMEM;
> +		mapping->npages = 0; /* update npages for reclaim */
> +		goto unlock_pages;
> +	}
> +
> +	gup_flags = FOLL_LONGTERM;
> +	if (region->flags & GH_MEM_ALLOW_WRITE)
> +		gup_flags |= FOLL_WRITE;
> +
> +	pinned = pin_user_pages_fast(region->userspace_addr, mapping->npages,
> +					gup_flags, mapping->pages);
> +	if (pinned < 0) {
> +		ret = pinned;
> +		goto free_pages;
> +	} else if (pinned != mapping->npages) {
> +		ret = -EFAULT;
> +		mapping->npages = pinned; /* update npages for reclaim */
> +		goto unpin_pages;
> +	}

Sorry if I missed it, but I still don't see where you reject file mappings
here.

This is also the wrong interface for upstream. Please get involved with
the fd-based guest memory discussions [1] and port your series to that. 

This patch cannot be merged in its current form.

Will

[1] https://lore.kernel.org/kvm/20221202061347.1070246-1-chao.p.peng@linux.intel.com/