| 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
| ||
|
Message-ID: <20210111035540.GC22447@shuo-intel.sh.intel.com>
Date: Mon, 11 Jan 2021 11:55:40 +0800
From: Shuo A Liu <shuo.a.liu@...el.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: linux-kernel@...r.kernel.org, x86@...nel.org,
"H . Peter Anvin" <hpa@...or.com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
Yu Wang <yu1.wang@...el.com>,
Reinette Chatre <reinette.chatre@...el.com>,
Zhi Wang <zhi.a.wang@...el.com>,
Zhenyu Wang <zhenyuw@...ux.intel.com>
Subject: Re: [PATCH v7 06/18] virt: acrn: Introduce VM management interfaces
On Fri 8.Jan'21 at 16:31:34 +0100, Greg Kroah-Hartman wrote:
>On Wed, Jan 06, 2021 at 03:50:43PM +0800, shuo.a.liu@...el.com wrote:
>> From: Shuo Liu <shuo.a.liu@...el.com>
>>
>> The VM management interfaces expose several VM operations to ACRN
>> userspace via ioctls. For example, creating VM, starting VM, destroying
>> VM and so on.
>>
>> The ACRN Hypervisor needs to exchange data with the ACRN userspace
>> during the VM operations. HSM provides VM operation ioctls to the ACRN
>> userspace and communicates with the ACRN Hypervisor for VM operations
>> via hypercalls.
>>
>> HSM maintains a list of User VM. Each User VM will be bound to an
>> existing file descriptor of /dev/acrn_hsm. The User VM will be
>> destroyed when the file descriptor is closed.
>>
>> Signed-off-by: Shuo Liu <shuo.a.liu@...el.com>
>> Reviewed-by: Zhi Wang <zhi.a.wang@...el.com>
>> Reviewed-by: Reinette Chatre <reinette.chatre@...el.com>
>> Cc: Zhi Wang <zhi.a.wang@...el.com>
>> Cc: Zhenyu Wang <zhenyuw@...ux.intel.com>
>> Cc: Yu Wang <yu1.wang@...el.com>
>> Cc: Reinette Chatre <reinette.chatre@...el.com>
>> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>> ---
>> .../userspace-api/ioctl/ioctl-number.rst | 1 +
>> MAINTAINERS | 1 +
>> drivers/virt/acrn/Makefile | 2 +-
>> drivers/virt/acrn/acrn_drv.h | 21 ++++-
>> drivers/virt/acrn/hsm.c | 76 +++++++++++++++++-
>> drivers/virt/acrn/hypercall.h | 78 +++++++++++++++++++
>> drivers/virt/acrn/vm.c | 68 ++++++++++++++++
>> include/uapi/linux/acrn.h | 55 +++++++++++++
>> 8 files changed, 298 insertions(+), 4 deletions(-)
>> create mode 100644 drivers/virt/acrn/hypercall.h
>> create mode 100644 drivers/virt/acrn/vm.c
>> create mode 100644 include/uapi/linux/acrn.h
>>
>> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
>> index a4c75a28c839..0aec83c01368 100644
>> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
>> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
>> @@ -319,6 +319,7 @@ Code Seq# Include File Comments
>> 0xA0 all linux/sdp/sdp.h Industrial Device Project
>> <mailto:kenji@...gate.com>
>> 0xA1 0 linux/vtpm_proxy.h TPM Emulator Proxy Driver
>> +0xA2 all uapi/linux/acrn.h ACRN hypervisor
>> 0xA3 80-8F Port ACL in development:
>> <mailto:tlewis@...dspring.com>
>> 0xA3 90-9F linux/dtlk.h
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index f1c481c829cf..69f3cefed7e5 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -443,6 +443,7 @@ S: Supported
>> W: https://projectacrn.org
>> F: Documentation/virt/acrn/
>> F: drivers/virt/acrn/
>> +F: include/uapi/linux/acrn.h
>>
>> AD1889 ALSA SOUND DRIVER
>> L: linux-parisc@...r.kernel.org
>> diff --git a/drivers/virt/acrn/Makefile b/drivers/virt/acrn/Makefile
>> index 6920ed798aaf..cf8b4ed5e74e 100644
>> --- a/drivers/virt/acrn/Makefile
>> +++ b/drivers/virt/acrn/Makefile
>> @@ -1,3 +1,3 @@
>> # SPDX-License-Identifier: GPL-2.0
>> obj-$(CONFIG_ACRN_HSM) := acrn.o
>> -acrn-y := hsm.o
>> +acrn-y := hsm.o vm.o
>> diff --git a/drivers/virt/acrn/acrn_drv.h b/drivers/virt/acrn/acrn_drv.h
>> index 29eedd696327..e5aba86cad8c 100644
>> --- a/drivers/virt/acrn/acrn_drv.h
>> +++ b/drivers/virt/acrn/acrn_drv.h
>> @@ -3,16 +3,35 @@
>> #ifndef __ACRN_HSM_DRV_H
>> #define __ACRN_HSM_DRV_H
>>
>> +#include <linux/acrn.h>
>> +#include <linux/dev_printk.h>
>> +#include <linux/miscdevice.h>
>> #include <linux/types.h>
>>
>> +#include "hypercall.h"
>> +
>> +extern struct miscdevice acrn_dev;
>> +
>> #define ACRN_INVALID_VMID (0xffffU)
>>
>> +#define ACRN_VM_FLAG_DESTROYED 0U
>> /**
>> * struct acrn_vm - Properties of ACRN User VM.
>> + * @list: Entry within global list of all VMs
>> * @vmid: User VM ID
>> + * @vcpu_num: Number of virtual CPUs in the VM
>> + * @flags: Flags (ACRN_VM_FLAG_*) of the VM. This is VM flag management
>> + * in HSM which is different from the &acrn_vm_creation.vm_flag.
>> */
>> struct acrn_vm {
>> - u16 vmid;
>> + struct list_head list;
>> + u16 vmid;
>> + int vcpu_num;
>> + unsigned long flags;
>> };
>>
>> +struct acrn_vm *acrn_vm_create(struct acrn_vm *vm,
>> + struct acrn_vm_creation *vm_param);
>> +int acrn_vm_destroy(struct acrn_vm *vm);
>> +
>> #endif /* __ACRN_HSM_DRV_H */
>> diff --git a/drivers/virt/acrn/hsm.c b/drivers/virt/acrn/hsm.c
>> index a8dcb250649d..5fd933471683 100644
>> --- a/drivers/virt/acrn/hsm.c
>> +++ b/drivers/virt/acrn/hsm.c
>> @@ -9,7 +9,6 @@
>> * Yakui Zhao <yakui.zhao@...el.com>
>> */
>>
>> -#include <linux/miscdevice.h>
>> #include <linux/mm.h>
>> #include <linux/module.h>
>> #include <linux/slab.h>
>> @@ -38,10 +37,82 @@ static int acrn_dev_open(struct inode *inode, struct file *filp)
>> return 0;
>> }
>>
>> +/*
>> + * HSM relies on hypercall layer of the ACRN hypervisor to do the
>> + * sanity check against the input parameters.
>> + */
>> +static long acrn_dev_ioctl(struct file *filp, unsigned int cmd,
>> + unsigned long ioctl_param)
>> +{
>> + struct acrn_vm *vm = filp->private_data;
>> + struct acrn_vm_creation *vm_param;
>> + int ret = 0;
>> +
>> + if (vm->vmid == ACRN_INVALID_VMID && cmd != ACRN_IOCTL_CREATE_VM) {
>> + dev_dbg(acrn_dev.this_device,
>> + "ioctl 0x%x: Invalid VM state!\n", cmd);
>> + return -EINVAL;
>> + }
>> +
>> + switch (cmd) {
>> + case ACRN_IOCTL_CREATE_VM:
>> + vm_param = memdup_user((void __user *)ioctl_param,
>> + sizeof(struct acrn_vm_creation));
>> + if (IS_ERR(vm_param))
>> + return PTR_ERR(vm_param);
>> +
>> + if ((vm_param->reserved0 | vm_param->reserved1) != 0)
>> + return -EINVAL;
>> +
>> + vm = acrn_vm_create(vm, vm_param);
>> + if (!vm) {
>> + ret = -EINVAL;
>> + kfree(vm_param);
>> + break;
>> + }
>> +
>> + if (copy_to_user((void __user *)ioctl_param, vm_param,
>> + sizeof(struct acrn_vm_creation))) {
>> + acrn_vm_destroy(vm);
>> + ret = -EFAULT;
>> + }
>> +
>> + kfree(vm_param);
>> + break;
>> + case ACRN_IOCTL_START_VM:
>> + ret = hcall_start_vm(vm->vmid);
>> + if (ret < 0)
>> + dev_dbg(acrn_dev.this_device,
>> + "Failed to start VM %u!\n", vm->vmid);
>> + break;
>> + case ACRN_IOCTL_PAUSE_VM:
>> + ret = hcall_pause_vm(vm->vmid);
>> + if (ret < 0)
>> + dev_dbg(acrn_dev.this_device,
>> + "Failed to pause VM %u!\n", vm->vmid);
>> + break;
>> + case ACRN_IOCTL_RESET_VM:
>> + ret = hcall_reset_vm(vm->vmid);
>> + if (ret < 0)
>> + dev_dbg(acrn_dev.this_device,
>> + "Failed to restart VM %u!\n", vm->vmid);
>> + break;
>> + case ACRN_IOCTL_DESTROY_VM:
>> + ret = acrn_vm_destroy(vm);
>> + break;
>> + default:
>> + dev_dbg(acrn_dev.this_device, "Unknown IOCTL 0x%x!\n", cmd);
>> + ret = -ENOTTY;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> static int acrn_dev_release(struct inode *inode, struct file *filp)
>> {
>> struct acrn_vm *vm = filp->private_data;
>>
>> + acrn_vm_destroy(vm);
>> kfree(vm);
>> return 0;
>> }
>> @@ -50,9 +121,10 @@ static const struct file_operations acrn_fops = {
>> .owner = THIS_MODULE,
>> .open = acrn_dev_open,
>> .release = acrn_dev_release,
>> + .unlocked_ioctl = acrn_dev_ioctl,
>> };
>>
>> -static struct miscdevice acrn_dev = {
>> +struct miscdevice acrn_dev = {
>> .minor = MISC_DYNAMIC_MINOR,
>> .name = "acrn_hsm",
>> .fops = &acrn_fops,
>> diff --git a/drivers/virt/acrn/hypercall.h b/drivers/virt/acrn/hypercall.h
>> new file mode 100644
>> index 000000000000..426b66cadb1f
>> --- /dev/null
>> +++ b/drivers/virt/acrn/hypercall.h
>> @@ -0,0 +1,78 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * ACRN HSM: hypercalls of ACRN Hypervisor
>> + */
>> +#ifndef __ACRN_HSM_HYPERCALL_H
>> +#define __ACRN_HSM_HYPERCALL_H
>> +#include <asm/acrn.h>
>> +
>> +/*
>> + * Hypercall IDs of the ACRN Hypervisor
>> + */
>> +#define _HC_ID(x, y) (((x) << 24) | (y))
>> +
>> +#define HC_ID 0x80UL
>> +
>> +#define HC_ID_VM_BASE 0x10UL
>> +#define HC_CREATE_VM _HC_ID(HC_ID, HC_ID_VM_BASE + 0x00)
>> +#define HC_DESTROY_VM _HC_ID(HC_ID, HC_ID_VM_BASE + 0x01)
>> +#define HC_START_VM _HC_ID(HC_ID, HC_ID_VM_BASE + 0x02)
>> +#define HC_PAUSE_VM _HC_ID(HC_ID, HC_ID_VM_BASE + 0x03)
>> +#define HC_RESET_VM _HC_ID(HC_ID, HC_ID_VM_BASE + 0x05)
>> +
>> +/**
>> + * hcall_create_vm() - Create a User VM
>> + * @vminfo: Service VM GPA of info of User VM creation
>> + *
>> + * Return: 0 on success, <0 on failure
>> + */
>> +static inline long hcall_create_vm(u64 vminfo)
>> +{
>> + return acrn_hypercall1(HC_CREATE_VM, vminfo);
>> +}
>> +
>> +/**
>> + * hcall_start_vm() - Start a User VM
>> + * @vmid: User VM ID
>> + *
>> + * Return: 0 on success, <0 on failure
>> + */
>> +static inline long hcall_start_vm(u64 vmid)
>> +{
>> + return acrn_hypercall1(HC_START_VM, vmid);
>> +}
>> +
>> +/**
>> + * hcall_pause_vm() - Pause a User VM
>> + * @vmid: User VM ID
>> + *
>> + * Return: 0 on success, <0 on failure
>> + */
>> +static inline long hcall_pause_vm(u64 vmid)
>> +{
>> + return acrn_hypercall1(HC_PAUSE_VM, vmid);
>> +}
>> +
>> +/**
>> + * hcall_destroy_vm() - Destroy a User VM
>> + * @vmid: User VM ID
>> + *
>> + * Return: 0 on success, <0 on failure
>> + */
>> +static inline long hcall_destroy_vm(u64 vmid)
>> +{
>> + return acrn_hypercall1(HC_DESTROY_VM, vmid);
>> +}
>> +
>> +/**
>> + * hcall_reset_vm() - Reset a User VM
>> + * @vmid: User VM ID
>> + *
>> + * Return: 0 on success, <0 on failure
>> + */
>> +static inline long hcall_reset_vm(u64 vmid)
>> +{
>> + return acrn_hypercall1(HC_RESET_VM, vmid);
>> +}
>> +
>> +#endif /* __ACRN_HSM_HYPERCALL_H */
>> diff --git a/drivers/virt/acrn/vm.c b/drivers/virt/acrn/vm.c
>> new file mode 100644
>> index 000000000000..3f667ac8ac1e
>> --- /dev/null
>> +++ b/drivers/virt/acrn/vm.c
>> @@ -0,0 +1,68 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * ACRN_HSM: Virtual Machine management
>> + *
>> + * Copyright (C) 2020 Intel Corporation. All rights reserved.
>> + *
>> + * Authors:
>> + * Jason Chen CJ <jason.cj.chen@...el.com>
>> + * Yakui Zhao <yakui.zhao@...el.com>
>> + */
>> +#include <linux/io.h>
>> +#include <linux/mm.h>
>> +#include <linux/slab.h>
>> +
>> +#include "acrn_drv.h"
>> +
>> +/* List of VMs */
>> +static LIST_HEAD(acrn_vm_list);
>> +/* To protect acrn_vm_list */
>> +static DEFINE_MUTEX(acrn_vm_list_lock);
>> +
>> +struct acrn_vm *acrn_vm_create(struct acrn_vm *vm,
>> + struct acrn_vm_creation *vm_param)
>> +{
>> + int ret;
>> +
>> + ret = hcall_create_vm(virt_to_phys(vm_param));
>> + if (ret < 0 || vm_param->vmid == ACRN_INVALID_VMID) {
>> + dev_err(acrn_dev.this_device,
>> + "Failed to create VM! Error: %d\n", ret);
>> + return NULL;
>> + }
>> +
>> + vm->vmid = vm_param->vmid;
>> + vm->vcpu_num = vm_param->vcpu_num;
>> +
>> + mutex_lock(&acrn_vm_list_lock);
>> + list_add(&vm->list, &acrn_vm_list);
>> + mutex_unlock(&acrn_vm_list_lock);
>> +
>> + dev_dbg(acrn_dev.this_device, "VM %u created.\n", vm->vmid);
>> + return vm;
>> +}
>> +
>> +int acrn_vm_destroy(struct acrn_vm *vm)
>> +{
>> + int ret;
>> +
>> + if (vm->vmid == ACRN_INVALID_VMID ||
>> + test_and_set_bit(ACRN_VM_FLAG_DESTROYED, &vm->flags))
>> + return 0;
>> +
>> + /* Remove from global VM list */
>> + mutex_lock(&acrn_vm_list_lock);
>> + list_del_init(&vm->list);
>> + mutex_unlock(&acrn_vm_list_lock);
>> +
>> + ret = hcall_destroy_vm(vm->vmid);
>> + if (ret < 0) {
>> + dev_err(acrn_dev.this_device,
>> + "Failed to destroy VM %u\n", vm->vmid);
>> + clear_bit(ACRN_VM_FLAG_DESTROYED, &vm->flags);
>> + return ret;
>> + }
>> + dev_dbg(acrn_dev.this_device, "VM %u destroyed.\n", vm->vmid);
>> + vm->vmid = ACRN_INVALID_VMID;
>> + return 0;
>> +}
>> diff --git a/include/uapi/linux/acrn.h b/include/uapi/linux/acrn.h
>> new file mode 100644
>> index 000000000000..e1608b8a50a2
>> --- /dev/null
>> +++ b/include/uapi/linux/acrn.h
>> @@ -0,0 +1,55 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +/*
>> + * Userspace interface for /dev/acrn_hsm - ACRN Hypervisor Service Module
>> + *
>> + * This file can be used by applications that need to communicate with the HSM
>> + * via the ioctl interface.
>
>No copyright notice on this file? Intel is being brave :)
Will add it. Thanks.
>
>
>> + */
>> +
>> +#ifndef _UAPI_ACRN_H
>> +#define _UAPI_ACRN_H
>> +
>> +#include <linux/types.h>
>> +#include <linux/uuid.h>
>> +
>> +/**
>> + * struct acrn_vm_creation - Info to create a User VM
>> + * @vmid: User VM ID returned from the hypervisor
>> + * @reserved0: Reserved and must be 0
>
>That's good, but you are not checking this. Why not?
I did do the check in ACRN_IOCTL_CREATE_VM branch in acrn_dev_ioctl().
if ((vm_param->reserved0 | vm_param->reserved1) != 0)
return -EINVAL;
>
>> + * @vcpu_num: Number of vCPU in the VM. Return from hypervisor.
>> + * @reserved1: Reserved and must be 0
>
>Again, I failed to see a check, so this "must be" is not true.
>
>> + * @uuid: UUID of the VM. Pass to hypervisor directly.
>> + * @vm_flag: Flag of the VM creating. Pass to hypervisor directly.
>> + * @ioreq_buf: Service VM GPA of I/O request buffer. Pass to
>> + * hypervisor directly.
>> + * @cpu_affinity: CPU affinity of the VM. Pass to hypervisor directly.
>
>What format is this affinity?
It's a bitmap. I will mention this in its documentation.
>
>> + */
>> +struct acrn_vm_creation {
>> + __u16 vmid;
>> + __u16 reserved0;
>> + __u16 vcpu_num;
>> + __u16 reserved1;
>> + guid_t uuid;
>> + __u64 vm_flag;
>> + __u64 ioreq_buf;
>> + __u64 cpu_affinity;
>> +};
>
>As these values go outside the kernel, you need to specify the endian of
>them. You do that for other ioctl structures, but not this one, why?
>From my understanding, these values are ARCH un-related. Both kernel and
hypervisor are running on a same machine, so we can pass these values to
hypervisor direclty as their endian are same.
For those ARCH related values, such as CPU registers, endian is
specified.
Thanks
shuo
Powered by blists - more mailing lists